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

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

Thanks!

Mimi


> I look at EVM code carefully and found, that this code was created to
> be used with SELinux/SMACK only, just look at
> security_inode_init_security(), evm_inode_init_security(),
> evm_init_hmac() or evm_calc_hmac_or_hash() - all this code count on
> SELinux/SMAC hooks during inode creation (and prevent EVM inode init
> if hooks is not set) and request at least one protected xattr that
> should be on inode all the time. This could be changed in easy way and
> make code universal:
> 1) We should correct evm_calc_hmac_or_hash() in order to work without
> protected xattrs, don't return -ENODATA if cycle is over successfully.
> 2) EVM should also work with new inode in
> security_inode_init_security() even if we don't have SELinux or any
> other xattr based lsm. Please note, security_inode_init_security()
> don't return -EOPNOTSUPP error, it return 0 instead. So, we safe to
> call evm_inode_init_security() if inode_init_security hook return
> -EOPNOTSUPP.
> 3) evm_inode_init_security() and evm_init_hmac() should count, that we
> may don't provide lsm_xattr in case we don't have xattr based lsm
> enabled.
> 
> After this changes, EVM will work with files covered by
> "dont_appraise" policy or not in policy in the same way as it work
> with SELinux enabled.
> This patch fix issue in the way that EVM work properly now for me too
> even with SELinux disabled. I did all previous tests and don't faced
> with any issues, I also don't see any errors in syslog now.
> 
> 
> Signed-off-by: Mikhail Kurinnoi <vie...@vi...>
> 
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -227,6 +227,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>  		xattr_size = size;
>  		crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size);
>  	}
> +	error = 0;
>  	if (type == EVM_IMA_XATTR_DIGSIG)
>  		hmac_add_misc_digsig(desc, inode, digest);
>  	else
> @@ -292,7 +293,8 @@ int evm_init_hmac(struct inode *inode
>  		return PTR_ERR(desc);
>  	}
>  
> -	crypto_shash_update(desc, lsm_xattr->value, lsm_xattr->value_len);
> +	if (lsm_xattr)
> +		crypto_shash_update(desc, lsm_xattr->value, lsm_xattr->value_len);
>  	hmac_add_misc(desc, inode, hmac_val);
>  	kfree(desc);
>  	return 0;
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -470,7 +526,7 @@ int evm_inode_init_security(struct inode *inode,
>  	struct evm_ima_xattr_data *xattr_data;
>  	int rc;
>  
> -	if (!evm_initialized || !evm_protected_xattr(lsm_xattr->name))
> +	if (!evm_initialized || (lsm_xattr && !evm_protected_xattr(lsm_xattr->name)))
>  		return 0;
>  
>  	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
> --- a/security/security.c
> +++ b/security/security.c
> @@ -384,10 +384,19 @@ int security_inode_init_security(struct inode *inode
>  						&lsm_xattr->name,
>  						&lsm_xattr->value,
>  						&lsm_xattr->value_len);
> -	if (ret)
> +	if (ret && (ret != -EOPNOTSUPP))
>  		goto out;
> -
> -	evm_xattr = lsm_xattr + 1;
> +	
> +	if (ret == -EOPNOTSUPP) {
> +		if (lsm_xattr->value != NULL)
> +			kfree(lsm_xattr->value);
> +		memset(new_xattrs, 0, sizeof(new_xattrs));
> +		lsm_xattr = NULL;
> +		evm_xattr = new_xattrs;
> +	}
> +	else
> +		evm_xattr = lsm_xattr + 1;
> +	
>  	ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
>  	if (ret)
>  		goto out;
>