diff mbox series

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

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

Commit Message

Roberto Sassu Nov. 11, 2020, 9:22 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.

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

Comments

kernel test robot Nov. 18, 2020, 5:58 p.m. UTC | #1
Hi Roberto,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on integrity/next-integrity]
[also build test ERROR on linus/master v5.10-rc4 next-20201118]
[cannot apply to security/next-testing]
[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/20201111-172839
base:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: h8300-randconfig-r023-20201118 (attached as .config)
compiler: h8300-linux-gcc (GCC) 9.3.0
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
        # https://github.com/0day-ci/linux/commit/980f241069945bb56197027fd204689af8ec07e5
        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/20201111-172839
        git checkout 980f241069945bb56197027fd204689af8ec07e5
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=h8300 

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:339:7: error: implicit declaration of function 'posix_acl_update_mode'; did you mean 'posix_acl_equiv_mode'? [-Werror=implicit-function-declaration]
     339 |  rc = posix_acl_update_mode(inode, &mode, &acl_res);
         |       ^~~~~~~~~~~~~~~~~~~~~
         |       posix_acl_equiv_mode
   cc1: some warnings being treated as errors

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

   313	
   314	/*
   315	 * evm_xattr_acl_change - check if passed ACL changes the inode mode
   316	 * @dentry: pointer to the affected dentry
   317	 * @xattr_name: requested xattr
   318	 * @xattr_value: requested xattr value
   319	 * @xattr_value_len: requested xattr value length
   320	 *
   321	 * Check if passed ACL changes the inode mode, which is protected by EVM.
   322	 *
   323	 * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
   324	 */
   325	static int evm_xattr_acl_change(struct dentry *dentry, const char *xattr_name,
   326					const void *xattr_value, size_t xattr_value_len)
   327	{
   328		umode_t mode;
   329		struct posix_acl *acl = NULL, *acl_res;
   330		struct inode *inode = d_backing_inode(dentry);
   331		int rc;
   332	
   333		/* UID/GID in ACL have been already converted from user to init ns */
   334		acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);
   335		if (!acl)
   336			return 1;
   337	
   338		acl_res = acl;
 > 339		rc = posix_acl_update_mode(inode, &mode, &acl_res);
   340	
   341		posix_acl_release(acl);
   342	
   343		if (rc)
   344			return 1;
   345	
   346		if (acl_res && inode->i_mode != mode)
   347			return 1;
   348	
   349		return 0;
   350	}
   351	

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

Patch

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 60ab700735ea..1b2eea30e11e 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>
@@ -310,6 +311,78 @@  static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
 	return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
 }
 
+/*
+ * evm_xattr_acl_change - check if passed ACL changes the inode mode
+ * @dentry: pointer to the affected dentry
+ * @xattr_name: requested xattr
+ * @xattr_value: requested xattr value
+ * @xattr_value_len: requested xattr value length
+ *
+ * Check if passed ACL changes the inode mode, which is protected by EVM.
+ *
+ * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
+ */
+static int evm_xattr_acl_change(struct dentry *dentry, const char *xattr_name,
+				const void *xattr_value, size_t xattr_value_len)
+{
+	umode_t mode;
+	struct posix_acl *acl = NULL, *acl_res;
+	struct inode *inode = d_backing_inode(dentry);
+	int rc;
+
+	/* UID/GID in ACL have been already converted from user to init ns */
+	acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);
+	if (!acl)
+		return 1;
+
+	acl_res = acl;
+	rc = posix_acl_update_mode(inode, &mode, &acl_res);
+
+	posix_acl_release(acl);
+
+	if (rc)
+		return 1;
+
+	if (acl_res && inode->i_mode != mode)
+		return 1;
+
+	return 0;
+}
+
+/*
+ * evm_xattr_change - check if passed xattr value differs from current value
+ * @dentry: pointer to the affected dentry
+ * @xattr_name: requested xattr
+ * @xattr_value: requested xattr value
+ * @xattr_value_len: requested xattr value length
+ *
+ * Check if passed xattr value differs from current value.
+ *
+ * Returns 1 if passed xattr value differs from current value, 0 otherwise.
+ */
+static int evm_xattr_change(struct dentry *dentry, const char *xattr_name,
+			    const void *xattr_value, size_t xattr_value_len)
+{
+	char *xattr_data = NULL;
+	int rc = 0;
+
+	if (posix_xattr_acl(xattr_name))
+		return evm_xattr_acl_change(dentry, xattr_name, xattr_value,
+					    xattr_value_len);
+
+	rc = vfs_getxattr_alloc(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
  *
@@ -376,6 +449,10 @@  static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
 	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
 		return 0;
 
+	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
+	    !evm_xattr_change(dentry, xattr_name, xattr_value, xattr_value_len))
+		return 0;
+
 	if (evm_status != INTEGRITY_PASS)
 		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 				    dentry->d_name.name, "appraise_metadata",
@@ -515,6 +592,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
@@ -551,6 +641,10 @@  int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 	     !(evm_initialized & EVM_INIT_HMAC)))
 		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);