Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v5 00/12] evm: Improve usability of portable signatures
@ 2021-04-07 10:52 Roberto Sassu
  2021-04-07 10:52 ` [PATCH v5 01/12] evm: Execute evm_inode_init_security() only when an HMAC key is loaded Roberto Sassu
                   ` (11 more replies)
  0 siblings, 12 replies; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 10:52 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, 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-9 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 10 makes it possible to use portable signatures when the IMA policy
requires file signatures and patch 11 shows portable signatures in the
measurement list when the ima-sig template is selected.

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

Changelog

v4:
- add patch to pass mnt_userns to EVM inode set/remove xattr hooks
  (suggested by Christian Brauner)
- pass mnt_userns to posix_acl_update_mode()
- use IS_ERR_OR_NULL() in evm_xattr_acl_change() (suggested by Mimi)

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 (12):
  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: Pass user namespace to set/remove xattr hooks
  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                       |  18 +-
 include/linux/ima.h                       |  18 ++
 include/linux/integrity.h                 |   1 +
 security/integrity/evm/evm_main.c         | 216 ++++++++++++++++++++--
 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                       |   5 +-
 15 files changed, 329 insertions(+), 44 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v5 01/12] evm: Execute evm_inode_init_security() only when an HMAC key is loaded
  2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
@ 2021-04-07 10:52 ` Roberto Sassu
  2021-04-07 10:52 ` [PATCH v5 02/12] evm: Load EVM key in ima_load_x509() to avoid appraisal Roberto Sassu
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 10:52 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Roberto Sassu, stable

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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v5 02/12] evm: Load EVM key in ima_load_x509() to avoid appraisal
  2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
  2021-04-07 10:52 ` [PATCH v5 01/12] evm: Execute evm_inode_init_security() only when an HMAC key is loaded Roberto Sassu
@ 2021-04-07 10:52 ` Roberto Sassu
  2021-04-07 10:52 ` [PATCH v5 03/12] evm: Refuse EVM_ALLOW_METADATA_WRITES only if an HMAC key is loaded Roberto Sassu
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 10:52 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, 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 0ba01847e836..d66b94c7c8d5 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -208,7 +208,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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v5 03/12] evm: Refuse EVM_ALLOW_METADATA_WRITES only if an HMAC key is loaded
  2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
  2021-04-07 10:52 ` [PATCH v5 01/12] evm: Execute evm_inode_init_security() only when an HMAC key is loaded Roberto Sassu
  2021-04-07 10:52 ` [PATCH v5 02/12] evm: Load EVM key in ima_load_x509() to avoid appraisal Roberto Sassu
@ 2021-04-07 10:52 ` Roberto Sassu
  2021-04-30 20:28   ` Mimi Zohar
  2021-04-07 10:52 ` [PATCH v5 04/12] ima: Move ima_reset_appraise_flags() call to post hooks Roberto Sassu
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 10:52 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Roberto Sassu, stable

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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v5 04/12] ima: Move ima_reset_appraise_flags() call to post hooks
  2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
                   ` (2 preceding siblings ...)
  2021-04-07 10:52 ` [PATCH v5 03/12] evm: Refuse EVM_ALLOW_METADATA_WRITES only if an HMAC key is loaded Roberto Sassu
@ 2021-04-07 10:52 ` Roberto Sassu
  2021-04-07 16:17   ` Casey Schaufler
  2021-04-07 10:52 ` [PATCH v5 05/12] evm: Introduce evm_status_revalidate() Roberto Sassu
                   ` (7 subsequent siblings)
  11 siblings, 1 reply; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 10:52 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Roberto Sassu, Casey Schaufler

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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v5 05/12] evm: Introduce evm_status_revalidate()
  2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
                   ` (3 preceding siblings ...)
  2021-04-07 10:52 ` [PATCH v5 04/12] ima: Move ima_reset_appraise_flags() call to post hooks Roberto Sassu
@ 2021-04-07 10:52 ` Roberto Sassu
  2021-04-07 10:52 ` [PATCH v5 06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe Roberto Sassu
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 10:52 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, 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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v5 06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe
  2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
                   ` (4 preceding siblings ...)
  2021-04-07 10:52 ` [PATCH v5 05/12] evm: Introduce evm_status_revalidate() Roberto Sassu
@ 2021-04-07 10:52 ` Roberto Sassu
  2021-05-03  0:12   ` Mimi Zohar
  2021-04-07 10:52 ` [PATCH v5 07/12] evm: Allow xattr/attr operations for portable signatures Roberto Sassu
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 10:52 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, 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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v5 07/12] evm: Allow xattr/attr operations for portable signatures
  2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
                   ` (5 preceding siblings ...)
  2021-04-07 10:52 ` [PATCH v5 06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe Roberto Sassu
@ 2021-04-07 10:52 ` Roberto Sassu
  2021-05-03  0:12   ` Mimi Zohar
  2021-04-07 10:52 ` [PATCH v5 08/12] evm: Pass user namespace to set/remove xattr hooks Roberto Sassu
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 10:52 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, 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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v5 08/12] evm: Pass user namespace to set/remove xattr hooks
  2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
                   ` (6 preceding siblings ...)
  2021-04-07 10:52 ` [PATCH v5 07/12] evm: Allow xattr/attr operations for portable signatures Roberto Sassu
