diff mbox series

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

Message ID 20210505113329.1410943-4-roberto.sassu@huawei.com (mailing list archive)
State New, archived
Headers show
Series evm: Improve usability of portable signatures | expand

Commit Message

Roberto Sassu May 5, 2021, 11:33 a.m. UTC
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 | 111 +++++++++++++++++++++++++++++-
 1 file changed, 110 insertions(+), 1 deletion(-)

Comments

Mimi Zohar May 11, 2021, 2:11 p.m. UTC | #1
Hi Roberto,

On Wed, 2021-05-05 at 13:33 +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.

I must be missing something.  If both the IMA and EVM status flags are
reset after xattr or attr modification, do we really need to prevent
any metadata - same or different - changes?  Both evm_protect_xattr()
and evm_inode_setattr() would need to be modified to allow
INTEGRITY_PASS_IMMUTABLE.

thanks,

Mimi
Roberto Sassu May 11, 2021, 2:21 p.m. UTC | #2
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Tuesday, May 11, 2021 4:12 PM
> Hi Roberto,
> 
> On Wed, 2021-05-05 at 13:33 +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.
> 
> I must be missing something.  If both the IMA and EVM status flags are
> reset after xattr or attr modification, do we really need to prevent
> any metadata - same or different - changes?  Both evm_protect_xattr()
> and evm_inode_setattr() would need to be modified to allow
> INTEGRITY_PASS_IMMUTABLE.

Hi Mimi

yes, given that the IMA and EVM flags are reset, it should not be
a problem to allow changes. However, I think it is useful to keep
the current behavior. For example, it would prevent an accidental
change of the SELinux label during the relabeling process.

Roberto

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

> thanks,
> 
> Mimi
Mimi Zohar May 11, 2021, 2:40 p.m. UTC | #3
On Tue, 2021-05-11 at 14:21 +0000, Roberto Sassu wrote:
> > 
> > On Wed, 2021-05-05 at 13:33 +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.
> > 
> > I must be missing something.  If both the IMA and EVM status flags are
> > reset after xattr or attr modification, do we really need to prevent
> > any metadata - same or different - changes?  Both evm_protect_xattr()
> > and evm_inode_setattr() would need to be modified to allow
> > INTEGRITY_PASS_IMMUTABLE.
> 
> yes, given that the IMA and EVM flags are reset, it should not be
> a problem to allow changes. However, I think it is useful to keep
> the current behavior. For example, it would prevent an accidental
> change of the SELinux label during the relabeling process.

I understand we might want to prevent accidental or malicious changes,
but that isn't the purpose of this patch set.  The patch description
would also need to be updated to reflect the real purpose.

thanks,

Mimi
Roberto Sassu May 11, 2021, 2:54 p.m. UTC | #4
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Tuesday, May 11, 2021 4:41 PM
> On Tue, 2021-05-11 at 14:21 +0000, Roberto Sassu wrote:
> > >
> > > On Wed, 2021-05-05 at 13:33 +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.
> > >
> > > I must be missing something.  If both the IMA and EVM status flags are
> > > reset after xattr or attr modification, do we really need to prevent
> > > any metadata - same or different - changes?  Both evm_protect_xattr()
> > > and evm_inode_setattr() would need to be modified to allow
> > > INTEGRITY_PASS_IMMUTABLE.
> >
> > yes, given that the IMA and EVM flags are reset, it should not be
> > a problem to allow changes. However, I think it is useful to keep
> > the current behavior. For example, it would prevent an accidental
> > change of the SELinux label during the relabeling process.
> 
> I understand we might want to prevent accidental or malicious changes,
> but that isn't the purpose of this patch set.  The patch description
> would also need to be updated to reflect the real purpose.

We would be changing the expectation that metadata changes
are denied, which was defined with the original patches.

I would prefer to keep the current behavior, but if your suggestion
is to allow metadata changes, I will modify the patch set.

Roberto

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

> thanks,
> 
> Mimi
Mimi Zohar May 11, 2021, 8 p.m. UTC | #5
On Tue, 2021-05-11 at 14:54 +0000, Roberto Sassu wrote:
> > On Tue, 2021-05-11 at 14:21 +0000, Roberto Sassu wrote:
> > > >
> > > > On Wed, 2021-05-05 at 13:33 +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.
> > > >
> > > > I must be missing something.  If both the IMA and EVM status flags are
> > > > reset after xattr or attr modification, do we really need to prevent
> > > > any metadata - same or different - changes?  Both evm_protect_xattr()
> > > > and evm_inode_setattr() would need to be modified to allow
> > > > INTEGRITY_PASS_IMMUTABLE.
> > >
> > > yes, given that the IMA and EVM flags are reset, it should not be
> > > a problem to allow changes. However, I think it is useful to keep
> > > the current behavior. For example, it would prevent an accidental
> > > change of the SELinux label during the relabeling process.
> > 
> > I understand we might want to prevent accidental or malicious changes,
> > but that isn't the purpose of this patch set.  The patch description
> > would also need to be updated to reflect the real purpose.
> 
> We would be changing the expectation that metadata changes
> are denied, which was defined with the original patches.
> 
> I would prefer to keep the current behavior, but if your suggestion
> is to allow metadata changes, I will modify the patch set.

Please re-write the patch description appropriately.

thanks,

Mimi
diff mbox series

Patch

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 3745c08c09e6..793e6556b004 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,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
  *
@@ -396,7 +481,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],
@@ -539,6 +630,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
@@ -569,6 +673,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);