On Tue, 2024-01-02 at 12:56 +0100, Roberto Sassu wrote:

On 12/26/2023 11:13 PM, Mimi Zohar wrote:

quoted

On Thu, 2023-12-14 at 18:08 +0100, Roberto Sassu wrote:

quoted

From: Roberto Sassu <roberto.sassu@huawei.com>

As for IMA, move hardcoded EVM function calls from various places in the
kernel to the LSM infrastructure, by introducing a new LSM named 'evm'
(last and always enabled like 'ima'). The order in the Makefile ensures
that 'evm' hooks are executed after 'ima' ones.

Make EVM functions as static (except for evm_inode_init_security(), which
is exported), and register them as hook implementations in init_evm_lsm().

Unlike before (see commit to move IMA to the LSM infrastructure),
evm_inode_post_setattr(), evm_inode_post_set_acl(),
evm_inode_post_remove_acl(), and evm_inode_post_removexattr() are not
executed for private inodes.

Missing is a comment on moving the inline function definitions -
evm_inode_remove_acl(), evm_inode_post_remove_acl(), and
evm_inode_post_set_acl() - to evm_main.c.

Ok.

quoted

quoted

Finally, add the LSM_ID_EVM case in lsm_list_modules_test.c

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---

[...]

quoted

@@ -2307,9 +2299,7 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
  
  	if (ret == 1)
  		ret = cap_inode_setxattr(dentry, name, value, size, flags);
-	if (ret)
-		return ret;
-	return evm_inode_setxattr(idmap, dentry, name, value, size, flags);
+	return ret;
  }

Even though capability will be called after EVM, it doesn't make a
difference in this instance.

[...]

quoted

  /**

@@ -2493,9 +2472,7 @@ int security_inode_removexattr(struct mnt_idmap *idmap,
  	ret = call_int_hook(inode_removexattr, 1, idmap, dentry, name);
  	if (ret == 1)
  		ret = cap_inode_removexattr(idmap, dentry, name);
-	if (ret)
-		return ret;
-	return evm_inode_removexattr(idmap, dentry, name);
+	return ret;
  }

'security.capability' is one of the EVM protected xattrs.  As
capability isn't an LSM, it will now be called after EVM, which is a
problem.

Uhm, according to this comment in security_inode_removexattr() and 
security_inode_setxattr():

	/*
	 * SELinux and Smack integrate the cap call,
	 * so assume that all LSMs supplying this call do so.
	 */

We can add the call to IMA and EVM as well, to be compliant.

SELinux and Smack are the only current LSMs that register the
security_inode_removexattr hook.  Both enforce mandatory access
control,
so their calling capabilities to enforce DAC kind of makes sense.  I'm
not sure it makes sense for IMA and EVM to call capability directly,
just because of the comment.

However, I'm missing why the two cases are different. It seems 
cap_inode_set/removexattr() are doing just checks.

Both IMA and EVM require CAP_SYS_ADMIN to write/remove security.ima and
security.evm respectively.  In addition, EVM must recalculate
security.evm if any protected security xattrs are set or
removed.   However, security.evm is updated on
security_inode_post_setxattr, not security_inode_setxattr.

Mimi