Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 461868 - sys-apps/portage: xattr support not allowed for security.* labels on SELinux
Summary: sys-apps/portage: xattr support not allowed for security.* labels on SELinux
Status: RESOLVED FIXED
Alias: None
Product: Portage Development
Classification: Unclassified
Component: Core (show other bugs)
Hardware: All Linux
: Normal normal (vote)
Assignee: Portage team
URL:
Whiteboard:
Keywords: InVCS
Depends on:
Blocks: 462382
  Show dependency tree
 
Reported: 2013-03-15 23:00 UTC by Vincent Brillault
Modified: 2013-03-27 17:07 UTC (History)
1 user (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
Example build log (audit-debug-buildlog.txt,491.27 KB, text/plain)
2013-03-16 18:48 UTC, Sven Vermeulen (RETIRED)
Details
Patch applied on portage for more verbose logs (debug-portage.patch,4.06 KB, patch)
2013-03-16 21:11 UTC, Vincent Brillault
Details | Diff
/usr/lib64/portage/pym/portage/util/movefile.py patch (movefile.patch,616 bytes, text/plain)
2013-03-26 19:30 UTC, Sven Vermeulen (RETIRED)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Vincent Brillault 2013-03-15 23:00:48 UTC
I'm having some errors recently when emerging certain packets. The installation dies with a '!!! Failed to copy extended attributes.'. In the deny logs I can see some 'relabelfrom/relabelto' deny. So far, I had problems with:
- asterisk
- auditd
- clamav

Log examples:
avc:  denied  { relabelfrom } for  pid=16879 comm="emerge" name=".keep_app-antivirus_clamav-0#new" dev="dm-6" ino=50 scontext=staff_u:sysadm_r:portage_t tcontext=staff_u:object_r:clamd_var_log_t tclass=file
avc:  denied  { relabelfrom } for  pid=25028 comm="emerge" name=".keep_net-misc_asterisk-0#new" dev="dm-6" ino=132518 scontext=staff_u:sysadm_r:portage_t tcontext=staff_u:object_r:asterisk_log_t tclass=file
avc:  denied  { relabelfrom } for  pid=26743 comm="emerge" name=".keep_sys-process_audit-0#new" dev="dm-6" ino=49 scontext=staff_u:sysadm_r:portage_t tcontext=staff_u:object_r:auditd_log_t tclass=file
avc:  denied  { relabelto } for  pid=4061 comm="emerge" name=".keep_sys-process_audit-0#new" dev="dm-6" ino=49 scontext=staff_u:sysadm_r:portage_t tcontext=staff_u:object_r:auditd_log_t tclass=file

The audit avc is kind of old now (20130210), but the clamav one is from tonight, on the last policy (2.20120725-r12). I'm able to fix those denies by adding
allow portage_t auditd_log_t:file {relabelfrom relabelto};
allow portage_t clamd_var_log_t:file {relabelfrom relabelto};
allow portage_t asterisk_log_t:file {relabelfrom relabelto};

Nevertheless, it's probably a bad idea to add those rules (It's a good thing to have restricted rules on log files). As prometheanfire said on IRC, perhaps we should add a file special context rule for those .keep-* files
Comment 1 Sven Vermeulen (RETIRED) gentoo-dev 2013-03-16 15:58:38 UTC
Can you give a full build log where this is shown as well?

Until now, portage doesn't require any relabeling rights (except for the portage_tmp_t part) as it calls setfiles (and thus transitions to setfiles before relabeling). But in this denial it looks like emerge does it directly.

Creating a specific type for the .keep-* files might prove to be troublesome, as there is no simple way to make an expression that isn't overshadowed by the current expressions...
Comment 2 Sven Vermeulen (RETIRED) gentoo-dev 2013-03-16 18:48:50 UTC
Created attachment 342284 [details]
Example build log
Comment 3 Vincent Brillault 2013-03-16 21:11:29 UTC
Created attachment 342302 [details, diff]
Patch applied on portage for more verbose logs

I tried to debug the problem a little more on my system.
I've added a lot of print to /usr/lib/portage/pym/portage/util/movefile.py in order to understand what happens (I've attached the diff to the bug repport)
I've also added 4 print on _selinux.py on the rename function (those are really simple so I didn't attached them).

The emerge output after those patch is:
'''
--- /var/log/audit/
Fea, not hard
False 1 2049==65030
Fea, selinux.remane called
/var/tmp/portage/sys-process/audit-2.1.3-r1/image/var/log/audit/.keep_sys-process_audit-0 -> /var/log/audit/.keep_sys-process_audit-0
(30, staff_u:object_r:auditd_log_t)
Fea selinux: os.renaming
renamefailed, err [Errno 18] Invalid cross-device link
Fea, renamefailed
0, then 33188, then True
Fea, stat.S_ISREG(sstat[stat.ST_MODE])
/var/tmp/portage/sys-process/audit-2.1.3-r1/image/var/log/audit/.keep_sys-process_audit-0 -> /var/log/audit/.keep_sys-process_audit-0#new
Fea, case 1
IOERROR copyxattr: [Errno 13] Permission denied
Fea, l263: Filesystem containing file '/var/log/audit/.keep_sys-process_audit-0#new' does not support extended attributes
'''