@ 2021-04-07 10:52 ` Roberto Sassu
  2021-04-07 12:06   ` Christian Brauner
  2021-04-07 10:52 ` [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata Roberto Sassu
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 10:52 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Roberto Sassu, Christian Brauner,
	Andreas Gruenbacher

In preparation for 'evm: Allow setxattr() and setattr() for unmodified
metadata', this patch passes mnt_userns to the inode set/remove xattr hooks
so that the GID of the inode on an idmapped mount is correctly determined
by posix_acl_update_mode().

Cc: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Andreas Gruenbacher <agruenba@redhat.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/evm.h               | 12 ++++++++----
 security/integrity/evm/evm_main.c | 17 +++++++++++------
 security/security.c               |  4 ++--
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/include/linux/evm.h b/include/linux/evm.h
index e5b7bcb152b9..8cad46bcec9d 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -23,13 +23,15 @@ extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
 					     struct integrity_iint_cache *iint);
 extern int evm_inode_setattr(struct dentry *dentry, struct iattr *attr);
 extern void evm_inode_post_setattr(struct dentry *dentry, int ia_valid);
-extern int evm_inode_setxattr(struct dentry *dentry, const char *name,
+extern int evm_inode_setxattr(struct user_namespace *mnt_userns,
+			      struct dentry *dentry, const char *name,
 			      const void *value, size_t size);
 extern void evm_inode_post_setxattr(struct dentry *dentry,
 				    const char *xattr_name,
 				    const void *xattr_value,
 				    size_t xattr_value_len);
-extern int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name);
+extern int evm_inode_removexattr(struct user_namespace *mnt_userns,
+				 struct dentry *dentry, const char *xattr_name);
 extern void evm_inode_post_removexattr(struct dentry *dentry,
 				       const char *xattr_name);
 extern int evm_inode_init_security(struct inode *inode,
@@ -72,7 +74,8 @@ static inline void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
 	return;
 }
 
-static inline int evm_inode_setxattr(struct dentry *dentry, const char *name,
+static inline int evm_inode_setxattr(struct user_namespace *mnt_userns,
+				     struct dentry *dentry, const char *name,
 				     const void *value, size_t size)
 {
 	return 0;
@@ -86,7 +89,8 @@ static inline void evm_inode_post_setxattr(struct dentry *dentry,
 	return;
 }
 
-static inline int evm_inode_removexattr(struct dentry *dentry,
+static inline int evm_inode_removexattr(struct user_namespace *mnt_userns,
+					struct dentry *dentry,
 					const char *xattr_name)
 {
 	return 0;
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index eab536fa260f..74f9f3a2ae53 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -340,7 +340,8 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
  * For posix xattr acls only, permit security.evm, even if it currently
  * doesn't exist, to be updated unless the EVM signature is immutable.
  */
-static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
+static int evm_protect_xattr(struct user_namespace *mnt_userns,
+			     struct dentry *dentry, const char *xattr_name,
 			     const void *xattr_value, size_t xattr_value_len)
 {
 	enum integrity_status evm_status;
@@ -398,6 +399,7 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
 
 /**
  * evm_inode_setxattr - protect the EVM extended attribute
+ * @mnt_userns: user namespace of the idmapped mount
  * @dentry: pointer to the affected dentry
  * @xattr_name: pointer to the affected extended attribute name
  * @xattr_value: pointer to the new extended attribute value
@@ -409,8 +411,9 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
  * userspace from writing HMAC value.  Writing 'security.evm' requires
  * requires CAP_SYS_ADMIN privileges.
  */
-int evm_inode_setxattr(struct dentry *dentry, const char *xattr_name,
-		       const void *xattr_value, size_t xattr_value_len)
+int evm_inode_setxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
+		       const char *xattr_name, const void *xattr_value,
+		       size_t xattr_value_len)
 {
 	const struct evm_ima_xattr_data *xattr_data = xattr_value;
 
@@ -427,19 +430,21 @@ int evm_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		    xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG)
 			return -EPERM;
 	}
-	return evm_protect_xattr(dentry, xattr_name, xattr_value,
+	return evm_protect_xattr(mnt_userns, dentry, xattr_name, xattr_value,
 				 xattr_value_len);
 }
 
 /**
  * evm_inode_removexattr - protect the EVM extended attribute
+ * @mnt_userns: user namespace of the idmapped mount
  * @dentry: pointer to the affected dentry
  * @xattr_name: pointer to the affected extended attribute name
  *
  * Removing 'security.evm' requires CAP_SYS_ADMIN privileges and that
  * the current value is valid.
  */
-int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name)
+int evm_inode_removexattr(struct user_namespace *mnt_userns,
+			  struct dentry *dentry, const char *xattr_name)
 {
 	/* Policy permits modification of the protected xattrs even though
 	 * there's no HMAC key loaded
@@ -447,7 +452,7 @@ int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name)
 	if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
 		return 0;
 
-	return evm_protect_xattr(dentry, xattr_name, NULL, 0);
+	return evm_protect_xattr(mnt_userns, dentry, xattr_name, NULL, 0);
 }
 
 static void evm_reset_status(struct inode *inode)
diff --git a/security/security.c b/security/security.c
index efb1f874dc41..7f14e59c4f8e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1310,7 +1310,7 @@ int security_inode_setxattr(struct user_namespace *mnt_userns,
 	ret = ima_inode_setxattr(dentry, name, value, size);
 	if (ret)
 		return ret;
-	return evm_inode_setxattr(dentry, name, value, size);
+	return evm_inode_setxattr(mnt_userns, dentry, name, value, size);
 }
 
 void security_inode_post_setxattr(struct dentry *dentry, const char *name,
@@ -1356,7 +1356,7 @@ int security_inode_removexattr(struct user_namespace *mnt_userns,
 	ret = ima_inode_removexattr(dentry, name);
 	if (ret)
 		return ret;
-	return evm_inode_removexattr(dentry, name);
+	return evm_inode_removexattr(mnt_userns, dentry, name);
 }
 
 int security_inode_need_killpriv(struct dentry *dentry)
-- 
2.26.2


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
  2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
                   ` (7 preceding siblings ...)
  2021-04-07 10:52 ` [PATCH v5 08/12] evm: Pass user namespace to set/remove xattr hooks Roberto Sassu
@ 2021-04-07 10:52 ` Roberto Sassu
  2021-04-07 12:05   ` Christian Brauner
                     ` (3 more replies)
  2021-04-07 10:52 ` [PATCH v5 10/12] ima: Allow imasig requirement to be satisfied by EVM portable signatures Roberto Sassu
                   ` (2 subsequent siblings)
  11 siblings, 4 replies; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 10:52 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Roberto Sassu, Christian Brauner,
	Andreas Gruenbacher

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.

