[PATCH v4 00/11] evm: Improve usability of portable signatures

[PATCH v4 00/11] evm: Improve usability of portable signatures

[Roberto Sassu]

EVM portable signatures are particularly suitable for the protection of
metadata of immutable files where metadata is signed by a software vendor.
They can be used for example in conjunction with an IMA policy that
appraises only executed and memory mapped files.

However, some usability issues are still unsolved, especially when EVM is
used without loading an HMAC key. This patch set attempts to fix the open
issues.

Patch 1 allows EVM to be used without loading an HMAC key. Patch 2 avoids
appraisal verification of public keys (they are already verified by the key
subsystem).

Patches 3-5 allow metadata verification to be turned off when no HMAC key
is loaded and to use this mode in a safe way (by ensuring that IMA
revalidates metadata when there is a change).

Patches 6-8 make portable signatures more usable if metadata verification
is not turned off, by ignoring the INTEGRITY_NOLABEL and INTEGRITY_NOXATTS
errors when possible, by accepting any metadata modification until
signature verification succeeds (useful when xattrs/attrs are copied
sequentially from a source) and by allowing operations that don't change
metadata.

Patch 9 makes it possible to use portable signatures when the IMA policy
requires file signatures and patch 10 shows portable signatures in the
measurement list when the ima-sig template is selected.

Lastly, patch 11 avoids undesired removal of security.ima when a file is
not selected by the IMA policy.

Changelog

v3:
- introduce evm_ignore_error_safe() to correctly ignore INTEGRITY_NOLABEL
  and INTEGRITY_NOXATTRS errors
- fix an error in evm_xattr_acl_change()
- replace #ifndef with !IS_ENABLED() in integrity_load_keys()
- reintroduce ima_inode_removexattr()
- adapt patches to apply on top of the idmapped mounts patch set

v2:
- replace EVM_RESET_STATUS flag with evm_status_revalidate()
- introduce IMA post hooks ima_inode_post_setxattr() and
  ima_inode_post_removexattr()
- remove ima_inode_removexattr()
- ignore INTEGRITY_NOLABEL error if the HMAC key is not loaded

v1:
- introduce EVM_RESET_STATUS integrity flag instead of clearing IMA flag
- introduce new template field evmsig
- add description of evm_xattr_acl_change() and evm_xattr_change()

Roberto Sassu (11):
  evm: Execute evm_inode_init_security() only when an HMAC key is loaded
  evm: Load EVM key in ima_load_x509() to avoid appraisal
  evm: Refuse EVM_ALLOW_METADATA_WRITES only if an HMAC key is loaded
  ima: Move ima_reset_appraise_flags() call to post hooks
  evm: Introduce evm_status_revalidate()
  evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are
    safe
  evm: Allow xattr/attr operations for portable signatures
  evm: Allow setxattr() and setattr() for unmodified metadata
  ima: Allow imasig requirement to be satisfied by EVM portable
    signatures
  ima: Introduce template field evmsig and write to field sig as
    fallback
  ima: Don't remove security.ima if file must not be appraised

 Documentation/ABI/testing/evm             |   5 +-
 Documentation/security/IMA-templates.rst  |   4 +-
 fs/xattr.c                                |   2 +
 include/linux/evm.h                       |   6 +
 include/linux/ima.h                       |  18 +++
 include/linux/integrity.h                 |   1 +
 security/integrity/evm/evm_main.c         | 188 ++++++++++++++++++++--
 security/integrity/evm/evm_secfs.c        |   4 +-
 security/integrity/iint.c                 |   4 +-
 security/integrity/ima/ima_appraise.c     |  55 +++++--
 security/integrity/ima/ima_init.c         |   4 +
 security/integrity/ima/ima_template.c     |   2 +
 security/integrity/ima/ima_template_lib.c |  33 +++-
 security/integrity/ima/ima_template_lib.h |   2 +
 security/security.c                       |   1 +
 15 files changed, 297 insertions(+), 32 deletions(-)

-- 
2.26.2

[PATCH v4 01/11] evm: Execute evm_inode_init_security() only when an HMAC key is loaded

[Roberto Sassu]

evm_inode_init_security() requires an HMAC key to calculate the HMAC on
initial xattrs provided by LSMs. However, it checks generically whether a
key has been loaded, including also public keys, which is not correct as
public keys are not suitable to calculate the HMAC.

Originally, support for signature verification was introduced to verify a
possibly immutable initial ram disk, when no new files are created, and to
switch to HMAC for the root filesystem. By that time, an HMAC key should
have been loaded and usable to calculate HMACs for new files.

More recently support for requiring an HMAC key was removed from the
kernel, so that signature verification can be used alone. Since this is a
legitimate use case, evm_inode_init_security() should not return an error
when no HMAC key has been loaded.

This patch fixes this problem by replacing the evm_key_loaded() check with
a check of the EVM_INIT_HMAC flag in evm_initialized.

Cc: stable@vger.kernel.org # 4.5.x
Fixes: 26ddabfe96b ("evm: enable EVM when X509 certificate is loaded")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/evm/evm_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 0de367aaa2d3..7ac5204c8d1f 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -521,7 +521,7 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
 }
 
 /*
- * evm_inode_init_security - initializes security.evm
+ * evm_inode_init_security - initializes security.evm HMAC value
  */
 int evm_inode_init_security(struct inode *inode,
 				 const struct xattr *lsm_xattr,
@@ -530,7 +530,8 @@ int evm_inode_init_security(struct inode *inode,
 	struct evm_xattr *xattr_data;
 	int rc;
 
-	if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name))
+	if (!(evm_initialized & EVM_INIT_HMAC) ||
+	    !evm_protected_xattr(lsm_xattr->name))
 		return 0;
 
 	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
-- 
2.26.2

[PATCH v4 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal

[Roberto Sassu]

The public builtin keys do not need to be appraised by IMA as the
restriction on the IMA/EVM trusted keyrings ensures that a key can be
loaded only if it is signed with a key on the builtin or secondary
keyrings.

However, when evm_load_x509() is called, appraisal is already enabled and
a valid IMA signature must be added to the EVM key to pass verification.

Since the restriction is applied on both IMA and EVM trusted keyrings, it
is safe to disable appraisal also when the EVM key is loaded. This patch
calls evm_load_x509() inside ima_load_x509() if CONFIG_IMA_LOAD_X509 is
enabled, which crosses the normal IMA and EVM boundary.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/iint.c         | 4 +++-
 security/integrity/ima/ima_init.c | 4 ++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 1d20003243c3..c487e74a35fb 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -200,7 +200,9 @@ int integrity_kernel_read(struct file *file, loff_t offset,
 void __init integrity_load_keys(void)
 {
 	ima_load_x509();
-	evm_load_x509();
+
+	if (!IS_ENABLED(CONFIG_IMA_LOAD_X509))
+		evm_load_x509();
 }
 
 static int __init integrity_fs_init(void)
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 6e8742916d1d..5076a7d9d23e 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -108,6 +108,10 @@ void __init ima_load_x509(void)
 
 	ima_policy_flag &= ~unset_flags;
 	integrity_load_x509(INTEGRITY_KEYRING_IMA, CONFIG_IMA_X509_PATH);
+
+	/* load also EVM key to avoid appraisal */
+	evm_load_x509();
+
 	ima_policy_flag |= unset_flags;
 }
 #endif
-- 
2.26.2

[PATCH v4 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if an HMAC key is loaded

[Roberto Sassu]

EVM_ALLOW_METADATA_WRITES is an EVM initialization flag that can be set to
temporarily disable metadata verification until all xattrs/attrs necessary
to verify an EVM portable signature are copied to the file. This flag is
cleared when EVM is initialized with an HMAC key, to avoid that the HMAC is
calculated on unverified xattrs/attrs.

Currently EVM unnecessarily denies setting this flag if EVM is initialized
with a public key, which is not a concern as it cannot be used to trust
xattrs/attrs updates. This patch removes this limitation.

Cc: stable@vger.kernel.org # 4.16.x
Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 Documentation/ABI/testing/evm      | 5 +++--
 security/integrity/evm/evm_secfs.c | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/evm b/Documentation/ABI/testing/evm
index 3c477ba48a31..eb6d70fd6fa2 100644
--- a/Documentation/ABI/testing/evm
+++ b/Documentation/ABI/testing/evm
@@ -49,8 +49,9 @@ Description:
 		modification of EVM-protected metadata and
 		disable all further modification of policy
 
-		Note that once a key has been loaded, it will no longer be
-		possible to enable metadata modification.
+		Note that once an HMAC key has been loaded, it will no longer
+		be possible to enable metadata modification and, if it is
+		already enabled, it will be disabled.
 
 		Until key loading has been signaled EVM can not create
 		or validate the 'security.evm' xattr, but returns
diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
index bbc85637e18b..197a4b83e534 100644
--- a/security/integrity/evm/evm_secfs.c
+++ b/security/integrity/evm/evm_secfs.c
@@ -81,10 +81,10 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf,
 		return -EINVAL;
 
 	/* Don't allow a request to freshly enable metadata writes if
-	 * keys are loaded.
+	 * an HMAC key is loaded.
 	 */
 	if ((i & EVM_ALLOW_METADATA_WRITES) &&
-	    ((evm_initialized & EVM_KEY_MASK) != 0) &&
+	    ((evm_initialized & EVM_INIT_HMAC) != 0) &&
 	    !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
 		return -EPERM;
 
-- 
2.26.2

[PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Roberto Sassu]

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),
+			xvalue->type == EVM_IMA_XATTR_DIGSIG);
+}
+
 int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
 {
 	int result;
 
 	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
 	if (result == 1) {
-		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
 		result = 0;
 	}
 	return result;
 }
+
+void ima_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
+{
+	int result;
+
+	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
+	if (result == 1)
+		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
+}
diff --git a/security/security.c b/security/security.c
index 5ac96b16f8fa..efb1f874dc41 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1319,6 +1319,7 @@ void security_inode_post_setxattr(struct dentry *dentry, const char *name,
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return;
 	call_void_hook(inode_post_setxattr, dentry, name, value, size, flags);
+	ima_inode_post_setxattr(dentry, name, value, size);
 	evm_inode_post_setxattr(dentry, name, value, size);
 }
 
-- 
2.26.2

[PATCH v4 05/11] evm: Introduce evm_status_revalidate()

[Roberto Sassu]

When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
metadata. Its main purpose is to allow users to freely set metadata when it
is protected by a portable signature, until an HMAC key is loaded.

However, callers of evm_verifyxattr() are not notified about metadata
changes and continue to rely on the last status returned by the function.
For example IMA, since it caches the appraisal result, will not call again
evm_verifyxattr() until the appraisal flags are cleared, and will grant
access to the file even if there was a metadata operation that made the
portable signature invalid.

This patch introduces evm_status_revalidate(), which callers of
evm_verifyxattr() can use in their xattr post hooks to determine whether
re-validation is necessary and to do the proper actions. IMA calls it in
its xattr post hooks to reset the appraisal flags, so that the EVM status
is re-evaluated after a metadata operation.

Lastly, this patch also adds a call to evm_reset_status() in
evm_inode_post_setattr() to invalidate the cached EVM status after a
setattr operation.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/evm.h                   |  6 +++++
 security/integrity/evm/evm_main.c     | 33 +++++++++++++++++++++++----
 security/integrity/ima/ima_appraise.c |  8 ++++---
 3 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/include/linux/evm.h b/include/linux/evm.h
index 8302bc29bb35..e5b7bcb152b9 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -35,6 +35,7 @@ extern void evm_inode_post_removexattr(struct dentry *dentry,
 extern int evm_inode_init_security(struct inode *inode,
 				   const struct xattr *xattr_array,
 				   struct xattr *evm);
+extern bool evm_status_revalidate(const char *xattr_name);
 #ifdef CONFIG_FS_POSIX_ACL
 extern int posix_xattr_acl(const char *xattrname);
 #else
@@ -104,5 +105,10 @@ static inline int evm_inode_init_security(struct inode *inode,
 	return 0;
 }
 
+static inline bool evm_status_revalidate(const char *xattr_name)
+{
+	return false;
+}
+
 #endif /* CONFIG_EVM */
 #endif /* LINUX_EVM_H */
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 7ac5204c8d1f..998818283fda 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -425,6 +425,30 @@ static void evm_reset_status(struct inode *inode)
 		iint->evm_status = INTEGRITY_UNKNOWN;
 }
 
