From 176377d97d6a3fae971ecb1f47d9b9cb7dab05ef Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Tue, 11 Aug 2020 14:26:20 -0500 Subject: ima: Pre-parse the list of keyrings in a KEY_CHECK rule The ima_keyrings buffer was used as a work buffer for strsep()-based parsing of the "keyrings=" option of an IMA policy rule. This parsing was re-performed each time an asymmetric key was added to a kernel keyring for each loaded policy rule that contained a "keyrings=" option. An example rule specifying this option is: measure func=KEY_CHECK keyrings=a|b|c The rule says to measure asymmetric keys added to any of the kernel keyrings named "a", "b", or "c". The size of the buffer size was equal to the size of the largest "keyrings=" value seen in a previously loaded rule (5 + 1 for the NUL-terminator in the previous example) and the buffer was pre-allocated at the time of policy load. The pre-allocated buffer approach suffered from a couple bugs: 1) There was no locking around the use of the buffer so concurrent key add operations, to two different keyrings, would result in the strsep() loop of ima_match_keyring() to modify the buffer at the same time. This resulted in unexpected results from ima_match_keyring() and, therefore, could cause unintended keys to be measured or keys to not be measured when IMA policy intended for them to be measured. 2) If the kstrdup() that initialized entry->keyrings in ima_parse_rule() failed, the ima_keyrings buffer was freed and set to NULL even when a valid KEY_CHECK rule was previously loaded. The next KEY_CHECK event would trigger a call to strcpy() with a NULL destination pointer and crash the kernel. Remove the need for a pre-allocated global buffer by parsing the list of keyrings in a KEY_CHECK rule at the time of policy load. The ima_rule_entry will contain an array of string pointers which point to the name of each keyring specified in the rule. No string processing needs to happen at the time of asymmetric key add so iterating through the list and doing a string comparison is all that's required at the time of policy check. In the process of changing how the "keyrings=" policy option is handled, a couple additional bugs were fixed: 1) The rule parser accepted rules containing invalid "keyrings=" values such as "a|b||c", "a|b|", or simply "|". 2) The /sys/kernel/security/ima/policy file did not display the entire "keyrings=" value if the list of keyrings was longer than what could fit in the fixed size tbuf buffer in ima_policy_show(). Fixes: 5c7bac9fb2c5 ("IMA: pre-allocate buffer to hold keyrings string") Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy") Signed-off-by: Tyler Hicks Reviewed-by: Lakshmi Ramasubramanian Reviewed-by: Nayna Jain Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 138 ++++++++++++++++++++++++------------ 1 file changed, 93 insertions(+), 45 deletions(-) (limited to 'security/integrity') diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 07f033634b27..ea224f00b305 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -59,6 +59,11 @@ enum policy_types { ORIGINAL_TCB = 1, DEFAULT_TCB }; enum policy_rule_list { IMA_DEFAULT_POLICY = 1, IMA_CUSTOM_POLICY }; +struct ima_rule_opt_list { + size_t count; + char *items[]; +}; + struct ima_rule_entry { struct list_head list; int action; @@ -78,7 +83,7 @@ struct ima_rule_entry { int type; /* audit type */ } lsm[MAX_LSM_RULES]; char *fsname; - char *keyrings; /* Measure keys added to these keyrings */ + struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */ struct ima_template_desc *template; }; @@ -206,10 +211,6 @@ static LIST_HEAD(ima_policy_rules); static LIST_HEAD(ima_temp_rules); static struct list_head *ima_rules = &ima_default_rules; -/* Pre-allocated buffer used for matching keyrings. */ -static char *ima_keyrings; -static size_t ima_keyrings_len; - static int ima_policy __initdata; static int __init default_measure_policy_setup(char *str) @@ -253,6 +254,72 @@ static int __init default_appraise_policy_setup(char *str) } __setup("ima_appraise_tcb", default_appraise_policy_setup); +static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src) +{ + struct ima_rule_opt_list *opt_list; + size_t count = 0; + char *src_copy; + char *cur, *next; + size_t i; + + src_copy = match_strdup(src); + if (!src_copy) + return ERR_PTR(-ENOMEM); + + next = src_copy; + while ((cur = strsep(&next, "|"))) { + /* Don't accept an empty list item */ + if (!(*cur)) { + kfree(src_copy); + return ERR_PTR(-EINVAL); + } + count++; + } + + /* Don't accept an empty list */ + if (!count) { + kfree(src_copy); + return ERR_PTR(-EINVAL); + } + + opt_list = kzalloc(struct_size(opt_list, items, count), GFP_KERNEL); + if (!opt_list) { + kfree(src_copy); + return ERR_PTR(-ENOMEM); + } + + /* + * strsep() has already replaced all instances of '|' with '\0', + * leaving a byte sequence of NUL-terminated strings. Reference each + * string with the array of items. + * + * IMPORTANT: Ownership of the allocated buffer is transferred from + * src_copy to the first element in the items array. To free the + * buffer, kfree() must only be called on the first element of the + * array. + */ + for (i = 0, cur = src_copy; i < count; i++) { + opt_list->items[i] = cur; + cur = strchr(cur, '\0') + 1; + } + opt_list->count = count; + + return opt_list; +} + +static void ima_free_rule_opt_list(struct ima_rule_opt_list *opt_list) +{ + if (!opt_list) + return; + + if (opt_list->count) { + kfree(opt_list->items[0]); + opt_list->count = 0; + } + + kfree(opt_list); +} + static void ima_lsm_free_rule(struct ima_rule_entry *entry) { int i; @@ -274,7 +341,7 @@ static void ima_free_rule(struct ima_rule_entry *entry) * the defined_templates list and cannot be freed here */ kfree(entry->fsname); - kfree(entry->keyrings); + ima_free_rule_opt_list(entry->keyrings); ima_lsm_free_rule(entry); kfree(entry); } @@ -394,8 +461,8 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event, static bool ima_match_keyring(struct ima_rule_entry *rule, const char *keyring, const struct cred *cred) { - char *next_keyring, *keyrings_ptr; bool matched = false; + size_t i; if ((rule->flags & IMA_UID) && !rule->uid_op(cred->uid, rule->uid)) return false; @@ -406,15 +473,8 @@ static bool ima_match_keyring(struct ima_rule_entry *rule, if (!keyring) return false; - strcpy(ima_keyrings, rule->keyrings); - - /* - * "keyrings=" is specified in the policy in the format below: - * keyrings=.builtin_trusted_keys|.ima|.evm - */ - keyrings_ptr = ima_keyrings; - while ((next_keyring = strsep(&keyrings_ptr, "|")) != NULL) { - if (!strcmp(next_keyring, keyring)) { + for (i = 0; i < rule->keyrings->count; i++) { + if (!strcmp(rule->keyrings->items[i], keyring)) { matched = true; break; } @@ -1065,7 +1125,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) bool uid_token; struct ima_template_desc *template_desc; int result = 0; - size_t keyrings_len; ab = integrity_audit_log_start(audit_context(), GFP_KERNEL, AUDIT_INTEGRITY_POLICY_RULE); @@ -1231,37 +1290,18 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) case Opt_keyrings: ima_log_string(ab, "keyrings", args[0].from); - keyrings_len = strlen(args[0].from) + 1; - - if ((entry->keyrings) || - (keyrings_len < 2)) { + if (entry->keyrings) { result = -EINVAL; break; } - if (keyrings_len > ima_keyrings_len) { - char *tmpbuf; - - tmpbuf = krealloc(ima_keyrings, keyrings_len, - GFP_KERNEL); - if (!tmpbuf) { - result = -ENOMEM; - break; - } - - ima_keyrings = tmpbuf; - ima_keyrings_len = keyrings_len; - } - - entry->keyrings = kstrdup(args[0].from, GFP_KERNEL); - if (!entry->keyrings) { - kfree(ima_keyrings); - ima_keyrings = NULL; - ima_keyrings_len = 0; - result = -ENOMEM; + entry->keyrings = ima_alloc_rule_opt_list(args); + if (IS_ERR(entry->keyrings)) { + result = PTR_ERR(entry->keyrings); + entry->keyrings = NULL; break; } - result = 0; + entry->flags |= IMA_KEYRINGS; break; case Opt_fsuuid: @@ -1574,6 +1614,15 @@ static void policy_func_show(struct seq_file *m, enum ima_hooks func) seq_printf(m, "func=%d ", func); } +static void ima_show_rule_opt_list(struct seq_file *m, + const struct ima_rule_opt_list *opt_list) +{ + size_t i; + + for (i = 0; i < opt_list->count; i++) + seq_printf(m, "%s%s", i ? "|" : "", opt_list->items[i]); +} + int ima_policy_show(struct seq_file *m, void *v) { struct ima_rule_entry *entry = v; @@ -1630,9 +1679,8 @@ int ima_policy_show(struct seq_file *m, void *v) } if (entry->flags & IMA_KEYRINGS) { - if (entry->keyrings != NULL) - snprintf(tbuf, sizeof(tbuf), "%s", entry->keyrings); - seq_printf(m, pt(Opt_keyrings), tbuf); + seq_puts(m, "keyrings="); + ima_show_rule_opt_list(m, entry->keyrings); seq_puts(m, " "); } -- cgit v1.2.3-70-g09d2 From 48ce1ddce16b0d1e3ff948da40a0d5125a4ee1a0 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Tue, 11 Aug 2020 14:26:21 -0500 Subject: ima: Fail rule parsing when asymmetric key measurement isn't supportable Measuring keys is currently only supported for asymmetric keys. In the future, this might change. For now, the "func=KEY_CHECK" and "keyrings=" options are only appropriate when CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS is enabled. Make this clear at policy load so that IMA policy authors don't assume that these policy language constructs are supported. Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy") Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys") Suggested-by: Nayna Jain Signed-off-by: Tyler Hicks Reviewed-by: Lakshmi Ramasubramanian Reviewed-by: Nayna Jain Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'security/integrity') diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index ea224f00b305..fe1df373c113 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -1233,7 +1233,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) entry->func = POLICY_CHECK; else if (strcmp(args[0].from, "KEXEC_CMDLINE") == 0) entry->func = KEXEC_CMDLINE; - else if (strcmp(args[0].from, "KEY_CHECK") == 0) + else if (IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) && + strcmp(args[0].from, "KEY_CHECK") == 0) entry->func = KEY_CHECK; else result = -EINVAL; @@ -1290,7 +1291,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) case Opt_keyrings: ima_log_string(ab, "keyrings", args[0].from); - if (entry->keyrings) { + if (!IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) || + entry->keyrings) { result = -EINVAL; break; } -- cgit v1.2.3-70-g09d2 From e44f128768bf28c17a5c0a35b5942bd04a8a64b0 Mon Sep 17 00:00:00 2001 From: Denis Efremov Date: Mon, 24 Aug 2020 15:54:35 +0300 Subject: integrity: Use current_uid() in integrity_audit_message() Modify integrity_audit_message() to use current_uid(). Signed-off-by: Denis Efremov Signed-off-by: Mimi Zohar --- security/integrity/integrity_audit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security/integrity') diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c index f25e7df099c8..29220056207f 100644 --- a/security/integrity/integrity_audit.c +++ b/security/integrity/integrity_audit.c @@ -47,7 +47,7 @@ void integrity_audit_message(int audit_msgno, struct inode *inode, ab = audit_log_start(audit_context(), GFP_KERNEL, audit_msgno); audit_log_format(ab, "pid=%d uid=%u auid=%u ses=%u", task_pid_nr(current), - from_kuid(&init_user_ns, current_cred()->uid), + from_kuid(&init_user_ns, current_uid()), from_kuid(&init_user_ns, audit_get_loginuid(current)), audit_get_sessionid(current)); audit_log_task_context(ab); -- cgit v1.2.3-70-g09d2 From 4afb28ab03d5d811b369af4472f3941557928569 Mon Sep 17 00:00:00 2001 From: Bruno Meneguele Date: Fri, 4 Sep 2020 16:40:57 -0300 Subject: ima: add check for enforced appraise option The "enforce" string is allowed as an option for ima_appraise= kernel paramenter per kernel-paramenters.txt and should be considered on the parameter setup checking as a matter of completeness. Also it allows futher checking on the options being passed by the user. Signed-off-by: Bruno Meneguele Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_appraise.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'security/integrity') diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 372d16382960..580b771e3458 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -31,6 +31,8 @@ static int __init default_appraise_setup(char *str) ima_appraise = IMA_APPRAISE_LOG; else if (strncmp(str, "fix", 3) == 0) ima_appraise = IMA_APPRAISE_FIX; + else if (strncmp(str, "enforce", 7) == 0) + ima_appraise = IMA_APPRAISE_ENFORCE; #endif return 1; } -- cgit v1.2.3-70-g09d2 From 7fe2bb7e7e5cf91d03ff9c35b7b997d088916cbc Mon Sep 17 00:00:00 2001 From: Bruno Meneguele Date: Fri, 4 Sep 2020 16:40:58 -0300 Subject: integrity: invalid kernel parameters feedback Don't silently ignore unknown or invalid ima_{policy,appraise,hash} and evm kernel boot command line options. Signed-off-by: Bruno Meneguele Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_main.c | 3 +++ security/integrity/ima/ima_appraise.c | 2 ++ security/integrity/ima/ima_main.c | 13 +++++++++---- security/integrity/ima/ima_policy.c | 2 ++ 4 files changed, 16 insertions(+), 4 deletions(-) (limited to 'security/integrity') diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 0d36259b690d..6ae00fee1d34 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -59,6 +59,9 @@ static int __init evm_set_fixmode(char *str) { if (strncmp(str, "fix", 3) == 0) evm_fixmode = 1; + else + pr_err("invalid \"%s\" mode", str); + return 0; } __setup("evm=", evm_set_fixmode); diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 580b771e3458..2193b51c2743 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -33,6 +33,8 @@ static int __init default_appraise_setup(char *str) ima_appraise = IMA_APPRAISE_FIX; else if (strncmp(str, "enforce", 7) == 0) ima_appraise = IMA_APPRAISE_ENFORCE; + else + pr_err("invalid \"%s\" appraise option", str); #endif return 1; } diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 8a91711ca79b..2b22932b140d 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -50,18 +50,23 @@ static int __init hash_setup(char *str) return 1; if (strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) == 0) { - if (strncmp(str, "sha1", 4) == 0) + if (strncmp(str, "sha1", 4) == 0) { ima_hash_algo = HASH_ALGO_SHA1; - else if (strncmp(str, "md5", 3) == 0) + } else if (strncmp(str, "md5", 3) == 0) { ima_hash_algo = HASH_ALGO_MD5; - else + } else { + pr_err("invalid hash algorithm \"%s\" for template \"%s\"", + str, IMA_TEMPLATE_IMA_NAME); return 1; + } goto out; } i = match_string(hash_algo_name, HASH_ALGO__LAST, str); - if (i < 0) + if (i < 0) { + pr_err("invalid hash algorithm \"%s\"", str); return 1; + } ima_hash_algo = i; out: diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index fe1df373c113..34221789c092 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -241,6 +241,8 @@ static int __init policy_setup(char *str) ima_use_secure_boot = true; else if (strcmp(p, "fail_securely") == 0) ima_fail_unverifiable_sigs = true; + else + pr_err("policy \"%s\" not found", p); } return 1; -- cgit v1.2.3-70-g09d2 From e4d7e2df3a0903601bf12eb6ef1f0c8fa1042204 Mon Sep 17 00:00:00 2001 From: Bruno Meneguele Date: Fri, 4 Sep 2020 22:20:20 -0300 Subject: ima: limit secure boot feedback scope for appraise Only emit an unknown/invalid message when setting the IMA appraise mode to anything other than "enforce", when secureboot is enabled. Signed-off-by: Bruno Meneguele [zohar@linux.ibm.com: updated commit message] Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_appraise.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) (limited to 'security/integrity') diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 2193b51c2743..4f028f6e8f8d 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -19,22 +19,29 @@ static int __init default_appraise_setup(char *str) { #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM - if (arch_ima_get_secureboot()) { - pr_info("Secure boot enabled: ignoring ima_appraise=%s boot parameter option", - str); - return 1; - } + bool sb_state = arch_ima_get_secureboot(); + int appraisal_state = ima_appraise; if (strncmp(str, "off", 3) == 0) - ima_appraise = 0; + appraisal_state = 0; else if (strncmp(str, "log", 3) == 0) - ima_appraise = IMA_APPRAISE_LOG; + appraisal_state = IMA_APPRAISE_LOG; else if (strncmp(str, "fix", 3) == 0) - ima_appraise = IMA_APPRAISE_FIX; + appraisal_state = IMA_APPRAISE_FIX; else if (strncmp(str, "enforce", 7) == 0) - ima_appraise = IMA_APPRAISE_ENFORCE; + appraisal_state = IMA_APPRAISE_ENFORCE; else pr_err("invalid \"%s\" appraise option", str); + + /* If appraisal state was changed, but secure boot is enabled, + * keep its default */ + if (sb_state) { + if (!(appraisal_state & IMA_APPRAISE_ENFORCE)) + pr_info("Secure boot enabled: ignoring ima_appraise=%s option", + str); + } else { + ima_appraise = appraisal_state; + } #endif return 1; } -- cgit v1.2.3-70-g09d2 From 8c2f516c99f0b7e59c53158f4d7a7fe229c5aea8 Mon Sep 17 00:00:00 2001 From: Bruno Meneguele Date: Fri, 4 Sep 2020 16:41:00 -0300 Subject: integrity: include keyring name for unknown key request Depending on the IMA policy rule a key may be searched for in multiple keyrings (e.g. .ima and .platform) and possibly not found. This patch improves feedback by including the keyring "description" (name) in the error message. Signed-off-by: Bruno Meneguele [zohar@linux.ibm.com: updated commit message] Signed-off-by: Mimi Zohar --- security/integrity/digsig_asymmetric.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'security/integrity') diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c index cfa4127d0518..14de98ef67f6 100644 --- a/security/integrity/digsig_asymmetric.c +++ b/security/integrity/digsig_asymmetric.c @@ -55,8 +55,14 @@ static struct key *request_asymmetric_key(struct key *keyring, uint32_t keyid) } if (IS_ERR(key)) { - pr_err_ratelimited("Request for unknown key '%s' err %ld\n", - name, PTR_ERR(key)); + if (keyring) + pr_err_ratelimited("Request for unknown key '%s' in '%s' keyring. err %ld\n", + name, keyring->description, + PTR_ERR(key)); + else + pr_err_ratelimited("Request for unknown key '%s' err %ld\n", + name, PTR_ERR(key)); + switch (PTR_ERR(key)) { /* Hide some search errors */ case -EACCES: -- cgit v1.2.3-70-g09d2 From f60c826d031817772b634c0cda37794ce6d7946a Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Wed, 9 Sep 2020 20:09:06 +0100 Subject: ima: Use kmemdup rather than kmalloc+memcpy Issue identified with Coccinelle. Signed-off-by: Alex Dewar Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'security/integrity') diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 34221789c092..5b4bf1790e47 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -353,15 +353,14 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) struct ima_rule_entry *nentry; int i; - nentry = kmalloc(sizeof(*nentry), GFP_KERNEL); - if (!nentry) - return NULL; - /* * Immutable elements are copied over as pointers and data; only * lsm rules can change */ - memcpy(nentry, entry, sizeof(*nentry)); + nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL); + if (!nentry) + return NULL; + memset(nentry->lsm, 0, sizeof_field(struct ima_rule_entry, lsm)); for (i = 0; i < MAX_LSM_RULES; i++) { -- cgit v1.2.3-70-g09d2 From 60386b854008adc951c470067f90a2d85b5d520f Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 4 Sep 2020 11:23:28 +0200 Subject: ima: Don't ignore errors from crypto_shash_update() Errors returned by crypto_shash_update() are not checked in ima_calc_boot_aggregate_tfm() and thus can be overwritten at the next iteration of the loop. This patch adds a check after calling crypto_shash_update() and returns immediately if the result is not zero. Cc: stable@vger.kernel.org Fixes: 3323eec921efd ("integrity: IMA as an integrity service provider") Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_crypto.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'security/integrity') diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index 011c3c76af86..21989fa0c107 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -829,6 +829,8 @@ static int ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id, /* now accumulate with current aggregate */ rc = crypto_shash_update(shash, d.digest, crypto_shash_digestsize(tfm)); + if (rc != 0) + return rc; } /* * Extend cumulative digest over TPM registers 8-9, which contain -- cgit v1.2.3-70-g09d2 From 4be92db3b5667f3a5c874a04635b037dc5e3f373 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 4 Sep 2020 11:23:29 +0200 Subject: ima: Remove semicolon at the end of ima_get_binary_runtime_size() This patch removes the unnecessary semicolon at the end of ima_get_binary_runtime_size(). Cc: stable@vger.kernel.org Fixes: d158847ae89a2 ("ima: maintain memory size needed for serializing the measurement list") Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_queue.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security/integrity') diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index fb4ec270f620..c096ef8945c7 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -133,7 +133,7 @@ unsigned long ima_get_binary_runtime_size(void) return ULONG_MAX; else return binary_runtime_size + sizeof(struct ima_kexec_hdr); -}; +} static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr) { -- cgit v1.2.3-70-g09d2 From 455b6c9112eff8d249e32ba165742085678a80a4 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 4 Sep 2020 11:23:30 +0200 Subject: evm: Check size of security.evm before using it This patch checks the size for the EVM_IMA_XATTR_DIGSIG and EVM_XATTR_PORTABLE_DIGSIG types to ensure that the algorithm is read from the buffer returned by vfs_getxattr_alloc(). Cc: stable@vger.kernel.org # 4.19.x Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures") Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/evm/evm_main.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'security/integrity') diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 6ae00fee1d34..76d19146d74b 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -184,6 +184,12 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, break; case EVM_IMA_XATTR_DIGSIG: case EVM_XATTR_PORTABLE_DIGSIG: + /* accept xattr with non-empty signature field */ + if (xattr_len <= sizeof(struct signature_v2_hdr)) { + evm_status = INTEGRITY_FAIL; + goto out; + } + hdr = (struct signature_v2_hdr *)xattr_data; digest.hdr.algo = hdr->hash_algo; rc = evm_calc_hash(dentry, xattr_name, xattr_value, -- cgit v1.2.3-70-g09d2 From aa662fc04f5b290b3979332588bf8d812b189962 Mon Sep 17 00:00:00 2001 From: KP Singh Date: Wed, 16 Sep 2020 18:02:42 +0000 Subject: ima: Fix NULL pointer dereference in ima_file_hash ima_file_hash can be called when there is no iint->ima_hash available even though the inode exists in the integrity cache. It is fairly common for a file to not have a hash. (e.g. an mknodat, prior to the file being closed). Another example where this can happen (suggested by Jann Horn): Process A does: while(1) { unlink("/tmp/imafoo"); fd = open("/tmp/imafoo", O_RDWR|O_CREAT|O_TRUNC, 0700); if (fd == -1) { perror("open"); continue; } write(fd, "A", 1); close(fd); } and Process B does: while (1) { int fd = open("/tmp/imafoo", O_RDONLY); if (fd == -1) continue; char *mapping = mmap(NULL, 0x1000, PROT_READ|PROT_EXEC, MAP_PRIVATE, fd, 0); if (mapping != MAP_FAILED) munmap(mapping, 0x1000); close(fd); } Due to the race to get the iint->mutex between ima_file_hash and process_measurement iint->ima_hash could still be NULL. Fixes: 6beea7afcc72 ("ima: add the ability to query the cached hash of a given file") Signed-off-by: KP Singh Reviewed-by: Florent Revest Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_main.c | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'security/integrity') diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 2b22932b140d..a5a2ae36a36d 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -536,6 +536,16 @@ int ima_file_hash(struct file *file, char *buf, size_t buf_size) return -EOPNOTSUPP; mutex_lock(&iint->mutex); + + /* + * ima_file_hash can be called when ima_collect_measurement has still + * not been called, we might not always have a hash. + */ + if (!iint->ima_hash) { + mutex_unlock(&iint->mutex); + return -EOPNOTSUPP; + } + if (buf) { size_t copied_size; -- cgit v1.2.3-70-g09d2