On Wed, 2021-04-28 at 15:35 +0000, Roberto Sassu wrote:

quoted

From: Roberto Sassu
Sent: Friday, March 5, 2021 4:19 PM
ima_inode_setxattr() and ima_inode_removexattr() hooks are called before
an
operation is performed. Thus, ima_reset_appraise_flags() should not be
called there, as flags might be unnecessarily reset if the operation is
denied.

This patch introduces the post hooks ima_inode_post_setxattr() and
ima_inode_post_removexattr(), and adds the call to
ima_reset_appraise_flags() in the new functions.

Cc: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 fs/xattr.c                            |  2 ++
 include/linux/ima.h                   | 18 ++++++++++++++++++
 security/integrity/ima/ima_appraise.c | 25 ++++++++++++++++++++++---
 security/security.c                   |  1 +
 4 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index b3444e06cded..81847f132d26 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c

@@ -16,6 +16,7 @@
 #include <linux/namei.h>
 #include <linux/security.h>
 #include <linux/evm.h>
+#include <linux/ima.h>
 #include <linux/syscalls.h>
 #include <linux/export.h>
 #include <linux/fsnotify.h>

@@ -502,6 +503,7 @@ __vfs_removexattr_locked(struct user_namespace

*mnt_userns,

 	if (!error) {
 		fsnotify_xattr(dentry);
+		ima_inode_post_removexattr(dentry, name);
 		evm_inode_post_removexattr(dentry, name);
 	}

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 61d5723ec303..5e059da43857 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h

@@ -171,7 +171,13 @@ extern void ima_inode_post_setattr(struct

user_namespace *mnt_userns,
 				   struct dentry *dentry);
 extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		       const void *xattr_value, size_t xattr_value_len);
+extern void ima_inode_post_setxattr(struct dentry *dentry,
+				    const char *xattr_name,
+				    const void *xattr_value,
+				    size_t xattr_value_len);
 extern int ima_inode_removexattr(struct dentry *dentry, const char
*xattr_name);
+extern void ima_inode_post_removexattr(struct dentry *dentry,
+				       const char *xattr_name);
 #else
 static inline bool is_ima_appraise_enabled(void)
 {

@@ -192,11 +198,23 @@ static inline int ima_inode_setxattr(struct dentry

*dentry,
 	return 0;
 }

+static inline void ima_inode_post_setxattr(struct dentry *dentry,
+					   const char *xattr_name,
+					   const void *xattr_value,
+					   size_t xattr_value_len)
+{
+}
+
 static inline int ima_inode_removexattr(struct dentry *dentry,
 					const char *xattr_name)
 {
 	return 0;
 }
+
+static inline void ima_inode_post_removexattr(struct dentry *dentry,
+					      const char *xattr_name)
+{
+}
 #endif /* CONFIG_IMA_APPRAISE */

 #if defined(CONFIG_IMA_APPRAISE) &&
defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)

diff --git a/security/integrity/ima/ima_appraise.c

b/security/integrity/ima/ima_appraise.c
index 565e33ff19d0..1f029e4c8d7f 100644

--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c

@@ -577,21 +577,40 @@ int ima_inode_setxattr(struct dentry *dentry, const

char *xattr_name,
 	if (result == 1) {
 		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
 			return -EINVAL;
-		ima_reset_appraise_flags(d_backing_inode(dentry),
-			xvalue->type == EVM_IMA_XATTR_DIGSIG);
 		result = 0;
 	}
 	return result;
 }

+void ima_inode_post_setxattr(struct dentry *dentry, const char
*xattr_name,
+			     const void *xattr_value, size_t xattr_value_len)
+{
+	const struct evm_ima_xattr_data *xvalue = xattr_value;
+	int result;
+
+	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
+				   xattr_value_len);
+	if (result == 1)
+		ima_reset_appraise_flags(d_backing_inode(dentry),

I found an issue in this patch.

Moving ima_reset_appraise_flags() to the post hook causes this
function to be executed also when __vfs_setxattr_noperm() is
called.

The problem is that at the end of a write IMA calls
ima_collect_measurement() to recalculate the file digest and
update security.ima. ima_collect_measurement() sets
IMA_COLLECTED.

However, after that __vfs_setxattr_noperm() causes
IMA_COLLECTED to be reset, and to unnecessarily recalculate
the file digest. This wouldn't happen if ima_reset_appraise_flags()
is in the pre hook.

I solved by replacing:
	iint->flags &= ~IMA_DONE_MASK;
with:
	iint->flags &= ~(IMA_DONE_MASK & ~IMA_COLLECTED);

just when the IMA_CHANGE_XATTR bit is set. It should
not be a problem since setting an xattr does not influence
the file content.

Mimi, what do you think?

Thank yor for noticing this.

Without seeing the actual change it is hard to tell.   The only place
that "iint->flags &= ~IMA_DONE_MASK;" occurs is in neither of the above
functions, but in process_measurement().  There it is a part of a
compound "if" statement.  Perhaps it would be ok to change it for just
the IMA_CHANGE_XATTR test, but definitely not for the other conditions,
like untrusted mounts.

Moving ima_reset_appraise_flags() to the post hooks is to minimize
resetting the flags unnecessarily.  That is really a performance fix,
not something necessary for making the EVM portable & immutable
signatures more usable.  As much as possible, please minimize the
changes to facilitate review and testing.

thanks,

Mimi