+/**
+ * evm_status_revalidate - report whether EVM status re-validation is necessary
+ * @xattr_name: pointer to the affected extended attribute name
+ *
+ * Report whether callers of evm_verifyxattr() should re-validate the
+ * EVM status.
+ *
+ * Return true if re-validation is necessary, false otherwise.
+ */
+bool evm_status_revalidate(const char *xattr_name)
+{
+	if (!evm_key_loaded())
+		return false;
+
+	/* evm_inode_post_setattr() passes NULL */
+	if (!xattr_name)
+		return true;
+
+	if (!evm_protected_xattr(xattr_name) && !posix_xattr_acl(xattr_name))
+		return false;
+
+	return true;
+}
+
 /**
  * evm_inode_post_setxattr - update 'security.evm' to reflect the changes
  * @dentry: pointer to the affected dentry
@@ -441,8 +465,7 @@ static void evm_reset_status(struct inode *inode)
 void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
 			     const void *xattr_value, size_t xattr_value_len)
 {
-	if (!evm_key_loaded() || (!evm_protected_xattr(xattr_name)
-				  && !posix_xattr_acl(xattr_name)))
+	if (!evm_status_revalidate(xattr_name))
 		return;
 
 	evm_reset_status(dentry->d_inode);
@@ -462,7 +485,7 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
  */
 void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
 {
-	if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
+	if (!evm_status_revalidate(xattr_name))
 		return;
 
 	evm_reset_status(dentry->d_inode);
@@ -513,9 +536,11 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
  */
 void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
 {
-	if (!evm_key_loaded())
+	if (!evm_status_revalidate(NULL))
 		return;
 
+	evm_reset_status(dentry->d_inode);
+
 	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
 		evm_update_evmxattr(dentry, NULL, NULL, 0);
 }
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 1f029e4c8d7f..d4b8db1acadd 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -586,13 +586,15 @@ 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 digsig = 0;
 	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),
-			xvalue->type == EVM_IMA_XATTR_DIGSIG);
+		digsig = (xvalue->type == EVM_IMA_XATTR_DIGSIG);
+	if (result == 1 || evm_status_revalidate(xattr_name))
+		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
 }
 
 int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
@@ -611,6 +613,6 @@ void ima_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
 	int result;
 
 	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
-	if (result == 1)
+	if (result == 1 || evm_status_revalidate(xattr_name))
 		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
 }
-- 
2.26.2

[PATCH v4 06/11] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe

[Roberto Sassu]

When a file is being created, LSMs can set the initial label with the
inode_init_security hook. If no HMAC key is loaded, the new file will have
LSM xattrs but not the HMAC. It is also possible that the file remains
without protected xattrs after creation if no active LSM provided it.

Unfortunately, EVM will deny any further metadata operation on new files,
as evm_protect_xattr() will always return the INTEGRITY_NOLABEL error, or
INTEGRITY_NOXATTRS if no protected xattrs exist. This would limit the
usability of EVM when only a public key is loaded, as commands such as cp
or tar with the option to preserve xattrs won't work.

This patch ignores these errors when they won't be an issue, if no HMAC key
is loaded and cannot be loaded in the future (which can be enforced by
setting the EVM_SETUP_COMPLETE initialization flag).

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_main.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 998818283fda..6556e8c22da9 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -90,6 +90,24 @@ static bool evm_key_loaded(void)
 	return (bool)(evm_initialized & EVM_KEY_MASK);
 }
 
+/*
+ * Ignoring INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS is safe if no HMAC key
+ * is loaded and the EVM_SETUP_COMPLETE initialization flag is set.
+ */
+static bool evm_ignore_error_safe(enum integrity_status evm_status)
+{
+	if (evm_initialized & EVM_INIT_HMAC)
+		return false;
+
+	if (!(evm_initialized & EVM_SETUP_COMPLETE))
+		return false;
+
+	if (evm_status != INTEGRITY_NOLABEL && evm_status != INTEGRITY_NOXATTRS)
+		return false;
+
+	return true;
+}
+
 static int evm_find_protected_xattrs(struct dentry *dentry)
 {
 	struct inode *inode = d_backing_inode(dentry);
@@ -354,6 +372,8 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
 				    -EPERM, 0);
 	}
 out:
+	if (evm_ignore_error_safe(evm_status))
+		return 0;
 	if (evm_status != INTEGRITY_PASS)
 		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 				    dentry->d_name.name, "appraise_metadata",
@@ -515,7 +535,8 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 		return 0;
 	evm_status = evm_verify_current_integrity(dentry);
 	if ((evm_status == INTEGRITY_PASS) ||
-	    (evm_status == INTEGRITY_NOXATTRS))
+	    (evm_status == INTEGRITY_NOXATTRS) ||
+	    (evm_ignore_error_safe(evm_status)))
 		return 0;
 	integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 			    dentry->d_name.name, "appraise_metadata",
-- 
2.26.2

[PATCH v4 07/11] evm: Allow xattr/attr operations for portable signatures

[Roberto Sassu]

If files with portable signatures are copied from one location to another
or are extracted from an archive, verification can temporarily fail until
all xattrs/attrs are set in the destination. Only portable signatures may
be moved or copied from one file to another, as they don't depend on
system-specific information such as the inode generation. Instead portable
signatures must include security.ima.

Unlike other security.evm types, EVM portable signatures are also
immutable. Thus, it wouldn't be a problem to allow xattr/attr operations
when verification fails, as portable signatures will never be replaced with
the HMAC on possibly corrupted xattrs/attrs.

This patch first introduces a new integrity status called
INTEGRITY_FAIL_IMMUTABLE, that allows callers of
evm_verify_current_integrity() to detect that a portable signature didn't
pass verification and then adds an exception in evm_protect_xattr() and
evm_inode_setattr() for this status and returns 0 instead of -EPERM.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 include/linux/integrity.h             |  1 +
 security/integrity/evm/evm_main.c     | 31 +++++++++++++++++++++------
 security/integrity/ima/ima_appraise.c |  2 ++
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index 2271939c5c31..2ea0f2f65ab6 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -13,6 +13,7 @@ enum integrity_status {
 	INTEGRITY_PASS = 0,
 	INTEGRITY_PASS_IMMUTABLE,
 	INTEGRITY_FAIL,
+	INTEGRITY_FAIL_IMMUTABLE,
 	INTEGRITY_NOLABEL,
 	INTEGRITY_NOXATTRS,
 	INTEGRITY_UNKNOWN,
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 6556e8c22da9..eab536fa260f 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -27,7 +27,8 @@
 int evm_initialized;
 
 static const char * const integrity_status_msg[] = {
-	"pass", "pass_immutable", "fail", "no_label", "no_xattrs", "unknown"
+	"pass", "pass_immutable", "fail", "fail_immutable", "no_label",
+	"no_xattrs", "unknown"
 };
 int evm_hmac_attrs;
 
@@ -155,7 +156,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 	enum integrity_status evm_status = INTEGRITY_PASS;
 	struct evm_digest digest;
 	struct inode *inode;
-	int rc, xattr_len;
+	int rc, xattr_len, evm_immutable = 0;
 
 	if (iint && (iint->evm_status == INTEGRITY_PASS ||
 		     iint->evm_status == INTEGRITY_PASS_IMMUTABLE))
@@ -200,8 +201,10 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		if (rc)
 			rc = -EINVAL;
 		break;
-	case EVM_IMA_XATTR_DIGSIG:
 	case EVM_XATTR_PORTABLE_DIGSIG:
+		evm_immutable = 1;
+		fallthrough;
+	case EVM_IMA_XATTR_DIGSIG:
 		/* accept xattr with non-empty signature field */
 		if (xattr_len <= sizeof(struct signature_v2_hdr)) {
 			evm_status = INTEGRITY_FAIL;
@@ -238,9 +241,12 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		break;
 	}
 
-	if (rc)
-		evm_status = (rc == -ENODATA) ?
-				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
+	if (rc) {
+		evm_status = INTEGRITY_NOXATTRS;
+		if (rc != -ENODATA)
+			evm_status = evm_immutable ?
+				     INTEGRITY_FAIL_IMMUTABLE : INTEGRITY_FAIL;
+	}
 out:
 	if (iint)
 		iint->evm_status = evm_status;
@@ -374,6 +380,14 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
 out:
 	if (evm_ignore_error_safe(evm_status))
 		return 0;
+
+	/*
+	 * Writing other xattrs is safe for portable signatures, as portable
+	 * signatures are immutable and can never be updated.
+	 */
+	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
+		return 0;
+
 	if (evm_status != INTEGRITY_PASS)
 		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 				    dentry->d_name.name, "appraise_metadata",
@@ -534,8 +548,13 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 	if (!(ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)))
 		return 0;
 	evm_status = evm_verify_current_integrity(dentry);
+	/*
+	 * Writing attrs is safe for portable signatures, as portable signatures
+	 * are immutable and can never be updated.
+	 */
 	if ((evm_status == INTEGRITY_PASS) ||
 	    (evm_status == INTEGRITY_NOXATTRS) ||
+	    (evm_status == INTEGRITY_FAIL_IMMUTABLE) ||
 	    (evm_ignore_error_safe(evm_status)))
 		return 0;
 	integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index d4b8db1acadd..24d59893aab0 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -416,6 +416,8 @@ int ima_appraise_measurement(enum ima_hooks func,
 	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
 		cause = "missing-HMAC";
 		goto out;
+	case INTEGRITY_FAIL_IMMUTABLE:
+		fallthrough;
 	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
 		cause = "invalid-HMAC";
 		goto out;
-- 
2.26.2

[PATCH v4 08/11] evm: Allow setxattr() and setattr() for unmodified metadata

[Roberto Sassu]

With the patch to allow xattr/attr operations if a portable signature
verification fails, cp and tar can copy all xattrs/attrs so that at the
end of the process verification succeeds.

However, it might happen that the xattrs/attrs are already set to the
correct value (taken at signing time) and signature verification succeeds
before the copy has completed. For example, an archive might contains files
owned by root and the archive is extracted by root.

Then, since portable signatures are immutable, all subsequent operations
fail (e.g. fchown()), even if the operation is legitimate (does not alter
the current value).

This patch avoids this problem by reporting successful operation to user
space when that operation does not alter the current value of xattrs/attrs.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_main.c | 96 +++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index eab536fa260f..a07516dcb920 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -18,6 +18,7 @@
 #include <linux/integrity.h>
 #include <linux/evm.h>
 #include <linux/magic.h>
+#include <linux/posix_acl_xattr.h>
 
 #include <crypto/hash.h>
 #include <crypto/hash_info.h>
@@ -328,6 +329,79 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
 	return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
 }
 
