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

[Mikhail K. <vie...@vi...>]

В 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.


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;