EVM portable signatures are particularly suitable for the protection of
metadata of immutable files where metadata is signed by a software vendor.
They can be used for example in conjunction with an IMA policy that
appraises only executed and memory mapped files.
However, some usability issues are still unsolved, especially when EVM is
used without loading an HMAC key. This patch set attempts to fix the open
issues.
Patch 1 allows EVM to be used without loading an HMAC key. Patch 2 avoids
appraisal verification of public keys (they are already verified by the key
subsystem).
Patches 3-5 allow metadata verification to be turned off when no HMAC key
is loaded and to use this mode in a safe way (by ensuring that IMA
revalidates metadata when there is a change).
Patches 6-9 make portable signatures more usable if metadata verification
is not turned off, by ignoring the INTEGRITY_NOLABEL and INTEGRITY_NOXATTS
errors when possible, by accepting any metadata modification until
signature verification succeeds (useful when xattrs/attrs are copied
sequentially from a source) and by allowing operations that don't change
metadata.
Patch 10 makes it possible to use portable signatures when the IMA policy
requires file signatures and patch 11 shows portable signatures in the
measurement list when the ima-sig template is selected.
Lastly, patch 12 avoids undesired removal of security.ima when a file is
not selected by the IMA policy.
Changelog
v4:
- add patch to pass mnt_userns to EVM inode set/remove xattr hooks
(suggested by Christian Brauner)
- pass mnt_userns to posix_acl_update_mode()
- use IS_ERR_OR_NULL() in evm_xattr_acl_change() (suggested by Mimi)
v3:
- introduce evm_ignore_error_safe() to correctly ignore INTEGRITY_NOLABEL
and INTEGRITY_NOXATTRS errors
- fix an error in evm_xattr_acl_change()
- replace #ifndef with !IS_ENABLED() in integrity_load_keys()
- reintroduce ima_inode_removexattr()
- adapt patches to apply on top of the idmapped mounts patch set
v2:
- replace EVM_RESET_STATUS flag with evm_status_revalidate()
- introduce IMA post hooks ima_inode_post_setxattr() and
ima_inode_post_removexattr()
- remove ima_inode_removexattr()
- ignore INTEGRITY_NOLABEL error if the HMAC key is not loaded
v1:
- introduce EVM_RESET_STATUS integrity flag instead of clearing IMA flag
- introduce new template field evmsig
- add description of evm_xattr_acl_change() and evm_xattr_change()
Roberto Sassu (12):
evm: Execute evm_inode_init_security() only when an HMAC key is loaded
evm: Load EVM key in ima_load_x509() to avoid appraisal
evm: Refuse EVM_ALLOW_METADATA_WRITES only if an HMAC key is loaded
ima: Move ima_reset_appraise_flags() call to post hooks
evm: Introduce evm_status_revalidate()
evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are
safe
evm: Allow xattr/attr operations for portable signatures
evm: Pass user namespace to set/remove xattr hooks
evm: Allow setxattr() and setattr() for unmodified metadata
ima: Allow imasig requirement to be satisfied by EVM portable
signatures
ima: Introduce template field evmsig and write to field sig as
fallback
ima: Don't remove security.ima if file must not be appraised
Documentation/ABI/testing/evm | 5 +-
Documentation/security/IMA-templates.rst | 4 +-
fs/xattr.c | 2 +
include/linux/evm.h | 18 +-
include/linux/ima.h | 18 ++
include/linux/integrity.h | 1 +
security/integrity/evm/evm_main.c | 216 ++++++++++++++++++++--
security/integrity/evm/evm_secfs.c | 4 +-
security/integrity/iint.c | 4 +-
security/integrity/ima/ima_appraise.c | 55 ++++--
security/integrity/ima/ima_init.c | 4 +
security/integrity/ima/ima_template.c | 2 +
security/integrity/ima/ima_template_lib.c | 33 +++-
security/integrity/ima/ima_template_lib.h | 2 +
security/security.c | 5 +-
15 files changed, 329 insertions(+), 44 deletions(-)
--
2.26.2
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 <[email protected]>
Cc: Andreas Gruenbacher <[email protected]>
Signed-off-by: Roberto Sassu <[email protected]>
---
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 <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,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(&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;
+
+ 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
*
@@ -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)
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
@@ -562,6 +664,11 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
(evm_status == INTEGRITY_FAIL_IMMUTABLE) ||
(evm_ignore_error_safe(evm_status)))
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.26.2
The public builtin keys do not need to be appraised by IMA as the
restriction on the IMA/EVM trusted keyrings ensures that a key can be
loaded only if it is signed with a key on the builtin or secondary
keyrings.
However, when evm_load_x509() is called, appraisal is already enabled and
a valid IMA signature must be added to the EVM key to pass verification.
Since the restriction is applied on both IMA and EVM trusted keyrings, it
is safe to disable appraisal also when the EVM key is loaded. This patch
calls evm_load_x509() inside ima_load_x509() if CONFIG_IMA_LOAD_X509 is
enabled, which crosses the normal IMA and EVM boundary.
Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Mimi Zohar <[email protected]>
---
security/integrity/iint.c | 4 +++-
security/integrity/ima/ima_init.c | 4 ++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 0ba01847e836..d66b94c7c8d5 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -208,7 +208,9 @@ int integrity_kernel_read(struct file *file, loff_t offset,
void __init integrity_load_keys(void)
{
ima_load_x509();
- evm_load_x509();
+
+ if (!IS_ENABLED(CONFIG_IMA_LOAD_X509))
+ evm_load_x509();
}
static int __init integrity_fs_init(void)
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 6e8742916d1d..5076a7d9d23e 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -108,6 +108,10 @@ void __init ima_load_x509(void)
ima_policy_flag &= ~unset_flags;
integrity_load_x509(INTEGRITY_KEYRING_IMA, CONFIG_IMA_X509_PATH);
+
+ /* load also EVM key to avoid appraisal */
+ evm_load_x509();
+
ima_policy_flag |= unset_flags;
}
#endif
--
2.26.2
When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
metadata. Its main purpose is to allow users to freely set metadata when it
is protected by a portable signature, until an HMAC key is loaded.
However, callers of evm_verifyxattr() are not notified about metadata
changes and continue to rely on the last status returned by the function.
For example IMA, since it caches the appraisal result, will not call again
evm_verifyxattr() until the appraisal flags are cleared, and will grant
access to the file even if there was a metadata operation that made the
portable signature invalid.
This patch introduces evm_status_revalidate(), which callers of
evm_verifyxattr() can use in their xattr post hooks to determine whether
re-validation is necessary and to do the proper actions. IMA calls it in
its xattr post hooks to reset the appraisal flags, so that the EVM status
is re-evaluated after a metadata operation.
Lastly, this patch also adds a call to evm_reset_status() in
evm_inode_post_setattr() to invalidate the cached EVM status after a
setattr operation.
Signed-off-by: Roberto Sassu <[email protected]>
---
include/linux/evm.h | 6 +++++
security/integrity/evm/evm_main.c | 33 +++++++++++++++++++++++----
security/integrity/ima/ima_appraise.c | 8 ++++---
3 files changed, 40 insertions(+), 7 deletions(-)
diff --git a/include/linux/evm.h b/include/linux/evm.h
index 8302bc29bb35..e5b7bcb152b9 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -35,6 +35,7 @@ extern void evm_inode_post_removexattr(struct dentry *dentry,
extern int evm_inode_init_security(struct inode *inode,
const struct xattr *xattr_array,
struct xattr *evm);
+extern bool evm_status_revalidate(const char *xattr_name);
#ifdef CONFIG_FS_POSIX_ACL
extern int posix_xattr_acl(const char *xattrname);
#else
@@ -104,5 +105,10 @@ static inline int evm_inode_init_security(struct inode *inode,
return 0;
}
+static inline bool evm_status_revalidate(const char *xattr_name)
+{
+ return false;
+}
+
#endif /* CONFIG_EVM */
#endif /* LINUX_EVM_H */
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 7ac5204c8d1f..998818283fda 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -425,6 +425,30 @@ static void evm_reset_status(struct inode *inode)
iint->evm_status = INTEGRITY_UNKNOWN;
}
+/**
+ * evm_status_revalidate - report whether EVM status re-validation is necessary
+ * @xattr_name: pointer to the affected extended attribute name
+ *
+ * Report whether callers of evm_verifyxattr() should re-validate the
+ * EVM status.
+ *
+ * Return true if re-validation is necessary, false otherwise.
+ */
+bool evm_status_revalidate(const char *xattr_name)
+{
+ if (!evm_key_loaded())
+ return false;
+
+ /* evm_inode_post_setattr() passes NULL */
+ if (!xattr_name)
+ return true;
+
+ if (!evm_protected_xattr(xattr_name) && !posix_xattr_acl(xattr_name))
+ return false;
+
+ return true;
+}
+
/**
* evm_inode_post_setxattr - update 'security.evm' to reflect the changes
* @dentry: pointer to the affected dentry
@@ -441,8 +465,7 @@ static void evm_reset_status(struct inode *inode)
void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
const void *xattr_value, size_t xattr_value_len)
{
- if (!evm_key_loaded() || (!evm_protected_xattr(xattr_name)
- && !posix_xattr_acl(xattr_name)))
+ if (!evm_status_revalidate(xattr_name))
return;
evm_reset_status(dentry->d_inode);
@@ -462,7 +485,7 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
*/
void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
{
- if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
+ if (!evm_status_revalidate(xattr_name))
return;
evm_reset_status(dentry->d_inode);
@@ -513,9 +536,11 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
*/
void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
{
- if (!evm_key_loaded())
+ if (!evm_status_revalidate(NULL))
return;
+ evm_reset_status(dentry->d_inode);
+
if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
evm_update_evmxattr(dentry, NULL, NULL, 0);
}
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 1f029e4c8d7f..d4b8db1acadd 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -586,13 +586,15 @@ void ima_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
const void *xattr_value, size_t xattr_value_len)
{
const struct evm_ima_xattr_data *xvalue = xattr_value;
+ int digsig = 0;
int result;
result = ima_protect_xattr(dentry, xattr_name, xattr_value,
xattr_value_len);
if (result == 1)
- ima_reset_appraise_flags(d_backing_inode(dentry),
- xvalue->type == EVM_IMA_XATTR_DIGSIG);
+ digsig = (xvalue->type == EVM_IMA_XATTR_DIGSIG);
+ if (result == 1 || evm_status_revalidate(xattr_name))
+ ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
}
int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
@@ -611,6 +613,6 @@ void ima_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
int result;
result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
- if (result == 1)
+ if (result == 1 || evm_status_revalidate(xattr_name))
ima_reset_appraise_flags(d_backing_inode(dentry), 0);
}
--
2.26.2
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 <[email protected]>
> Cc: Andreas Gruenbacher <[email protected]>
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> 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 <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,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(&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;
> +
> + 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);
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 <[email protected]>
> + 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)
> 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
> @@ -562,6 +664,11 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
> (evm_status == INTEGRITY_FAIL_IMMUTABLE) ||
> (evm_ignore_error_safe(evm_status)))
> 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.26.2
>
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 <[email protected]>
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, &mode, &acl_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(&init_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, &mode, &acl_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/[email protected]
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 <[email protected]>
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, &mode, &acl_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(&init_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, &mode, &acl_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/[email protected]
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 <[email protected]>
Cc: Andreas Gruenbacher <[email protected]>
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Christian Brauner <[email protected]>
---
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 <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,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
*
@@ -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",
@@ -532,6 +622,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
@@ -562,6 +665,11 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
(evm_status == INTEGRITY_FAIL_IMMUTABLE) ||
(evm_ignore_error_safe(evm_status)))
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.26.2
On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> @@ -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 the purpose of evm_protect_xattr() is to prevent allowing an invalid
security.evm xattr from being re-calculated and updated, making it
valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional. Any
time there is an attr or xattr change, including setting it to the
existing value, the status flag should be reset.
I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
prevent the file from being resigned.
> if (evm_status != INTEGRITY_PASS)
> integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
> dentry->d_name.name, "appraise_metadata",
This would then be updated to if not INTEGRITY_PASS or
INTEGRITY_PASS_IMMUTABLE. The subsequent "return" would need to be
updated as well.
thanks,
Mimi
> From: Mimi Zohar [mailto:[email protected]]
> Sent: Monday, May 3, 2021 3:00 PM
> On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
>
> > diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> > @@ -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 the purpose of evm_protect_xattr() is to prevent allowing an invalid
> security.evm xattr from being re-calculated and updated, making it
> valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional. Any
> time there is an attr or xattr change, including setting it to the
> existing value, the status flag should be reset.
The status is always reset if evm_protect_xattr() returns 0. This does not
change.
Not making INTEGRITY_PASS_IMMUTABLE conditional would cause issues.
Suppose that the status is INTEGRITY_FAIL. Writing the same xattr would
cause evm_protect_xattr() to return 0 and the HMAC to be updated.
> I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> prevent the file from being resigned.
INTEGRITY_FAIL_IMMUTABLE should be enough to continue the
operation.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> > if (evm_status != INTEGRITY_PASS)
> > integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> d_backing_inode(dentry),
> > dentry->d_name.name,
> "appraise_metadata",
>
> This would then be updated to if not INTEGRITY_PASS or
> INTEGRITY_PASS_IMMUTABLE. The subsequent "return" would need to be
> updated as well.
>
> thanks,
>
> Mimi
On Mon, 2021-05-03 at 14:48 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:[email protected]]
> > Sent: Monday, May 3, 2021 3:00 PM
> > On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> >
> > > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c
> > > @@ -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 the purpose of evm_protect_xattr() is to prevent allowing an invalid
> > security.evm xattr from being re-calculated and updated, making it
> > valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional. Any
> > time there is an attr or xattr change, including setting it to the
> > existing value, the status flag should be reset.
>
> The status is always reset if evm_protect_xattr() returns 0. This does not
> change.
>
> Not making INTEGRITY_PASS_IMMUTABLE conditional would cause issues.
> Suppose that the status is INTEGRITY_FAIL. Writing the same xattr would
> cause evm_protect_xattr() to return 0 and the HMAC to be updated.
This example is mixing security.evm types. Please clarify.
> > I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> > prevent the file from being resigned.
>
> INTEGRITY_FAIL_IMMUTABLE should be enough to continue the
> operation.
Agreed.
Mimi
> From: Mimi Zohar [mailto:[email protected]]
> Sent: Monday, May 3, 2021 5:13 PM
> On Mon, 2021-05-03 at 14:48 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:[email protected]]
> > > Sent: Monday, May 3, 2021 3:00 PM
> > > On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> > >
> > > > diff --git a/security/integrity/evm/evm_main.c
> > > b/security/integrity/evm/evm_main.c
> > > > @@ -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 the purpose of evm_protect_xattr() is to prevent allowing an invalid
> > > security.evm xattr from being re-calculated and updated, making it
> > > valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional.
> Any
> > > time there is an attr or xattr change, including setting it to the
> > > existing value, the status flag should be reset.
> >
> > The status is always reset if evm_protect_xattr() returns 0. This does not
> > change.
> >
> > Not making INTEGRITY_PASS_IMMUTABLE conditional would cause issues.
> > Suppose that the status is INTEGRITY_FAIL. Writing the same xattr would
> > cause evm_protect_xattr() to return 0 and the HMAC to be updated.
>
> This example is mixing security.evm types. Please clarify.
What I meant is that returning 0 when the xattr does not change should
be done only in the positive cases: for INTEGRITY_PASS it is not needed,
for INTEGRITY_PASS_IMMUTABLE it is needed as otherwise
evm_protect_xattr() would return -EPERM.
If your proposal was to return 0 only when the xattr does not change,
without checking the current status, we risk that someone does an
offline attack to corrupt xattrs and when the system is online, he simply
rewrites the same corrupted xattrs to obtain a valid HMAC.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> > > I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> > > prevent the file from being resigned.
> >
> > INTEGRITY_FAIL_IMMUTABLE should be enough to continue the
> > operation.
>
> Agreed.
>
> Mimi
> From: Mimi Zohar [mailto:[email protected]]
> Sent: Monday, May 3, 2021 5:26 PM
> On Mon, 2021-05-03 at 15:11 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:[email protected]]
> > > Sent: Monday, May 3, 2021 3:00 PM
> > > On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> > >
> > > > diff --git a/security/integrity/evm/evm_main.c
> > > b/security/integrity/evm/evm_main.c
> > > > @@ -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 the purpose of evm_protect_xattr() is to prevent allowing an invalid
> > > security.evm xattr from being re-calculated and updated, making it
> > > valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional.
> Any
> > > time there is an attr or xattr change, including setting it to the
> > > existing value, the status flag should be reset.
> > >
> > > I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> > > prevent the file from being resigned.
> > >
> > > > if (evm_status != INTEGRITY_PASS)
> > > > integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > d_backing_inode(dentry),
> > > > dentry->d_name.name,
> > > "appraise_metadata",
> > >
> > > This would then be updated to if not INTEGRITY_PASS or
> > > INTEGRITY_PASS_IMMUTABLE. The subsequent "return" would need to
> be
> > > updated as well.
> >
> > I agree on the first suggestion, to reduce the number of log messages.
> > For the second, if you meant that we should return 0 if the status is
> > INTEGRITY_PASS_IMMUTABLE, I thought we wanted to deny xattr
> > changes when there is an EVM portable signature.
>
> Why? I must be missing something. As long as we're not relying on the
> cached status, allowing the file metadata to be updated shouldn't be an
> issue.
We may want to prevent accidental changes, for example.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> From: Mimi Zohar [mailto:[email protected]]
> Sent: Monday, May 3, 2021 3:00 PM
> On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
>
> > diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> > @@ -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 the purpose of evm_protect_xattr() is to prevent allowing an invalid
> security.evm xattr from being re-calculated and updated, making it
> valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional. Any
> time there is an attr or xattr change, including setting it to the
> existing value, the status flag should be reset.
>
> I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> prevent the file from being resigned.
>
> > if (evm_status != INTEGRITY_PASS)
> > integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> d_backing_inode(dentry),
> > dentry->d_name.name,
> "appraise_metadata",
>
> This would then be updated to if not INTEGRITY_PASS or
> INTEGRITY_PASS_IMMUTABLE. The subsequent "return" would need to be
> updated as well.
I agree on the first suggestion, to reduce the number of log messages.
For the second, if you meant that we should return 0 if the status is
INTEGRITY_PASS_IMMUTABLE, I thought we wanted to deny xattr
changes when there is an EVM portable signature.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> thanks,
>
> Mimi
On Mon, 2021-05-03 at 15:32 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:[email protected]]
> > Sent: Monday, May 3, 2021 5:26 PM
> > On Mon, 2021-05-03 at 15:11 +0000, Roberto Sassu wrote:
> > > > From: Mimi Zohar [mailto:[email protected]]
> > > > Sent: Monday, May 3, 2021 3:00 PM
> > > > On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> > > >
> > > > > diff --git a/security/integrity/evm/evm_main.c
> > > > b/security/integrity/evm/evm_main.c
> > > > > @@ -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 the purpose of evm_protect_xattr() is to prevent allowing an invalid
> > > > security.evm xattr from being re-calculated and updated, making it
> > > > valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional.
> > Any
> > > > time there is an attr or xattr change, including setting it to the
> > > > existing value, the status flag should be reset.
> > > >
> > > > I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> > > > prevent the file from being resigned.
> > > >
> > > > > if (evm_status != INTEGRITY_PASS)
> > > > > integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > > d_backing_inode(dentry),
> > > > > dentry->d_name.name,
> > > > "appraise_metadata",
> > > >
> > > > This would then be updated to if not INTEGRITY_PASS or
> > > > INTEGRITY_PASS_IMMUTABLE. The subsequent "return" would need to
> > be
> > > > updated as well.
> > >
> > > I agree on the first suggestion, to reduce the number of log messages.
> > > For the second, if you meant that we should return 0 if the status is
> > > INTEGRITY_PASS_IMMUTABLE, I thought we wanted to deny xattr
> > > changes when there is an EVM portable signature.
> >
> > Why? I must be missing something. As long as we're not relying on the
> > cached status, allowing the file metadata to be updated shouldn't be an
> > issue.
>
> We may want to prevent accidental changes, for example.
Let's keep it simple, getting the basics working properly first. Then
we can decide if this is something that we really want/need to defend
against.
thanks,
Mimi
On Mon, 2021-05-03 at 15:11 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:[email protected]]
> > Sent: Monday, May 3, 2021 3:00 PM
> > On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> >
> > > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c
> > > @@ -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 the purpose of evm_protect_xattr() is to prevent allowing an invalid
> > security.evm xattr from being re-calculated and updated, making it
> > valid, INTEGRITY_PASS_IMMUTABLE shouldn't need to be conditional. Any
> > time there is an attr or xattr change, including setting it to the
> > existing value, the status flag should be reset.
> >
> > I'm wondering if making INTEGRITY_PASS_IMMUTABLE conditional would
> > prevent the file from being resigned.
> >
> > > if (evm_status != INTEGRITY_PASS)
> > > integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > d_backing_inode(dentry),
> > > dentry->d_name.name,
> > "appraise_metadata",
> >
> > This would then be updated to if not INTEGRITY_PASS or
> > INTEGRITY_PASS_IMMUTABLE. The subsequent "return" would need to be
> > updated as well.
>
> I agree on the first suggestion, to reduce the number of log messages.
> For the second, if you meant that we should return 0 if the status is
> INTEGRITY_PASS_IMMUTABLE, I thought we wanted to deny xattr
> changes when there is an EVM portable signature.
Why? I must be missing something. As long as we're not relying on the
cached status, allowing the file metadata to be updated shouldn't be an
issue.
Mimi