+/*
+ * evm_xattr_acl_change - check if passed ACL changes the inode mode
+ * @dentry: pointer to the affected dentry
+ * @xattr_name: requested xattr
+ * @xattr_value: requested xattr value
+ * @xattr_value_len: requested xattr value length
+ *
+ * Check if passed ACL changes the inode mode, which is protected by EVM.
+ *
+ * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
+ */
+static int evm_xattr_acl_change(struct dentry *dentry, const char *xattr_name,
+				const void *xattr_value, size_t xattr_value_len)
+{
+	umode_t mode;
+	struct posix_acl *acl = NULL, *acl_res;
+	struct inode *inode = d_backing_inode(dentry);
+	int rc;
+
+	/* UID/GID in ACL have been already converted from user to init ns */
+	acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);
+	if (!acl)
+		return 1;
+
+	acl_res = acl;
+	rc = posix_acl_update_mode(&init_user_ns, inode, &mode, &acl_res);
+
+	posix_acl_release(acl);
+
+	if (rc)
+		return 1;
+
+	if (inode->i_mode != mode)
+		return 1;
+
+	return 0;
+}
+
+/*
+ * evm_xattr_change - check if passed xattr value differs from current value
+ * @dentry: pointer to the affected dentry
+ * @xattr_name: requested xattr
+ * @xattr_value: requested xattr value
+ * @xattr_value_len: requested xattr value length
+ *
+ * Check if passed xattr value differs from current value.
+ *
+ * Returns 1 if passed xattr value differs from current value, 0 otherwise.
+ */
+static int evm_xattr_change(struct dentry *dentry, const char *xattr_name,
+			    const void *xattr_value, size_t xattr_value_len)
+{
+	char *xattr_data = NULL;
+	int rc = 0;
+
+	if (posix_xattr_acl(xattr_name))
+		return evm_xattr_acl_change(dentry, xattr_name, xattr_value,
+					    xattr_value_len);
+
+	rc = vfs_getxattr_alloc(&init_user_ns, dentry, xattr_name, &xattr_data,
+				0, GFP_NOFS);
+	if (rc < 0)
+		return 1;
+
+	if (rc == xattr_value_len)
+		rc = memcmp(xattr_value, xattr_data, rc);
+	else
+		rc = 1;
+
+	kfree(xattr_data);
+	return rc;
+}
+
 /*
  * evm_protect_xattr - protect the EVM extended attribute
  *
@@ -388,6 +462,10 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
 	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
 		return 0;
 
+	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
+	    !evm_xattr_change(dentry, xattr_name, xattr_value, xattr_value_len))
+		return 0;
+
 	if (evm_status != INTEGRITY_PASS)
 		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 				    dentry->d_name.name, "appraise_metadata",
@@ -527,6 +605,19 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
 	evm_update_evmxattr(dentry, xattr_name, NULL, 0);
 }
 
+static int evm_attr_change(struct dentry *dentry, struct iattr *attr)
+{
+	struct inode *inode = d_backing_inode(dentry);
+	unsigned int ia_valid = attr->ia_valid;
+
+	if ((!(ia_valid & ATTR_UID) || uid_eq(attr->ia_uid, inode->i_uid)) &&
+	    (!(ia_valid & ATTR_GID) || gid_eq(attr->ia_gid, inode->i_gid)) &&
+	    (!(ia_valid & ATTR_MODE) || attr->ia_mode == inode->i_mode))
+		return 0;
+
+	return 1;
+}
+
 /**
  * evm_inode_setattr - prevent updating an invalid EVM extended attribute
  * @dentry: pointer to the affected dentry
@@ -557,6 +648,11 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 	    (evm_status == INTEGRITY_FAIL_IMMUTABLE) ||
 	    (evm_ignore_error_safe(evm_status)))
 		return 0;
+
+	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
+	    !evm_attr_change(dentry, attr))
+		return 0;
+
 	integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 			    dentry->d_name.name, "appraise_metadata",
 			    integrity_status_msg[evm_status], -EPERM, 0);
-- 
2.26.2

[PATCH v4 09/11] ima: Allow imasig requirement to be satisfied by EVM portable signatures

[Roberto Sassu]

System administrators can require that all accessed files have a signature
by specifying appraise_type=imasig in a policy rule.

Currently, IMA signatures satisfy this requirement. Appended signatures may
also satisfy this requirement, but are not applicable as IMA signatures.
IMA/appended signatures ensure data source authentication for file content
and prevent any change. EVM signatures instead ensure data source
authentication for file metadata. Given that the digest or signature of the
file content must be included in the metadata, EVM signatures provide the
same file data guarantees of IMA signatures, as well as providing file
metadata guarantees.

This patch lets systems protected with EVM signatures pass appraisal
verification if the appraise_type=imasig requirement is specified in the
policy. This facilitates deployment in the scenarios where only EVM
signatures are available.

The patch makes the following changes:

file xattr types:
security.ima: IMA_XATTR_DIGEST/IMA_XATTR_DIGEST_NG
security.evm: EVM_XATTR_PORTABLE_DIGSIG

execve(), mmap(), open() behavior (with appraise_type=imasig):
before: denied (file without IMA signature, imasig requirement not met)
after: allowed (file with EVM portable signature, imasig requirement met)

open(O_WRONLY) behavior (without appraise_type=imasig):
before: allowed (file without IMA signature, not immutable)
after: denied (file with EVM portable signature, immutable)

In addition, similarly to IMA signatures, this patch temporarily allows
new files without or with incomplete metadata to be opened so that content
can be written.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/ima/ima_appraise.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 24d59893aab0..538ccbf972c8 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -242,12 +242,16 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 		hash_start = 1;
 		fallthrough;
 	case IMA_XATTR_DIGEST:
-		if (iint->flags & IMA_DIGSIG_REQUIRED) {
-			*cause = "IMA-signature-required";
-			*status = INTEGRITY_FAIL;
-			break;
+		if (*status != INTEGRITY_PASS_IMMUTABLE) {
+			if (iint->flags & IMA_DIGSIG_REQUIRED) {
+				*cause = "IMA-signature-required";
+				*status = INTEGRITY_FAIL;
+				break;
+			}
+			clear_bit(IMA_DIGSIG, &iint->atomic_flags);
+		} else {
+			set_bit(IMA_DIGSIG, &iint->atomic_flags);
 		}
-		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
 		if (xattr_len - sizeof(xattr_value->type) - hash_start >=
 				iint->ima_hash->length)
 			/*
@@ -417,6 +421,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 		cause = "missing-HMAC";
 		goto out;
 	case INTEGRITY_FAIL_IMMUTABLE:
+		set_bit(IMA_DIGSIG, &iint->atomic_flags);
 		fallthrough;
 	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
 		cause = "invalid-HMAC";
@@ -461,9 +466,12 @@ int ima_appraise_measurement(enum ima_hooks func,
 				status = INTEGRITY_PASS;
 		}
 
-		/* Permit new files with file signatures, but without data. */
+		/*
+		 * Permit new files with file/EVM portable signatures, but
+		 * without data.
+		 */
 		if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
-		    xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
+		    test_bit(IMA_DIGSIG, &iint->atomic_flags)) {
 			status = INTEGRITY_PASS;
 		}
 
@@ -595,6 +603,8 @@ void ima_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
 				   xattr_value_len);
 	if (result == 1)
 		digsig = (xvalue->type == EVM_IMA_XATTR_DIGSIG);
+	if (!strcmp(xattr_name, XATTR_NAME_EVM) && xattr_value_len > 0)
+		digsig = (xvalue->type == EVM_XATTR_PORTABLE_DIGSIG);
 	if (result == 1 || evm_status_revalidate(xattr_name))
 		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
 }
-- 
2.26.2

[PATCH v4 10/11] ima: Introduce template field evmsig and write to field sig as fallback

[Roberto Sassu]

With the patch to accept EVM portable signatures when the
appraise_type=imasig requirement is specified in the policy, appraisal can
be successfully done even if the file does not have an IMA signature.

However, remote attestation would not see that a different signature type
was used, as only IMA signatures can be included in the measurement list.
This patch solves the issue by introducing the new template field 'evmsig'
to show EVM portable signatures and by including its value in the existing
field 'sig' if the IMA signature is not found.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
---
 Documentation/security/IMA-templates.rst  |  4 ++-
 security/integrity/ima/ima_template.c     |  2 ++
 security/integrity/ima/ima_template_lib.c | 33 ++++++++++++++++++++++-
 security/integrity/ima/ima_template_lib.h |  2 ++
 4 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
index c5a8432972ef..9f3e86ab028a 100644
--- a/Documentation/security/IMA-templates.rst
+++ b/Documentation/security/IMA-templates.rst
@@ -70,9 +70,11 @@ descriptors by adding their identifier to the format string
    prefix is shown only if the hash algorithm is not SHA1 or MD5);
  - 'd-modsig': the digest of the event without the appended modsig;
  - 'n-ng': the name of the event, without size limitations;
- - 'sig': the file signature;
+ - 'sig': the file signature, or the EVM portable signature if the file
+   signature is not found;
  - 'modsig' the appended file signature;
  - 'buf': the buffer data that was used to generate the hash without size limitations;
+ - 'evmsig': the EVM portable signature;
 
 
 Below, there is the list of defined template descriptors:
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index e22e510ae92d..90e8a8282927 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -45,6 +45,8 @@ static const struct ima_template_field supported_fields[] = {
 	 .field_show = ima_show_template_digest_ng},
 	{.field_id = "modsig", .field_init = ima_eventmodsig_init,
 	 .field_show = ima_show_template_sig},
+	{.field_id = "evmsig", .field_init = ima_eventevmsig_init,
+	 .field_show = ima_show_template_sig},
 };
 
 /*
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index c022ee9e2a4e..4314d9a3514c 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -10,6 +10,7 @@
  */
 
 #include "ima_template_lib.h"
+#include <linux/xattr.h>
 
 static bool ima_template_hash_algo_allowed(u8 algo)
 {
@@ -438,7 +439,7 @@ int ima_eventsig_init(struct ima_event_data *event_data,
 	struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
 
 	if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
-		return 0;
+		return ima_eventevmsig_init(event_data, field_data);
 
 	return ima_write_template_field_data(xattr_value, event_data->xattr_len,
 					     DATA_FMT_HEX, field_data);
@@ -484,3 +485,33 @@ int ima_eventmodsig_init(struct ima_event_data *event_data,
 	return ima_write_template_field_data(data, data_len, DATA_FMT_HEX,
 					     field_data);
 }
+
+/*
+ *  ima_eventevmsig_init - include the EVM portable signature as part of the
+ *  template data
+ */
+int ima_eventevmsig_init(struct ima_event_data *event_data,
+			 struct ima_field_data *field_data)
+{
+	struct evm_ima_xattr_data *xattr_data = NULL;
+	int rc = 0;
+
+	if (!event_data->file)
+		return 0;
+
+	rc = vfs_getxattr_alloc(&init_user_ns, file_dentry(event_data->file),
+				XATTR_NAME_EVM, (char **)&xattr_data, 0,
+				GFP_NOFS);
+	if (rc <= 0)
+		return 0;
+
+	if (xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) {
+		kfree(xattr_data);
+		return 0;
+	}
+
+	rc = ima_write_template_field_data((char *)xattr_data, rc, DATA_FMT_HEX,
+					   field_data);
+	kfree(xattr_data);
+	return rc;
+}
diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
index 6b3b880637a0..f4b2a2056d1d 100644
--- a/security/integrity/ima/ima_template_lib.h
+++ b/security/integrity/ima/ima_template_lib.h
@@ -46,4 +46,6 @@ int ima_eventbuf_init(struct ima_event_data *event_data,
 		      struct ima_field_data *field_data);
 int ima_eventmodsig_init(struct ima_event_data *event_data,
 			 struct ima_field_data *field_data);
+int ima_eventevmsig_init(struct ima_event_data *event_data,
+			 struct ima_field_data *field_data);
 #endif /* __LINUX_IMA_TEMPLATE_LIB_H */
-- 
2.26.2

[PATCH v4 11/11] ima: Don't remove security.ima if file must not be appraised

[Roberto Sassu]

Files might come from a remote source and might have xattrs, including
security.ima. It should not be IMA task to decide whether security.ima
should be kept or not. This patch removes the removexattr() system
call in ima_inode_post_setattr().

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/ima/ima_appraise.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 538ccbf972c8..45e244fc2ef2 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -532,8 +532,6 @@ void ima_inode_post_setattr(struct user_namespace *mnt_userns,
 		return;
 
 	action = ima_must_appraise(mnt_userns, inode, MAY_ACCESS, POST_SETATTR);
-	if (!action)
-		__vfs_removexattr(&init_user_ns, dentry, XATTR_NAME_IMA);
 	iint = integrity_iint_find(inode);
 	if (iint) {
 		set_bit(IMA_CHANGE_ATTR, &iint->atomic_flags);
-- 
2.26.2

Re: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Casey Schaufler]

On 3/5/2021 7:19 AM, Roberto Sassu wrote:
> 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.

I don't see anything wrong with this patch in light of the way
IMA and EVM have been treated to date.

However ...

The special casing of IMA and EVM in security.c is getting out of
hand, and appears to be unnecessary. By my count there are 9 IMA
hooks and 5 EVM hooks that have been hard coded. Adding this IMA
hook makes 10. It would be really easy to register IMA and EVM as
security modules. That would remove the dependency they currently
have on security sub-system approval for changes like this one.
I know there has been resistance to "IMA as an LSM" in the past,
but it's pretty hard to see how it wouldn't be a win.

Short of that ...

Many of the places where IMA is invoked immediately invoke EVM
as well. Instead of:

	ima_do_stuff(x, y, z);
	evm_do_stuff(x, y, z);

how about

	integrity_do_stuff(x, y, z);


>
> 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),
> +			xvalue->type == EVM_IMA_XATTR_DIGSIG);
> +}
> +
>  int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
>  {
>  	int result;
>  
>  	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
>  	if (result == 1) {
> -		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
>  		result = 0;
>  	}
>  	return result;
>  }
> +
> +void ima_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
> +{
> +	int result;
> +
> +	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
> +	if (result == 1)
> +		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
> +}
> diff --git a/security/security.c b/security/security.c
> index 5ac96b16f8fa..efb1f874dc41 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1319,6 +1319,7 @@ void security_inode_post_setxattr(struct dentry *dentry, const char *name,
>  	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>  		return;
>  	call_void_hook(inode_post_setxattr, dentry, name, value, size, flags);
> +	ima_inode_post_setxattr(dentry, name, value, size);
>  	evm_inode_post_setxattr(dentry, name, value, size);
>  }
>  