Cc: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Andreas Gruenbacher <agruenba@redhat.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_main.c | 107 ++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 74f9f3a2ae53..2a8fcba67d47 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,89 @@ 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
+ * @mnt_userns: user namespace of the idmapped mount
+ * @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 user_namespace *mnt_userns,
+				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;
+
+	/* user_ns is not relevant here, ACL_USER/ACL_GROUP don't have impact
+	 * on the inode mode (see posix_acl_equiv_mode()).
+	 */
+	acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);
+	if (IS_ERR_OR_NULL(acl))
+		return 1;
+
+	acl_res = acl;
+	/* Passing mnt_userns is necessary to correctly determine the GID in
+	 * an idmapped mount, as the GID is used to clear the setgid bit in
+	 * the inode mode.
+	 */
+	rc = posix_acl_update_mode(mnt_userns, 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
+ * @mnt_userns: user namespace of the idmapped mount
+ * @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 user_namespace *mnt_userns,
+			    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(mnt_userns, 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
  *
@@ -389,6 +473,11 @@ static int evm_protect_xattr(struct user_namespace *mnt_userns,
 	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
 		return 0;
 
+	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
+	    !evm_xattr_change(mnt_userns, 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",
@@ -532,6 +621,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
@@ -562,6 +664,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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v5 10/12] ima: Allow imasig requirement to be satisfied by EVM portable signatures
  2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
                   ` (8 preceding siblings ...)
  2021-04-07 10:52 ` [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata Roberto Sassu
@ 2021-04-07 10:52 ` Roberto Sassu
  2021-04-07 10:52 ` [PATCH v5 11/12] ima: Introduce template field evmsig and write to field sig as fallback Roberto Sassu
  2021-04-07 10:52 ` [PATCH v5 12/12] ima: Don't remove security.ima if file must not be appraised Roberto Sassu
  11 siblings, 0 replies; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 10:52 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, 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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v5 11/12] ima: Introduce template field evmsig and write to field sig as fallback
  2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
                   ` (9 preceding siblings ...)
  2021-04-07 10:52 ` [PATCH v5 10/12] ima: Allow imasig requirement to be satisfied by EVM portable signatures Roberto Sassu
@ 2021-04-07 10:52 ` Roberto Sassu
  2021-04-07 10:52 ` [PATCH v5 12/12] ima: Don't remove security.ima if file must not be appraised Roberto Sassu
  11 siblings, 0 replies; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 10:52 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, 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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v5 12/12] ima: Don't remove security.ima if file must not be appraised
  2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
                   ` (10 preceding siblings ...)
  2021-04-07 10:52 ` [PATCH v5 11/12] ima: Introduce template field evmsig and write to field sig as fallback Roberto Sassu
@ 2021-04-07 10:52 ` Roberto Sassu
  11 siblings, 0 replies; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 10:52 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, 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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
  2021-04-07 10:52 ` [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata Roberto Sassu
@ 2021-04-07 12:05   ` Christian Brauner
  2021-04-07 15:23   ` kernel test robot
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 40+ messages in thread
From: Christian Brauner @ 2021-04-07 12:05 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, mjg59, linux-integrity, linux-security-module,
	linux-fsdevel, linux-kernel, Andreas Gruenbacher

On Wed, Apr 07, 2021 at 12:52:49PM +0200, Roberto Sassu wrote:
> 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.
> 
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
> Cc: Andreas Gruenbacher <agruenba@redhat.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/evm/evm_main.c | 107 ++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 74f9f3a2ae53..2a8fcba67d47 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,89 @@ 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
> + * @mnt_userns: user namespace of the idmapped mount
> + * @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 user_namespace *mnt_userns,
> +				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;
> +
> +	/* user_ns is not relevant here, ACL_USER/ACL_GROUP don't have impact
> +	 * on the inode mode (see posix_acl_equiv_mode()).
> +	 */
> +	acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);
> +	if (IS_ERR_OR_NULL(acl))
> +		return 1;
> +
> +	acl_res = acl;
> +	/* Passing mnt_userns is necessary to correctly determine the GID in
> +	 * an idmapped mount, as the GID is used to clear the setgid bit in
> +	 * the inode mode.
> +	 */
> +	rc = posix_acl_update_mode(mnt_userns, 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
> + * @mnt_userns: user namespace of the idmapped mount
> + * @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 user_namespace *mnt_userns,
> +			    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(mnt_userns, 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);

Afaik memcmp() can return values greater than 1 and less than 0 so it
might make sense to explicitly do sm like:
rc = memcmp() ? 1 : 0;
or
!!memcmp()
or alter the comment for evm_xattr_change().

other than that

Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>

> +	else
> +		rc = 1;
> +
> +	kfree(xattr_data);
> +	return rc;
> +}
> +
>  /*
>   * evm_protect_xattr - protect the EVM extended attribute
>   *
> @@ -389,6 +473,11 @@ static int evm_protect_xattr(struct user_namespace *mnt_userns,
>  	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
>  		return 0;
>  
> +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> +	    !evm_xattr_change(mnt_userns, 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",
> @@ -532,6 +621,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
> @@ -562,6 +664,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
> 

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 08/12] evm: Pass user namespace to set/remove xattr hooks
  2021-04-07 10:52 ` [PATCH v5 08/12] evm: Pass user namespace to set/remove xattr hooks Roberto Sassu
@ 2021-04-07 12:06   ` Christian Brauner
  0 siblings, 0 replies; 40+ messages in thread
From: Christian Brauner @ 2021-04-07 12:06 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, mjg59, linux-integrity, linux-security-module,
	linux-fsdevel, linux-kernel, Andreas Gruenbacher

On Wed, Apr 07, 2021 at 12:52:48PM +0200, Roberto Sassu wrote:
> In preparation for 'evm: Allow setxattr() and setattr() for unmodified
> metadata', this patch passes mnt_userns to the inode set/remove xattr hooks
> so that the GID of the inode on an idmapped mount is correctly determined
> by posix_acl_update_mode().
> 
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
> Cc: Andreas Gruenbacher <agruenba@redhat.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---

Looks good,
Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
  2021-04-07 10:52 ` [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata Roberto Sassu
  2021-04-07 12:05   ` Christian Brauner
@ 2021-04-07 15:23   ` kernel test robot
  2021-04-07 18:14   ` kernel test robot
  2021-05-03 13:00   ` [PATCH " Mimi Zohar
  3 siblings, 0 replies; 40+ messages in thread
From: kernel test robot @ 2021-04-07 15:23 UTC (permalink / raw)
  To: Roberto Sassu, zohar, mjg59
  Cc: kbuild-all, clang-built-linux, linux-integrity,
	linux-security-module, linux-fsdevel, linux-kernel,
	Roberto Sassu, Christian Brauner, Andreas Gruenbacher


[-- Attachment #1: Type: text/plain, Size: 3782 bytes --]

Hi Roberto,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on security/next-testing]
[also build test ERROR on integrity/next-integrity linus/master v5.12-rc6 next-20210407]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Roberto-Sassu/evm-Improve-usability-of-portable-signatures/20210407-185747
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-testing
config: s390-randconfig-r034-20210407 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project c060945b23a1c54d4b2a053ff4b093a2277b303d)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/0day-ci/linux/commit/1bdae98f0b81260a925cf7acf785dc10bb7787fe
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Roberto-Sassu/evm-Improve-usability-of-portable-signatures/20210407-185747
        git checkout 1bdae98f0b81260a925cf7acf785dc10bb7787fe
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> security/integrity/evm/evm_main.c:365:7: error: implicit declaration of function 'posix_acl_update_mode' [-Werror,-Wimplicit-function-declaration]
           rc = posix_acl_update_mode(mnt_userns, inode, &mode, &acl_res);
                ^
   1 error generated.


vim +/posix_acl_update_mode +365 security/integrity/evm/evm_main.c

   331	
   332	/*
   333	 * evm_xattr_acl_change - check if passed ACL changes the inode mode
   334	 * @mnt_userns: user namespace of the idmapped mount
   335	 * @dentry: pointer to the affected dentry
   336	 * @xattr_name: requested xattr
   337	 * @xattr_value: requested xattr value
   338	 * @xattr_value_len: requested xattr value length
   339	 *
   340	 * Check if passed ACL changes the inode mode, which is protected by EVM.
   341	 *
   342	 * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
   343	 */
   344	static int evm_xattr_acl_change(struct user_namespace *mnt_userns,
   345					struct dentry *dentry, const char *xattr_name,
   346					const void *xattr_value, size_t xattr_value_len)
   347	{
   348		umode_t mode;
   349		struct posix_acl *acl = NULL, *acl_res;
   350		struct inode *inode = d_backing_inode(dentry);
   351		int rc;
   352	
   353		/* user_ns is not relevant here, ACL_USER/ACL_GROUP don't have impact
   354		 * on the inode mode (see posix_acl_equiv_mode()).
   355		 */
   356		acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);
   357		if (IS_ERR_OR_NULL(acl))
   358			return 1;
   359	
   360		acl_res = acl;
   361		/* Passing mnt_userns is necessary to correctly determine the GID in
   362		 * an idmapped mount, as the GID is used to clear the setgid bit in
   363		 * the inode mode.
   364		 */
 > 365		rc = posix_acl_update_mode(mnt_userns, inode, &mode, &acl_res);
   366	
   367		posix_acl_release(acl);
   368	
   369		if (rc)
   370			return 1;
   371	
   372		if (inode->i_mode != mode)
   373			return 1;
   374	
   375		return 0;
   376	}
   377	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 15153 bytes --]

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 04/12] ima: Move ima_reset_appraise_flags() call to post hooks
  2021-04-07 10:52 ` [PATCH v5 04/12] ima: Move ima_reset_appraise_flags() call to post hooks Roberto Sassu
@ 2021-04-07 16:17   ` Casey Schaufler
  2021-04-07 16:31     ` Roberto Sassu
  2021-04-26 19:49     ` Mimi Zohar
  0 siblings, 2 replies; 40+ messages in thread
From: Casey Schaufler @ 2021-04-07 16:17 UTC (permalink / raw)
  To: Roberto Sassu, zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Casey Schaufler

On 4/7/2021 3:52 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.
>
> 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);
> +}
> +

Now you're calling ima_protect_xattr() twice for each setxattr.
Is that safe? Is it performant? Does it matter?

>  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);
> +}

Now you're calling ima_protect_xattr() twice for each removexattr.
Is that safe? Is it performant? Does it matter?

> 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);
>  }
>  


