linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Roberto Sassu <roberto.sassu@huawei.com>
To: <zohar@linux.ibm.com>, <mjg59@srcf.ucam.org>
Cc: <linux-integrity@vger.kernel.org>,
	<linux-security-module@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	Roberto Sassu <roberto.sassu@huawei.com>,
	Christian Brauner <christian.brauner@ubuntu.com>,
	Andreas Gruenbacher <agruenba@redhat.com>,
	kernel test robot <lkp@intel.com>
Subject: [PATCH v7 08/12] evm: Allow setxattr() and setattr() for unmodified metadata
Date: Fri, 14 May 2021 17:27:49 +0200	[thread overview]
Message-ID: <20210514152753.982958-9-roberto.sassu@huawei.com> (raw)
In-Reply-To: <20210514152753.982958-1-roberto.sassu@huawei.com>

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.

With this patch, the one that introduces evm_hmac_disabled() and the one
that allows a metadata operation on the INTEGRITY_FAIL_IMMUTABLE error, EVM
portable signatures can be used without disabling metadata verification
(by setting EVM_ALLOW_METADATA_WRITES). Due to keeping metadata
verification enabled, altering immutable metadata protected with a portable
signature that was successfully verified will be denied (existing
behavior).

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 | 113 +++++++++++++++++++++++++++++-
 1 file changed, 112 insertions(+), 1 deletion(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 12cb0ff590af..e8a59d5a0ced 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>
@@ -330,6 +331,92 @@ 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
  *
@@ -396,7 +483,13 @@ static int evm_protect_xattr(struct user_namespace *mnt_userns,
 	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
 		return 0;
 
-	if (evm_status != INTEGRITY_PASS)
+	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 &&
+	    evm_status != INTEGRITY_PASS_IMMUTABLE)
 		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 				    dentry->d_name.name, "appraise_metadata",
 				    integrity_status_msg[evm_status],
@@ -552,6 +645,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
@@ -582,6 +688,11 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 	    (evm_status == INTEGRITY_FAIL_IMMUTABLE) ||
 	    (evm_hmac_disabled() && evm_status == INTEGRITY_NOLABEL))
 		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.25.1


  parent reply	other threads:[~2021-05-14 15:29 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-14 15:27 [PATCH v7 00/12] evm: Improve usability of portable signatures Roberto Sassu
2021-05-14 15:27 ` [PATCH v7 01/12] evm: Execute evm_inode_init_security() only when an HMAC key is loaded Roberto Sassu
2021-05-14 15:27 ` [PATCH v7 02/12] evm: Load EVM key in ima_load_x509() to avoid appraisal Roberto Sassu
2021-05-14 15:27 ` [PATCH v7 03/12] evm: Refuse EVM_ALLOW_METADATA_WRITES only if an HMAC key is loaded Roberto Sassu
2021-05-14 15:27 ` [PATCH v7 04/12] evm: Introduce evm_revalidate_status() Roberto Sassu
2021-05-14 15:27 ` [PATCH v7 05/12] evm: Introduce evm_hmac_disabled() to safely ignore verification errors Roberto Sassu
2021-05-20  8:48   ` [RESEND][PATCH " Roberto Sassu
2021-05-20  8:51     ` Roberto Sassu
2021-05-14 15:27 ` [PATCH v7 06/12] evm: Allow xattr/attr operations for portable signatures Roberto Sassu
2021-05-14 15:27 ` [PATCH v7 07/12] evm: Pass user namespace to set/remove xattr hooks Roberto Sassu
2021-05-14 15:27 ` Roberto Sassu [this message]
2021-05-14 15:27 ` [PATCH v7 09/12] evm: Deprecate EVM_ALLOW_METADATA_WRITES Roberto Sassu
2021-05-14 15:27 ` [PATCH v7 10/12] ima: Allow imasig requirement to be satisfied by EVM portable signatures Roberto Sassu
2021-05-14 15:27 ` [PATCH v7 11/12] ima: Introduce template field evmsig and write to field sig as fallback Roberto Sassu
2021-05-14 15:27 ` [PATCH v7 12/12] ima: Don't remove security.ima if file must not be appraised Roberto Sassu
2021-05-20 18:55 ` [PATCH v7 00/12] evm: Improve usability of portable signatures Mimi Zohar
2021-05-21  7:07   ` Roberto Sassu
2021-05-21 17:31     ` Mimi Zohar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210514152753.982958-9-roberto.sassu@huawei.com \
    --to=roberto.sassu@huawei.com \
    --cc=agruenba@redhat.com \
    --cc=christian.brauner@ubuntu.com \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=lkp@intel.com \
    --cc=mjg59@srcf.ucam.org \
    --cc=zohar@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).