RE: [PATCH v4 08/11] evm: Allow setxattr() and setattr() for unmodified metadata

[Roberto Sassu]

> From: Roberto Sassu
> Sent: Friday, March 5, 2021 4:19 PM
> With the patch to allow xattr/attr operations if a portable signature
> verification fails, cp and tar can copy all xattrs/attrs so that at the
> end of the process verification succeeds.
> 
> However, it might happen that the xattrs/attrs are already set to the
> correct value (taken at signing time) and signature verification succeeds
> before the copy has completed. For example, an archive might contains files
> owned by root and the archive is extracted by root.
> 
> Then, since portable signatures are immutable, all subsequent operations
> fail (e.g. fchown()), even if the operation is legitimate (does not alter
> the current value).
> 
> This patch avoids this problem by reporting successful operation to user
> space when that operation does not alter the current value of xattrs/attrs.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/evm/evm_main.c | 96
> +++++++++++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
> 
> diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> index eab536fa260f..a07516dcb920 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -18,6 +18,7 @@
>  #include <linux/integrity.h>
>  #include <linux/evm.h>
>  #include <linux/magic.h>
> +#include <linux/posix_acl_xattr.h>
> 
>  #include <crypto/hash.h>
>  #include <crypto/hash_info.h>
> @@ -328,6 +329,79 @@ static enum integrity_status
> evm_verify_current_integrity(struct dentry *dentry)
>  	return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
>  }
> 
> +/*
> + * evm_xattr_acl_change - check if passed ACL changes the inode mode
> + * @dentry: pointer to the affected dentry
> + * @xattr_name: requested xattr
> + * @xattr_value: requested xattr value
> + * @xattr_value_len: requested xattr value length
> + *
> + * Check if passed ACL changes the inode mode, which is protected by
> EVM.
> + *
> + * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
> + */
> +static int evm_xattr_acl_change(struct dentry *dentry, const char
> *xattr_name,
> +				const void *xattr_value, size_t
> xattr_value_len)
> +{
> +	umode_t mode;
> +	struct posix_acl *acl = NULL, *acl_res;
> +	struct inode *inode = d_backing_inode(dentry);
> +	int rc;
> +
> +	/* UID/GID in ACL have been already converted from user to init ns
> */
> +	acl = posix_acl_from_xattr(&init_user_ns, xattr_value,
> xattr_value_len);
> +	if (!acl)

Based on Mimi's review, I will change this to:

if (IS_ERR_OR_NULL(acl))

> +		return 1;
> +
> +	acl_res = acl;
> +	rc = posix_acl_update_mode(&init_user_ns, inode, &mode,
> &acl_res);

About this part, probably it is not correct.

I'm writing a test for this patch that checks if operations
that don't change the file mode succeed and those that
do fail.

mount-idmapped --map-mount b:3001:0:1 /mnt /mnt-idmapped
pushd /mnt
echo "test" > test-file
chown 3001 test-file
chgrp 3001 test-file
chmod 2644 test-file
<check enabled>
setfacl --set u::rw,g::r,o::r,m:r test-file (expected to succeed, caller has CAP_FSETID, so S_ISGID is not dropped)
setfacl --set u::rw,g::r,o::r,m:rw test-file (expected to fail)
pushd /mnt-idmapped
capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-file (expected to succeed, caller is in the owning group of test-file, so S_ISGID is not dropped)

After adding a debug line in posix_acl_update_mode():
printk("%s: %d(%d) %d\n", __func__, in_group_p(i_gid_into_mnt(mnt_userns, inode)), __kgid_val(i_gid_into_mnt(mnt_userns, inode)), capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID));

without passing mnt_userns:
[  748.262582] setfacl --set u::rw,g::r,o::r,m:r test-file
[  748.268021] posix_acl_update_mode: 0(3001) 1
[  748.268035] posix_acl_update_mode: 0(3001) 1
[  748.268570] setfacl --set u::rw,g::r,o::r,m:rw test-file
[  748.274193] posix_acl_update_mode: 0(3001) 1
[  748.279198] capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-file
[  748.287894] posix_acl_update_mode: 0(3001) 0

passing mnt_userns:
[   81.159766] setfacl --set u::rw,g::r,o::r,m:r test-file
[   81.165207] posix_acl_update_mode: 0(3001) 1
[   81.165226] posix_acl_update_mode: 0(3001) 1
[   81.165732] setfacl --set u::rw,g::r,o::r,m:rw test-file
[   81.170978] posix_acl_update_mode: 0(3001) 1
[   81.176014] capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-file
[   81.184648] posix_acl_update_mode: 1(0) 0
[   81.184663] posix_acl_update_mode: 1(0) 0

The difference is that, by passing mnt_userns, the caller (root) is
in the owning group of the file (3001 -> 0). Without passing mnt_userns,
it is not (3001 -> 3001).

Christian, Andreas, could you confirm that this is correct?

If there are no objections, I will send an additional patch to pass
mnt_userns to EVM.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> +
> +	posix_acl_release(acl);
> +
> +	if (rc)
> +		return 1;
> +
> +	if (inode->i_mode != mode)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +/*
> + * evm_xattr_change - check if passed xattr value differs from current
> value
> + * @dentry: pointer to the affected dentry
> + * @xattr_name: requested xattr
> + * @xattr_value: requested xattr value
> + * @xattr_value_len: requested xattr value length
> + *
> + * Check if passed xattr value differs from current value.
> + *
> + * Returns 1 if passed xattr value differs from current value, 0 otherwise.
> + */
> +static int evm_xattr_change(struct dentry *dentry, const char
> *xattr_name,
> +			    const void *xattr_value, size_t xattr_value_len)
> +{
> +	char *xattr_data = NULL;
> +	int rc = 0;
> +
> +	if (posix_xattr_acl(xattr_name))
> +		return evm_xattr_acl_change(dentry, xattr_name,
> xattr_value,
> +					    xattr_value_len);
> +
> +	rc = vfs_getxattr_alloc(&init_user_ns, dentry, xattr_name,
> &xattr_data,
> +				0, GFP_NOFS);
> +	if (rc < 0)
> +		return 1;
> +
> +	if (rc == xattr_value_len)
> +		rc = memcmp(xattr_value, xattr_data, rc);
> +	else
> +		rc = 1;
> +
> +	kfree(xattr_data);
> +	return rc;
> +}
> +
>  /*
>   * evm_protect_xattr - protect the EVM extended attribute
>   *
> @@ -388,6 +462,10 @@ static int evm_protect_xattr(struct dentry *dentry,
> const char *xattr_name,
>  	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
>  		return 0;
> 
> +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> +	    !evm_xattr_change(dentry, xattr_name, xattr_value,
> xattr_value_len))
> +		return 0;
> +
>  	if (evm_status != INTEGRITY_PASS)
>  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> d_backing_inode(dentry),
>  				    dentry->d_name.name,
> "appraise_metadata",
> @@ -527,6 +605,19 @@ void evm_inode_post_removexattr(struct dentry
> *dentry, const char *xattr_name)
>  	evm_update_evmxattr(dentry, xattr_name, NULL, 0);
>  }
> 
> +static int evm_attr_change(struct dentry *dentry, struct iattr *attr)
> +{
> +	struct inode *inode = d_backing_inode(dentry);
> +	unsigned int ia_valid = attr->ia_valid;
> +
> +	if ((!(ia_valid & ATTR_UID) || uid_eq(attr->ia_uid, inode->i_uid))
> &&
> +	    (!(ia_valid & ATTR_GID) || gid_eq(attr->ia_gid, inode->i_gid)) &&
> +	    (!(ia_valid & ATTR_MODE) || attr->ia_mode == inode->i_mode))
> +		return 0;
> +
> +	return 1;
> +}
> +
>  /**
>   * evm_inode_setattr - prevent updating an invalid EVM extended
> attribute
>   * @dentry: pointer to the affected dentry
> @@ -557,6 +648,11 @@ int evm_inode_setattr(struct dentry *dentry, struct
> iattr *attr)
>  	    (evm_status == INTEGRITY_FAIL_IMMUTABLE) ||
>  	    (evm_ignore_error_safe(evm_status)))
>  		return 0;
> +
> +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> +	    !evm_attr_change(dentry, attr))
> +		return 0;
> +
>  	integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> d_backing_inode(dentry),
>  			    dentry->d_name.name, "appraise_metadata",
>  			    integrity_status_msg[evm_status], -EPERM, 0);
> --
> 2.26.2

Re: [PATCH v4 08/11] evm: Allow setxattr() and setattr() for unmodified metadata

[Christian Brauner]

On Thu, Mar 25, 2021 at 10:53:43AM +0000, Roberto Sassu wrote:
> > From: Roberto Sassu
> > Sent: Friday, March 5, 2021 4:19 PM
> > With the patch to allow xattr/attr operations if a portable signature
> > verification fails, cp and tar can copy all xattrs/attrs so that at the
> > end of the process verification succeeds.
> > 
> > However, it might happen that the xattrs/attrs are already set to the
> > correct value (taken at signing time) and signature verification succeeds
> > before the copy has completed. For example, an archive might contains files
> > owned by root and the archive is extracted by root.
> > 
> > Then, since portable signatures are immutable, all subsequent operations
> > fail (e.g. fchown()), even if the operation is legitimate (does not alter
> > the current value).
> > 
> > This patch avoids this problem by reporting successful operation to user
> > space when that operation does not alter the current value of xattrs/attrs.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  security/integrity/evm/evm_main.c | 96
> > +++++++++++++++++++++++++++++++
> >  1 file changed, 96 insertions(+)
> > 
> > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c
> > index eab536fa260f..a07516dcb920 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/integrity.h>
> >  #include <linux/evm.h>
> >  #include <linux/magic.h>
> > +#include <linux/posix_acl_xattr.h>
> > 
> >  #include <crypto/hash.h>
> >  #include <crypto/hash_info.h>
> > @@ -328,6 +329,79 @@ static enum integrity_status
> > evm_verify_current_integrity(struct dentry *dentry)
> >  	return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
> >  }
> > 
> > +/*
> > + * evm_xattr_acl_change - check if passed ACL changes the inode mode
> > + * @dentry: pointer to the affected dentry
> > + * @xattr_name: requested xattr
> > + * @xattr_value: requested xattr value
> > + * @xattr_value_len: requested xattr value length
> > + *
> > + * Check if passed ACL changes the inode mode, which is protected by
> > EVM.
> > + *
> > + * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
> > + */
> > +static int evm_xattr_acl_change(struct dentry *dentry, const char
> > *xattr_name,
> > +				const void *xattr_value, size_t
> > xattr_value_len)
> > +{
> > +	umode_t mode;
> > +	struct posix_acl *acl = NULL, *acl_res;
> > +	struct inode *inode = d_backing_inode(dentry);
> > +	int rc;
> > +
> > +	/* UID/GID in ACL have been already converted from user to init ns
> > */
> > +	acl = posix_acl_from_xattr(&init_user_ns, xattr_value,
> > xattr_value_len);
> > +	if (!acl)
> 
> Based on Mimi's review, I will change this to:
> 
> if (IS_ERR_OR_NULL(acl))
> 
> > +		return 1;
> > +
> > +	acl_res = acl;
> > +	rc = posix_acl_update_mode(&init_user_ns, inode, &mode,
> > &acl_res);
> 
> About this part, probably it is not correct.
> 
> I'm writing a test for this patch that checks if operations
> that don't change the file mode succeed and those that
> do fail.
> 
> mount-idmapped --map-mount b:3001:0:1 /mnt /mnt-idmapped
> pushd /mnt
> echo "test" > test-file
> chown 3001 test-file
> chgrp 3001 test-file
> chmod 2644 test-file
> <check enabled>
> setfacl --set u::rw,g::r,o::r,m:r test-file (expected to succeed, caller has CAP_FSETID, so S_ISGID is not dropped)
> setfacl --set u::rw,g::r,o::r,m:rw test-file (expected to fail)
> pushd /mnt-idmapped
> capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-file (expected to succeed, caller is in the owning group of test-file, so S_ISGID is not dropped)
> 
> After adding a debug line in posix_acl_update_mode():
> printk("%s: %d(%d) %d\n", __func__, in_group_p(i_gid_into_mnt(mnt_userns, inode)), __kgid_val(i_gid_into_mnt(mnt_userns, inode)), capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID));
> 
> without passing mnt_userns:
> [  748.262582] setfacl --set u::rw,g::r,o::r,m:r test-file
> [  748.268021] posix_acl_update_mode: 0(3001) 1
> [  748.268035] posix_acl_update_mode: 0(3001) 1
> [  748.268570] setfacl --set u::rw,g::r,o::r,m:rw test-file
> [  748.274193] posix_acl_update_mode: 0(3001) 1
> [  748.279198] capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-file
> [  748.287894] posix_acl_update_mode: 0(3001) 0
> 
> passing mnt_userns:
> [   81.159766] setfacl --set u::rw,g::r,o::r,m:r test-file
> [   81.165207] posix_acl_update_mode: 0(3001) 1
> [   81.165226] posix_acl_update_mode: 0(3001) 1
> [   81.165732] setfacl --set u::rw,g::r,o::r,m:rw test-file
> [   81.170978] posix_acl_update_mode: 0(3001) 1
> [   81.176014] capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-file
> [   81.184648] posix_acl_update_mode: 1(0) 0
> [   81.184663] posix_acl_update_mode: 1(0) 0
> 
> The difference is that, by passing mnt_userns, the caller (root) is
> in the owning group of the file (3001 -> 0). Without passing mnt_userns,
> it is not (3001 -> 3001).
> 
> Christian, Andreas, could you confirm that this is correct?