^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 04/12] ima: Move ima_reset_appraise_flags() call to post hooks
  2021-04-07 16:17   ` Casey Schaufler
@ 2021-04-07 16:31     ` Roberto Sassu
  2021-04-26 19:49     ` Mimi Zohar
  1 sibling, 0 replies; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 16:31 UTC (permalink / raw)
  To: Casey Schaufler, zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-kernel

> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> Sent: Wednesday, April 7, 2021 6:18 PM
> On 4/7/2021 3:52 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.
> >
> > 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);
> > +}
> > +
> 
> Now you're calling ima_protect_xattr() twice for each setxattr.
> Is that safe? Is it performant? Does it matter?

Hi Casey

I would expect that this does not have a significant impact on
the performance (it is just a strcmp on the xattr name).

Thanks

Roberto

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

> >  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);
> > +}
> 
> Now you're calling ima_protect_xattr() twice for each removexattr.
> Is that safe? Is it performant? Does it matter?
> 
> > 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);
> >  }
> >


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
  2021-04-07 10:52 ` [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata Roberto Sassu
  2021-04-07 12:05   ` Christian Brauner
  2021-04-07 15:23   ` kernel test robot
@ 2021-04-07 18:14   ` kernel test robot
  2021-04-07 19:28     ` [RESEND][PATCH " Roberto Sassu
  2021-05-03 13:00   ` [PATCH " Mimi Zohar
  3 siblings, 1 reply; 40+ messages in thread
From: kernel test robot @ 2021-04-07 18:14 UTC (permalink / raw)
  To: Roberto Sassu, zohar, mjg59
  Cc: kbuild-all, linux-integrity, linux-security-module,
	linux-fsdevel, linux-kernel, Roberto Sassu, Christian Brauner,
	Andreas Gruenbacher


[-- Attachment #1: Type: text/plain, Size: 3886 bytes --]

Hi Roberto,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on security/next-testing]
[also build test ERROR on integrity/next-integrity linus/master v5.12-rc6 next-20210407]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Roberto-Sassu/evm-Improve-usability-of-portable-signatures/20210407-185747
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-testing
config: nios2-randconfig-s031-20210407 (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-279-g6d5d9b42-dirty
        # https://github.com/0day-ci/linux/commit/1bdae98f0b81260a925cf7acf785dc10bb7787fe
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Roberto-Sassu/evm-Improve-usability-of-portable-signatures/20210407-185747
        git checkout 1bdae98f0b81260a925cf7acf785dc10bb7787fe
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=nios2 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   security/integrity/evm/evm_main.c: In function 'evm_xattr_acl_change':
>> security/integrity/evm/evm_main.c:365:7: error: implicit declaration of function 'posix_acl_update_mode'; did you mean 'posix_acl_equiv_mode'? [-Werror=implicit-function-declaration]
     365 |  rc = posix_acl_update_mode(mnt_userns, inode, &mode, &acl_res);
         |       ^~~~~~~~~~~~~~~~~~~~~
         |       posix_acl_equiv_mode
   cc1: some warnings being treated as errors


vim +365 security/integrity/evm/evm_main.c

   331	
   332	/*
   333	 * evm_xattr_acl_change - check if passed ACL changes the inode mode
   334	 * @mnt_userns: user namespace of the idmapped mount
   335	 * @dentry: pointer to the affected dentry
   336	 * @xattr_name: requested xattr
   337	 * @xattr_value: requested xattr value
   338	 * @xattr_value_len: requested xattr value length
   339	 *
   340	 * Check if passed ACL changes the inode mode, which is protected by EVM.
   341	 *
   342	 * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
   343	 */
   344	static int evm_xattr_acl_change(struct user_namespace *mnt_userns,
   345					struct dentry *dentry, const char *xattr_name,
   346					const void *xattr_value, size_t xattr_value_len)
   347	{
   348		umode_t mode;
   349		struct posix_acl *acl = NULL, *acl_res;
   350		struct inode *inode = d_backing_inode(dentry);
   351		int rc;
   352	
   353		/* user_ns is not relevant here, ACL_USER/ACL_GROUP don't have impact
   354		 * on the inode mode (see posix_acl_equiv_mode()).
   355		 */
   356		acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);
   357		if (IS_ERR_OR_NULL(acl))
   358			return 1;
   359	
   360		acl_res = acl;
   361		/* Passing mnt_userns is necessary to correctly determine the GID in
   362		 * an idmapped mount, as the GID is used to clear the setgid bit in
   363		 * the inode mode.
   364		 */
 > 365		rc = posix_acl_update_mode(mnt_userns, inode, &mode, &acl_res);
   366	
   367		posix_acl_release(acl);
   368	
   369		if (rc)
   370			return 1;
   371	
   372		if (inode->i_mode != mode)
   373			return 1;
   374	
   375		return 0;
   376	}
   377	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29037 bytes --]

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RESEND][PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
  2021-04-07 18:14   ` kernel test robot
@ 2021-04-07 19:28     ` Roberto Sassu
  0 siblings, 0 replies; 40+ messages in thread
From: Roberto Sassu @ 2021-04-07 19:28 UTC (permalink / raw)
  To: zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Roberto Sassu, Christian Brauner,
	Andreas Gruenbacher, kernel test robot

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.

Cc: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Andreas Gruenbacher <agruenba@redhat.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 security/integrity/evm/evm_main.c | 108 ++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 74f9f3a2ae53..8e80af97021e 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,90 @@ 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
+ * @mnt_userns: user namespace of the idmapped mount
+ * @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 user_namespace *mnt_userns,
+				struct dentry *dentry, const char *xattr_name,
+				const void *xattr_value, size_t xattr_value_len)
+{
+#ifdef CONFIG_FS_POSIX_ACL
+	umode_t mode;
+	struct posix_acl *acl = NULL, *acl_res;
+	struct inode *inode = d_backing_inode(dentry);
+	int rc;
+
+	/* user_ns is not relevant here, ACL_USER/ACL_GROUP don't have impact
+	 * on the inode mode (see posix_acl_equiv_mode()).
+	 */
+	acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);
+	if (IS_ERR_OR_NULL(acl))
+		return 1;
+
+	acl_res = acl;
+	/* Passing mnt_userns is necessary to correctly determine the GID in
+	 * an idmapped mount, as the GID is used to clear the setgid bit in
+	 * the inode mode.
+	 */
+	rc = posix_acl_update_mode(mnt_userns, inode, &mode, &acl_res);
+
+	posix_acl_release(acl);
+
+	if (rc)
+		return 1;
+
+	if (inode->i_mode != mode)
+		return 1;
+#endif
+	return 0;
+}
+
+/*
+ * evm_xattr_change - check if passed xattr value differs from current value
+ * @mnt_userns: user namespace of the idmapped mount
+ * @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 user_namespace *mnt_userns,
+			    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(mnt_userns, 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
  *
@@ -389,6 +474,11 @@ static int evm_protect_xattr(struct user_namespace *mnt_userns,
 	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
 		return 0;
 
+	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
+	    !evm_xattr_change(mnt_userns, 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",
@@ -532,6 +622,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
@@ -562,6 +665,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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 04/12] ima: Move ima_reset_appraise_flags() call to post hooks
  2021-04-07 16:17   ` Casey Schaufler
  2021-04-07 16:31     ` Roberto Sassu
@ 2021-04-26 19:49     ` Mimi Zohar
  1 sibling, 0 replies; 40+ messages in thread
From: Mimi Zohar @ 2021-04-26 19:49 UTC (permalink / raw)
  To: Casey Schaufler, Roberto Sassu, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-kernel

On Wed, 2021-04-07 at 09:17 -0700, Casey Schaufler 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),
> > +                     xvalue->type == EVM_IMA_XATTR_DIGSIG);
> > +}
> > +
> 
> Now you're calling ima_protect_xattr() twice for each setxattr.
> Is that safe? Is it performant? Does it matter?

The first time the call to ima_protect_xattr() prevents the
security.ima from being inappropriately modified.  The second time it
resets the cached status flags.  From a performance perspective,
unnecessarily re-calcuating the file hash is worse than rechecking the
security xattr string.
 
Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 03/12] evm: Refuse EVM_ALLOW_METADATA_WRITES only if an HMAC key is loaded
  2021-04-07 10:52 ` [PATCH v5 03/12] evm: Refuse EVM_ALLOW_METADATA_WRITES only if an HMAC key is loaded Roberto Sassu
@ 2021-04-30 20:28   ` Mimi Zohar
  0 siblings, 0 replies; 40+ messages in thread
From: Mimi Zohar @ 2021-04-30 20:28 UTC (permalink / raw)
  To: Roberto Sassu, mjg59, ericchiang, bweeks
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, stable

Hi Roberto,

On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> 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;
>  

The comment "freshly enable" is confusing.  Perhaps the original intent
was to enable flags before loading any keys.  So the comment and code
were kind of in sync.  With this change, enabling metadata writes may
be triggered after loading an x509 certificate.  Unless someone
comments, I don't have problems with this change.

Once metadata writes are enabled, the only way of disabling them is by
loading and enabling an HMAC key.  With this change "freshly enable"
only refers to after an HMAC key is loaded, when the setup completion
flag is not set.  The code can be simplified by just checking if an
HMAC key is loaded.

