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

[Mimi Z. <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?

Mimi