Hey Robert,

Thanks for the Cc and thanks for testing this with and without idmapped
mounts; very much appreciated.

> 
> If there are no objections, I will send an additional patch to pass
> mnt_userns to EVM.

Yes, since you're starting to verify attrs and posix_acl changes that
deal with uids/gids you need to account for the mnt_userns. I've pulled
and applied your patch locally and looked through it. I think you need
to change:

- evm_inode_setxattr()
- evm_inode_removexattr()

to take a mnt_userns. That should be straightforward. I already changed
security_inode_setxattr() to pass down the mnt_userns so you need to
simply pass that further down:

- security_inode_setxattr(mnt_userns, ...)
  -> evm_inode_setxattr(mnt_userns, ...)

- security_inode_removexattr(mnt_userns, ...)
  -> evm_inode_removexattr(mnt_userns, ...)

The rest looks sane to me.

Fwiw, I'm mainting a large test-suite that I wrote for idmapped mounts
but that aims to cover all vfs operations independent of them. It aims
for:
- test vfs feature x on regular mounts
- test vfs feature on idmapped mounts
- test vfs feature in user namespaces
- test vfs feature on idmapped mount in user namespaces
I'm in the process of upstreaming it for xfstests (cf. [1]). It also
includes tests for xattrs/acls and fscaps. if ima and evm want to add
something to this that'd be great but if you maintain your own testing
that's of course totally ok.
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/xfstests-dev.git/log/?h=idmapped_mounts

Thanks!
Christian

Re: [PATCH v4 08/11] evm: Allow setxattr() and setattr() for unmodified metadata

[Christian Brauner]

On Thu, Mar 25, 2021 at 01:13:41PM +0100, Christian Brauner wrote:
> On Thu, Mar 25, 2021 at 10:53:43AM +0000, Roberto Sassu wrote:
> > > From: Roberto Sassu
> > > Sent: Friday, March 5, 2021 4:19 PM
> > > With the patch to allow xattr/attr operations if a portable signature
> > > verification fails, cp and tar can copy all xattrs/attrs so that at the
> > > end of the process verification succeeds.
> > > 
> > > However, it might happen that the xattrs/attrs are already set to the
> > > correct value (taken at signing time) and signature verification succeeds
> > > before the copy has completed. For example, an archive might contains files
> > > owned by root and the archive is extracted by root.
> > > 
> > > Then, since portable signatures are immutable, all subsequent operations
> > > fail (e.g. fchown()), even if the operation is legitimate (does not alter
> > > the current value).
> > > 
> > > This patch avoids this problem by reporting successful operation to user
> > > space when that operation does not alter the current value of xattrs/attrs.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  security/integrity/evm/evm_main.c | 96
> > > +++++++++++++++++++++++++++++++
> > >  1 file changed, 96 insertions(+)
> > > 
> > > diff --git a/security/integrity/evm/evm_main.c
> > > b/security/integrity/evm/evm_main.c
> > > index eab536fa260f..a07516dcb920 100644
> > > --- a/security/integrity/evm/evm_main.c
> > > +++ b/security/integrity/evm/evm_main.c
> > > @@ -18,6 +18,7 @@
> > >  #include <linux/integrity.h>
> > >  #include <linux/evm.h>
> > >  #include <linux/magic.h>
> > > +#include <linux/posix_acl_xattr.h>
> > > 
> > >  #include <crypto/hash.h>
> > >  #include <crypto/hash_info.h>
> > > @@ -328,6 +329,79 @@ static enum integrity_status
> > > evm_verify_current_integrity(struct dentry *dentry)
> > >  	return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
> > >  }
> > > 
> > > +/*
> > > + * evm_xattr_acl_change - check if passed ACL changes the inode mode
> > > + * @dentry: pointer to the affected dentry
> > > + * @xattr_name: requested xattr
> > > + * @xattr_value: requested xattr value
> > > + * @xattr_value_len: requested xattr value length
> > > + *
> > > + * Check if passed ACL changes the inode mode, which is protected by
> > > EVM.
> > > + *
> > > + * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
> > > + */
> > > +static int evm_xattr_acl_change(struct dentry *dentry, const char
> > > *xattr_name,
> > > +				const void *xattr_value, size_t
> > > xattr_value_len)
> > > +{
> > > +	umode_t mode;
> > > +	struct posix_acl *acl = NULL, *acl_res;
> > > +	struct inode *inode = d_backing_inode(dentry);
> > > +	int rc;
> > > +
> > > +	/* UID/GID in ACL have been already converted from user to init ns
> > > */
> > > +	acl = posix_acl_from_xattr(&init_user_ns, xattr_value,
> > > xattr_value_len);
> > > +	if (!acl)
> > 
> > Based on Mimi's review, I will change this to:
> > 
> > if (IS_ERR_OR_NULL(acl))
> > 
> > > +		return 1;
> > > +
> > > +	acl_res = acl;
> > > +	rc = posix_acl_update_mode(&init_user_ns, inode, &mode,
> > > &acl_res);
> > 
> > About this part, probably it is not correct.
> > 
> > I'm writing a test for this patch that checks if operations
> > that don't change the file mode succeed and those that
> > do fail.
> > 
> > mount-idmapped --map-mount b:3001:0:1 /mnt /mnt-idmapped
> > pushd /mnt
> > echo "test" > test-file
> > chown 3001 test-file
> > chgrp 3001 test-file
> > chmod 2644 test-file
> > <check enabled>
> > setfacl --set u::rw,g::r,o::r,m:r test-file (expected to succeed, caller has CAP_FSETID, so S_ISGID is not dropped)
> > setfacl --set u::rw,g::r,o::r,m:rw test-file (expected to fail)
> > pushd /mnt-idmapped
> > capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-file (expected to succeed, caller is in the owning group of test-file, so S_ISGID is not dropped)
> > 
> > After adding a debug line in posix_acl_update_mode():
> > printk("%s: %d(%d) %d\n", __func__, in_group_p(i_gid_into_mnt(mnt_userns, inode)), __kgid_val(i_gid_into_mnt(mnt_userns, inode)), capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID));
> > 
> > without passing mnt_userns:
> > [  748.262582] setfacl --set u::rw,g::r,o::r,m:r test-file
> > [  748.268021] posix_acl_update_mode: 0(3001) 1
> > [  748.268035] posix_acl_update_mode: 0(3001) 1
> > [  748.268570] setfacl --set u::rw,g::r,o::r,m:rw test-file
> > [  748.274193] posix_acl_update_mode: 0(3001) 1
> > [  748.279198] capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-file
> > [  748.287894] posix_acl_update_mode: 0(3001) 0
> > 
> > passing mnt_userns:
> > [   81.159766] setfacl --set u::rw,g::r,o::r,m:r test-file
> > [   81.165207] posix_acl_update_mode: 0(3001) 1
> > [   81.165226] posix_acl_update_mode: 0(3001) 1
> > [   81.165732] setfacl --set u::rw,g::r,o::r,m:rw test-file
> > [   81.170978] posix_acl_update_mode: 0(3001) 1
> > [   81.176014] capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-file
> > [   81.184648] posix_acl_update_mode: 1(0) 0
> > [   81.184663] posix_acl_update_mode: 1(0) 0
> > 
> > The difference is that, by passing mnt_userns, the caller (root) is
> > in the owning group of the file (3001 -> 0). Without passing mnt_userns,
> > it is not (3001 -> 3001).
> > 
> > Christian, Andreas, could you confirm that this is correct?
> 
> Hey Robert,

s/Robert/Roberto/

Sorry for the typo.

RE: [PATCH v4 08/11] evm: Allow setxattr() and setattr() for unmodified metadata

[Roberto Sassu]

> From: Christian Brauner [mailto:christian.brauner@ubuntu.com]
> Sent: Thursday, March 25, 2021 1:14 PM
> On Thu, Mar 25, 2021 at 10:53:43AM +0000, Roberto Sassu wrote:
> > > From: Roberto Sassu
> > > Sent: Friday, March 5, 2021 4:19 PM
> > > With the patch to allow xattr/attr operations if a portable signature
> > > verification fails, cp and tar can copy all xattrs/attrs so that at the
> > > end of the process verification succeeds.
> > >
> > > However, it might happen that the xattrs/attrs are already set to the
> > > correct value (taken at signing time) and signature verification succeeds
> > > before the copy has completed. For example, an archive might contains
> files
> > > owned by root and the archive is extracted by root.
> > >
> > > Then, since portable signatures are immutable, all subsequent
> operations
> > > fail (e.g. fchown()), even if the operation is legitimate (does not alter
> > > the current value).
> > >
> > > This patch avoids this problem by reporting successful operation to user
> > > space when that operation does not alter the current value of
> xattrs/attrs.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  security/integrity/evm/evm_main.c | 96
> > > +++++++++++++++++++++++++++++++
> > >  1 file changed, 96 insertions(+)
> > >
> > > diff --git a/security/integrity/evm/evm_main.c
> > > b/security/integrity/evm/evm_main.c
> > > index eab536fa260f..a07516dcb920 100644
> > > --- a/security/integrity/evm/evm_main.c
> > > +++ b/security/integrity/evm/evm_main.c
> > > @@ -18,6 +18,7 @@
> > >  #include <linux/integrity.h>
> > >  #include <linux/evm.h>
> > >  #include <linux/magic.h>
> > > +#include <linux/posix_acl_xattr.h>
> > >
> > >  #include <crypto/hash.h>
> > >  #include <crypto/hash_info.h>
> > > @@ -328,6 +329,79 @@ static enum integrity_status
> > > evm_verify_current_integrity(struct dentry *dentry)
> > >  	return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
> > >  }
> > >
> > > +/*
> > > + * evm_xattr_acl_change - check if passed ACL changes the inode
> mode
> > > + * @dentry: pointer to the affected dentry
> > > + * @xattr_name: requested xattr
> > > + * @xattr_value: requested xattr value
> > > + * @xattr_value_len: requested xattr value length
> > > + *
> > > + * Check if passed ACL changes the inode mode, which is protected by
> > > EVM.
> > > + *
> > > + * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
> > > + */
> > > +static int evm_xattr_acl_change(struct dentry *dentry, const char
> > > *xattr_name,
> > > +				const void *xattr_value, size_t
> > > xattr_value_len)
> > > +{
> > > +	umode_t mode;
> > > +	struct posix_acl *acl = NULL, *acl_res;
> > > +	struct inode *inode = d_backing_inode(dentry);
> > > +	int rc;
> > > +
> > > +	/* UID/GID in ACL have been already converted from user to init ns
> > > */
> > > +	acl = posix_acl_from_xattr(&init_user_ns, xattr_value,
> > > xattr_value_len);
> > > +	if (!acl)
> >
> > Based on Mimi's review, I will change this to:
> >
> > if (IS_ERR_OR_NULL(acl))
> >
> > > +		return 1;
> > > +
> > > +	acl_res = acl;
> > > +	rc = posix_acl_update_mode(&init_user_ns, inode, &mode,
> > > &acl_res);
> >
> > About this part, probably it is not correct.
> >
> > I'm writing a test for this patch that checks if operations
> > that don't change the file mode succeed and those that
> > do fail.
> >
> > mount-idmapped --map-mount b:3001:0:1 /mnt /mnt-idmapped
> > pushd /mnt
> > echo "test" > test-file
> > chown 3001 test-file
> > chgrp 3001 test-file
> > chmod 2644 test-file
> > <check enabled>
> > setfacl --set u::rw,g::r,o::r,m:r test-file (expected to succeed, caller has
> CAP_FSETID, so S_ISGID is not dropped)
> > setfacl --set u::rw,g::r,o::r,m:rw test-file (expected to fail)
> > pushd /mnt-idmapped
> > capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-file (expected
> to succeed, caller is in the owning group of test-file, so S_ISGID is not
> dropped)
> >
> > After adding a debug line in posix_acl_update_mode():
> > printk("%s: %d(%d) %d\n", __func__,
> in_group_p(i_gid_into_mnt(mnt_userns, inode)),
> __kgid_val(i_gid_into_mnt(mnt_userns, inode)),
> capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID));
> >
> > without passing mnt_userns:
> > [  748.262582] setfacl --set u::rw,g::r,o::r,m:r test-file
> > [  748.268021] posix_acl_update_mode: 0(3001) 1
> > [  748.268035] posix_acl_update_mode: 0(3001) 1
> > [  748.268570] setfacl --set u::rw,g::r,o::r,m:rw test-file
> > [  748.274193] posix_acl_update_mode: 0(3001) 1
> > [  748.279198] capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-
> file
> > [  748.287894] posix_acl_update_mode: 0(3001) 0
> >
> > passing mnt_userns:
> > [   81.159766] setfacl --set u::rw,g::r,o::r,m:r test-file
> > [   81.165207] posix_acl_update_mode: 0(3001) 1
> > [   81.165226] posix_acl_update_mode: 0(3001) 1
> > [   81.165732] setfacl --set u::rw,g::r,o::r,m:rw test-file
> > [   81.170978] posix_acl_update_mode: 0(3001) 1
> > [   81.176014] capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-
> file
> > [   81.184648] posix_acl_update_mode: 1(0) 0
> > [   81.184663] posix_acl_update_mode: 1(0) 0
> >
> > The difference is that, by passing mnt_userns, the caller (root) is
> > in the owning group of the file (3001 -> 0). Without passing mnt_userns,
> > it is not (3001 -> 3001).
> >
> > Christian, Andreas, could you confirm that this is correct?
> 
> Hey Robert,
> 
> Thanks for the Cc and thanks for testing this with and without idmapped
> mounts; very much appreciated.
> 
> >
> > If there are no objections, I will send an additional patch to pass
> > mnt_userns to EVM.
> 
> Yes, since you're starting to verify attrs and posix_acl changes that
> deal with uids/gids you need to account for the mnt_userns. I've pulled
> and applied your patch locally and looked through it. I think you need
> to change:
> 
> - evm_inode_setxattr()
> - evm_inode_removexattr()
> 
> to take a mnt_userns. That should be straightforward. I already changed
> security_inode_setxattr() to pass down the mnt_userns so you need to
> simply pass that further down:
> 
> - security_inode_setxattr(mnt_userns, ...)
>   -> evm_inode_setxattr(mnt_userns, ...)
> 
> - security_inode_removexattr(mnt_userns, ...)
>   -> evm_inode_removexattr(mnt_userns, ...)

