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

From: Christian Brauner [mailto:christian.brauner@ubuntu.com]
Sent: Thursday, March 25, 2021 1:21 PM
On Thu, Mar 25, 2021 at 01:13:41PM +0100, Christian Brauner wrote:

quoted

On Thu, Mar 25, 2021 at 10:53:43AM +0000, Roberto Sassu wrote:

quoted

quoted

From: Roberto Sassu
Sent: Friday, March 5, 2021 4:19 PM
With the patch to allow xattr/attr operations if a portable signature
verification fails, cp and tar can copy all xattrs/attrs so that at the
end of the process verification succeeds.

However, it might happen that the xattrs/attrs are already set to the
correct value (taken at signing time) and signature verification

succeeds

quoted

quoted

quoted

before the copy has completed. For example, an archive might

contains files

quoted

quoted

quoted

owned by root and the archive is extracted by root.

Then, since portable signatures are immutable, all subsequent

operations

quoted

quoted

quoted

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

quoted

quoted

quoted

space when that operation does not alter the current value of

xattrs/attrs.

quoted

quoted

quoted

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

diff --git a/security/integrity/evm/evm_main.c

b/security/integrity/evm/evm_main.c
index eab536fa260f..a07516dcb920 100644

--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c

@@ -18,6 +18,7 @@
 #include <linux/integrity.h>
 #include <linux/evm.h>
 #include <linux/magic.h>
+#include <linux/posix_acl_xattr.h>

 #include <crypto/hash.h>
 #include <crypto/hash_info.h>

@@ -328,6 +329,79 @@ static enum integrity_status

evm_verify_current_integrity(struct dentry *dentry)
 	return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
 }

+/*
+ * evm_xattr_acl_change - check if passed ACL changes the inode

mode

quoted

quoted

quoted

+ * @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

quoted

quoted

quoted

EVM.
+ *
+ * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
+ */
+static int evm_xattr_acl_change(struct dentry *dentry, const char
*xattr_name,
+				const void *xattr_value, size_t
xattr_value_len)
+{
+	umode_t mode;
+	struct posix_acl *acl = NULL, *acl_res;
+	struct inode *inode = d_backing_inode(dentry);
+	int rc;
+
+	/* UID/GID in ACL have been already converted from user to init ns
*/
+	acl = posix_acl_from_xattr(&init_user_ns, xattr_value,
xattr_value_len);
+	if (!acl)

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

if (IS_ERR_OR_NULL(acl))

quoted

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

About this part, probably it is not correct.

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

mount-idmapped --map-mount b:3001:0:1 /mnt /mnt-idmapped
pushd /mnt
echo "test" > test-file
chown 3001 test-file
chgrp 3001 test-file
chmod 2644 test-file
<check enabled>
setfacl --set u::rw,g::r,o::r,m:r test-file (expected to succeed, caller has

CAP_FSETID, so S_ISGID is not dropped)

quoted

quoted

setfacl --set u::rw,g::r,o::r,m:rw test-file (expected to fail)
pushd /mnt-idmapped
capsh --drop=cap_fsetid -- -c setfacl --set u::rw,g::r,o::r test-file

(expected to succeed, caller is in the owning group of test-file, so S_ISGID is
not dropped)

quoted

quoted

After adding a debug line in posix_acl_update_mode():
printk("%s: %d(%d) %d\n", __func__,

in_group_p(i_gid_into_mnt(mnt_userns, inode)),
__kgid_val(i_gid_into_mnt(mnt_userns, inode)),
capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID));

quoted

quoted

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

test-file

quoted

quoted

[  748.287894] posix_acl_update_mode: 0(3001) 0

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

test-file

quoted

quoted

[   81.184648] posix_acl_update_mode: 1(0) 0
[   81.184663] posix_acl_update_mode: 1(0) 0

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

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

Hey Robert,

s/Robert/Roberto/

Sorry for the typo.

No worries!

Roberto

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