From 5adedd42245af0860ebda8fe0949f24f5204c1b1 Mon Sep 17 00:00:00 2001 From: Nikolaus Voss Date: Wed, 19 Oct 2022 18:38:20 +0200 Subject: KEYS: encrypted: fix key instantiation with user-provided data Commit cd3bc044af48 ("KEYS: encrypted: Instantiate key with user-provided decrypted data") added key instantiation with user provided decrypted data. The user data is hex-ascii-encoded but was just memcpy'ed to the binary buffer. Fix this to use hex2bin instead. Old keys created from user provided decrypted data saved with "keyctl pipe" are still valid, however if the key is recreated from decrypted data the old key must be converted to the correct format. This can be done with a small shell script, e.g.: BROKENKEY=abcdefABCDEF1234567890aaaaaaaaaa NEWKEY=$(echo -ne $BROKENKEY | xxd -p -c32) keyctl add user masterkey "$(cat masterkey.bin)" @u keyctl add encrypted testkey "new user:masterkey 32 $NEWKEY" @u However, NEWKEY is still broken: If for BROKENKEY 32 bytes were specified, a brute force attacker knowing the key properties would only need to try at most 2^(16*8) keys, as if the key was only 16 bytes long. The security issue is a result of the combination of limiting the input range to hex-ascii and using memcpy() instead of hex2bin(). It could have been fixed either by allowing binary input or using hex2bin() (and doubling the ascii input key length). This patch implements the latter. The corresponding test for the Linux Test Project ltp has also been fixed (see link below). Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user-provided decrypted data") Cc: stable@kernel.org Link: https://lore.kernel.org/ltp/20221006081709.92303897@mail.steuer-voss.de/ Reviewed-by: Mimi Zohar Signed-off-by: Nikolaus Voss Signed-off-by: Mimi Zohar --- security/keys/encrypted-keys/encrypted.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'security') diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index e05cfc2e49ae..1e313982af02 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -627,7 +627,7 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key, pr_err("encrypted key: instantiation of keys using provided decrypted data is disabled since CONFIG_USER_DECRYPTED_DATA is set to false\n"); return ERR_PTR(-EINVAL); } - if (strlen(decrypted_data) != decrypted_datalen) { + if (strlen(decrypted_data) != decrypted_datalen * 2) { pr_err("encrypted key: decrypted data provided does not match decrypted data length provided\n"); return ERR_PTR(-EINVAL); } @@ -791,8 +791,8 @@ static int encrypted_init(struct encrypted_key_payload *epayload, ret = encrypted_key_decrypt(epayload, format, hex_encoded_iv); } else if (decrypted_data) { get_random_bytes(epayload->iv, ivsize); - memcpy(epayload->decrypted_data, decrypted_data, - epayload->decrypted_datalen); + ret = hex2bin(epayload->decrypted_data, decrypted_data, + epayload->decrypted_datalen); } else { get_random_bytes(epayload->iv, ivsize); get_random_bytes(epayload->decrypted_data, epayload->decrypted_datalen); -- cgit v1.2.3 From 0be56a116220f9e5731a6609e66a11accfe8d8e2 Mon Sep 17 00:00:00 2001 From: Aditya Garg Date: Thu, 27 Oct 2022 10:01:43 +0000 Subject: efi: Add iMac Pro 2017 to uefi skip cert quirk The iMac Pro 2017 is also a T2 Mac. Thus add it to the list of uefi skip cert. Cc: stable@vger.kernel.org Fixes: 155ca952c7ca ("efi: Do not import certificates from UEFI Secure Boot for T2 Macs") Link: https://lore.kernel.org/linux-integrity/9D46D92F-1381-4F10-989C-1A12CD2FFDD8@live.com/ Signed-off-by: Aditya Garg Signed-off-by: Mimi Zohar --- security/integrity/platform_certs/load_uefi.c | 1 + 1 file changed, 1 insertion(+) (limited to 'security') diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c index b78753d27d8e..d1fdd113450a 100644 --- a/security/integrity/platform_certs/load_uefi.c +++ b/security/integrity/platform_certs/load_uefi.c @@ -35,6 +35,7 @@ static const struct dmi_system_id uefi_skip_cert[] = { { UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacPro7,1") }, { UEFI_QUIRK_SKIP_CERT("Apple Inc.", "iMac20,1") }, { UEFI_QUIRK_SKIP_CERT("Apple Inc.", "iMac20,2") }, + { UEFI_QUIRK_SKIP_CERT("Apple Inc.", "iMacPro1,1") }, { } }; -- cgit v1.2.3 From 11220db412edae8dba58853238f53258268bdb88 Mon Sep 17 00:00:00 2001 From: Huaxin Lu Date: Thu, 3 Nov 2022 00:09:49 +0800 Subject: ima: Fix a potential NULL pointer access in ima_restore_measurement_list In restore_template_fmt, when kstrdup fails, a non-NULL value will still be returned, which causes a NULL pointer access in template_desc_init_fields. Fixes: c7d09367702e ("ima: support restoring multiple template formats") Cc: stable@kernel.org Co-developed-by: Jiaming Li Signed-off-by: Jiaming Li Signed-off-by: Huaxin Lu Reviewed-by: Stefan Berger 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 c25079faa208..49f0626928a1 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -340,8 +340,11 @@ static struct ima_template_desc *restore_template_fmt(char *template_name) template_desc->name = ""; template_desc->fmt = kstrdup(template_name, GFP_KERNEL); - if (!template_desc->fmt) + if (!template_desc->fmt) { + kfree(template_desc); + template_desc = NULL; goto out; + } spin_lock(&template_list); list_add_tail_rcu(&template_desc->list, &defined_templates); -- cgit v1.2.3 From d57378d3aa4d864d9e590482602068af1b20c0c5 Mon Sep 17 00:00:00 2001 From: GUO Zihua Date: Wed, 21 Sep 2022 20:58:03 +0800 Subject: ima: Simplify ima_lsm_copy_rule Currently ima_lsm_copy_rule() set the arg_p field of the source rule to NULL, so that the source rule could be freed afterward. It does not make sense for this behavior to be inside a "copy" function. So move it outside and let the caller handle this field. ima_lsm_copy_rule() now produce a shallow copy of the original entry including args_p field. Meaning only the lsm.rule and the rule itself should be freed for the original rule. Thus, instead of calling ima_lsm_free_rule() which frees lsm.rule as well as args_p field, free the lsm.rule directly. Signed-off-by: GUO Zihua Reviewed-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index a8802b8da946..8040215c0252 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -398,12 +398,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) nentry->lsm[i].type = entry->lsm[i].type; nentry->lsm[i].args_p = entry->lsm[i].args_p; - /* - * Remove the reference from entry so that the associated - * memory will not be freed during a later call to - * ima_lsm_free_rule(entry). - */ - entry->lsm[i].args_p = NULL; ima_filter_rule_init(nentry->lsm[i].type, Audit_equal, nentry->lsm[i].args_p, @@ -417,6 +411,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) static int ima_lsm_update_rule(struct ima_rule_entry *entry) { + int i; struct ima_rule_entry *nentry; nentry = ima_lsm_copy_rule(entry); @@ -431,7 +426,8 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry) * references and the entry itself. All other memory references will now * be owned by nentry. */ - ima_lsm_free_rule(entry); + for (i = 0; i < MAX_LSM_RULES; i++) + ima_filter_rule_free(entry->lsm[i].rule); kfree(entry); return 0; -- cgit v1.2.3 From c7423dbdbc9ecef7fff5239d144cad4b9887f4de Mon Sep 17 00:00:00 2001 From: GUO Zihua Date: Wed, 21 Sep 2022 20:58:04 +0800 Subject: ima: Handle -ESTALE returned by ima_filter_rule_match() IMA relies on the blocking LSM policy notifier callback to update the LSM based IMA policy rules. When SELinux update its policies, IMA would be notified and starts updating all its lsm rules one-by-one. During this time, -ESTALE would be returned by ima_filter_rule_match() if it is called with a LSM rule that has not yet been updated. In ima_match_rules(), -ESTALE is not handled, and the LSM rule is considered a match, causing extra files to be measured by IMA. Fix it by re-initializing a temporary rule if -ESTALE is returned by ima_filter_rule_match(). The origin rule in the rule list would be updated by the LSM policy notifier callback. Fixes: b16942455193 ("ima: use the lsm policy update notifier") Signed-off-by: GUO Zihua Reviewed-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 41 +++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 9 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 8040215c0252..2edff7f58c25 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -545,6 +545,9 @@ static bool ima_match_rules(struct ima_rule_entry *rule, const char *func_data) { int i; + bool result = false; + struct ima_rule_entry *lsm_rule = rule; + bool rule_reinitialized = false; if ((rule->flags & IMA_FUNC) && (rule->func != func && func != POST_SETATTR)) @@ -606,35 +609,55 @@ static bool ima_match_rules(struct ima_rule_entry *rule, int rc = 0; u32 osid; - if (!rule->lsm[i].rule) { - if (!rule->lsm[i].args_p) + if (!lsm_rule->lsm[i].rule) { + if (!lsm_rule->lsm[i].args_p) continue; else return false; } + +retry: switch (i) { case LSM_OBJ_USER: case LSM_OBJ_ROLE: case LSM_OBJ_TYPE: security_inode_getsecid(inode, &osid); - rc = ima_filter_rule_match(osid, rule->lsm[i].type, + rc = ima_filter_rule_match(osid, lsm_rule->lsm[i].type, Audit_equal, - rule->lsm[i].rule); + lsm_rule->lsm[i].rule); break; case LSM_SUBJ_USER: case LSM_SUBJ_ROLE: case LSM_SUBJ_TYPE: - rc = ima_filter_rule_match(secid, rule->lsm[i].type, + rc = ima_filter_rule_match(secid, lsm_rule->lsm[i].type, Audit_equal, - rule->lsm[i].rule); + lsm_rule->lsm[i].rule); break; default: break; } - if (!rc) - return false; + + if (rc == -ESTALE && !rule_reinitialized) { + lsm_rule = ima_lsm_copy_rule(rule); + if (lsm_rule) { + rule_reinitialized = true; + goto retry; + } + } + if (!rc) { + result = false; + goto out; + } } - return true; + result = true; + +out: + if (rule_reinitialized) { + for (i = 0; i < MAX_LSM_RULES; i++) + ima_filter_rule_free(lsm_rule->lsm[i].rule); + kfree(lsm_rule); + } + return result; } /* -- cgit v1.2.3 From 8c1d6a050a0f16e0a9d32eaf53b965c77279c6f8 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Wed, 2 Nov 2022 17:30:06 +0100 Subject: ima: Fix memory leak in __ima_inode_hash() Commit f3cc6b25dcc5 ("ima: always measure and audit files in policy") lets measurement or audit happen even if the file digest cannot be calculated. As a result, iint->ima_hash could have been allocated despite ima_collect_measurement() returning an error. Since ima_hash belongs to a temporary inode metadata structure, declared at the beginning of __ima_inode_hash(), just add a kfree() call if ima_collect_measurement() returns an error different from -ENOMEM (in that case, ima_hash should not have been allocated). Cc: stable@vger.kernel.org Fixes: 280fe8367b0d ("ima: Always return a file measurement in ima_file_hash()") Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_main.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 040b03ddc1c7..4a207a3ef7ef 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -542,8 +542,13 @@ static int __ima_inode_hash(struct inode *inode, struct file *file, char *buf, rc = ima_collect_measurement(&tmp_iint, file, NULL, 0, ima_hash_algo, NULL); - if (rc < 0) + if (rc < 0) { + /* ima_hash could be allocated in case of failure. */ + if (rc != -ENOMEM) + kfree(tmp_iint.ima_hash); + return -EOPNOTSUPP; + } iint = &tmp_iint; mutex_lock(&iint->mutex); -- cgit v1.2.3 From 39419ef7af0916cc3620ecf1ed42d29659109bf3 Mon Sep 17 00:00:00 2001 From: GUO Zihua Date: Fri, 11 Nov 2022 18:13:17 +0800 Subject: integrity: Fix memory leakage in keyring allocation error path Key restriction is allocated in integrity_init_keyring(). However, if keyring allocation failed, it is not freed, causing memory leaks. Fixes: 2b6aa412ff23 ("KEYS: Use structure to capture key restriction function and data") Signed-off-by: GUO Zihua Signed-off-by: Mimi Zohar --- security/integrity/digsig.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c index 8a82a6c7f48a..f2193c531f4a 100644 --- a/security/integrity/digsig.c +++ b/security/integrity/digsig.c @@ -126,6 +126,7 @@ int __init integrity_init_keyring(const unsigned int id) { struct key_restriction *restriction; key_perm_t perm; + int ret; perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH; @@ -154,7 +155,10 @@ int __init integrity_init_keyring(const unsigned int id) perm |= KEY_USR_WRITE; out: - return __integrity_init_keyring(id, perm, restriction); + ret = __integrity_init_keyring(id, perm, restriction); + if (ret) + kfree(restriction); + return ret; } static int __init integrity_add_key(const unsigned int id, const void *data, -- cgit v1.2.3 From 25369175ce84813dd99d6604e710dc2491f68523 Mon Sep 17 00:00:00 2001 From: Xiu Jianfeng Date: Sat, 12 Nov 2022 17:27:19 +0800 Subject: ima: Fix misuse of dereference of pointer in template_desc_init_fields() The input parameter @fields is type of struct ima_template_field ***, so when allocates array memory for @fields, the size of element should be sizeof(**field) instead of sizeof(*field). Actually the original code would not cause any runtime error, but it's better to make it logically right. Fixes: adf53a778a0a ("ima: new templates management mechanism") Signed-off-by: Xiu Jianfeng Reviewed-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_template.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index 49f0626928a1..04c49f05cb74 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -245,11 +245,11 @@ int template_desc_init_fields(const char *template_fmt, } if (fields && num_fields) { - *fields = kmalloc_array(i, sizeof(*fields), GFP_KERNEL); + *fields = kmalloc_array(i, sizeof(**fields), GFP_KERNEL); if (*fields == NULL) return -ENOMEM; - memcpy(*fields, found_fields, i * sizeof(*fields)); + memcpy(*fields, found_fields, i * sizeof(**fields)); *num_fields = i; } -- cgit v1.2.3 From b6018af440a07bd0d74b58c4e18045f4a8dbfe6b Mon Sep 17 00:00:00 2001 From: Tianjia Zhang Date: Fri, 25 Nov 2022 20:18:40 +0800 Subject: ima: Fix hash dependency to correct algorithm Commit d2825fa9365d ("crypto: sm3,sm4 - move into crypto directory") moves the SM3 and SM4 stand-alone library and the algorithm implementation for the Crypto API into the same directory, and the corresponding relationship of Kconfig is modified, CONFIG_CRYPTO_SM3/4 corresponds to the stand-alone library of SM3/4, and CONFIG_CRYPTO_SM3/4_GENERIC corresponds to the algorithm implementation for the Crypto API. Therefore, it is necessary for this module to depend on the correct algorithm. Fixes: d2825fa9365d ("crypto: sm3,sm4 - move into crypto directory") Cc: Jason A. Donenfeld Cc: stable@vger.kernel.org # v5.19+ Signed-off-by: Tianjia Zhang Signed-off-by: Mimi Zohar --- security/integrity/ima/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index 7249f16257c7..39caeca47444 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -112,7 +112,7 @@ choice config IMA_DEFAULT_HASH_SM3 bool "SM3" - depends on CRYPTO_SM3=y + depends on CRYPTO_SM3_GENERIC=y endchoice config IMA_DEFAULT_HASH -- cgit v1.2.3