Hi Christian

yes, I changed both.

> The rest looks sane to me.
> 
> Fwiw, I'm mainting a large test-suite that I wrote for idmapped mounts
> but that aims to cover all vfs operations independent of them. It aims
> for:
> - test vfs feature x on regular mounts
> - test vfs feature on idmapped mounts
> - test vfs feature in user namespaces
> - test vfs feature on idmapped mount in user namespaces
> I'm in the process of upstreaming it for xfstests (cf. [1]). It also
> includes tests for xattrs/acls and fscaps. if ima and evm want to add
> something to this that'd be great but if you maintain your own testing
> that's of course totally ok.
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/xfstests-
> dev.git/log/?h=idmapped_mounts

Yes, I think it would be good to have more tests and compare
IMA/EVM behavior between non and idmapped mounts.

For now, I wanted to be sure that my patch set works well
on top of your patches. Thanks a lot for reviewing this!

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> Thanks!
> Christian

RE: [PATCH v4 08/11] evm: Allow setxattr() and setattr() for unmodified metadata

[Roberto Sassu]

> From: Christian Brauner [mailto:christian.brauner@ubuntu.com]
> Sent: Thursday, March 25, 2021 1:21 PM
> On Thu, Mar 25, 2021 at 01:13:41PM +0100, Christian Brauner wrote:
> > On Thu, Mar 25, 2021 at 10:53:43AM +0000, Roberto Sassu wrote:
> > > > From: Roberto Sassu
> > > > Sent: Friday, March 5, 2021 4:19 PM
> > > > With the patch to allow xattr/attr operations if a portable signature
> > > > verification fails, cp and tar can copy all xattrs/attrs so that at the
> > > > end of the process verification succeeds.
> > > >
> > > > However, it might happen that the xattrs/attrs are already set to the
> > > > correct value (taken at signing time) and signature verification
> succeeds
> > > > before the copy has completed. For example, an archive might
> contains files
> > > > owned by root and the archive is extracted by root.
> > > >
> > > > Then, since portable signatures are immutable, all subsequent
> operations
> > > > fail (e.g. fchown()), even if the operation is legitimate (does not alter
> > > > the current value).
> > > >
> > > > This patch avoids this problem by reporting successful operation to
> user
> > > > space when that operation does not alter the current value of
> xattrs/attrs.
> > > >
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > ---
> > > >  security/integrity/evm/evm_main.c | 96
> > > > +++++++++++++++++++++++++++++++
> > > >  1 file changed, 96 insertions(+)
> > > >
> > > > diff --git a/security/integrity/evm/evm_main.c
> > > > b/security/integrity/evm/evm_main.c
> > > > index eab536fa260f..a07516dcb920 100644
> > > > --- a/security/integrity/evm/evm_main.c
> > > > +++ b/security/integrity/evm/evm_main.c
> > > > @@ -18,6 +18,7 @@
> > > >  #include <linux/integrity.h>
> > > >  #include <linux/evm.h>
> > > >  #include <linux/magic.h>
> > > > +#include <linux/posix_acl_xattr.h>
> > > >
> > > >  #include <crypto/hash.h>
> > > >  #include <crypto/hash_info.h>
> > > > @@ -328,6 +329,79 @@ static enum integrity_status
> > > > evm_verify_current_integrity(struct dentry *dentry)
> > > >  	return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
> > > >  }
> > > >
> > > > +/*
> > > > + * evm_xattr_acl_change - check if passed ACL changes the inode
> mode
> > > > + * @dentry: pointer to the affected dentry
> > > > + * @xattr_name: requested xattr
> > > > + * @xattr_value: requested xattr value
> > > > + * @xattr_value_len: requested xattr value length
> > > > + *
> > > > + * Check if passed ACL changes the inode mode, which is protected
> by
> > > > EVM.
> > > > + *
> > > > + * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
> > > > + */
> > > > +static int evm_xattr_acl_change(struct dentry *dentry, const char
> > > > *xattr_name,
> > > > +				const void *xattr_value, size_t
> > > > xattr_value_len)
> > > > +{
> > > > +	umode_t mode;
> > > > +	struct posix_acl *acl = NULL, *acl_res;
> > > > +	struct inode *inode = d_backing_inode(dentry);
> > > > +	int rc;
> > > > +
> > > > +	/* UID/GID in ACL have been already converted from user to init ns
> > > > */
> > > > +	acl = posix_acl_from_xattr(&init_user_ns, xattr_value,
> > > > xattr_value_len);
> > > > +	if (!acl)
> > >
> > > Based on Mimi's review, I will change this to:
> > >
> > > if (IS_ERR_OR_NULL(acl))
> > >
> > > > +		return 1;
> > > > +
> > > > +	acl_res = acl;
> > > > +	rc = posix_acl_update_mode(&init_user_ns, inode, &mode,
> > > > &acl_res);
> > >
> > > About this part, probably it is not correct.
> > >
> > > I'm writing a test for this patch that checks if operations
> > > that don't change the file mode succeed and those that
> > > do fail.
> > >
> > > mount-idmapped --map-mount b:3001:0:1 /mnt /mnt-idmapped
> > > pushd /mnt
> > > echo "test" > test-file
> > > chown 3001 test-file
> > > chgrp 3001 test-file
> > > chmod 2644 test-file
> > > <check enabled>
> > > setfacl --set u::rw,g::r,o::r,m:r test-file (expected to succeed, caller has
> CAP_FSETID, so S_ISGID is not dropped)
> > > setfacl --set u::rw,g::r,o::r,m:rw test-file (expected to fail)
> > > pushd /mnt-idmapped
> > > capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-file
> (expected to succeed, caller is in the owning group of test-file, so S_ISGID is
> not dropped)
> > >
> > > After adding a debug line in posix_acl_update_mode():
> > > printk("%s: %d(%d) %d\n", __func__,
> in_group_p(i_gid_into_mnt(mnt_userns, inode)),
> __kgid_val(i_gid_into_mnt(mnt_userns, inode)),
> capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID));
> > >
> > > without passing mnt_userns:
> > > [  748.262582] setfacl --set u::rw,g::r,o::r,m:r test-file
> > > [  748.268021] posix_acl_update_mode: 0(3001) 1
> > > [  748.268035] posix_acl_update_mode: 0(3001) 1
> > > [  748.268570] setfacl --set u::rw,g::r,o::r,m:rw test-file
> > > [  748.274193] posix_acl_update_mode: 0(3001) 1
> > > [  748.279198] capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r
> test-file
> > > [  748.287894] posix_acl_update_mode: 0(3001) 0
> > >
> > > passing mnt_userns:
> > > [   81.159766] setfacl --set u::rw,g::r,o::r,m:r test-file
> > > [   81.165207] posix_acl_update_mode: 0(3001) 1
> > > [   81.165226] posix_acl_update_mode: 0(3001) 1
> > > [   81.165732] setfacl --set u::rw,g::r,o::r,m:rw test-file
> > > [   81.170978] posix_acl_update_mode: 0(3001) 1
> > > [   81.176014] capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r
> test-file
> > > [   81.184648] posix_acl_update_mode: 1(0) 0
> > > [   81.184663] posix_acl_update_mode: 1(0) 0
> > >
> > > The difference is that, by passing mnt_userns, the caller (root) is
> > > in the owning group of the file (3001 -> 0). Without passing mnt_userns,
> > > it is not (3001 -> 3001).
> > >
> > > Christian, Andreas, could you confirm that this is correct?
> >
> > Hey Robert,
> 
> s/Robert/Roberto/
> 
> Sorry for the typo.

No worries!

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Mimi Zohar]

On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:
> On 3/5/2021 7:19 AM, Roberto Sassu wrote:
> > 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.
> 
> I don't see anything wrong with this patch in light of the way
> IMA and EVM have been treated to date.
> 
> However ...
> 
> The special casing of IMA and EVM in security.c is getting out of
> hand, and appears to be unnecessary. By my count there are 9 IMA
> hooks and 5 EVM hooks that have been hard coded. Adding this IMA
> hook makes 10. It would be really easy to register IMA and EVM as
> security modules. That would remove the dependency they currently
> have on security sub-system approval for changes like this one.
> I know there has been resistance to "IMA as an LSM" in the past,
> but it's pretty hard to see how it wouldn't be a win.

Somehow I missed the new "lsm=" boot command line option, which
dynamically allows enabling/disabling LSMs, being upstreamed.  This
would be one of the reasons for not making IMA/EVM full LSMs.

Both IMA and EVM file data/metadata is persistent across boots.  If
either one or the other is not enabled the file data hash or file
metadata HMAC will not properly be updated, potentially preventing the
system from booting when re-enabled.  Re-enabling IMA and EVM would
require "fixing" the mutable file data hash and HMAC, without any
knowledge of what the "fixed" values should be.  Dave Safford referred
to this as "blessing" the newly calculated values.

Mimi

RE: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, April 26, 2021 9:49 PM
> On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:
> > On 3/5/2021 7:19 AM, Roberto Sassu wrote:
> > > 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.
> >
> > I don't see anything wrong with this patch in light of the way
> > IMA and EVM have been treated to date.
> >
> > However ...
> >
> > The special casing of IMA and EVM in security.c is getting out of
> > hand, and appears to be unnecessary. By my count there are 9 IMA
> > hooks and 5 EVM hooks that have been hard coded. Adding this IMA
> > hook makes 10. It would be really easy to register IMA and EVM as
> > security modules. That would remove the dependency they currently
> > have on security sub-system approval for changes like this one.
> > I know there has been resistance to "IMA as an LSM" in the past,
> > but it's pretty hard to see how it wouldn't be a win.
> 
> Somehow I missed the new "lsm=" boot command line option, which
> dynamically allows enabling/disabling LSMs, being upstreamed.  This
> would be one of the reasons for not making IMA/EVM full LSMs.

