Re: [Linux-ima-user] [PATCH] evm: add exceptions for inodes without getxattr() support.

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

В Thu, 29 Dec 2016 10:42:13 -0500
Mimi Zohar <zo...@li...> пишет:

> On Thu, 2016-12-29 at 07:02 +0300, Mikhail Kurinnoi wrote:
> > В Wed, 28 Dec 2016 15:18:20 -0500
> > Mimi Zohar <zo...@li...> пишет:
> >   
> > > On Wed, 2016-12-28 at 21:08 +0300, Mikhail Kurinnoi wrote:  
> > > > В Wed, 28 Dec 2016 09:24:36 -0500
> > > > Mimi Zohar <zo...@li...> пишет:
> > > >     
> > > > > On Wed, 2016-12-28 at 15:51 +0300, Mikhail Kurinnoi wrote:    
> > > > > > Removed from kernel boot command line "ima_appraise=fix
> > > > > > evm=fix", I faced with a little bit strange audit log
> > > > > > output. For example: ...
> > > > > > pid=3237 uid=0 auid=4294967295 ses=4294967295
> > > > > > op="appraise_metadata" cause="fail" comm="cp"
> > > > > > name="depconfig" fowner=0 dev="tmpfs" ino=6839 res=0
> > > > > > pid=4418 uid=0 auid=4294967295 ses=4294967295
> > > > > > op="appraise_metadata" cause="fail" comm="chgrp" name="utmp"
> > > > > > fowner=0 dev="tmpfs" ino=11295 res=0
> > > > > > pid=4419 uid=0 auid=4294967295 ses=4294967295
> > > > > > op="appraise_metadata" cause="fail" comm="chmod" name="utmp"
> > > > > > fowner=0 dev="tmpfs" ino=11295 res=0
> > > > > > pid=5040 uid=0 auid=4294967295 ses=4294967295
> > > > > > op="appraise_metadata" cause="fail" comm="cupsd" name="0"
> > > > > > fowner=0 dev="tmpfs" ino=10778 res=0
> > > > > > pid=5611 uid=1000 auid=1000 ses=3 op="appraise_metadata"
> > > > > > cause="fail" comm="X" name=".tX0-lock" fowner=0 dev="tmpfs"
> > > > > > ino=11650 res=0 ...
> > > > > > 
> > > > > > This output were logged with default policy
> > > > > > (ima_appraise_tcb ima_tcb). As you can see
> > > > > > (op="appraise_metadata"), this issue connected to
> > > > > > evm_inode_setattr(), evm_inode_removexattr() and
> > > > > > evm_inode_setxattr(). After some digging I found, that we
> > > > > > don't count on getxattr() support in inode. I mean, we
> > > > > > don't count on EOPNOTSUPP error code from
> > > > > > evm_find_protected_xattrs(), as result, evm_verify_hmac()
> > > > > > will return only INTEGRITY_FAIL error and legitimate
> > > > > > attr/xattr(acls) changes will be not allowed by EVM.      
> > > > > 
> > > > > Before EVM allows a file to write file metadata it validates
> > > > > the existing security.evm xattr.  Only if it is valid, does
> > > > > EVM permit the file metadata to change.   Otherwise the
> > > > > updated security.evm xattr would be based on bogus file
> > > > > metadata.  Is the issue really that getxattr() isn't
> > > > > supported or rather that these files were created under a
> > > > > different policy, which didn't label the files properly?    
> > > > 
> > > > As I mentioned, I also test with default policy this time from
> > > > the beginning to be sure this is not my policy issue again. For
> > > > tmpfs in default policy we have only 2 lines with
> > > > "dont_appraise" + "dont_measure" connected to magic number, and
> > > > this is upper lines. My custom policy have same upper lines
> > > > with magic numbers.
> > > > 
> > > > Tmpfs support xattrs for sure, I tested this.
> > > > Also, I add test ext4 FS partition with "dont_appraise" +
> > > > "dont_measure" rules as upper lines of policy file. Result was
> > > > the same. evm_find_protected_xattrs() return EOPNOTSUPP if I
> > > > create new file and use chmod on it. If I change test ext4 FS
> > > > partition policy to "appraise" I have no issues. So, this is
> > > > not tmpfs or xattrs support issue.    
> > > 
> > > It sounds like the file does not have the security.evm xattr
> > > before you do the chmod.  Therefore it is failing.  Please show
> > > the test steps with the security xattrs after each step.  
> > 
> > 
> > 1.
> > 
> > /tmp - tmpfs mount point
> > default policy ("dont_appraise" + "dont_measure" for tmpfs).
> > 
> > # touch /tmp/test
> > # getfattr -m . -d -e hex /tmp/test
> > (no output, file don't have any xattrs)
> > # chmod 666 /tmp/test
> >   chmod: changing permissions of '/tmp/test': Operation not
> > permitted  
> 
> Before fixing things, let's make sure we understand the real problem.
> Assuming the following Kconfig options are enabled,
> evm_protect_xattr(), if there weren't any xattrs, would return 0.
> 
> CONFIG_TMPFS=y
> CONFIG_TMPFS_POSIX_ACL=y
> CONFIG_TMPFS_XATTR=y

Yes, this options are enabled.

> If these aren't enabled, then -EOPNOTSUPP would be returned.  

Not exactly, even on FS with xattrs support, xattrs could be disabled
in inode "on the fly" by removing ->list, ->get, and ->set handler
operations. In this case -EOPNOTSUPP also will be returned.

we have 2 situation connected to -EOPNOTSUPP error: 
1) FS don't support xattrs (!inode->i_op->getxattr return true);
2) FS support xattrs, but xattrs disabled in inode
(!inode->i_op->getxattr return false, but, inode->i_op->getxattr()
return -EOPNOTSUPP).

