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

[Serge E. H. <se...@ha...>]

Quoting Mimi Zohar (zo...@li...):
> [Cc'ing Serge]
> 
> On Wed, 2017-01-04 at 18:17 +0300, Mikhail Kurinnoi wrote:
> > В Wed, 04 Jan 2017 09:15:47 -0500
> > Mimi Zohar <zo...@li...> пишет:
> > 
> > > On Wed, 2017-01-04 at 05:49 +0300, Mikhail Kurinnoi wrote:
> > > > В Tue, 03 Jan 2017 18:42:34 -0500
> > > > Mimi Zohar <zo...@li...> пишет:
> > > >   
> > > > > On Tue, 2017-01-03 at 21:03 +0300, Mikhail Kurinnoi wrote:  
> > > > > > В Tue, 03 Jan 2017 08:13:42 -0500
> > > > > > Mimi Zohar <zo...@li...> пишет:    
> > > > >   
> > > > > > > > Let me fix this misunderstanding by tests.
> > > > > > > > Test with default code:
> > > > > > > > 
> > > > > > > > # touch /var/tmp/test
> > > > > > > > # getfattr -m . -d -e hex /var/tmp/test
> > > > > > > > (no output, file don't have any xattrs)
> > > > > > > > # setfattr -n security.foo -v "123" /var/tmp/test      
> > > > > > > 
> > > > > > > Right, the ops exist.
> > > > > > >     
> > > > > > > > # getfattr -m . -d -e hex /var/tmp/test
> > > > > > > >     getfattr: Removing leading '/' from absolute path names
> > > > > > > >     # file: var/tmp/test
> > > > > > > >     security.foo=0x313233
> > > > > > > > # chmod 666 /var/tmp/test
> > > > > > > >     chmod: changing permissions of '/var/tmp/test':
> > > > > > > > Operation not permitted      
> > > > > > > 
> > > > > > > All files in the IMA appraise policy require an IMA hash or
> > > > > > > signature. With the "ima_appraise_policy", all files owned by
> > > > > > > root must be appraised either based on the file hash or file
> > > > > > > signature.  Other policies might require file signatures.    
> > > > > > 
> > > > > > 
> > > > > > I think the issue also related to evm_calc_hmac_or_hash().  
> > > 
> > > > > > Before cycle, we set error to -ENODATA. In the same time, if we
> > > > > > don't have any xattrs for protection, error will never changed
> > > > > > from -ENODATA to 0. So, we will return -ENODATA error into
> > > > > > evm_update_evmxattr(). In this case, evm_update_evmxattr() will
> > > > > > not protect uid/gid/mode by security.evm, but remove
> > > > > > security.evm instead, if it's available.    
> > > > > 
> > > > > For files not in policy (there aren't any xattrs), I was able to
> > > > > do the chmod without problems.
> > > > > 
> > > > > # touch /var/tmp/test
> > > > > # getfattr -m ^security -e hex --dump /var/tmp/test
> > > > > # chmod 666 /var/tmp/test
> > > > > # getfattr -m ^security -e hex --dump /var/tmp/test
> > > > > # ls -lat /var/tmp/test
> > > > > -rw-rw-rw- 1 root root 0 Jan  3 18:16 /var/tmp/test
> > > > >
> > > > > We've already shown that you can write security xattrs
> > > > > (security.foo). So it isn't an issue that the xattr ops don't
> > > > > exist.  Why isn't it returning INTEGRITY_NOXATTRS?  
> > > > 
> > > > I have exactly same question, and I still don't have answer for it.
> > > > It must returning INTEGRITY_NOXATTRS or don't be able set any
> > > > "security." xattrs. 
> > > > Plus, we should have -EOPNOTSUPP error returned by
> > > > vfs_getxattr_alloc() if we have xattrs support issue, I don't
> > > > understand why -EOPNOTSUPP error returned by
> > > > evm_find_protected_xattrs() instead.  
> > > 
> > >  Unlike SELinux, it seems that kernel/commoncaps:
> > > get_vfs_caps_from_disk() treats -ENODATA and -EOPNOTSUPP the same, as
> > > though there is no data.  Could you see if modifying
> > > evm_find_protected_xattrs() like below fixes the problem?
> > > 
> > >                if (error < 0) {
> > >                         if (error == -ENODATA || error == -EOPNOTSUPP)
> > >                                 continue;
> > >                         return error;
> > >                 }
> > >                 count++;
> > > 
> > 
> > Mimi, I can confirm that this code modification fix the issue.
> > I can create now files without xattrs and use chmod on both - tmpfs
> > and ext4 partitions covered by "dont_appraise" policy.
> 
> Really strange that getxattr for capabilities returns -EOPNOTSUPP, even
> though the inode->i_opflags supports xattrs.  Serge, any insights?

cap_inode_getxattr and cap_inode_getsecurity don't exist, so the EOPNOTSUPP
is actually returned by securit/security.c itself i think.

I suspect if you check a kernel before the xattr rewrite in Sep 2016 you'll
find it behaved differently?