thanks,

Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe
  2021-04-07 10:52 ` [PATCH v5 06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe Roberto Sassu
@ 2021-05-03  0:12   ` Mimi Zohar
  2021-05-03  7:55     ` Roberto Sassu
  0 siblings, 1 reply; 40+ messages in thread
From: Mimi Zohar @ 2021-05-03  0:12 UTC (permalink / raw)
  To: Roberto Sassu, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-kernel

Hi Roberto,

On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> 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;

I agree with the concept, but the function name doesn't provide enough
context.  Perhaps defining a function more along the lines of
"evm_hmac_disabled()" would be more appropriate and at the same time
self documenting.

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

It would also remove the INTEGRITY_NOXATTRS test duplication here.

thanks,

Mimi

>  		return 0;
>  	integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
>  			    dentry->d_name.name, "appraise_metadata",


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 07/12] evm: Allow xattr/attr operations for portable signatures
  2021-04-07 10:52 ` [PATCH v5 07/12] evm: Allow xattr/attr operations for portable signatures Roberto Sassu
@ 2021-05-03  0:12   ` Mimi Zohar
  2021-05-04 14:28     ` Roberto Sassu
  0 siblings, 1 reply; 40+ messages in thread
From: Mimi Zohar @ 2021-05-03  0:12 UTC (permalink / raw)
  To: Roberto Sassu, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-kernel

Hi Roberto,

> 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
> 
> @@ -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;

The original code made an exception for the -ENODATA case.   Using a
ternary operator made sense in that case.   Inverting the test makes
the code less readable.  Please use the standard "if" statement
instead.

thanks,

Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe
  2021-05-03  0:12   ` Mimi Zohar
@ 2021-05-03  7:55     ` Roberto Sassu
  2021-05-03 12:07       ` Mimi Zohar
  2021-05-03 14:15       ` Roberto Sassu
  0 siblings, 2 replies; 40+ messages in thread
From: Roberto Sassu @ 2021-05-03  7:55 UTC (permalink / raw)
  To: Mimi Zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-kernel

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, May 3, 2021 2:13 AM
> Hi Roberto,
> 
> On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> > 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;
> 
> I agree with the concept, but the function name doesn't provide enough
> context.  Perhaps defining a function more along the lines of
> "evm_hmac_disabled()" would be more appropriate and at the same time
> self documenting.

Since the function checks if the passed error can be ignored,
would evm_ignore_error_hmac_disabled() also be ok?

> >  	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)))
> 
> It would also remove the INTEGRITY_NOXATTRS test duplication here.

Ok.

Thanks

Roberto

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

> thanks,
> 
> Mimi
> 
> >  		return 0;
> >  	integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> d_backing_inode(dentry),
> >  			    dentry->d_name.name, "appraise_metadata",


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe
  2021-05-03  7:55     ` Roberto Sassu
@ 2021-05-03 12:07       ` Mimi Zohar
  2021-05-03 14:15       ` Roberto Sassu
  1 sibling, 0 replies; 40+ messages in thread
From: Mimi Zohar @ 2021-05-03 12:07 UTC (permalink / raw)
  To: Roberto Sassu, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-kernel

On Mon, 2021-05-03 at 07:55 +0000, Roberto Sassu wrote:
> 
> > > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c

> > > @@ -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;
> > 
> > I agree with the concept, but the function name doesn't provide enough
> > context.  Perhaps defining a function more along the lines of
> > "evm_hmac_disabled()" would be more appropriate and at the same time
> > self documenting.
> 
> Since the function checks if the passed error can be ignored,
> would evm_ignore_error_hmac_disabled() also be ok?

The purpose of evm_protect_xattr() is to prevent allowing an invalid
security.evm xattr from being re-calculated and updated, making it
valid.   Refer to the first line of the function description.  That
hasn't changed.

One of the reasons for defining a new function is to avoid code
duplication, but it should not come at the expense of clear and easily
understood code.   In this case, the reason for "ignoring" certain
return codes needs to be highlighted, not hidden.  
(is_)evm_hmac_disabled() makes this very clear.

Please update the function description to include the reason why making
an exception is safe.

thanks,

Mimi

> > >  	if (evm_status != INTEGRITY_PASS)
> > >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > d_backing_inode(dentry),
> > >  				    dentry->d_name.name,
> > "appraise_metadata",


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
  2021-04-07 10:52 ` [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata Roberto Sassu
                     ` (2 preceding siblings ...)
  2021-04-07 18:14   ` kernel test robot
@ 2021-05-03 13:00   ` Mimi Zohar
  2021-05-03 14:48     ` Roberto Sassu
  2021-05-03 15:11     ` Roberto Sassu
  3 siblings, 2 replies; 40+ messages in thread
From: Mimi Zohar @ 2021-05-03 13:00 UTC (permalink / raw)
  To: Roberto Sassu, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Christian Brauner, Andreas Gruenbacher

On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:

> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> @@ -389,6 +473,11 @@ static int evm_protect_xattr(struct user_namespace *mnt_userns,
>  	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
>  		return 0;
>  
> +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> +	    !evm_xattr_change(mnt_userns, dentry, xattr_name, xattr_value,
> +			      xattr_value_len))
> +		return 0;
> +

If the purpose of evm_protect_xattr() is to prevent allowing an invalid
security.evm xattr from being re-calculated and updated, making it
valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional.  Any
time there is an attr or xattr change, including setting it to the
existing value, the status flag should be reset.

I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
prevent the file from being resigned.

>  	if (evm_status != INTEGRITY_PASS)
>  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
>  				    dentry->d_name.name, "appraise_metadata",

This would then be updated to if not INTEGRITY_PASS or
INTEGRITY_PASS_IMMUTABLE.  The subsequent "return" would need to be
updated as well.

thanks,

Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe
  2021-05-03  7:55     ` Roberto Sassu
  2021-05-03 12:07       ` Mimi Zohar
@ 2021-05-03 14:15       ` Roberto Sassu
  2021-05-03 14:34         ` Mimi Zohar
  1 sibling, 1 reply; 40+ messages in thread
From: Roberto Sassu @ 2021-05-03 14:15 UTC (permalink / raw)
  To: Roberto Sassu, Mimi Zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-kernel

> From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> Sent: Monday, May 3, 2021 9:55 AM
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Monday, May 3, 2021 2:13 AM
> > Hi Roberto,
> >
> > On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> > > 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;
> >
> > I agree with the concept, but the function name doesn't provide enough
> > context.  Perhaps defining a function more along the lines of
> > "evm_hmac_disabled()" would be more appropriate and at the same time
> > self documenting.
> 
> Since the function checks if the passed error can be ignored,
> would evm_ignore_error_hmac_disabled() also be ok?
> 
> > >  	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)))
> >
> > It would also remove the INTEGRITY_NOXATTRS test duplication here.
> 
> Ok.

Actually, it does not seem a duplication. Currently, INTEGRITY_NOXATTRS
is ignored also when the HMAC key is loaded.

Roberto

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

> Thanks
> 
> Roberto
> 
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Li Jian, Shi Yanli
> 
> > thanks,
> >
> > Mimi
> >
> > >  		return 0;
> > >  	integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > d_backing_inode(dentry),
> > >  			    dentry->d_name.name, "appraise_metadata",


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe
  2021-05-03 14:15       ` Roberto Sassu
@ 2021-05-03 14:34         ` Mimi Zohar
  2021-05-04 13:16           ` Roberto Sassu
  0 siblings, 1 reply; 40+ messages in thread
From: Mimi Zohar @ 2021-05-03 14:34 UTC (permalink / raw)
  To: Roberto Sassu, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-kernel

On Mon, 2021-05-03 at 14:15 +0000, Roberto Sassu wrote:

> > > >  	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)))
> > >
> > > It would also remove the INTEGRITY_NOXATTRS test duplication here.
> > 
> > Ok.
> 
> Actually, it does not seem a duplication. Currently, INTEGRITY_NOXATTRS
> is ignored also when the HMAC key is loaded.