As I underdand it:
- It's not a hardlink candidate (didn't check why)
- On line 235, I think that sstat.st_dev != dstat.st_dev means that the source and destination files are not on the same device (which is true on my system), but as selinux_enabled is true, we still enter the function
- The selinux.rename call calls os.rename(src,dest) with src and dest on two different devices, and thus fails (not on the same device) -> renamefailed=1
- stat.S_ISREG(sstat[stat.ST_MODE]) is true, it's a regular file so we enter this part of the code
- '_copyfile' works
- there is then a '_copyxattr', that fails with a '[Errno 13] Permission denied' that should be caused by the AVC I have.

The _copyxattr uses 'xattr.set(dest, attr, xattr.get(src, attr))' thus is trying to relabel the file.

If we look at the avc, the first one is 'relabelfrom' and then 'relabelto' (which appear only after I allowed the relabelfrom). Thus I think that the _copyfile creates the file with the correct labels and then xattr.set try to re-set them with the same value.

I see three issues:
- Modify xattr.set (didn't checked the source code yet) so that it sets only xattr that are not already set
- Check if src and dest have the same attributs, if so, do not call xattr.set
- Do not call xattr.set at all on selinux xattr (Bad idea I think. They should be created correctly by the _copyfile, but an ebuild *could* use some chcon internally for some customizable types, right ?)
Comment 4 Vincent Brillault 2013-03-16 22:28:23 UTC
After some thoughts (and code reading), the simplest & easiest solution would be to modify the ''xattr.set(dest, attr, xattr.get(src, attr))'':

- In a 'ask permission' style:
'''
--- movefile.py
+++ movefile.py
@@ -47,7 +47,9 @@
                def _copyxattr(src, dest):
                        for attr in xattr.list(src):
                                try:
-                                       xattr.set(dest, attr, xattr.get(src, attr))
+                                       src_xattr = xattr.get(src, attr)
+                                       if xattr.get(dest, attr) != src_xattr:
+                                               xattr.set(dest, attr, src_xattr)
                                        raise_exception = False
                                except IOError:
                                        raise_exception = True
'''

- In a 'ask forgiveness' style:
'''
--- movefile.py
+++ movefile.py
@@ -51,6 +51,11 @@
                                        raise_exception = False
                                except IOError:
                                        raise_exception = True
+                                       try:
+                                               if xattr.get(src, attr) == xattr.get(dest, attr):
+                                                       raise_exception = False
+                                       except IOError:
+                                               pass
                                if raise_exception:
                                        raise OperationNotSupported("Filesystem containing file '%s' does not support extended attributes" % dest)
        else:
'''

As the 'ask forgiveness' will generate an AVC in cases like this bug, I think the 'ask permission' style is better, even if it could generate more syscalls. I tried the 'ask permission' version and it worked :)

PS: I you want to try to reproduce the bug, I'm using:
- python2.7 by default (python3.2 installed but not selected and PYTHON_TARGETS="python2_7", USE_PYTHON='2.7' in make.conf)
- sys-apps/portage-2.1.11.52
- dev-python/pyxattr-0.5.0
- sys-apps/attr-2.4.46-r1
- SELinux policy 2.20120725-r12
- Separate partitions for /var/tmp/portage and /var/log
- 'emerge audit' (2.1.3-r1)
Comment 5 Sven Vermeulen (RETIRED) gentoo-dev 2013-03-17 10:02:37 UTC
I think an even more future proof method is just to ignore the security.* attributes while copying the extended attributes (or at least the security.selinux, although I can imagine that the security.ima and security.evm attributes are better left untouched as well as they are managed by the IMA/EVM subsystems).
Comment 6 Sven Vermeulen (RETIRED) gentoo-dev 2013-03-26 19:09:15 UTC
I get the same problem with my ~arch VMs now as well (haven't updated my stable ones yet). So doesn't seem to be related to /var/log being on a different system. Also doesn't seem to be only for .keep files...
Comment 7 Sven Vermeulen (RETIRED) gentoo-dev 2013-03-26 19:30:13 UTC
Created attachment 343338 [details]
/usr/lib64/portage/pym/portage/util/movefile.py patch

This patch checks if the attribute is a security related attribute or not (i.e. starts with "security."). If it does, then the attribute is ignored, otherwise the attribute is copied over. The PaX markings are in the "user." namespace so are properly copied over.

We can't just copy over security.* attributes: they are governed by the subsystem responsible for it. In case of "security.selinux", this requires the copying domain full relabelfrom/relabelto rights on all possible files, which we don't want to assign to portage_t (portage relabels files through setfiles, which includes a domain transition).

Also, attributes like security.ima (hashes of files) and security.evm (hashes of extended attributes) should not be changed manually - the IMA/EVM subsystems handle this transparently (or through the evmctl commands).
Comment 8 Sven Vermeulen (RETIRED) gentoo-dev 2013-03-26 19:31:17 UTC
portage-utils team (hoping this is the correct team, since it's in the portage/util directory), care to look at the mentioned patch?
Comment 9 SpanKY gentoo-dev 2013-03-26 21:17:42 UTC
(In reply to comment #8)

that is not the meaning of the portage-utils team ;)
Comment 10 Zac Medico gentoo-dev 2013-03-27 03:27:40 UTC
(In reply to comment #7)
> Created attachment 343338 [details]
> /usr/lib64/portage/pym/portage/util/movefile.py patch

I'd like to make this configurable with a variable containing a space separated list of patterns, maybe called PORTAGE_XATTR_EXCLUDE. If we use fnmatch syntax, then we would have PORTAGE_XATTR_EXCLUDE="security.*" as the default setting.
Comment 12 Zac Medico gentoo-dev 2013-03-27 17:07:00 UTC
This is fixed in 2.1.11.59 and 2.2.0_alpha170.