Hi Mimi

one could argue why IMA/EVM should receive a special
treatment. I understand that this was a necessity without
LSM stacking. Now that LSM stacking is available, I don't
see any valid reason why IMA/EVM should not be managed
by the LSM infrastructure.

> Both IMA and EVM file data/metadata is persistent across boots.  If
> either one or the other is not enabled the file data hash or file
> metadata HMAC will not properly be updated, potentially preventing the
> system from booting when re-enabled.  Re-enabling IMA and EVM would
> require "fixing" the mutable file data hash and HMAC, without any
> knowledge of what the "fixed" values should be.  Dave Safford referred
> to this as "blessing" the newly calculated values.

IMA/EVM can be easily disabled in other ways, for example
by moving the IMA policy or the EVM keys elsewhere.

Also other LSMs rely on a dynamic and persistent state
(for example for file transitions in SELinux), which cannot be
trusted anymore if LSMs are even temporarily disabled.

If IMA/EVM have to be enabled to prevent misconfiguration,
I think the same can be achieved if they are full LSMs, for
example by preventing that the list of enabled LSMs changes
at run-time.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Mimi Zohar]

On Tue, 2021-04-27 at 09:25 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Monday, April 26, 2021 9:49 PM
> > On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:

> > > However ...
> > >
> > > The special casing of IMA and EVM in security.c is getting out of
> > > hand, and appears to be unnecessary. By my count there are 9 IMA
> > > hooks and 5 EVM hooks that have been hard coded. Adding this IMA
> > > hook makes 10. It would be really easy to register IMA and EVM as
> > > security modules. That would remove the dependency they currently
> > > have on security sub-system approval for changes like this one.
> > > I know there has been resistance to "IMA as an LSM" in the past,
> > > but it's pretty hard to see how it wouldn't be a win.

It sholdn't be one way.  Are you willing to also make the existing
IMA/EVM hooks that are not currently security hooks, security hooks
too?   And accept any new IMA/EVM hooks would result in new security
hooks?  Are you also willing to add dependency tracking between LSMs?

> > 
> > Somehow I missed the new "lsm=" boot command line option, which
> > dynamically allows enabling/disabling LSMs, being upstreamed.  This
> > would be one of the reasons for not making IMA/EVM full LSMs.
> 
> Hi Mimi
> 
> one could argue why IMA/EVM should receive a special
> treatment. I understand that this was a necessity without
> LSM stacking. Now that LSM stacking is available, I don't
> see any valid reason why IMA/EVM should not be managed
> by the LSM infrastructure.
> 
> > Both IMA and EVM file data/metadata is persistent across boots.  If
> > either one or the other is not enabled the file data hash or file
> > metadata HMAC will not properly be updated, potentially preventing the
> > system from booting when re-enabled.  Re-enabling IMA and EVM would
> > require "fixing" the mutable file data hash and HMAC, without any
> > knowledge of what the "fixed" values should be.  Dave Safford referred
> > to this as "blessing" the newly calculated values.
> 
> IMA/EVM can be easily disabled in other ways, for example
> by moving the IMA policy or the EVM keys elsewhere.

Dynamically disabling IMA/EVM is very different than removing keys and
preventing the system from booting.  Restoring the keys should result
in being able to re-boot the system.  Re-enabling IMA/EVM, requires re-
labeling the filesystem in "fix" mode, which "blesses" any changes made
when IMA/EVM were not enabled.

> Also other LSMs rely on a dynamic and persistent state
> (for example for file transitions in SELinux), which cannot be
> trusted anymore if LSMs are even temporarily disabled.

Your argument is because this is a problem for SELinux, make it also a
problem for IMA/EVM too?!   ("Two wrongs make a right")

> If IMA/EVM have to be enabled to prevent misconfiguration,
> I think the same can be achieved if they are full LSMs, for
> example by preventing that the list of enabled LSMs changes
> at run-time.

That ship sailed when "security=" was deprecated in favor of "lsm="
support, which dynamically enables/disables LSMs at runtime.

Mimi

RE: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Tuesday, April 27, 2021 5:35 PM
> On Tue, 2021-04-27 at 09:25 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Monday, April 26, 2021 9:49 PM
> > > On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:
> 
> > > > However ...
> > > >
> > > > The special casing of IMA and EVM in security.c is getting out of
> > > > hand, and appears to be unnecessary. By my count there are 9 IMA
> > > > hooks and 5 EVM hooks that have been hard coded. Adding this IMA
> > > > hook makes 10. It would be really easy to register IMA and EVM as
> > > > security modules. That would remove the dependency they currently
> > > > have on security sub-system approval for changes like this one.
> > > > I know there has been resistance to "IMA as an LSM" in the past,
> > > > but it's pretty hard to see how it wouldn't be a win.
> 
> It sholdn't be one way.  Are you willing to also make the existing
> IMA/EVM hooks that are not currently security hooks, security hooks
> too?   And accept any new IMA/EVM hooks would result in new security
> hooks?  Are you also willing to add dependency tracking between LSMs?

I already have a preliminary branch where IMA/EVM are full LSMs.

Indeed, the biggest problem would be to have the new hooks
accepted. I can send the patch set for evaluation to see what
people think.

> > > Somehow I missed the new "lsm=" boot command line option, which
> > > dynamically allows enabling/disabling LSMs, being upstreamed.  This
> > > would be one of the reasons for not making IMA/EVM full LSMs.
> >
> > Hi Mimi
> >
> > one could argue why IMA/EVM should receive a special
> > treatment. I understand that this was a necessity without
> > LSM stacking. Now that LSM stacking is available, I don't
> > see any valid reason why IMA/EVM should not be managed
> > by the LSM infrastructure.
> >
> > > Both IMA and EVM file data/metadata is persistent across boots.  If
> > > either one or the other is not enabled the file data hash or file
> > > metadata HMAC will not properly be updated, potentially preventing the
> > > system from booting when re-enabled.  Re-enabling IMA and EVM would
> > > require "fixing" the mutable file data hash and HMAC, without any
> > > knowledge of what the "fixed" values should be.  Dave Safford referred
> > > to this as "blessing" the newly calculated values.
> >
> > IMA/EVM can be easily disabled in other ways, for example
> > by moving the IMA policy or the EVM keys elsewhere.
> 
> Dynamically disabling IMA/EVM is very different than removing keys and
> preventing the system from booting.  Restoring the keys should result
> in being able to re-boot the system.  Re-enabling IMA/EVM, requires re-
> labeling the filesystem in "fix" mode, which "blesses" any changes made
> when IMA/EVM were not enabled.

Uhm, I thought that if you move the HMAC key for example
and you boot the system, you invalidate all files that change,
because the HMAC is not updated.

> > Also other LSMs rely on a dynamic and persistent state
> > (for example for file transitions in SELinux), which cannot be
> > trusted anymore if LSMs are even temporarily disabled.
> 
> Your argument is because this is a problem for SELinux, make it also a
> problem for IMA/EVM too?!   ("Two wrongs make a right")

To me it seems reasonable to give the ability to people to
disable the LSMs if they want to do so, and at the same time
to try to prevent accidental disable when the LSMs should be
enabled.

> > If IMA/EVM have to be enabled to prevent misconfiguration,
> > I think the same can be achieved if they are full LSMs, for
> > example by preventing that the list of enabled LSMs changes
> > at run-time.
> 
> That ship sailed when "security=" was deprecated in favor of "lsm="
> support, which dynamically enables/disables LSMs at runtime.

Maybe this possibility can be disabled with a new kernel option.
I will think a more concrete solution.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Mimi Zohar]

On Tue, 2021-04-27 at 15:57 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Tuesday, April 27, 2021 5:35 PM
> > On Tue, 2021-04-27 at 09:25 +0000, Roberto Sassu wrote:
> > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > > Sent: Monday, April 26, 2021 9:49 PM
> > > > On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:
> > 
> > > > > However ...
> > > > >
> > > > > The special casing of IMA and EVM in security.c is getting out of
> > > > > hand, and appears to be unnecessary. By my count there are 9 IMA
> > > > > hooks and 5 EVM hooks that have been hard coded. Adding this IMA
> > > > > hook makes 10. It would be really easy to register IMA and EVM as
> > > > > security modules. That would remove the dependency they currently
> > > > > have on security sub-system approval for changes like this one.
> > > > > I know there has been resistance to "IMA as an LSM" in the past,
> > > > > but it's pretty hard to see how it wouldn't be a win.
> > 
> > It sholdn't be one way.  Are you willing to also make the existing
> > IMA/EVM hooks that are not currently security hooks, security hooks
> > too?   And accept any new IMA/EVM hooks would result in new security
> > hooks?  Are you also willing to add dependency tracking between LSMs?
> 
> I already have a preliminary branch where IMA/EVM are full LSMs.
> 
> Indeed, the biggest problem would be to have the new hooks
> accepted. I can send the patch set for evaluation to see what
> people think.

Defining new security hooks is pretty straight forward.   Perhaps at
least wait until Casey responds before posting the patches.

> 
> > > > Somehow I missed the new "lsm=" boot command line option, which
> > > > dynamically allows enabling/disabling LSMs, being upstreamed.  This
> > > > would be one of the reasons for not making IMA/EVM full LSMs.
> > >
> > > Hi Mimi
> > >
> > > one could argue why IMA/EVM should receive a special
> > > treatment. I understand that this was a necessity without
> > > LSM stacking. Now that LSM stacking is available, I don't
> > > see any valid reason why IMA/EVM should not be managed
> > > by the LSM infrastructure.
> > >
> > > > Both IMA and EVM file data/metadata is persistent across boots.  If
> > > > either one or the other is not enabled the file data hash or file
> > > > metadata HMAC will not properly be updated, potentially preventing the
> > > > system from booting when re-enabled.  Re-enabling IMA and EVM would
> > > > require "fixing" the mutable file data hash and HMAC, without any
> > > > knowledge of what the "fixed" values should be.  Dave Safford referred
> > > > to this as "blessing" the newly calculated values.
> > >
> > > IMA/EVM can be easily disabled in other ways, for example
> > > by moving the IMA policy or the EVM keys elsewhere.
> > 
> > Dynamically disabling IMA/EVM is very different than removing keys and
> > preventing the system from booting.  Restoring the keys should result
> > in being able to re-boot the system.  Re-enabling IMA/EVM, requires re-
> > labeling the filesystem in "fix" mode, which "blesses" any changes made
> > when IMA/EVM were not enabled.
> 
> Uhm, I thought that if you move the HMAC key for example
> and you boot the system, you invalidate all files that change,
> because the HMAC is not updated.

More likely you wouldn't be able to boot the system without the HMAC
key.

Mimi

> 
> > > Also other LSMs rely on a dynamic and persistent state
> > > (for example for file transitions in SELinux), which cannot be
> > > trusted anymore if LSMs are even temporarily disabled.
> > 
> > Your argument is because this is a problem for SELinux, make it also a
> > problem for IMA/EVM too?!   ("Two wrongs make a right")
> 
> To me it seems reasonable to give the ability to people to
> disable the LSMs if they want to do so, and at the same time
> to try to prevent accidental disable when the LSMs should be
> enabled.
> 
> > > If IMA/EVM have to be enabled to prevent misconfiguration,
> > > I think the same can be achieved if they are full LSMs, for
> > > example by preventing that the list of enabled LSMs changes
> > > at run-time.
> > 
> > That ship sailed when "security=" was deprecated in favor of "lsm="
> > support, which dynamically enables/disables LSMs at runtime.
> 
> Maybe this possibility can be disabled with a new kernel option.
> I will think a more concrete solution.
> 
> Roberto
> 
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Casey Schaufler]

On 4/27/2021 8:57 AM, Roberto Sassu wrote:
>> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
>> Sent: Tuesday, April 27, 2021 5:35 PM
>> On Tue, 2021-04-27 at 09:25 +0000, Roberto Sassu wrote:
>>>> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
>>>> Sent: Monday, April 26, 2021 9:49 PM
>>>> On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:
>>>>> However ...
>>>>>
>>>>> The special casing of IMA and EVM in security.c is getting out of
>>>>> hand, and appears to be unnecessary. By my count there are 9 IMA
>>>>> hooks and 5 EVM hooks that have been hard coded. Adding this IMA
>>>>> hook makes 10. It would be really easy to register IMA and EVM as
>>>>> security modules. That would remove the dependency they currently
>>>>> have on security sub-system approval for changes like this one.
>>>>> I know there has been resistance to "IMA as an LSM" in the past,
>>>>> but it's pretty hard to see how it wouldn't be a win.
>> It sholdn't be one way.  Are you willing to also make the existing
>> IMA/EVM hooks that are not currently security hooks, security hooks
>> too?   And accept any new IMA/EVM hooks would result in new security
>> hooks?  Are you also willing to add dependency tracking between LSMs?
> I already have a preliminary branch where IMA/EVM are full LSMs.