The existing INTEGRITY_NOXATTRS exemption is more general and includes
the new case of when EVM HMAC is disabled.  The additional exemption is
only needed for INTEGRITY_NOLABEL, when EVM HMAC is disabled.

Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
  2021-05-03 13:00   ` [PATCH " Mimi Zohar
@ 2021-05-03 14:48     ` Roberto Sassu
  2021-05-03 15:13       ` Mimi Zohar
  2021-05-03 15:11     ` Roberto Sassu
  1 sibling, 1 reply; 40+ messages in thread
From: Roberto Sassu @ 2021-05-03 14:48 UTC (permalink / raw)
  To: Mimi Zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Christian Brauner, Andreas Gruenbacher

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, May 3, 2021 3:00 PM
> On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> 
> > diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> > @@ -389,6 +473,11 @@ static int evm_protect_xattr(struct
> user_namespace *mnt_userns,
> >  	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
> >  		return 0;
> >
> > +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> > +	    !evm_xattr_change(mnt_userns, dentry, xattr_name, xattr_value,
> > +			      xattr_value_len))
> > +		return 0;
> > +
> 
> If the purpose of evm_protect_xattr() is to prevent allowing an invalid
> security.evm xattr from being re-calculated and updated, making it
> valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional.  Any
> time there is an attr or xattr change, including setting it to the
> existing value, the status flag should be reset.

The status is always reset if evm_protect_xattr() returns 0. This does not
change.

Not making INTEGRITY_PASS_IMMUTABLE conditional would cause issues.
Suppose that the status is INTEGRITY_FAIL. Writing the same xattr would
cause evm_protect_xattr() to return 0 and the HMAC to be updated.

> I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> prevent the file from being resigned.

INTEGRITY_FAIL_IMMUTABLE should be enough to continue the
operation.

Roberto

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

> >  	if (evm_status != INTEGRITY_PASS)
> >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> d_backing_inode(dentry),
> >  				    dentry->d_name.name,
> "appraise_metadata",
> 
> This would then be updated to if not INTEGRITY_PASS or
> INTEGRITY_PASS_IMMUTABLE.  The subsequent "return" would need to be
> updated as well.
> 
> thanks,
> 
> Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
  2021-05-03 13:00   ` [PATCH " Mimi Zohar
  2021-05-03 14:48     ` Roberto Sassu
@ 2021-05-03 15:11     ` Roberto Sassu
  2021-05-03 15:26       ` Mimi Zohar
  1 sibling, 1 reply; 40+ messages in thread
From: Roberto Sassu @ 2021-05-03 15:11 UTC (permalink / raw)
  To: Mimi Zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Christian Brauner, Andreas Gruenbacher

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, May 3, 2021 3:00 PM
> On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> 
> > diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> > @@ -389,6 +473,11 @@ static int evm_protect_xattr(struct
> user_namespace *mnt_userns,
> >  	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
> >  		return 0;
> >
> > +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> > +	    !evm_xattr_change(mnt_userns, dentry, xattr_name, xattr_value,
> > +			      xattr_value_len))
> > +		return 0;
> > +
> 
> If the purpose of evm_protect_xattr() is to prevent allowing an invalid
> security.evm xattr from being re-calculated and updated, making it
> valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional.  Any
> time there is an attr or xattr change, including setting it to the
> existing value, the status flag should be reset.
> 
> I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> prevent the file from being resigned.
> 
> >  	if (evm_status != INTEGRITY_PASS)
> >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> d_backing_inode(dentry),
> >  				    dentry->d_name.name,
> "appraise_metadata",
> 
> This would then be updated to if not INTEGRITY_PASS or
> INTEGRITY_PASS_IMMUTABLE.  The subsequent "return" would need to be
> updated as well.

I agree on the first suggestion, to reduce the number of log messages.
For the second, if you meant that we should return 0 if the status is
INTEGRITY_PASS_IMMUTABLE, I thought we wanted to deny xattr
changes when there is an EVM portable signature.

Roberto

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

> thanks,
> 
> Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
  2021-05-03 14:48     ` Roberto Sassu
@ 2021-05-03 15:13       ` Mimi Zohar
  2021-05-03 15:30         ` Roberto Sassu
  0 siblings, 1 reply; 40+ messages in thread
From: Mimi Zohar @ 2021-05-03 15:13 UTC (permalink / raw)
  To: Roberto Sassu, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Christian Brauner, Andreas Gruenbacher

On Mon, 2021-05-03 at 14:48 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Monday, May 3, 2021 3:00 PM
> > On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> > 
> > > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c
> > > @@ -389,6 +473,11 @@ static int evm_protect_xattr(struct
> > user_namespace *mnt_userns,
> > >  	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
> > >  		return 0;
> > >
> > > +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> > > +	    !evm_xattr_change(mnt_userns, dentry, xattr_name, xattr_value,
> > > +			      xattr_value_len))
> > > +		return 0;
> > > +
> > 
> > If the purpose of evm_protect_xattr() is to prevent allowing an invalid
> > security.evm xattr from being re-calculated and updated, making it
> > valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional.  Any
> > time there is an attr or xattr change, including setting it to the
> > existing value, the status flag should be reset.
> 
> The status is always reset if evm_protect_xattr() returns 0. This does not
> change.
> 
> Not making INTEGRITY_PASS_IMMUTABLE conditional would cause issues.
> Suppose that the status is INTEGRITY_FAIL. Writing the same xattr would
> cause evm_protect_xattr() to return 0 and the HMAC to be updated.

This example is mixing security.evm types.  Please clarify.

> > I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> > prevent the file from being resigned.
> 
> INTEGRITY_FAIL_IMMUTABLE should be enough to continue the
> operation.

Agreed.

Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
  2021-05-03 15:11     ` Roberto Sassu
@ 2021-05-03 15:26       ` Mimi Zohar
  2021-05-03 15:32         ` Roberto Sassu
  0 siblings, 1 reply; 40+ messages in thread
From: Mimi Zohar @ 2021-05-03 15:26 UTC (permalink / raw)
  To: Roberto Sassu, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Christian Brauner, Andreas Gruenbacher

On Mon, 2021-05-03 at 15:11 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Monday, May 3, 2021 3:00 PM
> > On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> > 
> > > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c
> > > @@ -389,6 +473,11 @@ static int evm_protect_xattr(struct
> > user_namespace *mnt_userns,
> > >  	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
> > >  		return 0;
> > >
> > > +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> > > +	    !evm_xattr_change(mnt_userns, dentry, xattr_name, xattr_value,
> > > +			      xattr_value_len))
> > > +		return 0;
> > > +
> > 
> > If the purpose of evm_protect_xattr() is to prevent allowing an invalid
> > security.evm xattr from being re-calculated and updated, making it
> > valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional.  Any
> > time there is an attr or xattr change, including setting it to the
> > existing value, the status flag should be reset.
> > 
> > I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> > prevent the file from being resigned.
> > 
> > >  	if (evm_status != INTEGRITY_PASS)
> > >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > d_backing_inode(dentry),
> > >  				    dentry->d_name.name,
> > "appraise_metadata",
> > 
> > This would then be updated to if not INTEGRITY_PASS or
> > INTEGRITY_PASS_IMMUTABLE.  The subsequent "return" would need to be
> > updated as well.
> 
> I agree on the first suggestion, to reduce the number of log messages.
> For the second, if you meant that we should return 0 if the status is
> INTEGRITY_PASS_IMMUTABLE, I thought we wanted to deny xattr
> changes when there is an EVM portable signature.

Why?  I must be missing something.  As long as we're not relying on the
cached status, allowing the file metadata to be updated shouldn't be an
issue.

Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
  2021-05-03 15:13       ` Mimi Zohar
@ 2021-05-03 15:30         ` Roberto Sassu
  0 siblings, 0 replies; 40+ messages in thread
