From 49219d9b8785ba712575c40e48ce0f7461254626 Mon Sep 17 00:00:00 2001 From: Mimi Zohar Date: Mon, 26 Apr 2021 18:13:45 -0400 Subject: evm: fix writing /evm overflow EVM_SETUP_COMPLETE is defined as 0x80000000, which is larger than INT_MAX. The "-fno-strict-overflow" compiler option properly prevents signaling EVM that the EVM policy setup is complete. Define and read an unsigned int. Fixes: f00d79750712 ("EVM: Allow userspace to signal an RSA key has been loaded") Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_secfs.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'security') diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c index bbc85637e18b..0007d3362754 100644 --- a/security/integrity/evm/evm_secfs.c +++ b/security/integrity/evm/evm_secfs.c @@ -66,12 +66,13 @@ static ssize_t evm_read_key(struct file *filp, char __user *buf, static ssize_t evm_write_key(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { - int i, ret; + unsigned int i; + int ret; if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_SETUP_COMPLETE)) return -EPERM; - ret = kstrtoint_from_user(buf, count, 0, &i); + ret = kstrtouint_from_user(buf, count, 0, &i); if (ret) return ret; -- cgit v1.2.3 From 9eea2904292c2d8fa98df141d3bf7c41ec9dc1b5 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 14 May 2021 17:27:42 +0200 Subject: evm: Execute evm_inode_init_security() only when an HMAC key is loaded evm_inode_init_security() requires an HMAC key to calculate the HMAC on initial xattrs provided by LSMs. However, it checks generically whether a key has been loaded, including also public keys, which is not correct as public keys are not suitable to calculate the HMAC. Originally, support for signature verification was introduced to verify a possibly immutable initial ram disk, when no new files are created, and to switch to HMAC for the root filesystem. By that time, an HMAC key should have been loaded and usable to calculate HMACs for new files. More recently support for requiring an HMAC key was removed from the kernel, so that signature verification can be used alone. Since this is a legitimate use case, evm_inode_init_security() should not return an error when no HMAC key has been loaded. This patch fixes this problem by replacing the evm_key_loaded() check with a check of the EVM_INIT_HMAC flag in evm_initialized. Fixes: 26ddabfe96b ("evm: enable EVM when X509 certificate is loaded") Signed-off-by: Roberto Sassu Cc: stable@vger.kernel.org # 4.5.x Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_main.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'security') diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 0de367aaa2d3..7ac5204c8d1f 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -521,7 +521,7 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid) } /* - * evm_inode_init_security - initializes security.evm + * evm_inode_init_security - initializes security.evm HMAC value */ int evm_inode_init_security(struct inode *inode, const struct xattr *lsm_xattr, @@ -530,7 +530,8 @@ int evm_inode_init_security(struct inode *inode, struct evm_xattr *xattr_data; int rc; - if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name)) + if (!(evm_initialized & EVM_INIT_HMAC) || + !evm_protected_xattr(lsm_xattr->name)) return 0; xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS); -- cgit v1.2.3 From aa2ead71d9daa1b6645e1d25b1f14a6286b114d0 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 14 May 2021 17:27:43 +0200 Subject: evm: Load EVM key in ima_load_x509() to avoid appraisal 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 Signed-off-by: Mimi Zohar --- security/integrity/iint.c | 4 +++- security/integrity/ima/ima_init.c | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/iint.c b/security/integrity/iint.c index fca8a9409e4a..8638976f7990 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 -- cgit v1.2.3 From 9acc89d31f0c94c8e573ed61f3e4340bbd526d0c Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 14 May 2021 17:27:44 +0200 Subject: evm: Refuse EVM_ALLOW_METADATA_WRITES only if an HMAC key is loaded EVM_ALLOW_METADATA_WRITES is an EVM initialization flag that can be set to temporarily disable metadata verification until all xattrs/attrs necessary to verify an EVM portable signature are copied to the file. This flag is cleared when EVM is initialized with an HMAC key, to avoid that the HMAC is calculated on unverified xattrs/attrs. Currently EVM unnecessarily denies setting this flag if EVM is initialized with a public key, which is not a concern as it cannot be used to trust xattrs/attrs updates. This patch removes this limitation. Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata") Signed-off-by: Roberto Sassu Cc: stable@vger.kernel.org # 4.16.x Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_secfs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'security') diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c index 0007d3362754..5f0da41bccd0 100644 --- a/security/integrity/evm/evm_secfs.c +++ b/security/integrity/evm/evm_secfs.c @@ -81,12 +81,12 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf, if (!i || (i & ~EVM_INIT_MASK) != 0) return -EINVAL; - /* Don't allow a request to freshly enable metadata writes if - * keys are loaded. + /* + * Don't allow a request to enable metadata writes if + * an HMAC key is loaded. */ if ((i & EVM_ALLOW_METADATA_WRITES) && - ((evm_initialized & EVM_KEY_MASK) != 0) && - !(evm_initialized & EVM_ALLOW_METADATA_WRITES)) + (evm_initialized & EVM_INIT_HMAC) != 0) return -EPERM; if (i & EVM_INIT_HMAC) { -- cgit v1.2.3 From e3ccfe1ad7d895487977ef64eda3441d16c9851a Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 14 May 2021 17:27:45 +0200 Subject: evm: Introduce evm_revalidate_status() 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_revalidate_status(), which callers of evm_verifyxattr() can use in their xattr hooks to determine whether re-validation is necessary and to do the proper actions. IMA calls it in its xattr 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 Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_main.c | 40 +++++++++++++++++++++++++++++++---- security/integrity/ima/ima_appraise.c | 15 ++++++++----- 2 files changed, 46 insertions(+), 9 deletions(-) (limited to 'security') diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 7ac5204c8d1f..782915117175 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -425,6 +425,31 @@ static void evm_reset_status(struct inode *inode) iint->evm_status = INTEGRITY_UNKNOWN; } +/** + * evm_revalidate_status - 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_revalidate_status(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) && + strcmp(xattr_name, XATTR_NAME_EVM)) + return false; + + return true; +} + /** * evm_inode_post_setxattr - update 'security.evm' to reflect the changes * @dentry: pointer to the affected dentry @@ -441,12 +466,14 @@ 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_revalidate_status(xattr_name)) return; evm_reset_status(dentry->d_inode); + if (!strcmp(xattr_name, XATTR_NAME_EVM)) + return; + evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len); } @@ -462,11 +489,14 @@ 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_revalidate_status(xattr_name)) return; evm_reset_status(dentry->d_inode); + if (!strcmp(xattr_name, XATTR_NAME_EVM)) + return; + evm_update_evmxattr(dentry, xattr_name, NULL, 0); } @@ -513,9 +543,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_revalidate_status(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 4e5eb0236278..03894769dffa 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -570,6 +570,7 @@ int ima_inode_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, @@ -577,9 +578,12 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name, if (result == 1) { if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST)) return -EINVAL; - ima_reset_appraise_flags(d_backing_inode(dentry), - xvalue->type == EVM_IMA_XATTR_DIGSIG); - result = 0; + digsig = (xvalue->type == EVM_IMA_XATTR_DIGSIG); + } + if (result == 1 || evm_revalidate_status(xattr_name)) { + ima_reset_appraise_flags(d_backing_inode(dentry), digsig); + if (result == 1) + result = 0; } return result; } @@ -589,9 +593,10 @@ int ima_inode_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_revalidate_status(xattr_name)) { ima_reset_appraise_flags(d_backing_inode(dentry), 0); - result = 0; + if (result == 1) + result = 0; } return result; } -- cgit v1.2.3 From 4a804b8a4572dfc81c3a59709d49ae206e4370ba Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Thu, 20 May 2021 10:48:31 +0200 Subject: evm: Introduce evm_hmac_disabled() to safely ignore verification errors When a file is being created, LSMs can set the initial label with the inode_init_security hook. If no HMAC key is loaded, the new file will have LSM xattrs but not the HMAC. It is also possible that the file remains without protected xattrs after creation if no active LSM provided it, or because the filesystem does not support them. Unfortunately, EVM will deny any further metadata operation on new files, as evm_protect_xattr() will return the INTEGRITY_NOLABEL error if protected xattrs exist without security.evm, INTEGRITY_NOXATTRS if no protected xattrs exist or INTEGRITY_UNKNOWN if xattrs are not supported. This would limit the usability of EVM when only a public key is loaded, as commands such as cp or tar with the option to preserve xattrs won't work. This patch introduces the evm_hmac_disabled() function to determine whether or not it is safe to ignore verification errors, based on the ability of EVM to calculate HMACs. If the HMAC key is not loaded, and it cannot be loaded in the future due to the EVM_SETUP_COMPLETE initialization flag, allowing an operation despite the attrs/xattrs being found invalid will not make them valid. Since the post hooks can be executed even when the HMAC key is not loaded, this patch also ensures that the EVM_INIT_HMAC initialization flag is set before the post hooks call evm_update_evmxattr(). Signed-off-by: Roberto Sassu Suggested-by: Mimi Zohar (for ensuring EVM_INIT_HMAC is set) Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_main.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 782915117175..4206c7e492ae 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -90,6 +90,24 @@ static bool evm_key_loaded(void) return (bool)(evm_initialized & EVM_KEY_MASK); } +/* + * This function determines whether or not it is safe to ignore verification + * errors, based on the ability of EVM to calculate HMACs. If the HMAC key + * is not loaded, and it cannot be loaded in the future due to the + * EVM_SETUP_COMPLETE initialization flag, allowing an operation despite the + * attrs/xattrs being found invalid will not make them valid. + */ +static bool evm_hmac_disabled(void) +{ + if (evm_initialized & EVM_INIT_HMAC) + return false; + + if (!(evm_initialized & EVM_SETUP_COMPLETE)) + return false; + + return true; +} + static int evm_find_protected_xattrs(struct dentry *dentry) { struct inode *inode = d_backing_inode(dentry); @@ -338,6 +356,10 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name, if (evm_status == INTEGRITY_NOXATTRS) { struct integrity_iint_cache *iint; + /* Exception if the HMAC is not going to be calculated. */ + if (evm_hmac_disabled()) + return 0; + iint = integrity_iint_find(d_backing_inode(dentry)); if (iint && (iint->flags & IMA_NEW_FILE)) return 0; @@ -354,6 +376,10 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name, -EPERM, 0); } out: + /* Exception if the HMAC is not going to be calculated. */ + if (evm_hmac_disabled() && (evm_status == INTEGRITY_NOLABEL || + evm_status == INTEGRITY_UNKNOWN)) + return 0; if (evm_status != INTEGRITY_PASS) integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry), dentry->d_name.name, "appraise_metadata", @@ -474,6 +500,9 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name, if (!strcmp(xattr_name, XATTR_NAME_EVM)) return; + if (!(evm_initialized & EVM_INIT_HMAC)) + return; + evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len); } @@ -497,6 +526,9 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name) if (!strcmp(xattr_name, XATTR_NAME_EVM)) return; + if (!(evm_initialized & EVM_INIT_HMAC)) + return; + evm_update_evmxattr(dentry, xattr_name, NULL, 0); } @@ -522,7 +554,9 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr) return 0; evm_status = evm_verify_current_integrity(dentry); if ((evm_status == INTEGRITY_PASS) || - (evm_status == INTEGRITY_NOXATTRS)) + (evm_status == INTEGRITY_NOXATTRS) || + (evm_hmac_disabled() && (evm_status == INTEGRITY_NOLABEL || + evm_status == INTEGRITY_UNKNOWN))) return 0; integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry), dentry->d_name.name, "appraise_metadata", @@ -548,6 +582,9 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid) evm_reset_status(dentry->d_inode); + if (!(evm_initialized & EVM_INIT_HMAC)) + return; + if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) evm_update_evmxattr(dentry, NULL, NULL, 0); } -- cgit v1.2.3 From cdef685be5b4ae55c3959289e72d520402839c29 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 14 May 2021 17:27:47 +0200 Subject: evm: Allow xattr/attr operations for portable signatures If files with portable signatures are copied from one location to another or are extracted from an archive, verification can temporarily fail until all xattrs/attrs are set in the destination. Only portable signatures may be moved or copied from one file to another, as they don't depend on system-specific information such as the inode generation. Instead portable signatures must include security.ima. Unlike other security.evm types, EVM portable signatures are also immutable. Thus, it wouldn't be a problem to allow xattr/attr operations when verification fails, as portable signatures will never be replaced with the HMAC on possibly corrupted xattrs/attrs. This patch first introduces a new integrity status called INTEGRITY_FAIL_IMMUTABLE, that allows callers of evm_verify_current_integrity() to detect that a portable signature didn't pass verification and then adds an exception in evm_protect_xattr() and evm_inode_setattr() for this status and returns 0 instead of -EPERM. Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_main.c | 33 +++++++++++++++++++++++++++------ security/integrity/ima/ima_appraise.c | 2 ++ 2 files changed, 29 insertions(+), 6 deletions(-) (limited to 'security') diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 4206c7e492ae..333524e879b5 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -27,7 +27,8 @@ int evm_initialized; static const char * const integrity_status_msg[] = { - "pass", "pass_immutable", "fail", "no_label", "no_xattrs", "unknown" + "pass", "pass_immutable", "fail", "fail_immutable", "no_label", + "no_xattrs", "unknown" }; int evm_hmac_attrs; @@ -155,7 +156,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, enum integrity_status evm_status = INTEGRITY_PASS; struct evm_digest digest; struct inode *inode; - int rc, xattr_len; + int rc, xattr_len, evm_immutable = 0; if (iint && (iint->evm_status == INTEGRITY_PASS || iint->evm_status == INTEGRITY_PASS_IMMUTABLE)) @@ -200,8 +201,10 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, if (rc) rc = -EINVAL; break; - case EVM_IMA_XATTR_DIGSIG: case EVM_XATTR_PORTABLE_DIGSIG: + evm_immutable = 1; + fallthrough; + case EVM_IMA_XATTR_DIGSIG: /* accept xattr with non-empty signature field */ if (xattr_len <= sizeof(struct signature_v2_hdr)) { evm_status = INTEGRITY_FAIL; @@ -238,9 +241,14 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, break; } - if (rc) - evm_status = (rc == -ENODATA) ? - INTEGRITY_NOXATTRS : INTEGRITY_FAIL; + if (rc) { + if (rc == -ENODATA) + evm_status = INTEGRITY_NOXATTRS; + else if (evm_immutable) + evm_status = INTEGRITY_FAIL_IMMUTABLE; + else + evm_status = INTEGRITY_FAIL; + } out: if (iint) iint->evm_status = evm_status; @@ -380,6 +388,14 @@ out: if (evm_hmac_disabled() && (evm_status == INTEGRITY_NOLABEL || evm_status == INTEGRITY_UNKNOWN)) return 0; + + /* + * Writing other xattrs is safe for portable signatures, as portable + * signatures are immutable and can never be updated. + */ + if (evm_status == INTEGRITY_FAIL_IMMUTABLE) + return 0; + if (evm_status != INTEGRITY_PASS) integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry), dentry->d_name.name, "appraise_metadata", @@ -553,8 +569,13 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr) if (!(ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))) return 0; evm_status = evm_verify_current_integrity(dentry); + /* + * Writing attrs is safe for portable signatures, as portable signatures + * are immutable and can never be updated. + */ if ((evm_status == INTEGRITY_PASS) || (evm_status == INTEGRITY_NOXATTRS) || + (evm_status == INTEGRITY_FAIL_IMMUTABLE) || (evm_hmac_disabled() && (evm_status == INTEGRITY_NOLABEL || evm_status == INTEGRITY_UNKNOWN))) return 0; diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 03894769dffa..9bb351b933fb 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -416,6 +416,8 @@ int ima_appraise_measurement(enum ima_hooks func, case INTEGRITY_NOLABEL: /* No security.evm xattr. */ cause = "missing-HMAC"; goto out; + case INTEGRITY_FAIL_IMMUTABLE: + fallthrough; case INTEGRITY_FAIL: /* Invalid HMAC/signature. */ cause = "invalid-HMAC"; goto out; -- cgit v1.2.3 From 7e135dc725417ecc0629afb4b3b24457d2a4869d Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 14 May 2021 17:27:48 +0200 Subject: evm: Pass user namespace to set/remove xattr hooks In preparation for 'evm: Allow setxattr() and setattr() for unmodified metadata', this patch passes mnt_userns to the inode set/remove xattr hooks so that the GID of the inode on an idmapped mount is correctly determined by posix_acl_update_mode(). Signed-off-by: Roberto Sassu Reviewed-by: Christian Brauner Cc: Andreas Gruenbacher Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_main.c | 17 +++++++++++------ security/security.c | 4 ++-- 2 files changed, 13 insertions(+), 8 deletions(-) (limited to 'security') diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 333524e879b5..300df6906e05 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -342,7 +342,8 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry) * For posix xattr acls only, permit security.evm, even if it currently * doesn't exist, to be updated unless the EVM signature is immutable. */ -static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name, +static int evm_protect_xattr(struct user_namespace *mnt_userns, + struct dentry *dentry, const char *xattr_name, const void *xattr_value, size_t xattr_value_len) { enum integrity_status evm_status; @@ -406,6 +407,7 @@ out: /** * evm_inode_setxattr - protect the EVM extended attribute + * @mnt_userns: user namespace of the idmapped mount * @dentry: pointer to the affected dentry * @xattr_name: pointer to the affected extended attribute name * @xattr_value: pointer to the new extended attribute value @@ -417,8 +419,9 @@ out: * userspace from writing HMAC value. Writing 'security.evm' requires * requires CAP_SYS_ADMIN privileges. */ -int evm_inode_setxattr(struct dentry *dentry, const char *xattr_name, - const void *xattr_value, size_t xattr_value_len) +int evm_inode_setxattr(struct user_namespace *mnt_userns, struct dentry *dentry, + const char *xattr_name, const void *xattr_value, + size_t xattr_value_len) { const struct evm_ima_xattr_data *xattr_data = xattr_value; @@ -435,19 +438,21 @@ int evm_inode_setxattr(struct dentry *dentry, const char *xattr_name, xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) return -EPERM; } - return evm_protect_xattr(dentry, xattr_name, xattr_value, + return evm_protect_xattr(mnt_userns, dentry, xattr_name, xattr_value, xattr_value_len); } /** * evm_inode_removexattr - protect the EVM extended attribute + * @mnt_userns: user namespace of the idmapped mount * @dentry: pointer to the affected dentry * @xattr_name: pointer to the affected extended attribute name * * Removing 'security.evm' requires CAP_SYS_ADMIN privileges and that * the current value is valid. */ -int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name) +int evm_inode_removexattr(struct user_namespace *mnt_userns, + struct dentry *dentry, const char *xattr_name) { /* Policy permits modification of the protected xattrs even though * there's no HMAC key loaded @@ -455,7 +460,7 @@ int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name) if (evm_initialized & EVM_ALLOW_METADATA_WRITES) return 0; - return evm_protect_xattr(dentry, xattr_name, NULL, 0); + return evm_protect_xattr(mnt_userns, dentry, xattr_name, NULL, 0); } static void evm_reset_status(struct inode *inode) diff --git a/security/security.c b/security/security.c index b38155b2de83..e9f8010a2341 100644 --- a/security/security.c +++ b/security/security.c @@ -1354,7 +1354,7 @@ int security_inode_setxattr(struct user_namespace *mnt_userns, ret = ima_inode_setxattr(dentry, name, value, size); if (ret) return ret; - return evm_inode_setxattr(dentry, name, value, size); + return evm_inode_setxattr(mnt_userns, dentry, name, value, size); } void security_inode_post_setxattr(struct dentry *dentry, const char *name, @@ -1399,7 +1399,7 @@ int security_inode_removexattr(struct user_namespace *mnt_userns, ret = ima_inode_removexattr(dentry, name); if (ret) return ret; - return evm_inode_removexattr(dentry, name); + return evm_inode_removexattr(mnt_userns, dentry, name); } int security_inode_need_killpriv(struct dentry *dentry) -- cgit v1.2.3 From 1886ab01a3fb98ee7f7739ae50eb9492f5df3641 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 14 May 2021 17:27:49 +0200 Subject: 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. With this patch, the one that introduces evm_hmac_disabled() and the one that allows a metadata operation on the INTEGRITY_FAIL_IMMUTABLE error, EVM portable signatures can be used without disabling metadata verification (by setting EVM_ALLOW_METADATA_WRITES). Due to keeping metadata verification enabled, altering immutable metadata protected with a portable signature that was successfully verified will be denied (existing behavior). Reported-by: kernel test robot [implicit declaration of function] Signed-off-by: Roberto Sassu Reviewed-by: Christian Brauner Cc: Andreas Gruenbacher Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_main.c | 113 +++++++++++++++++++++++++++++++++++++- 1 file changed, 112 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 300df6906e05..0196168aeb7d 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 @@ -330,6 +331,92 @@ 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 * @@ -397,7 +484,13 @@ out: 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], @@ -553,6 +646,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 @@ -584,6 +690,11 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr) (evm_hmac_disabled() && (evm_status == INTEGRITY_NOLABEL || evm_status == INTEGRITY_UNKNOWN))) 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); -- cgit v1.2.3 From 7aa5783d95646f924b99d245338d5b7aa7a2b3c0 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 14 May 2021 17:27:51 +0200 Subject: ima: Allow imasig requirement to be satisfied by EVM portable signatures System administrators can require that all accessed files have a signature by specifying appraise_type=imasig in a policy rule. Currently, IMA signatures satisfy this requirement. Appended signatures may also satisfy this requirement, but are not applicable as IMA signatures. IMA/appended signatures ensure data source authentication for file content and prevent any change. EVM signatures instead ensure data source authentication for file metadata. Given that the digest or signature of the file content must be included in the metadata, EVM signatures provide the same file data guarantees of IMA signatures, as well as providing file metadata guarantees. This patch lets systems protected with EVM signatures pass appraisal verification if the appraise_type=imasig requirement is specified in the policy. This facilitates deployment in the scenarios where only EVM signatures are available. The patch makes the following changes: file xattr types: security.ima: IMA_XATTR_DIGEST/IMA_XATTR_DIGEST_NG security.evm: EVM_XATTR_PORTABLE_DIGSIG execve(), mmap(), open() behavior (with appraise_type=imasig): before: denied (file without IMA signature, imasig requirement not met) after: allowed (file with EVM portable signature, imasig requirement met) open(O_WRONLY) behavior (without appraise_type=imasig): before: allowed (file without IMA signature, not immutable) after: denied (file with EVM portable signature, immutable) In addition, similarly to IMA signatures, this patch temporarily allows new files without or with incomplete metadata to be opened so that content can be written. Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_appraise.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 9bb351b933fb..d9a627de3930 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -242,12 +242,16 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, hash_start = 1; fallthrough; case IMA_XATTR_DIGEST: - if (iint->flags & IMA_DIGSIG_REQUIRED) { - *cause = "IMA-signature-required"; - *status = INTEGRITY_FAIL; - break; + if (*status != INTEGRITY_PASS_IMMUTABLE) { + if (iint->flags & IMA_DIGSIG_REQUIRED) { + *cause = "IMA-signature-required"; + *status = INTEGRITY_FAIL; + break; + } + clear_bit(IMA_DIGSIG, &iint->atomic_flags); + } else { + set_bit(IMA_DIGSIG, &iint->atomic_flags); } - clear_bit(IMA_DIGSIG, &iint->atomic_flags); if (xattr_len - sizeof(xattr_value->type) - hash_start >= iint->ima_hash->length) /* @@ -417,6 +421,7 @@ int ima_appraise_measurement(enum ima_hooks func, cause = "missing-HMAC"; goto out; case INTEGRITY_FAIL_IMMUTABLE: + set_bit(IMA_DIGSIG, &iint->atomic_flags); fallthrough; case INTEGRITY_FAIL: /* Invalid HMAC/signature. */ cause = "invalid-HMAC"; @@ -461,9 +466,12 @@ out: status = INTEGRITY_PASS; } - /* Permit new files with file signatures, but without data. */ + /* + * Permit new files with file/EVM portable signatures, but + * without data. + */ if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE && - xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) { + test_bit(IMA_DIGSIG, &iint->atomic_flags)) { status = INTEGRITY_PASS; } @@ -581,6 +589,8 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name, if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST)) return -EINVAL; digsig = (xvalue->type == EVM_IMA_XATTR_DIGSIG); + } else if (!strcmp(xattr_name, XATTR_NAME_EVM) && xattr_value_len > 0) { + digsig = (xvalue->type == EVM_XATTR_PORTABLE_DIGSIG); } if (result == 1 || evm_revalidate_status(xattr_name)) { ima_reset_appraise_flags(d_backing_inode(dentry), digsig); -- cgit v1.2.3 From 026d7fc92a9d629630779c999fe49ecae93f9d63 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 14 May 2021 17:27:52 +0200 Subject: ima: Introduce template field evmsig and write to field sig as fallback With the patch to accept EVM portable signatures when the appraise_type=imasig requirement is specified in the policy, appraisal can be successfully done even if the file does not have an IMA signature. However, remote attestation would not see that a different signature type was used, as only IMA signatures can be included in the measurement list. This patch solves the issue by introducing the new template field 'evmsig' to show EVM portable signatures and by including its value in the existing field 'sig' if the IMA signature is not found. Suggested-by: Mimi Zohar Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_template.c | 2 ++ security/integrity/ima/ima_template_lib.c | 33 ++++++++++++++++++++++++++++++- security/integrity/ima/ima_template_lib.h | 2 ++ 3 files changed, 36 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index 4e081e650047..7a60848c04a5 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -45,6 +45,8 @@ static const struct ima_template_field supported_fields[] = { .field_show = ima_show_template_digest_ng}, {.field_id = "modsig", .field_init = ima_eventmodsig_init, .field_show = ima_show_template_sig}, + {.field_id = "evmsig", .field_init = ima_eventevmsig_init, + .field_show = ima_show_template_sig}, }; /* diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index c022ee9e2a4e..4314d9a3514c 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -10,6 +10,7 @@ */ #include "ima_template_lib.h" +#include static bool ima_template_hash_algo_allowed(u8 algo) { @@ -438,7 +439,7 @@ int ima_eventsig_init(struct ima_event_data *event_data, struct evm_ima_xattr_data *xattr_value = event_data->xattr_value; if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG)) - return 0; + return ima_eventevmsig_init(event_data, field_data); return ima_write_template_field_data(xattr_value, event_data->xattr_len, DATA_FMT_HEX, field_data); @@ -484,3 +485,33 @@ int ima_eventmodsig_init(struct ima_event_data *event_data, return ima_write_template_field_data(data, data_len, DATA_FMT_HEX, field_data); } + +/* + * ima_eventevmsig_init - include the EVM portable signature as part of the + * template data + */ +int ima_eventevmsig_init(struct ima_event_data *event_data, + struct ima_field_data *field_data) +{ + struct evm_ima_xattr_data *xattr_data = NULL; + int rc = 0; + + if (!event_data->file) + return 0; + + rc = vfs_getxattr_alloc(&init_user_ns, file_dentry(event_data->file), + XATTR_NAME_EVM, (char **)&xattr_data, 0, + GFP_NOFS); + if (rc <= 0) + return 0; + + if (xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) { + kfree(xattr_data); + return 0; + } + + rc = ima_write_template_field_data((char *)xattr_data, rc, DATA_FMT_HEX, + field_data); + kfree(xattr_data); + return rc; +} diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h index 6b3b880637a0..f4b2a2056d1d 100644 --- a/security/integrity/ima/ima_template_lib.h +++ b/security/integrity/ima/ima_template_lib.h @@ -46,4 +46,6 @@ int ima_eventbuf_init(struct ima_event_data *event_data, struct ima_field_data *field_data); int ima_eventmodsig_init(struct ima_event_data *event_data, struct ima_field_data *field_data); +int ima_eventevmsig_init(struct ima_event_data *event_data, + struct ima_field_data *field_data); #endif /* __LINUX_IMA_TEMPLATE_LIB_H */ -- cgit v1.2.3 From ed1b472fc15aeaa20ddeeb93fd25190014e50d17 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 14 May 2021 17:27:53 +0200 Subject: ima: Don't remove security.ima if file must not be appraised Files might come from a remote source and might have xattrs, including security.ima. It should not be IMA task to decide whether security.ima should be kept or not. This patch removes the removexattr() system call in ima_inode_post_setattr(). Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_appraise.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index d9a627de3930..940695e7b535 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -532,8 +532,6 @@ void ima_inode_post_setattr(struct user_namespace *mnt_userns, return; action = ima_must_appraise(mnt_userns, inode, MAY_ACCESS, POST_SETATTR); - if (!action) - __vfs_removexattr(&init_user_ns, dentry, XATTR_NAME_IMA); iint = integrity_iint_find(inode); if (iint) { set_bit(IMA_CHANGE_ATTR, &iint->atomic_flags); -- cgit v1.2.3 From cde1391a0b4014b0e8fc09cd316272f478b54c0f Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 28 May 2021 09:38:06 +0200 Subject: ima: Add ima_show_template_uint() template library function This patch introduces the new function ima_show_template_uint(). This can be used for showing integers of different sizes in ASCII format. The function ima_show_template_data_ascii() automatically determines how to print a stored integer by checking the integer size. If integers have been written in canonical format, ima_show_template_data_ascii() calls the appropriate leXX_to_cpu() function to correctly display the value. Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_template_lib.c | 38 ++++++++++++++++++++++++++++++- security/integrity/ima/ima_template_lib.h | 2 ++ 2 files changed, 39 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 4314d9a3514c..f23296c33da1 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -24,7 +24,8 @@ enum data_formats { DATA_FMT_DIGEST = 0, DATA_FMT_DIGEST_WITH_ALGO, DATA_FMT_STRING, - DATA_FMT_HEX + DATA_FMT_HEX, + DATA_FMT_UINT }; static int ima_write_template_field_data(const void *data, const u32 datalen, @@ -88,6 +89,35 @@ static void ima_show_template_data_ascii(struct seq_file *m, case DATA_FMT_STRING: seq_printf(m, "%s", buf_ptr); break; + case DATA_FMT_UINT: + switch (field_data->len) { + case sizeof(u8): + seq_printf(m, "%u", *(u8 *)buf_ptr); + break; + case sizeof(u16): + if (ima_canonical_fmt) + seq_printf(m, "%u", + le16_to_cpu(*(u16 *)buf_ptr)); + else + seq_printf(m, "%u", *(u16 *)buf_ptr); + break; + case sizeof(u32): + if (ima_canonical_fmt) + seq_printf(m, "%u", + le32_to_cpu(*(u32 *)buf_ptr)); + else + seq_printf(m, "%u", *(u32 *)buf_ptr); + break; + case sizeof(u64): + if (ima_canonical_fmt) + seq_printf(m, "%llu", + le64_to_cpu(*(u64 *)buf_ptr)); + else + seq_printf(m, "%llu", *(u64 *)buf_ptr); + break; + default: + break; + } default: break; } @@ -163,6 +193,12 @@ void ima_show_template_buf(struct seq_file *m, enum ima_show_type show, ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data); } +void ima_show_template_uint(struct seq_file *m, enum ima_show_type show, + struct ima_field_data *field_data) +{ + ima_show_template_field_data(m, show, DATA_FMT_UINT, field_data); +} + /** * ima_parse_buf() - Parses lengths and data from an input buffer * @bufstartp: Buffer start address. diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h index f4b2a2056d1d..54b67c80b315 100644 --- a/security/integrity/ima/ima_template_lib.h +++ b/security/integrity/ima/ima_template_lib.h @@ -27,6 +27,8 @@ void ima_show_template_sig(struct seq_file *m, enum ima_show_type show, struct ima_field_data *field_data); void ima_show_template_buf(struct seq_file *m, enum ima_show_type show, struct ima_field_data *field_data); +void ima_show_template_uint(struct seq_file *m, enum ima_show_type show, + struct ima_field_data *field_data); int ima_parse_buf(void *bufstartp, void *bufendp, void **bufcurp, int maxfields, struct ima_field_data *fields, int *curfields, unsigned long *len_mask, int enforce_mask, char *bufname); -- cgit v1.2.3 From 7dcfeacc5a9d0c130160b86de23279793a8732c8 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 28 May 2021 09:38:07 +0200 Subject: ima: Define new template fields iuid and igid This patch defines the new template fields iuid and igid, which include respectively the inode UID and GID. For idmapped mounts, still the original UID and GID are provided. These fields can be used to verify the EVM portable signature, if it was included with the template fields sig or evmsig. Signed-off-by: Roberto Sassu Acked-by: Christian Brauner Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_template.c | 4 +++ security/integrity/ima/ima_template_lib.c | 45 +++++++++++++++++++++++++++++++ security/integrity/ima/ima_template_lib.h | 4 +++ 3 files changed, 53 insertions(+) (limited to 'security') diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index 7a60848c04a5..a5ecd9e2581b 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -47,6 +47,10 @@ static const struct ima_template_field supported_fields[] = { .field_show = ima_show_template_sig}, {.field_id = "evmsig", .field_init = ima_eventevmsig_init, .field_show = ima_show_template_sig}, + {.field_id = "iuid", .field_init = ima_eventinodeuid_init, + .field_show = ima_show_template_uint}, + {.field_id = "igid", .field_init = ima_eventinodegid_init, + .field_show = ima_show_template_uint}, }; /* diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index f23296c33da1..87b40f391739 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -551,3 +551,48 @@ int ima_eventevmsig_init(struct ima_event_data *event_data, kfree(xattr_data); return rc; } + +static int ima_eventinodedac_init_common(struct ima_event_data *event_data, + struct ima_field_data *field_data, + bool get_uid) +{ + unsigned int id; + + if (!event_data->file) + return 0; + + if (get_uid) + id = i_uid_read(file_inode(event_data->file)); + else + id = i_gid_read(file_inode(event_data->file)); + + if (ima_canonical_fmt) { + if (sizeof(id) == sizeof(u16)) + id = cpu_to_le16(id); + else + id = cpu_to_le32(id); + } + + return ima_write_template_field_data((void *)&id, sizeof(id), + DATA_FMT_UINT, field_data); +} + +/* + * ima_eventinodeuid_init - include the inode UID as part of the template + * data + */ +int ima_eventinodeuid_init(struct ima_event_data *event_data, + struct ima_field_data *field_data) +{ + return ima_eventinodedac_init_common(event_data, field_data, true); +} + +/* + * ima_eventinodegid_init - include the inode GID as part of the template + * data + */ +int ima_eventinodegid_init(struct ima_event_data *event_data, + struct ima_field_data *field_data) +{ + return ima_eventinodedac_init_common(event_data, field_data, false); +} diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h index 54b67c80b315..b0aaf109f386 100644 --- a/security/integrity/ima/ima_template_lib.h +++ b/security/integrity/ima/ima_template_lib.h @@ -50,4 +50,8 @@ int ima_eventmodsig_init(struct ima_event_data *event_data, struct ima_field_data *field_data); int ima_eventevmsig_init(struct ima_event_data *event_data, struct ima_field_data *field_data); +int ima_eventinodeuid_init(struct ima_event_data *event_data, + struct ima_field_data *field_data); +int ima_eventinodegid_init(struct ima_event_data *event_data, + struct ima_field_data *field_data); #endif /* __LINUX_IMA_TEMPLATE_LIB_H */ -- cgit v1.2.3 From f8216f6b957f5657c5f4c97f4b037120c6f236bc Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 28 May 2021 09:38:08 +0200 Subject: ima: Define new template field imode This patch defines the new template field imode, which includes the inode mode. It can be used by a remote verifier to verify the EVM portable signature, if it was included with the template fields sig or evmsig. Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_template.c | 2 ++ security/integrity/ima/ima_template_lib.c | 22 ++++++++++++++++++++++ security/integrity/ima/ima_template_lib.h | 2 ++ 3 files changed, 26 insertions(+) (limited to 'security') diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index a5ecd9e2581b..43784f2bf8bd 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -51,6 +51,8 @@ static const struct ima_template_field supported_fields[] = { .field_show = ima_show_template_uint}, {.field_id = "igid", .field_init = ima_eventinodegid_init, .field_show = ima_show_template_uint}, + {.field_id = "imode", .field_init = ima_eventinodemode_init, + .field_show = ima_show_template_uint}, }; /* diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 87b40f391739..3156fb34b1af 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -596,3 +596,25 @@ int ima_eventinodegid_init(struct ima_event_data *event_data, { return ima_eventinodedac_init_common(event_data, field_data, false); } + +/* + * ima_eventinodemode_init - include the inode mode as part of the template + * data + */ +int ima_eventinodemode_init(struct ima_event_data *event_data, + struct ima_field_data *field_data) +{ + struct inode *inode; + umode_t mode; + + if (!event_data->file) + return 0; + + inode = file_inode(event_data->file); + mode = inode->i_mode; + if (ima_canonical_fmt) + mode = cpu_to_le16(mode); + + return ima_write_template_field_data((char *)&mode, sizeof(mode), + DATA_FMT_UINT, field_data); +} diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h index b0aaf109f386..6509af4a97ee 100644 --- a/security/integrity/ima/ima_template_lib.h +++ b/security/integrity/ima/ima_template_lib.h @@ -54,4 +54,6 @@ int ima_eventinodeuid_init(struct ima_event_data *event_data, struct ima_field_data *field_data); int ima_eventinodegid_init(struct ima_event_data *event_data, struct ima_field_data *field_data); +int ima_eventinodemode_init(struct ima_event_data *event_data, + struct ima_field_data *field_data); #endif /* __LINUX_IMA_TEMPLATE_LIB_H */ -- cgit v1.2.3 From 8c7a703ec9787a1b45b024e9acd253328422dcbd Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 28 May 2021 09:38:09 +0200 Subject: evm: Verify portable signatures against all protected xattrs Currently, the evm_config_default_xattrnames array contains xattr names only related to LSMs which are enabled in the kernel configuration. However, EVM portable signatures do not depend on local information and a vendor might include in the signature calculation xattrs that are not enabled in the target platform. Just including all xattrs names in evm_config_default_xattrnames is not a safe approach, because a target system might have already calculated signatures or HMACs based only on the enabled xattrs. After applying this patch, EVM would verify those signatures and HMACs with all xattrs instead. The non-enabled ones, which could possibly exist, would cause a verification error. Thus, this patch adds a new field named enabled to the xattr_list structure, which is set to true if the LSM associated to a given xattr name is enabled in the kernel configuration. The non-enabled xattrs are taken into account only in evm_calc_hmac_or_hash(), if the passed security.evm type is EVM_XATTR_PORTABLE_DIGSIG. The new function evm_protected_xattr_if_enabled() has been defined so that IMA can include all protected xattrs and not only the enabled ones in the measurement list, if the new template fields xattrnames, xattrlengths or xattrvalues have been included in the template format. Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/evm/evm.h | 1 + security/integrity/evm/evm_crypto.c | 7 +++++ security/integrity/evm/evm_main.c | 56 ++++++++++++++++++++++++++++++------- security/integrity/evm/evm_secfs.c | 16 +++++++++-- 4 files changed, 68 insertions(+), 12 deletions(-) (limited to 'security') diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h index f2fef2b5ed51..0d44f41d16f8 100644 --- a/security/integrity/evm/evm.h +++ b/security/integrity/evm/evm.h @@ -29,6 +29,7 @@ struct xattr_list { struct list_head list; char *name; + bool enabled; }; extern int evm_initialized; diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index d76b006cbcc4..1628e2ca9862 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -216,6 +216,13 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, if (strcmp(xattr->name, XATTR_NAME_IMA) == 0) is_ima = true; + /* + * Skip non-enabled xattrs for locally calculated + * signatures/HMACs. + */ + if (type != EVM_XATTR_PORTABLE_DIGSIG && !xattr->enabled) + continue; + if ((req_xattr_name && req_xattr_value) && !strcmp(xattr->name, req_xattr_name)) { error = 0; diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 0196168aeb7d..ee4e17a790fb 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -34,24 +34,44 @@ static const char * const integrity_status_msg[] = { int evm_hmac_attrs; static struct xattr_list evm_config_default_xattrnames[] = { + {.name = XATTR_NAME_SELINUX, #ifdef CONFIG_SECURITY_SELINUX - {.name = XATTR_NAME_SELINUX}, + .enabled = true #endif + }, + {.name = XATTR_NAME_SMACK, #ifdef CONFIG_SECURITY_SMACK - {.name = XATTR_NAME_SMACK}, + .enabled = true +#endif + }, + {.name = XATTR_NAME_SMACKEXEC, +#ifdef CONFIG_EVM_EXTRA_SMACK_XATTRS + .enabled = true +#endif + }, + {.name = XATTR_NAME_SMACKTRANSMUTE, #ifdef CONFIG_EVM_EXTRA_SMACK_XATTRS - {.name = XATTR_NAME_SMACKEXEC}, - {.name = XATTR_NAME_SMACKTRANSMUTE}, - {.name = XATTR_NAME_SMACKMMAP}, + .enabled = true #endif + }, + {.name = XATTR_NAME_SMACKMMAP, +#ifdef CONFIG_EVM_EXTRA_SMACK_XATTRS + .enabled = true #endif + }, + {.name = XATTR_NAME_APPARMOR, #ifdef CONFIG_SECURITY_APPARMOR - {.name = XATTR_NAME_APPARMOR}, + .enabled = true #endif + }, + {.name = XATTR_NAME_IMA, #ifdef CONFIG_IMA_APPRAISE - {.name = XATTR_NAME_IMA}, + .enabled = true #endif - {.name = XATTR_NAME_CAPS}, + }, + {.name = XATTR_NAME_CAPS, + .enabled = true + }, }; LIST_HEAD(evm_config_xattrnames); @@ -76,7 +96,9 @@ static void __init evm_init_config(void) pr_info("Initialising EVM extended attributes:\n"); for (i = 0; i < xattrs; i++) { - pr_info("%s\n", evm_config_default_xattrnames[i].name); + pr_info("%s%s\n", evm_config_default_xattrnames[i].name, + !evm_config_default_xattrnames[i].enabled ? + " (disabled)" : ""); list_add_tail(&evm_config_default_xattrnames[i].list, &evm_config_xattrnames); } @@ -257,7 +279,8 @@ out: return evm_status; } -static int evm_protected_xattr(const char *req_xattr_name) +static int evm_protected_xattr_common(const char *req_xattr_name, + bool all_xattrs) { int namelen; int found = 0; @@ -265,6 +288,9 @@ static int evm_protected_xattr(const char *req_xattr_name) namelen = strlen(req_xattr_name); list_for_each_entry_lockless(xattr, &evm_config_xattrnames, list) { + if (!all_xattrs && !xattr->enabled) + continue; + if ((strlen(xattr->name) == namelen) && (strncmp(req_xattr_name, xattr->name, namelen) == 0)) { found = 1; @@ -281,6 +307,16 @@ static int evm_protected_xattr(const char *req_xattr_name) return found; } +static int evm_protected_xattr(const char *req_xattr_name) +{ + return evm_protected_xattr_common(req_xattr_name, false); +} + +int evm_protected_xattr_if_enabled(const char *req_xattr_name) +{ + return evm_protected_xattr_common(req_xattr_name, true); +} + /** * evm_verifyxattr - verify the integrity of the requested xattr * @dentry: object of the verify xattr diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c index 5f0da41bccd0..a99676eb7f41 100644 --- a/security/integrity/evm/evm_secfs.c +++ b/security/integrity/evm/evm_secfs.c @@ -139,8 +139,12 @@ static ssize_t evm_read_xattrs(struct file *filp, char __user *buf, if (rc) return -ERESTARTSYS; - list_for_each_entry(xattr, &evm_config_xattrnames, list) + list_for_each_entry(xattr, &evm_config_xattrnames, list) { + if (!xattr->enabled) + continue; + size += strlen(xattr->name) + 1; + } temp = kmalloc(size + 1, GFP_KERNEL); if (!temp) { @@ -149,6 +153,9 @@ static ssize_t evm_read_xattrs(struct file *filp, char __user *buf, } list_for_each_entry(xattr, &evm_config_xattrnames, list) { + if (!xattr->enabled) + continue; + sprintf(temp + offset, "%s\n", xattr->name); offset += strlen(xattr->name) + 1; } @@ -199,6 +206,7 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf, goto out; } + xattr->enabled = true; xattr->name = memdup_user_nul(buf, count); if (IS_ERR(xattr->name)) { err = PTR_ERR(xattr->name); @@ -245,6 +253,10 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf, list_for_each_entry(tmp, &evm_config_xattrnames, list) { if (strcmp(xattr->name, tmp->name) == 0) { err = -EEXIST; + if (!tmp->enabled) { + tmp->enabled = true; + err = count; + } mutex_unlock(&xattr_list_mutex); goto out; } @@ -256,7 +268,7 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf, audit_log_end(ab); return count; out: - audit_log_format(ab, " res=%d", err); + audit_log_format(ab, " res=%d", (err < 0) ? err : 0); audit_log_end(ab); if (xattr) { kfree(xattr->name); -- cgit v1.2.3 From 8314b6732ae4e600bb933e108f96ce0176acb09c Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Tue, 1 Jun 2021 10:23:38 +0200 Subject: ima: Define new template fields xattrnames, xattrlengths and xattrvalues This patch defines the new template fields xattrnames, xattrlengths and xattrvalues, which contain respectively a list of xattr names (strings, separated by |), lengths (u32, hex) and values (hex). If an xattr is not present, the name and length are not displayed in the measurement list. Reported-by: kernel test robot (Missing prototype def) Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_main.c | 69 +++++++++++++++++++++++++++++++ security/integrity/ima/ima_template.c | 9 ++++ security/integrity/ima/ima_template_lib.c | 64 ++++++++++++++++++++++++++++ security/integrity/ima/ima_template_lib.h | 6 +++ 4 files changed, 148 insertions(+) (limited to 'security') diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index ee4e17a790fb..2c226e634ae9 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -317,6 +317,75 @@ int evm_protected_xattr_if_enabled(const char *req_xattr_name) return evm_protected_xattr_common(req_xattr_name, true); } +/** + * evm_read_protected_xattrs - read EVM protected xattr names, lengths, values + * @dentry: dentry of the read xattrs + * @inode: inode of the read xattrs + * @buffer: buffer xattr names, lengths or values are copied to + * @buffer_size: size of buffer + * @type: n: names, l: lengths, v: values + * @canonical_fmt: data format (true: little endian, false: native format) + * + * Read protected xattr names (separated by |), lengths (u32) or values for a + * given dentry and return the total size of copied data. If buffer is NULL, + * just return the total size. + * + * Returns the total size on success, a negative value on error. + */ +int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer, + int buffer_size, char type, bool canonical_fmt) +{ + struct xattr_list *xattr; + int rc, size, total_size = 0; + + list_for_each_entry_lockless(xattr, &evm_config_xattrnames, list) { + rc = __vfs_getxattr(dentry, d_backing_inode(dentry), + xattr->name, NULL, 0); + if (rc < 0 && rc == -ENODATA) + continue; + else if (rc < 0) + return rc; + + switch (type) { + case 'n': + size = strlen(xattr->name) + 1; + if (buffer) { + if (total_size) + *(buffer + total_size - 1) = '|'; + + memcpy(buffer + total_size, xattr->name, size); + } + break; + case 'l': + size = sizeof(u32); + if (buffer) { + if (canonical_fmt) + rc = cpu_to_le32(rc); + + *(u32 *)(buffer + total_size) = rc; + } + break; + case 'v': + size = rc; + if (buffer) { + rc = __vfs_getxattr(dentry, + d_backing_inode(dentry), xattr->name, + buffer + total_size, + buffer_size - total_size); + if (rc < 0) + return rc; + } + break; + default: + return -EINVAL; + } + + total_size += size; + } + + return total_size; +} + /** * evm_verifyxattr - verify the integrity of the requested xattr * @dentry: object of the verify xattr diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index 43784f2bf8bd..159a31d2fcdf 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -53,6 +53,15 @@ static const struct ima_template_field supported_fields[] = { .field_show = ima_show_template_uint}, {.field_id = "imode", .field_init = ima_eventinodemode_init, .field_show = ima_show_template_uint}, + {.field_id = "xattrnames", + .field_init = ima_eventinodexattrnames_init, + .field_show = ima_show_template_string}, + {.field_id = "xattrlengths", + .field_init = ima_eventinodexattrlengths_init, + .field_show = ima_show_template_sig}, + {.field_id = "xattrvalues", + .field_init = ima_eventinodexattrvalues_init, + .field_show = ima_show_template_sig}, }; /* diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 3156fb34b1af..518fd50ea48a 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -11,6 +11,7 @@ #include "ima_template_lib.h" #include +#include static bool ima_template_hash_algo_allowed(u8 algo) { @@ -618,3 +619,66 @@ int ima_eventinodemode_init(struct ima_event_data *event_data, return ima_write_template_field_data((char *)&mode, sizeof(mode), DATA_FMT_UINT, field_data); } + +static int ima_eventinodexattrs_init_common(struct ima_event_data *event_data, + struct ima_field_data *field_data, + char type) +{ + u8 *buffer = NULL; + int rc; + + if (!event_data->file) + return 0; + + rc = evm_read_protected_xattrs(file_dentry(event_data->file), NULL, 0, + type, ima_canonical_fmt); + if (rc < 0) + return 0; + + buffer = kmalloc(rc, GFP_KERNEL); + if (!buffer) + return 0; + + rc = evm_read_protected_xattrs(file_dentry(event_data->file), buffer, + rc, type, ima_canonical_fmt); + if (rc < 0) { + rc = 0; + goto out; + } + + rc = ima_write_template_field_data((char *)buffer, rc, DATA_FMT_HEX, + field_data); +out: + kfree(buffer); + return rc; +} + +/* + * ima_eventinodexattrnames_init - include a list of xattr names as part of the + * template data + */ +int ima_eventinodexattrnames_init(struct ima_event_data *event_data, + struct ima_field_data *field_data) +{ + return ima_eventinodexattrs_init_common(event_data, field_data, 'n'); +} + +/* + * ima_eventinodexattrlengths_init - include a list of xattr lengths as part of + * the template data + */ +int ima_eventinodexattrlengths_init(struct ima_event_data *event_data, + struct ima_field_data *field_data) +{ + return ima_eventinodexattrs_init_common(event_data, field_data, 'l'); +} + +/* + * ima_eventinodexattrvalues_init - include a list of xattr values as part of + * the template data + */ +int ima_eventinodexattrvalues_init(struct ima_event_data *event_data, + struct ima_field_data *field_data) +{ + return ima_eventinodexattrs_init_common(event_data, field_data, 'v'); +} diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h index 6509af4a97ee..c71f1de95753 100644 --- a/security/integrity/ima/ima_template_lib.h +++ b/security/integrity/ima/ima_template_lib.h @@ -56,4 +56,10 @@ int ima_eventinodegid_init(struct ima_event_data *event_data, struct ima_field_data *field_data); int ima_eventinodemode_init(struct ima_event_data *event_data, struct ima_field_data *field_data); +int ima_eventinodexattrnames_init(struct ima_event_data *event_data, + struct ima_field_data *field_data); +int ima_eventinodexattrlengths_init(struct ima_event_data *event_data, + struct ima_field_data *field_data); +int ima_eventinodexattrvalues_init(struct ima_event_data *event_data, + struct ima_field_data *field_data); #endif /* __LINUX_IMA_TEMPLATE_LIB_H */ -- cgit v1.2.3 From 88016de3ab075790e1f1bf047576e9b557c22d19 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Thu, 3 Jun 2021 15:17:05 +0200 Subject: ima: Define new template evm-sig With the recent introduction of the evmsig template field, remote verifiers can obtain the EVM portable signature instead of the IMA signature, to verify file metadata. After introducing the new fields to include file metadata in the measurement list, this patch finally defines the evm-sig template, whose format is: d-ng|n-ng|evmsig|xattrnames|xattrlengths|xattrvalues|iuid|igid|imode xattrnames, xattrlengths and xattrvalues are populated only from defined EVM protected xattrs, i.e. the ones that EVM considers to verify the portable signature. xattrnames and xattrlengths are populated only if the xattr is present. xattrnames and xattrlengths are not necessary for verifying the EVM portable signature, but they are included for completeness of information, if a remote verifier wants to infer more from file metadata. Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_template.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index 159a31d2fcdf..a85963853a91 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -22,6 +22,8 @@ static struct ima_template_desc builtin_templates[] = { {.name = "ima-sig", .fmt = "d-ng|n-ng|sig"}, {.name = "ima-buf", .fmt = "d-ng|n-ng|buf"}, {.name = "ima-modsig", .fmt = "d-ng|n-ng|sig|d-modsig|modsig"}, + {.name = "evm-sig", + .fmt = "d-ng|n-ng|evmsig|xattrnames|xattrlengths|xattrvalues|iuid|igid|imode"}, {.name = "", .fmt = ""}, /* placeholder for a custom format */ }; @@ -69,7 +71,8 @@ static const struct ima_template_field supported_fields[] = { * need to be accounted for since they shouldn't be defined in the same template * description as 'd-ng' and 'n-ng' respectively. */ -#define MAX_TEMPLATE_NAME_LEN sizeof("d-ng|n-ng|sig|buf|d-modisg|modsig") +#define MAX_TEMPLATE_NAME_LEN \ + sizeof("d-ng|n-ng|evmsig|xattrnames|xattrlengths|xattrvalues|iuid|igid|imode") static struct ima_template_desc *ima_template; static struct ima_template_desc *ima_buf_template; -- cgit v1.2.3 From d721c15fd519c08819fbc6de39b713e2ed1d9894 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 28 May 2021 09:38:12 +0200 Subject: evm: Don't return an error in evm_write_xattrs() if audit is not enabled This patch avoids that evm_write_xattrs() returns an error when audit is not enabled. The ab variable can be NULL and still be passed to the other audit_log_() functions, as those functions do not include any instruction. Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_secfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c index a99676eb7f41..8a9db7dfca7e 100644 --- a/security/integrity/evm/evm_secfs.c +++ b/security/integrity/evm/evm_secfs.c @@ -197,7 +197,7 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf, ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_INTEGRITY_EVM_XATTR); - if (!ab) + if (!ab && IS_ENABLED(CONFIG_AUDIT)) return -ENOMEM; xattr = kmalloc(sizeof(struct xattr_list), GFP_KERNEL); -- cgit v1.2.3 From 24c9ae23bdfa0642228e747849dd052fd4295c6c Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Tue, 8 Jun 2021 14:31:21 +0200 Subject: ima: Set correct casting types The code expects that the values being parsed from a buffer when the ima_canonical_fmt global variable is true are in little endian. Thus, this patch sets the casting types accordingly. Reported-by: kernel test robot Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_template.c | 8 ++++---- security/integrity/ima/ima_template_lib.c | 11 ++++++----- 2 files changed, 10 insertions(+), 9 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index a85963853a91..694560396be0 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -423,9 +423,9 @@ int ima_restore_measurement_list(loff_t size, void *buf) return 0; if (ima_canonical_fmt) { - khdr->version = le16_to_cpu(khdr->version); - khdr->count = le64_to_cpu(khdr->count); - khdr->buffer_size = le64_to_cpu(khdr->buffer_size); + khdr->version = le16_to_cpu((__force __le16)khdr->version); + khdr->count = le64_to_cpu((__force __le64)khdr->count); + khdr->buffer_size = le64_to_cpu((__force __le64)khdr->buffer_size); } if (khdr->version != 1) { @@ -515,7 +515,7 @@ int ima_restore_measurement_list(loff_t size, void *buf) } entry->pcr = !ima_canonical_fmt ? *(u32 *)(hdr[HDR_PCR].data) : - le32_to_cpu(*(u32 *)(hdr[HDR_PCR].data)); + le32_to_cpu(*(__le32 *)(hdr[HDR_PCR].data)); ret = ima_restore_measurement_entry(entry); if (ret < 0) break; diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 518fd50ea48a..3f8d53a03612 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -98,21 +98,21 @@ static void ima_show_template_data_ascii(struct seq_file *m, case sizeof(u16): if (ima_canonical_fmt) seq_printf(m, "%u", - le16_to_cpu(*(u16 *)buf_ptr)); + le16_to_cpu(*(__le16 *)buf_ptr)); else seq_printf(m, "%u", *(u16 *)buf_ptr); break; case sizeof(u32): if (ima_canonical_fmt) seq_printf(m, "%u", - le32_to_cpu(*(u32 *)buf_ptr)); + le32_to_cpu(*(__le32 *)buf_ptr)); else seq_printf(m, "%u", *(u32 *)buf_ptr); break; case sizeof(u64): if (ima_canonical_fmt) seq_printf(m, "%llu", - le64_to_cpu(*(u64 *)buf_ptr)); + le64_to_cpu(*(__le64 *)buf_ptr)); else seq_printf(m, "%llu", *(u64 *)buf_ptr); break; @@ -226,9 +226,10 @@ int ima_parse_buf(void *bufstartp, void *bufendp, void **bufcurp, if (bufp > (bufendp - sizeof(u32))) break; - fields[i].len = *(u32 *)bufp; if (ima_canonical_fmt) - fields[i].len = le32_to_cpu(fields[i].len); + fields[i].len = le32_to_cpu(*(__le32 *)bufp); + else + fields[i].len = *(u32 *)bufp; bufp += sizeof(u32); } -- cgit v1.2.3 From 6b26285f44c9306747c609cb304f787f1933594c Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Tue, 8 Jun 2021 14:31:22 +0200 Subject: ima/evm: Fix type mismatch The endianness of a variable written to the measurement list cannot be determined at compile time, as it depends on the value of the ima_canonical_fmt global variable (set through a kernel option with the same name if the machine is big endian). If ima_canonical_fmt is false, the endianness of a variable is the same as the machine; if ima_canonical_fmt is true, the endianness is little endian. The warning arises due to this type of instruction: var = cpu_to_leXX(var) which tries to assign a value in little endian to a variable with native endianness (little or big endian). Given that the variables set with this instruction are not used in any operation but just written to a buffer, it is safe to force the type of the value being set to be the same of the type of the variable with: var = (__force )cpu_to_leXX(var) Reported-by: kernel test robot Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_main.c | 2 +- security/integrity/ima/ima_crypto.c | 4 ++-- security/integrity/ima/ima_fs.c | 6 +++--- security/integrity/ima/ima_template_lib.c | 11 ++++++----- 4 files changed, 12 insertions(+), 11 deletions(-) (limited to 'security') diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 2c226e634ae9..977208aecd06 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -360,7 +360,7 @@ int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer, size = sizeof(u32); if (buffer) { if (canonical_fmt) - rc = cpu_to_le32(rc); + rc = (__force int)cpu_to_le32(rc); *(u32 *)(buffer + total_size) = rc; } diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index f6a7e9643b54..a7206cc1d7d1 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -598,8 +598,8 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data, u8 buffer[IMA_EVENT_NAME_LEN_MAX + 1] = { 0 }; u8 *data_to_hash = field_data[i].data; u32 datalen = field_data[i].len; - u32 datalen_to_hash = - !ima_canonical_fmt ? datalen : cpu_to_le32(datalen); + u32 datalen_to_hash = !ima_canonical_fmt ? + datalen : (__force u32)cpu_to_le32(datalen); if (strcmp(td->name, IMA_TEMPLATE_IMA_NAME) != 0) { rc = crypto_shash_update(shash, diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index ea8ff8a07b36..3d8e9d5db5aa 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -147,7 +147,7 @@ int ima_measurements_show(struct seq_file *m, void *v) * PCR used defaults to the same (config option) in * little-endian format, unless set in policy */ - pcr = !ima_canonical_fmt ? e->pcr : cpu_to_le32(e->pcr); + pcr = !ima_canonical_fmt ? e->pcr : (__force u32)cpu_to_le32(e->pcr); ima_putc(m, &pcr, sizeof(e->pcr)); /* 2nd: template digest */ @@ -155,7 +155,7 @@ int ima_measurements_show(struct seq_file *m, void *v) /* 3rd: template name size */ namelen = !ima_canonical_fmt ? strlen(template_name) : - cpu_to_le32(strlen(template_name)); + (__force u32)cpu_to_le32(strlen(template_name)); ima_putc(m, &namelen, sizeof(namelen)); /* 4th: template name */ @@ -167,7 +167,7 @@ int ima_measurements_show(struct seq_file *m, void *v) if (!is_ima_template) { template_data_len = !ima_canonical_fmt ? e->template_data_len : - cpu_to_le32(e->template_data_len); + (__force u32)cpu_to_le32(e->template_data_len); ima_putc(m, &template_data_len, sizeof(e->template_data_len)); } diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 3f8d53a03612..8e2a121af5e1 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -133,7 +133,8 @@ static void ima_show_template_data_binary(struct seq_file *m, strlen(field_data->data) : field_data->len; if (show != IMA_SHOW_BINARY_NO_FIELD_LEN) { - u32 field_len = !ima_canonical_fmt ? len : cpu_to_le32(len); + u32 field_len = !ima_canonical_fmt ? + len : (__force u32)cpu_to_le32(len); ima_putc(m, &field_len, sizeof(field_len)); } @@ -570,9 +571,9 @@ static int ima_eventinodedac_init_common(struct ima_event_data *event_data, if (ima_canonical_fmt) { if (sizeof(id) == sizeof(u16)) - id = cpu_to_le16(id); + id = (__force u16)cpu_to_le16(id); else - id = cpu_to_le32(id); + id = (__force u32)cpu_to_le32(id); } return ima_write_template_field_data((void *)&id, sizeof(id), @@ -607,7 +608,7 @@ int ima_eventinodemode_init(struct ima_event_data *event_data, struct ima_field_data *field_data) { struct inode *inode; - umode_t mode; + u16 mode; if (!event_data->file) return 0; @@ -615,7 +616,7 @@ int ima_eventinodemode_init(struct ima_event_data *event_data, inode = file_inode(event_data->file); mode = inode->i_mode; if (ima_canonical_fmt) - mode = cpu_to_le16(mode); + mode = (__force u16)cpu_to_le16(mode); return ima_write_template_field_data((char *)&mode, sizeof(mode), DATA_FMT_UINT, field_data); -- cgit v1.2.3 From 8c559415f66a42721fcfdf321cb7a58df01a4c74 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Tue, 8 Jun 2021 14:31:23 +0200 Subject: ima: Include header defining ima_post_key_create_or_update() This patch fixes the sparse warning for ima_post_key_create_or_update() by adding the header file that defines the prototype (linux/ima.h). Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_asymmetric_keys.c | 1 + 1 file changed, 1 insertion(+) (limited to 'security') diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c index 1fb0b0e09559..c985418698a4 100644 --- a/security/integrity/ima/ima_asymmetric_keys.c +++ b/security/integrity/ima/ima_asymmetric_keys.c @@ -11,6 +11,7 @@ #include #include +#include #include "ima.h" /** -- cgit v1.2.3 From 531bf6a88d9bd6c13d4fc3f05d2de799d627de3b Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Tue, 8 Jun 2021 14:31:24 +0200 Subject: ima: Pass NULL instead of 0 to ima_get_action() in ima_file_mprotect() This patch fixes the sparse warning: sparse: warning: Using plain integer as NULL pointer Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 906c1d8e0b71..287b90509006 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -433,7 +433,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) inode = file_inode(vma->vm_file); action = ima_get_action(file_mnt_user_ns(vma->vm_file), inode, current_cred(), secid, MAY_EXEC, MMAP_CHECK, - &pcr, &template, 0); + &pcr, &template, NULL); /* Is the mmap'ed file in policy? */ if (!(action & (IMA_MEASURE | IMA_APPRAISE_SUBMASK))) -- cgit v1.2.3 From 7d2201d46218df951004fc48897f89c6eb510b69 Mon Sep 17 00:00:00 2001 From: "Gustavo A. R. Silva" Date: Mon, 7 Jun 2021 15:49:34 -0500 Subject: ima: Fix fall-through warning for Clang In preparation to enable -Wimplicit-fallthrough for Clang, fix a fall-through warning by explicitly adding a break statement instead of just letting the code fall through to the next case. Link: https://github.com/KSPP/linux/issues/115 Signed-off-by: Gustavo A. R. Silva Reviewed-by: Kees Cook Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_template_lib.c | 1 + 1 file changed, 1 insertion(+) (limited to 'security') diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 8e2a121af5e1..ca017cae73eb 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -119,6 +119,7 @@ static void ima_show_template_data_ascii(struct seq_file *m, default: break; } + break; default: break; } -- cgit v1.2.3 From 55748ac6a6d3e35f8fd0f5c9284df7c7f3b1705a Mon Sep 17 00:00:00 2001 From: Mimi Zohar Date: Wed, 2 Jun 2021 16:33:39 -0400 Subject: ima: differentiate between EVM failures in the audit log Differentiate between an invalid EVM portable signature failure from other EVM HMAC/signature failures. Reviewed-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_appraise.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 940695e7b535..ef9dcfce45d4 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -422,7 +422,8 @@ int ima_appraise_measurement(enum ima_hooks func, goto out; case INTEGRITY_FAIL_IMMUTABLE: set_bit(IMA_DIGSIG, &iint->atomic_flags); - fallthrough; + cause = "invalid-fail-immutable"; + goto out; case INTEGRITY_FAIL: /* Invalid HMAC/signature. */ cause = "invalid-HMAC"; goto out; -- cgit v1.2.3 From c67913492fec317bc53ffdff496b6ba856d2868c Mon Sep 17 00:00:00 2001 From: Lakshmi Ramasubramanian Date: Thu, 10 Jun 2021 10:15:52 -0700 Subject: ima: Fix warning: no previous prototype for function 'ima_add_kexec_buffer' The function prototype for ima_add_kexec_buffer() is present in 'linux/ima.h'. But this header file is not included in ima_kexec.c where the function is implemented. This results in the following compiler warning when "-Wmissing-prototypes" flag is turned on: security/integrity/ima/ima_kexec.c:81:6: warning: no previous prototype for function 'ima_add_kexec_buffer' [-Wmissing-prototypes] Include the header file 'linux/ima.h' in ima_kexec.c to fix the compiler warning. Fixes: dce92f6b11c3 (arm64: Enable passing IMA log to next kernel on kexec) Reported-by: kernel test robot Signed-off-by: Lakshmi Ramasubramanian Acked-by: Rob Herring Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_kexec.c | 1 + 1 file changed, 1 insertion(+) (limited to 'security') diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index 667887665823..f799cc278a9a 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -11,6 +11,7 @@ #include #include #include +#include #include "ima.h" #ifdef CONFIG_IMA_KEXEC -- cgit v1.2.3 From 52c208397c246f0c31d031eb8c41f9c7e9fdec0e Mon Sep 17 00:00:00 2001 From: Tushar Sugandhi Date: Mon, 10 May 2021 12:09:39 -0700 Subject: IMA: support for duplicate measurement records IMA measures contents of a given file/buffer/critical-data record, and properly re-measures it on change. However, IMA does not measure the duplicate value for a given record, since TPM extend is a very expensive operation. For example, if the record changes from value 'v#1' to 'v#2', and then back to 'v#1', IMA will not measure and log the last change to 'v#1', since the hash of 'v#1' for that record is already present in the IMA htable. This limits the ability of an external attestation service to accurately determine the current state of the system. The service would incorrectly conclude that the latest value of the given record on the system is 'v#2', and act accordingly. Define and use a new Kconfig option IMA_DISABLE_HTABLE to permit duplicate records in the IMA measurement list. In addition to the duplicate measurement records described above, other duplicate file measurement records may be included in the log, when CONFIG_IMA_DISABLE_HTABLE is enabled. For example, - i_version is not enabled, - i_generation changed, - same file present on different filesystems, - an inode is evicted from dcache Signed-off-by: Tushar Sugandhi Reviewed-by: Petr Vorel [zohar@linux.ibm.com: updated list of duplicate measurement records] Signed-off-by: Mimi Zohar --- security/integrity/ima/Kconfig | 7 +++++++ security/integrity/ima/ima_queue.c | 5 +++-- 2 files changed, 10 insertions(+), 2 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index 12e9250c1bec..d0ceada99243 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -334,3 +334,10 @@ config IMA_SECURE_AND_OR_TRUSTED_BOOT help This option is selected by architectures to enable secure and/or trusted boot based on IMA runtime policies. + +config IMA_DISABLE_HTABLE + bool "Disable htable to allow measurement of duplicate records" + depends on IMA + default n + help + This option disables htable to allow measurement of duplicate records. diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index c096ef8945c7..532da87ce519 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -168,7 +168,7 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, int result = 0, tpmresult = 0; mutex_lock(&ima_extend_list_mutex); - if (!violation) { + if (!violation && !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE)) { if (ima_lookup_digest_entry(digest, entry->pcr)) { audit_cause = "hash_exists"; result = -EEXIST; @@ -176,7 +176,8 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, } } - result = ima_add_digest_entry(entry, 1); + result = ima_add_digest_entry(entry, + !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE)); if (result < 0) { audit_cause = "ENOMEM"; audit_info = 0; -- cgit v1.2.3 From 87ac3d002d567fac3527d6612865e81cfd783727 Mon Sep 17 00:00:00 2001 From: Mimi Zohar Date: Thu, 13 May 2021 18:36:04 -0400 Subject: evm: output EVM digest calculation info Output the data used in calculating the EVM digest and the resulting digest as ascii hexadecimal strings. Suggested-by: Lakshmi Ramasubramanian (CONFIG_DYNAMIC_DEBUG) Reviewed-by: Lakshmi Ramasubramanian Reported-by: kernel test robot (Use %zu for size_t) Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_crypto.c | 43 +++++++++++++++++++++++++++++++++++++ security/integrity/evm/evm_main.c | 4 ++++ 2 files changed, 47 insertions(+) (limited to 'security') diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 1628e2ca9862..bebe160c57b9 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -10,6 +10,8 @@ * Using root's kernel master key (kmk), calculate the HMAC */ +#define pr_fmt(fmt) "EVM: "fmt + #include #include #include @@ -175,6 +177,30 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode, type != EVM_XATTR_PORTABLE_DIGSIG) crypto_shash_update(desc, (u8 *)&inode->i_sb->s_uuid, UUID_SIZE); crypto_shash_final(desc, digest); + + pr_debug("hmac_misc: (%zu) [%*phN]\n", sizeof(struct h_misc), + (int)sizeof(struct h_misc), &hmac_misc); +} + +/* + * Dump large security xattr values as a continuous ascii hexademical string. + * (pr_debug is limited to 64 bytes.) + */ +static void dump_security_xattr(const char *prefix, const void *src, + size_t count) +{ +#if defined(DEBUG) || defined(CONFIG_DYNAMIC_DEBUG) + char *asciihex, *p; + + p = asciihex = kmalloc(count * 2 + 1, GFP_KERNEL); + if (!asciihex) + return; + + p = bin2hex(p, src, count); + *p = 0; + pr_debug("%s: (%zu) %.*s\n", prefix, count, (int)count * 2, asciihex); + kfree(asciihex); +#endif } /* @@ -230,6 +256,16 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, req_xattr_value_len); if (is_ima) ima_present = true; + + if (req_xattr_value_len < 64) + pr_debug("%s: (%zu) [%*phN]\n", req_xattr_name, + req_xattr_value_len, + (int)req_xattr_value_len, + req_xattr_value); + else + dump_security_xattr(req_xattr_name, + req_xattr_value, + req_xattr_value_len); continue; } size = vfs_getxattr_alloc(&init_user_ns, dentry, xattr->name, @@ -246,6 +282,13 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size); if (is_ima) ima_present = true; + + if (xattr_size < 64) + pr_debug("%s: (%zu) [%*phN]", xattr->name, xattr_size, + (int)xattr_size, xattr_value); + else + dump_security_xattr(xattr->name, xattr_value, + xattr_size); } hmac_add_misc(desc, inode, type, data->digest); diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 977208aecd06..1c8435dfabee 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -11,6 +11,8 @@ * evm_inode_removexattr, and evm_verifyxattr */ +#define pr_fmt(fmt) "EVM: "fmt + #include #include #include @@ -272,6 +274,8 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, else evm_status = INTEGRITY_FAIL; } + pr_debug("digest: (%d) [%*phN]\n", digest.hdr.length, digest.hdr.length, + digest.digest); out: if (iint) iint->evm_status = evm_status; -- cgit v1.2.3 From 907a399de7b0566236c480d0c01ff52220532fb1 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Mon, 21 Jun 2021 14:29:12 +0200 Subject: evm: Check xattr size discrepancy between kernel and user The kernel and the user obtain an xattr value in two different ways: kernel (EVM): uses vfs_getxattr_alloc() which obtains the xattr value from the filesystem handler (raw value); user (ima-evm-utils): uses vfs_getxattr() which obtains the xattr value from the LSMs (normalized value). Normally, this does not have an impact unless security.selinux is set with setfattr, with a value not terminated by '\0' (this is not the recommended way, security.selinux should be set with the appropriate tools such as chcon and restorecon). In this case, the kernel and the user see two different xattr values: the former sees the xattr value without '\0' (raw value), the latter sees the value with '\0' (value normalized by SELinux). This could result in two different verification outcomes from EVM and ima-evm-utils, if a signature was calculated with a security.selinux value terminated by '\0' and the value set in the filesystem is not terminated by '\0'. The former would report verification failure due to the missing '\0', while the latter would report verification success (because it gets the normalized value with '\0'). This patch mitigates this issue by comparing in evm_calc_hmac_or_hash() the size of the xattr returned by the two xattr functions and by warning the user if there is a discrepancy. Signed-off-by: Roberto Sassu Suggested-by: Mimi Zohar Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_crypto.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index bebe160c57b9..0450d79afdc8 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -222,7 +222,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, size_t xattr_size = 0; char *xattr_value = NULL; int error; - int size; + int size, user_space_size; bool ima_present = false; if (!(inode->i_opflags & IOP_XATTR) || @@ -277,6 +277,12 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, if (size < 0) continue; + user_space_size = vfs_getxattr(&init_user_ns, dentry, + xattr->name, NULL, 0); + if (user_space_size != size) + pr_debug("file %s: xattr %s size mismatch (kernel: %d, user: %d)\n", + dentry->d_name.name, xattr->name, size, + user_space_size); error = 0; xattr_size = size; crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size); -- cgit v1.2.3