[RESEND][PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
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
Cc: Andreas Gruenbacher
Reported-by: kernel test robot
Signed-off-by: Roberto Sassu
Reviewed-by: Christian Brauner
---
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
#include
#include
+#include
#include
#include
@@ -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(_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, , _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(_user_ns, dentry, xattr_name, _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",
@@
Re: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
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
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, , _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(_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, , _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-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
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
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, , _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(_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, , _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-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
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
> Cc: Andreas Gruenbacher
> Signed-off-by: Roberto Sassu
> ---
> 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
> #include
> #include
> +#include
>
> #include
> #include
> @@ -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(_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, , _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(_user_ns, dentry, xattr_name, _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
> + 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 ==
[PATCH v5 09/12] evm: Allow setxattr() and setattr() for unmodified metadata
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
Cc: Andreas Gruenbacher
Signed-off-by: Roberto Sassu
---
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
#include
#include
+#include
#include
#include
@@ -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(_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, , _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(_user_ns, dentry, xattr_name, _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)