From: Roberto Sassu @ 2021-05-03 15:30 UTC (permalink / raw)
  To: Mimi Zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Christian Brauner, Andreas Gruenbacher

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, May 3, 2021 5:13 PM
> On Mon, 2021-05-03 at 14:48 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Monday, May 3, 2021 3:00 PM
> > > On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> > >
> > > > diff --git a/security/integrity/evm/evm_main.c
> > > b/security/integrity/evm/evm_main.c
> > > > @@ -389,6 +473,11 @@ static int evm_protect_xattr(struct
> > > user_namespace *mnt_userns,
> > > >  	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
> > > >  		return 0;
> > > >
> > > > +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> > > > +	    !evm_xattr_change(mnt_userns, dentry, xattr_name, xattr_value,
> > > > +			      xattr_value_len))
> > > > +		return 0;
> > > > +
> > >
> > > If the purpose of evm_protect_xattr() is to prevent allowing an invalid
> > > security.evm xattr from being re-calculated and updated, making it
> > > valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional.
> Any
> > > time there is an attr or xattr change, including setting it to the
> > > existing value, the status flag should be reset.
> >
> > The status is always reset if evm_protect_xattr() returns 0. This does not
> > change.
> >
> > Not making INTEGRITY_PASS_IMMUTABLE conditional would cause issues.
> > Suppose that the status is INTEGRITY_FAIL. Writing the same xattr would
> > cause evm_protect_xattr() to return 0 and the HMAC to be updated.
> 
> This example is mixing security.evm types.  Please clarify.

What I meant is that returning 0 when the xattr does not change should
be done only in the positive cases: for INTEGRITY_PASS it is not needed,
for INTEGRITY_PASS_IMMUTABLE it is needed as otherwise
evm_protect_xattr() would return -EPERM.

If your proposal was to return 0 only when the xattr does not change,
without checking the current status, we risk that someone does an
offline attack to corrupt xattrs and when the system is online, he simply
rewrites the same corrupted xattrs to obtain a valid HMAC.

Roberto

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

> > > I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> > > prevent the file from being resigned.
> >
> > INTEGRITY_FAIL_IMMUTABLE should be enough to continue the
> > operation.
> 
> Agreed.
> 
> Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
  2021-05-03 15:26       ` Mimi Zohar
@ 2021-05-03 15:32         ` Roberto Sassu
  2021-05-03 15:48           ` Mimi Zohar
  0 siblings, 1 reply; 40+ messages in thread
From: Roberto Sassu @ 2021-05-03 15:32 UTC (permalink / raw)
  To: Mimi Zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Christian Brauner, Andreas Gruenbacher

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, May 3, 2021 5:26 PM
> On Mon, 2021-05-03 at 15:11 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Monday, May 3, 2021 3:00 PM
> > > On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> > >
> > > > diff --git a/security/integrity/evm/evm_main.c
> > > b/security/integrity/evm/evm_main.c
> > > > @@ -389,6 +473,11 @@ static int evm_protect_xattr(struct
> > > user_namespace *mnt_userns,
> > > >  	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
> > > >  		return 0;
> > > >
> > > > +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> > > > +	    !evm_xattr_change(mnt_userns, dentry, xattr_name, xattr_value,
> > > > +			      xattr_value_len))
> > > > +		return 0;
> > > > +
> > >
> > > If the purpose of evm_protect_xattr() is to prevent allowing an invalid
> > > security.evm xattr from being re-calculated and updated, making it
> > > valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional.
> Any
> > > time there is an attr or xattr change, including setting it to the
> > > existing value, the status flag should be reset.
> > >
> > > I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> > > prevent the file from being resigned.
> > >
> > > >  	if (evm_status != INTEGRITY_PASS)
> > > >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > d_backing_inode(dentry),
> > > >  				    dentry->d_name.name,
> > > "appraise_metadata",
> > >
> > > This would then be updated to if not INTEGRITY_PASS or
> > > INTEGRITY_PASS_IMMUTABLE.  The subsequent "return" would need to
> be
> > > updated as well.
> >
> > I agree on the first suggestion, to reduce the number of log messages.
> > For the second, if you meant that we should return 0 if the status is
> > INTEGRITY_PASS_IMMUTABLE, I thought we wanted to deny xattr
> > changes when there is an EVM portable signature.
> 
> Why?  I must be missing something.  As long as we're not relying on the
> cached status, allowing the file metadata to be updated shouldn't be an
> issue.

We may want to prevent accidental changes, for example.

Roberto

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
  2021-05-03 15:32         ` Roberto Sassu
@ 2021-05-03 15:48           ` Mimi Zohar
  0 siblings, 0 replies; 40+ messages in thread
From: Mimi Zohar @ 2021-05-03 15:48 UTC (permalink / raw)
  To: Roberto Sassu, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel,
	linux-kernel, Christian Brauner, Andreas Gruenbacher

On Mon, 2021-05-03 at 15:32 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Monday, May 3, 2021 5:26 PM
> > On Mon, 2021-05-03 at 15:11 +0000, Roberto Sassu wrote:
> > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > > Sent: Monday, May 3, 2021 3:00 PM
> > > > On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> > > >
> > > > > diff --git a/security/integrity/evm/evm_main.c
> > > > b/security/integrity/evm/evm_main.c
> > > > > @@ -389,6 +473,11 @@ static int evm_protect_xattr(struct
> > > > user_namespace *mnt_userns,
> > > > >  	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
> > > > >  		return 0;
> > > > >
> > > > > +	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> > > > > +	    !evm_xattr_change(mnt_userns, dentry, xattr_name, xattr_value,
> > > > > +			      xattr_value_len))
> > > > > +		return 0;
> > > > > +
> > > >
> > > > If the purpose of evm_protect_xattr() is to prevent allowing an invalid
> > > > security.evm xattr from being re-calculated and updated, making it
> > > > valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional.
> > Any
> > > > time there is an attr or xattr change, including setting it to the
> > > > existing value, the status flag should be reset.
> > > >
> > > > I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> > > > prevent the file from being resigned.
> > > >
> > > > >  	if (evm_status != INTEGRITY_PASS)
> > > > >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > > d_backing_inode(dentry),
> > > > >  				    dentry->d_name.name,
> > > > "appraise_metadata",
> > > >
> > > > This would then be updated to if not INTEGRITY_PASS or
> > > > INTEGRITY_PASS_IMMUTABLE.  The subsequent "return" would need to
> > be
> > > > updated as well.
> > >
> > > I agree on the first suggestion, to reduce the number of log messages.
> > > For the second, if you meant that we should return 0 if the status is
> > > INTEGRITY_PASS_IMMUTABLE, I thought we wanted to deny xattr
> > > changes when there is an EVM portable signature.
> > 
> > Why?  I must be missing something.  As long as we're not relying on the
> > cached status, allowing the file metadata to be updated shouldn't be an
> > issue.
> 
> We may want to prevent accidental changes, for example.

Let's keep it simple, getting the basics working properly first.  Then
we can decide if this is something that we really want/need to defend
against.

thanks,

Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe
  2021-05-03 14:34         ` Mimi Zohar
@ 2021-05-04 13:16           ` Roberto Sassu
  2021-05-04 13:45             ` Mimi Zohar
  0 siblings, 1 reply; 40+ messages in thread
From: Roberto Sassu @ 2021-05-04 13:16 UTC (permalink / raw)
  To: Mimi Zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-kernel

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, May 3, 2021 4:35 PM
> On Mon, 2021-05-03 at 14:15 +0000, Roberto Sassu wrote:
> 
> > > > >  	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)))
> > > >
> > > > It would also remove the INTEGRITY_NOXATTRS test duplication here.
> > >
> > > Ok.
> >
> > Actually, it does not seem a duplication. Currently, INTEGRITY_NOXATTRS
> > is ignored also when the HMAC key is loaded.
> 
> The existing INTEGRITY_NOXATTRS exemption is more general and includes
> the new case of when EVM HMAC is disabled.  The additional exemption is
> only needed for INTEGRITY_NOLABEL, when EVM HMAC is disabled.