In both cases EVM should care about xattr support in FS/inode and don't
corrupt system work as it doing now.

> IMA is
> dependent on evm_verifyxattr() returning INTEGRITY_UNKNOWN, so we
> can't just return 0 in this case as well.

I think we should keep INTEGRITY_UNKNOWN for -EOPNOTSUPP.
My changes (patch) will switch return for evm_verifyxattr() from
INTEGRITY_FAIL to INTEGRITY_UNKNOWN in case of
evm_find_protected_xattrs() return -EOPNOTSUPP.
This is same situation in evm_verify_hmac() what we have with
vfs_getxattr_alloc() return -EOPNOTSUPP.

> > # getfattr -m . -d -e hex /tmp/test
> > (no output, file don't have any xattrs)
> > # evmctl sign -a sha512 --imasig --key /privkey_ima.pem /tmp/test
> >   setxattr failed: /tmp/test
> >   errno: Operation not permitted (1)  
> 
> If the above Kconfig options weren't enabled, then you wouldn't be
> able to write any security xattrs (eg. security.foo).  This would be
> independent of the EVM/IMA.
> 
> On Fedora, "touch" creates the selinux label, which results in an EVM
> label as well.  Everything works as expected.  

I don't use SELinux, with "dont_appraise" policy "touch" don't creates 
any xattrs that should be protected. And, since I see -EOPNOTSUPP error
from getxattr(), this mean ->list, ->get, and ->set xattr handler
operations removed in inode and I have disabled xattr support in inode.
Have no idea why it's so, this is probably some kind of xattrs related
work issue.

I tested "security.foo" xattr, but this is not working. Looks like this
work only with xattrs that should be protected by EVM.

In case of "appraise" policy, "touch" creates ima xattr, and I can work
with attr/xattr changes.

I think, we probably also have some xattrs related issue in EVM
code, since FS remove xattr support from inode that EVM only work
with (in case of "dont_appraise", without SELinux and so on). In the
same time I don't have any issues if EVM disabled.

> When I get back from
> vacation, I'll set up a test environment.

This would be nice, Mimi.

-- 
Best regards,
Mikhail Kurinnoi