I don't think that IMA/EVM need to be full LSMs to address my whinging.
I don't think that changing the existing integrity_whatever() to
security_whatever() is necessary. All that I'm really objecting to is
special cases in security.c:

{
	int ret;

	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return 0;
	/*
	 * SELinux and Smack integrate the cap call,
	 * so assume that all LSMs supplying this call do so.
	 */
	ret = call_int_hook(inode_removexattr, 1, mnt_userns, dentry, name);
	if (ret == 1)
		ret = cap_inode_removexattr(mnt_userns, dentry, name);
	if (ret)
		return ret;
	ret = ima_inode_removexattr(dentry, name);
	if (ret)
		return ret;
	return evm_inode_removexattr(dentry, name);
}

Not all uses of ima/evm functions in security.c make sense to convert
to LSM hooks. In fact, I could be completely wrong that it makes sense
to change anything. What I see is something that looks like it ought to
be normalized. If there's good reason (and it looks like there may be)
for it to be different there's no reason to pay attention to me.

>
> Indeed, the biggest problem would be to have the new hooks
> accepted. I can send the patch set for evaluation to see what
> people think.
>
>>>> Somehow I missed the new "lsm=" boot command line option, which
>>>> dynamically allows enabling/disabling LSMs, being upstreamed.  This
>>>> would be one of the reasons for not making IMA/EVM full LSMs.
>>> Hi Mimi
>>>
>>> one could argue why IMA/EVM should receive a special
>>> treatment. I understand that this was a necessity without
>>> LSM stacking. Now that LSM stacking is available, I don't
>>> see any valid reason why IMA/EVM should not be managed
>>> by the LSM infrastructure.
>>>
>>>> Both IMA and EVM file data/metadata is persistent across boots.  If
>>>> either one or the other is not enabled the file data hash or file
>>>> metadata HMAC will not properly be updated, potentially preventing the
>>>> system from booting when re-enabled.  Re-enabling IMA and EVM would
>>>> require "fixing" the mutable file data hash and HMAC, without any
>>>> knowledge of what the "fixed" values should be.  Dave Safford referred
>>>> to this as "blessing" the newly calculated values.
>>> IMA/EVM can be easily disabled in other ways, for example
>>> by moving the IMA policy or the EVM keys elsewhere.
>> Dynamically disabling IMA/EVM is very different than removing keys and
>> preventing the system from booting.  Restoring the keys should result
>> in being able to re-boot the system.  Re-enabling IMA/EVM, requires re-
>> labeling the filesystem in "fix" mode, which "blesses" any changes made
>> when IMA/EVM were not enabled.
> Uhm, I thought that if you move the HMAC key for example
> and you boot the system, you invalidate all files that change,
> because the HMAC is not updated.
>
>>> Also other LSMs rely on a dynamic and persistent state
>>> (for example for file transitions in SELinux), which cannot be
>>> trusted anymore if LSMs are even temporarily disabled.
>> Your argument is because this is a problem for SELinux, make it also a
>> problem for IMA/EVM too?!   ("Two wrongs make a right")
> To me it seems reasonable to give the ability to people to
> disable the LSMs if they want to do so, and at the same time
> to try to prevent accidental disable when the LSMs should be
> enabled.
>
>>> If IMA/EVM have to be enabled to prevent misconfiguration,
>>> I think the same can be achieved if they are full LSMs, for
>>> example by preventing that the list of enabled LSMs changes
>>> at run-time.
>> That ship sailed when "security=" was deprecated in favor of "lsm="
>> support, which dynamically enables/disables LSMs at runtime.

security= is still supported and works the same as ever. lsm= is
more powerful than security= but also harder to use.

> Maybe this possibility can be disabled with a new kernel option.
> I will think a more concrete solution.
>
> Roberto
>
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Mimi Zohar]

Hi Casey,

On Tue, 2021-04-27 at 09:39 -0700, Casey Schaufler wrote:
> >> That ship sailed when "security=" was deprecated in favor of "lsm="
> >> support, which dynamically enables/disables LSMs at runtime.
> 
> security= is still supported and works the same as ever. lsm= is
> more powerful than security= but also harder to use.

I understand that it still exists, but the documentation says it's been
deprecated.
From Documentation/admin-guide/kernel-parameters.txt:

        security=  [SECURITY] Choose a legacy "major" security module to
                        enable at boot. This has been deprecated by the
                        "lsm=" parameter.

Mimi

RE: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Roberto Sassu]

> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> Sent: Tuesday, April 27, 2021 6:40 PM
> On 4/27/2021 8:57 AM, Roberto Sassu wrote:
> >> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> >> Sent: Tuesday, April 27, 2021 5:35 PM
> >> On Tue, 2021-04-27 at 09:25 +0000, Roberto Sassu wrote:
> >>>> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> >>>> Sent: Monday, April 26, 2021 9:49 PM
> >>>> On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:
> >>>>> However ...
> >>>>>
> >>>>> The special casing of IMA and EVM in security.c is getting out of
> >>>>> hand, and appears to be unnecessary. By my count there are 9 IMA
> >>>>> hooks and 5 EVM hooks that have been hard coded. Adding this IMA
> >>>>> hook makes 10. It would be really easy to register IMA and EVM as
> >>>>> security modules. That would remove the dependency they currently
> >>>>> have on security sub-system approval for changes like this one.
> >>>>> I know there has been resistance to "IMA as an LSM" in the past,
> >>>>> but it's pretty hard to see how it wouldn't be a win.
> >> It sholdn't be one way.  Are you willing to also make the existing
> >> IMA/EVM hooks that are not currently security hooks, security hooks
> >> too?   And accept any new IMA/EVM hooks would result in new security
> >> hooks?  Are you also willing to add dependency tracking between LSMs?
> > I already have a preliminary branch where IMA/EVM are full LSMs.
> 
> I don't think that IMA/EVM need to be full LSMs to address my whinging.
> I don't think that changing the existing integrity_whatever() to
> security_whatever() is necessary. All that I'm really objecting to is
> special cases in security.c:
> 
> {
> 	int ret;
> 
> 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> 		return 0;
> 	/*
> 	 * SELinux and Smack integrate the cap call,
> 	 * so assume that all LSMs supplying this call do so.
> 	 */
> 	ret = call_int_hook(inode_removexattr, 1, mnt_userns, dentry,
> name);
> 	if (ret == 1)
> 		ret = cap_inode_removexattr(mnt_userns, dentry, name);
> 	if (ret)
> 		return ret;
> 	ret = ima_inode_removexattr(dentry, name);
> 	if (ret)
> 		return ret;
> 	return evm_inode_removexattr(dentry, name);
> }
> 
> Not all uses of ima/evm functions in security.c make sense to convert
> to LSM hooks. In fact, I could be completely wrong that it makes sense
> to change anything. What I see is something that looks like it ought to
> be normalized. If there's good reason (and it looks like there may be)
> for it to be different there's no reason to pay attention to me.

You are right. When I said that I don't see any valid reason for
not moving IMA/EVM to the LSM infrastructure, I probably should
have said just that it is technically feasible. Apologies for being
too resolute in my statement.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> > Indeed, the biggest problem would be to have the new hooks
> > accepted. I can send the patch set for evaluation to see what
> > people think.
> >
> >>>> Somehow I missed the new "lsm=" boot command line option, which
> >>>> dynamically allows enabling/disabling LSMs, being upstreamed.  This
> >>>> would be one of the reasons for not making IMA/EVM full LSMs.
> >>> Hi Mimi
> >>>
> >>> one could argue why IMA/EVM should receive a special
> >>> treatment. I understand that this was a necessity without
> >>> LSM stacking. Now that LSM stacking is available, I don't
> >>> see any valid reason why IMA/EVM should not be managed
> >>> by the LSM infrastructure.
> >>>
> >>>> Both IMA and EVM file data/metadata is persistent across boots.  If
> >>>> either one or the other is not enabled the file data hash or file
> >>>> metadata HMAC will not properly be updated, potentially preventing the
> >>>> system from booting when re-enabled.  Re-enabling IMA and EVM
> would
> >>>> require "fixing" the mutable file data hash and HMAC, without any
> >>>> knowledge of what the "fixed" values should be.  Dave Safford referred
> >>>> to this as "blessing" the newly calculated values.
> >>> IMA/EVM can be easily disabled in other ways, for example
> >>> by moving the IMA policy or the EVM keys elsewhere.
> >> Dynamically disabling IMA/EVM is very different than removing keys and
> >> preventing the system from booting.  Restoring the keys should result
> >> in being able to re-boot the system.  Re-enabling IMA/EVM, requires re-
> >> labeling the filesystem in "fix" mode, which "blesses" any changes made
> >> when IMA/EVM were not enabled.
> > Uhm, I thought that if you move the HMAC key for example
> > and you boot the system, you invalidate all files that change,
> > because the HMAC is not updated.
> >
> >>> Also other LSMs rely on a dynamic and persistent state
> >>> (for example for file transitions in SELinux), which cannot be
> >>> trusted anymore if LSMs are even temporarily disabled.
> >> Your argument is because this is a problem for SELinux, make it also a
> >> problem for IMA/EVM too?!   ("Two wrongs make a right")
> > To me it seems reasonable to give the ability to people to
> > disable the LSMs if they want to do so, and at the same time
> > to try to prevent accidental disable when the LSMs should be
> > enabled.
> >
> >>> If IMA/EVM have to be enabled to prevent misconfiguration,
> >>> I think the same can be achieved if they are full LSMs, for
> >>> example by preventing that the list of enabled LSMs changes
> >>> at run-time.
> >> That ship sailed when "security=" was deprecated in favor of "lsm="
> >> support, which dynamically enables/disables LSMs at runtime.
> 
> security= is still supported and works the same as ever. lsm= is
> more powerful than security= but also harder to use.
> 
> > Maybe this possibility can be disabled with a new kernel option.
> > I will think a more concrete solution.
> >
> > Roberto
> >
> > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> > Managing Director: Li Peng, Li Jian, Shi Yanli

RE: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Roberto Sassu]

> 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?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> +			xvalue->type == EVM_IMA_XATTR_DIGSIG);
> +}
> +
>  int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
>  {
>  	int result;
> 
>  	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
>  	if (result == 1) {
> -		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
>  		result = 0;
>  	}
>  	return result;
>  }
> +
> +void ima_inode_post_removexattr(struct dentry *dentry, const char
> *xattr_name)
> +{
> +	int result;
> +
> +	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
> +	if (result == 1)
> +		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
> +}
> diff --git a/security/security.c b/security/security.c
> index 5ac96b16f8fa..efb1f874dc41 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1319,6 +1319,7 @@ void security_inode_post_setxattr(struct dentry
> *dentry, const char *name,
>  	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>  		return;
>  	call_void_hook(inode_post_setxattr, dentry, name, value, size, flags);
> +	ima_inode_post_setxattr(dentry, name, value, size);
>  	evm_inode_post_setxattr(dentry, name, value, size);
>  }
> 
> --
> 2.26.2

Re: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Mimi Zohar]

On Wed, 2021-04-28 at 15:35 +0000, Roberto Sassu wrote:
> > 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

RE: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, April 30, 2021 8:00 PM
> On Wed, 2021-04-28 at 15:35 +0000, Roberto Sassu wrote:
> > > 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.

Ok. Should I include this change in this patch or in a separate patch?
	
> 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.

Ok.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> thanks,
> 
> Mimi

Re: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Mimi Zohar]

On Mon, 2021-05-03 at 07:41 +0000, Roberto Sassu wrote:
> 
> > > > 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.
> 
> Ok. Should I include this change in this patch or in a separate patch?
> 	
> > 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.
> 
> Ok.

I'm really sorry, but the more I'm looking at the patch set, the more
I'm realizing that a number of the changes are a result of this
"performance improvement".   Without this change, reviewing the code
would be simplified.  So yes, the patch set needs to be updated to
reflect only what is needed to support making EVM portable & immutable
signatures more usable.

thanks,

Mimi