Unfortunately, evm_ignore_error_safe() is called by both evm_protect_xattr()
and evm_inode_setattr(). The former requires an exemption also for
INTEGRITY_NOXATTRS.

I would keep the function as it is. In the worst case, when the status is
INTEGRITY_NOXATTRS in evm_inode_setattr(), the function will not
be called.

Roberto

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe
  2021-05-04 13:16           ` Roberto Sassu
@ 2021-05-04 13:45             ` Mimi Zohar
  0 siblings, 0 replies; 40+ messages in thread
From: Mimi Zohar @ 2021-05-04 13:45 UTC (permalink / raw)
  To: Roberto Sassu, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-kernel

On Tue, 2021-05-04 at 13:16 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Monday, May 3, 2021 4:35 PM
> > On Mon, 2021-05-03 at 14:15 +0000, Roberto Sassu wrote:
> > 
> > > > > >  	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)))
> > > > >
> > > > > It would also remove the INTEGRITY_NOXATTRS test duplication here.
> > > >
> > > > Ok.
> > >
> > > Actually, it does not seem a duplication. Currently, INTEGRITY_NOXATTRS
> > > is ignored also when the HMAC key is loaded.
> > 
> > The existing INTEGRITY_NOXATTRS exemption is more general and includes
> > the new case of when EVM HMAC is disabled.  The additional exemption is
> > only needed for INTEGRITY_NOLABEL, when EVM HMAC is disabled.
> 
> Unfortunately, evm_ignore_error_safe() is called by both evm_protect_xattr()
> and evm_inode_setattr(). The former requires an exemption also for
> INTEGRITY_NOXATTRS.
> 
> I would keep the function as it is. In the worst case, when the status is
> INTEGRITY_NOXATTRS in evm_inode_setattr(), the function will not
> be called.

Right, which is another reason for replacing evm_ignore_eror_safe()
with (is_)evm_hmac_disabled() and inlining the error tests.

thanks,

Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 07/12] evm: Allow xattr/attr operations for portable signatures
  2021-05-03  0:12   ` Mimi Zohar
@ 2021-05-04 14:28     ` Roberto Sassu
  2021-05-04 14:49       ` Mimi Zohar
  0 siblings, 1 reply; 40+ messages in thread
From: Roberto Sassu @ 2021-05-04 14:28 UTC (permalink / raw)
  To: Mimi Zohar, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-kernel

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, May 3, 2021 2:13 AM
> Hi Roberto,
> 
> > 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
> >
> > @@ -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;
> 
> The original code made an exception for the -ENODATA case.   Using a
> ternary operator made sense in that case.   Inverting the test makes
> the code less readable.  Please use the standard "if" statement
> instead.

Did I understand correctly that the code should be:

                evm_status = INTEGRITY_NOXATTRS;
                if (rc != -ENODATA) {
                        evm_status = INTEGRITY_FAIL;
                        if (evm_immutable)
                                evm_status = INTEGRITY_FAIL_IMMUTABLE;
                }

Thanks

Roberto

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

> thanks,
> 
> Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 07/12] evm: Allow xattr/attr operations for portable signatures
  2021-05-04 14:28     ` Roberto Sassu
@ 2021-05-04 14:49       ` Mimi Zohar
  0 siblings, 0 replies; 40+ messages in thread
From: Mimi Zohar @ 2021-05-04 14:49 UTC (permalink / raw)
  To: Roberto Sassu, mjg59
  Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-kernel

On Tue, 2021-05-04 at 14:28 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Monday, May 3, 2021 2:13 AM
> > Hi Roberto,
> > 
> > > 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
> > >
> > > @@ -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;
> > 
> > The original code made an exception for the -ENODATA case.   Using a
> > ternary operator made sense in that case.   Inverting the test makes
> > the code less readable.  Please use the standard "if" statement
> > instead.
> 
> Did I understand correctly that the code should be:
> 
>                 evm_status = INTEGRITY_NOXATTRS;
>                 if (rc != -ENODA
>                         evm_status = INTEGRITY_FAIL;
>                         if (evm_immutable)
>                                 evm_status = INTEGRITY_FAIL_IMMUTABLE;
>                 }
> 
 
                if (rc == -ENODATA)
                        evm_status = INTEGRITY_NOXATTRS;
                else if (evm_status == evm_immutable)
                        evm_status = INTEGRITY_FAIL_IMMUTABLE;
                else
                        evm_status = INTEGRITY_FAIL;

I think keeping it simple makes it really clear that ENODATA is an
exception.

thanks,

Mimi


^ permalink raw reply	[flat|nested] 40+ messages in thread

end of thread, back to index

Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-07 10:52 [PATCH v5 00/12] evm: Improve usability of portable signatures Roberto Sassu
2021-04-07 10:52 ` [PATCH v5 01/12] evm: Execute evm_inode_init_security() only when an HMAC key is loaded Roberto Sassu
2021-04-07 10:52 ` [PATCH v5 02/12] evm: Load EVM key in ima_load_x509() to avoid appraisal Roberto Sassu
2021-04-07 10:52 ` [PATCH v5 03/12] evm: Refuse EVM_ALLOW_METADATA_WRITES only if an HMAC key is loaded Roberto Sassu
2021-04-30 20:28   ` Mimi Zohar
2021-04-07 10:52 ` [PATCH v5 04/12] ima: Move ima_reset_appraise_flags() call to post hooks Roberto Sassu
2021-04-07 16:17   ` Casey Schaufler
2021-04-07 16:31     ` Roberto Sassu
2021-04-26 19:49     ` Mimi Zohar
2021-04-07 10:52 ` [PATCH v5 05/12] evm: Introduce evm_status_revalidate() Roberto Sassu
2021-04-07 10:52 ` [PATCH v5 06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe Roberto Sassu
2021-05-03  0:12   ` Mimi Zohar
2021-05-03  7:55     ` Roberto Sassu
2021-05-03 12:07       ` Mimi Zohar
2021-05-03 14:15       ` Roberto Sassu
2021-05-03 14:34         ` Mimi Zohar
2021-05-04 13:16           ` Roberto Sassu
2021-05-04 13:45             ` Mimi Zohar
2021-04-07 10:52 ` [PATCH v5 07/12] evm: Allow xattr/attr operations for portable signatures Roberto Sassu
2021-05-03  0:12   ` Mimi Zohar
2021-05-04 14:28     ` Roberto Sassu
2021-05-04 14:49       ` Mimi Zohar
2021-04-07 10:52 ` [PATCH v5 08/12] evm: Pass user namespace to set/remove xattr hooks Roberto Sassu
2021-04-07 12:06   ` Christian Brauner
2021-04-07 10:52 ` [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata Roberto Sassu
2021-04-07 12:05   ` Christian Brauner
2021-04-07 15:23   ` kernel test robot
2021-04-07 18:14   ` kernel test robot
2021-04-07 19:28     ` [RESEND][PATCH " Roberto Sassu
2021-05-03 13:00   ` [PATCH " Mimi Zohar
2021-05-03 14:48     ` Roberto Sassu
2021-05-03 15:13       ` Mimi Zohar
2021-05-03 15:30         ` Roberto Sassu
2021-05-03 15:11     ` Roberto Sassu
2021-05-03 15:26       ` Mimi Zohar
2021-05-03 15:32         ` Roberto Sassu
2021-05-03 15:48           ` Mimi Zohar
2021-04-07 10:52 ` [PATCH v5 10/12] ima: Allow imasig requirement to be satisfied by EVM portable signatures Roberto Sassu
2021-04-07 10:52 ` [PATCH v5 11/12] ima: Introduce template field evmsig and write to field sig as fallback Roberto Sassu
2021-04-07 10:52 ` [PATCH v5 12/12] ima: Don't remove security.ima if file must not be appraised Roberto Sassu

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git