From 2f845882ecd2f6808f87f387dfa9b03d71a68e01 Mon Sep 17 00:00:00 2001 From: Lakshmi Ramasubramanian Date: Thu, 18 Jun 2020 14:10:11 -0700 Subject: [PATCH 01/18] integrity: Add errno field in audit message Error code is not included in the audit messages logged by the integrity subsystem. Define a new function integrity_audit_message() that takes error code in the "errno" parameter. Add "errno" field in the audit messages logged by the integrity subsystem and set the value passed in the "errno" parameter. [ 6.303048] audit: type=1804 audit(1592506281.627:2): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=kernel op=measuring_key cause=ENOMEM comm="swapper/0" name=".builtin_trusted_keys" res=0 errno=-12 [ 7.987647] audit: type=1802 audit(1592506283.312:9): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0 op=policy_update cause=completed comm="systemd" res=1 errno=0 [ 8.019432] audit: type=1804 audit(1592506283.344:10): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0 op=measuring_kexec_cmdline cause=hashing_error comm="systemd" name="kexec-cmdline" res=0 errno=-22 Signed-off-by: Lakshmi Ramasubramanian Suggested-by: Steve Grubb Suggested-by: Mimi Zohar Signed-off-by: Mimi Zohar --- security/integrity/integrity.h | 13 +++++++++++++ security/integrity/integrity_audit.c | 11 ++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 16c1894c29bb..413c803c5208 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -239,6 +239,11 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode, const unsigned char *fname, const char *op, const char *cause, int result, int info); +void integrity_audit_message(int audit_msgno, struct inode *inode, + const unsigned char *fname, const char *op, + const char *cause, int result, int info, + int errno); + static inline struct audit_buffer * integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type) { @@ -253,6 +258,14 @@ static inline void integrity_audit_msg(int audit_msgno, struct inode *inode, { } +static inline void integrity_audit_message(int audit_msgno, + struct inode *inode, + const unsigned char *fname, + const char *op, const char *cause, + int result, int info, int errno) +{ +} + static inline struct audit_buffer * integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type) { diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c index 5109173839cc..f25e7df099c8 100644 --- a/security/integrity/integrity_audit.c +++ b/security/integrity/integrity_audit.c @@ -28,6 +28,15 @@ __setup("integrity_audit=", integrity_audit_setup); void integrity_audit_msg(int audit_msgno, struct inode *inode, const unsigned char *fname, const char *op, const char *cause, int result, int audit_info) +{ + integrity_audit_message(audit_msgno, inode, fname, op, cause, + result, audit_info, 0); +} + +void integrity_audit_message(int audit_msgno, struct inode *inode, + const unsigned char *fname, const char *op, + const char *cause, int result, int audit_info, + int errno) { struct audit_buffer *ab; char name[TASK_COMM_LEN]; @@ -53,6 +62,6 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode, audit_log_untrustedstring(ab, inode->i_sb->s_id); audit_log_format(ab, " ino=%lu", inode->i_ino); } - audit_log_format(ab, " res=%d", !result); + audit_log_format(ab, " res=%d errno=%d", !result, errno); audit_log_end(ab); } From 34e980bb83a07b533d175221436d698c1312c4a3 Mon Sep 17 00:00:00 2001 From: Lakshmi Ramasubramanian Date: Thu, 18 Jun 2020 14:10:12 -0700 Subject: [PATCH 02/18] IMA: Add audit log for failure conditions process_buffer_measurement() and ima_alloc_key_entry() functions need to log an audit message for auditing integrity measurement failures. Add audit message in these two functions. Remove "pr_devel" log message in process_buffer_measurement(). Sample audit messages: [ 6.303048] audit: type=1804 audit(1592506281.627:2): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=kernel op=measuring_key cause=ENOMEM comm="swapper/0" name=".builtin_trusted_keys" res=0 errno=-12 [ 8.019432] audit: type=1804 audit(1592506283.344:10): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0 op=measuring_kexec_cmdline cause=hashing_error comm="systemd" name="kexec-cmdline" res=0 errno=-22 Signed-off-by: Lakshmi Ramasubramanian Suggested-by: Mimi Zohar Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 48 ++++++++++++++++--------- security/integrity/ima/ima_main.c | 18 +++++++--- security/integrity/ima/ima_policy.c | 2 +- security/integrity/ima/ima_queue_keys.c | 5 +++ 4 files changed, 51 insertions(+), 22 deletions(-) diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 9d94080bdad8..4515975cc540 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -186,27 +186,43 @@ static inline unsigned int ima_hash_key(u8 *digest) return (digest[0] | digest[1] << 8) % IMA_MEASURE_HTABLE_SIZE; } -#define __ima_hooks(hook) \ - hook(NONE) \ - hook(FILE_CHECK) \ - hook(MMAP_CHECK) \ - hook(BPRM_CHECK) \ - hook(CREDS_CHECK) \ - hook(POST_SETATTR) \ - hook(MODULE_CHECK) \ - hook(FIRMWARE_CHECK) \ - hook(KEXEC_KERNEL_CHECK) \ - hook(KEXEC_INITRAMFS_CHECK) \ - hook(POLICY_CHECK) \ - hook(KEXEC_CMDLINE) \ - hook(KEY_CHECK) \ - hook(MAX_CHECK) -#define __ima_hook_enumify(ENUM) ENUM, +#define __ima_hooks(hook) \ + hook(NONE, none) \ + hook(FILE_CHECK, file) \ + hook(MMAP_CHECK, mmap) \ + hook(BPRM_CHECK, bprm) \ + hook(CREDS_CHECK, creds) \ + hook(POST_SETATTR, post_setattr) \ + hook(MODULE_CHECK, module) \ + hook(FIRMWARE_CHECK, firmware) \ + hook(KEXEC_KERNEL_CHECK, kexec_kernel) \ + hook(KEXEC_INITRAMFS_CHECK, kexec_initramfs) \ + hook(POLICY_CHECK, policy) \ + hook(KEXEC_CMDLINE, kexec_cmdline) \ + hook(KEY_CHECK, key) \ + hook(MAX_CHECK, none) + +#define __ima_hook_enumify(ENUM, str) ENUM, +#define __ima_stringify(arg) (#arg) +#define __ima_hook_measuring_stringify(ENUM, str) \ + (__ima_stringify(measuring_ ##str)), enum ima_hooks { __ima_hooks(__ima_hook_enumify) }; +static const char * const ima_hooks_measure_str[] = { + __ima_hooks(__ima_hook_measuring_stringify) +}; + +static inline const char *func_measure_str(enum ima_hooks func) +{ + if (func >= MAX_CHECK) + return ima_hooks_measure_str[NONE]; + + return ima_hooks_measure_str[func]; +} + extern const char *const func_tokens[]; struct modsig; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index c1583d98c5e5..8351b2fd48e0 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -740,6 +740,7 @@ void process_buffer_measurement(const void *buf, int size, int pcr, const char *keyring) { int ret = 0; + const char *audit_cause = "ENOMEM"; struct ima_template_entry *entry = NULL; struct integrity_iint_cache iint = {}; struct ima_event_data event_data = {.iint = &iint, @@ -794,21 +795,28 @@ void process_buffer_measurement(const void *buf, int size, iint.ima_hash->length = hash_digest_size[ima_hash_algo]; ret = ima_calc_buffer_hash(buf, size, iint.ima_hash); - if (ret < 0) + if (ret < 0) { + audit_cause = "hashing_error"; goto out; + } ret = ima_alloc_init_template(&event_data, &entry, template); - if (ret < 0) + if (ret < 0) { + audit_cause = "alloc_entry"; goto out; + } ret = ima_store_template(entry, violation, NULL, buf, pcr); - - if (ret < 0) + if (ret < 0) { + audit_cause = "store_entry"; ima_free_template_entry(entry); + } out: if (ret < 0) - pr_devel("%s: failed, result: %d\n", __func__, ret); + integrity_audit_message(AUDIT_INTEGRITY_PCR, NULL, eventname, + func_measure_str(func), + audit_cause, ret, 0, ret); return; } diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index e493063a3c34..66aa3e17a888 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -1414,7 +1414,7 @@ void ima_delete_rules(void) } } -#define __ima_hook_stringify(str) (#str), +#define __ima_hook_stringify(func, str) (#func), const char *const func_tokens[] = { __ima_hooks(__ima_hook_stringify) diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c index cb3e3f501593..56ce24a18b66 100644 --- a/security/integrity/ima/ima_queue_keys.c +++ b/security/integrity/ima/ima_queue_keys.c @@ -68,6 +68,7 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring, size_t payload_len) { int rc = 0; + const char *audit_cause = "ENOMEM"; struct ima_key_entry *entry; entry = kzalloc(sizeof(*entry), GFP_KERNEL); @@ -88,6 +89,10 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring, out: if (rc) { + integrity_audit_message(AUDIT_INTEGRITY_PCR, NULL, + keyring->description, + func_measure_str(KEY_CHECK), + audit_cause, rc, 0, rc); ima_free_key_entry(entry); entry = NULL; } From 9ff8a616dfab96a4fa0ddd36190907dc68886d9b Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 9 Jul 2020 01:19:00 -0500 Subject: [PATCH 03/18] ima: Have the LSM free its audit rule Ask the LSM to free its audit rule rather than directly calling kfree(). Both AppArmor and SELinux do additional work in their audit_rule_free() hooks. Fix memory leaks by allowing the LSMs to perform necessary work. Fixes: b16942455193 ("ima: use the lsm policy update notifier") Signed-off-by: Tyler Hicks Cc: Janne Karhunen Cc: Casey Schaufler Reviewed-by: Mimi Zohar Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 5 +++++ security/integrity/ima/ima_policy.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 4515975cc540..59ec28f5c117 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -420,6 +420,7 @@ static inline void ima_free_modsig(struct modsig *modsig) #ifdef CONFIG_IMA_LSM_RULES #define security_filter_rule_init security_audit_rule_init +#define security_filter_rule_free security_audit_rule_free #define security_filter_rule_match security_audit_rule_match #else @@ -430,6 +431,10 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr, return -EINVAL; } +static inline void security_filter_rule_free(void *lsmrule) +{ +} + static inline int security_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule) { diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 66aa3e17a888..d7c268c2b0ce 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -258,7 +258,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry) int i; for (i = 0; i < MAX_LSM_RULES; i++) { - kfree(entry->lsm[i].rule); + security_filter_rule_free(entry->lsm[i].rule); kfree(entry->lsm[i].args_p); } kfree(entry); From 465aee77aae857b5fcde56ee192b33dc369fba04 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 9 Jul 2020 01:19:01 -0500 Subject: [PATCH 04/18] ima: Free the entire rule when deleting a list of rules Create a function, ima_free_rule(), to free all memory associated with an ima_rule_entry. Use the new function to fix memory leaks of allocated ima_rule_entry members, such as .fsname and .keyrings, when deleting a list of rules. Make the existing ima_lsm_free_rule() function specific to the LSM audit rule array of an ima_rule_entry and require that callers make an additional call to kfree to free the ima_rule_entry itself. This fixes a memory leak seen when loading by a valid rule that contains an additional piece of allocated memory, such as an fsname, followed by an invalid rule that triggers a policy load failure: # echo -e "dont_measure fsname=securityfs\nbad syntax" > \ /sys/kernel/security/ima/policy -bash: echo: write error: Invalid argument # echo scan > /sys/kernel/debug/kmemleak # cat /sys/kernel/debug/kmemleak unreferenced object 0xffff9bab67ca12c0 (size 16): comm "bash", pid 684, jiffies 4295212803 (age 252.344s) hex dump (first 16 bytes): 73 65 63 75 72 69 74 79 66 73 00 6b 6b 6b 6b a5 securityfs.kkkk. backtrace: [<00000000adc80b1b>] kstrdup+0x2e/0x60 [<00000000d504cb0d>] ima_parse_add_rule+0x7d4/0x1020 [<00000000444825ac>] ima_write_policy+0xab/0x1d0 [<000000002b7f0d6c>] vfs_write+0xde/0x1d0 [<0000000096feedcf>] ksys_write+0x68/0xe0 [<0000000052b544a2>] do_syscall_64+0x56/0xa0 [<000000007ead1ba7>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name") Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy") Signed-off-by: Tyler Hicks Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index d7c268c2b0ce..bf00b966e87f 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -261,6 +261,21 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry) security_filter_rule_free(entry->lsm[i].rule); kfree(entry->lsm[i].args_p); } +} + +static void ima_free_rule(struct ima_rule_entry *entry) +{ + if (!entry) + return; + + /* + * entry->template->fields may be allocated in ima_parse_rule() but that + * reference is owned by the corresponding ima_template_desc element in + * the defined_templates list and cannot be freed here + */ + kfree(entry->fsname); + kfree(entry->keyrings); + ima_lsm_free_rule(entry); kfree(entry); } @@ -302,6 +317,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) out_err: ima_lsm_free_rule(nentry); + kfree(nentry); return NULL; } @@ -315,7 +331,14 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry) list_replace_rcu(&entry->list, &nentry->list); synchronize_rcu(); + /* + * ima_lsm_copy_rule() shallow copied all references, except for the + * LSM references, from entry to nentry so we only want to free the LSM + * references and the entry itself. All other memory refrences will now + * be owned by nentry. + */ ima_lsm_free_rule(entry); + kfree(entry); return 0; } @@ -1402,15 +1425,11 @@ ssize_t ima_parse_add_rule(char *rule) void ima_delete_rules(void) { struct ima_rule_entry *entry, *tmp; - int i; temp_ima_appraise = 0; list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) { - for (i = 0; i < MAX_LSM_RULES; i++) - kfree(entry->lsm[i].args_p); - list_del(&entry->list); - kfree(entry); + ima_free_rule(entry); } } From 2bdd737c5687d6dec30e205953146ede8a87dbdd Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 9 Jul 2020 01:19:02 -0500 Subject: [PATCH 05/18] ima: Free the entire rule if it fails to parse Use ima_free_rule() to fix memory leaks of allocated ima_rule_entry members, such as .fsname and .keyrings, when an error is encountered during rule parsing. Set the args_p pointer to NULL after freeing it in the error path of ima_lsm_rule_init() so that it isn't freed twice. This fixes a memory leak seen when loading an rule that contains an additional piece of allocated memory, such as an fsname, followed by an invalid conditional: # echo "measure fsname=tmpfs bad=cond" > /sys/kernel/security/ima/policy -bash: echo: write error: Invalid argument # echo scan > /sys/kernel/debug/kmemleak # cat /sys/kernel/debug/kmemleak unreferenced object 0xffff98e7e4ece6c0 (size 8): comm "bash", pid 672, jiffies 4294791843 (age 21.855s) hex dump (first 8 bytes): 74 6d 70 66 73 00 6b a5 tmpfs.k. backtrace: [<00000000abab7413>] kstrdup+0x2e/0x60 [<00000000f11ede32>] ima_parse_add_rule+0x7d4/0x1020 [<00000000f883dd7a>] ima_write_policy+0xab/0x1d0 [<00000000b17cf753>] vfs_write+0xde/0x1d0 [<00000000b8ddfdea>] ksys_write+0x68/0xe0 [<00000000b8e21e87>] do_syscall_64+0x56/0xa0 [<0000000089ea7b98>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name") Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy") Signed-off-by: Tyler Hicks Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index bf00b966e87f..e458cd47c099 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -913,6 +913,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry, if (ima_rules == &ima_default_rules) { kfree(entry->lsm[lsm_rule].args_p); + entry->lsm[lsm_rule].args_p = NULL; result = -EINVAL; } else result = 0; @@ -1404,7 +1405,7 @@ ssize_t ima_parse_add_rule(char *rule) result = ima_parse_rule(p, entry); if (result) { - kfree(entry); + ima_free_rule(entry); integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL, op, "invalid-policy", result, audit_info); From 712183437ebebc89cd086ef96cf9a521fd97fd09 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 9 Jul 2020 01:19:03 -0500 Subject: [PATCH 06/18] ima: Fail rule parsing when buffer hook functions have an invalid action Buffer based hook functions, such as KEXEC_CMDLINE and KEY_CHECK, can only measure. The process_buffer_measurement() function quietly ignores all actions except measure so make this behavior clear at the time of policy load. The parsing of the keyrings conditional had a check to ensure that it was only specified with measure actions but the check should be on the hook function and not the keyrings conditional since "appraise func=KEY_CHECK" is not a valid rule. Fixes: b0935123a183 ("IMA: Define a new hook to measure the kexec boot command line arguments") Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys") Signed-off-by: Tyler Hicks Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 40 +++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index e458cd47c099..40c28f1a6a5a 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -973,6 +973,43 @@ static void check_template_modsig(const struct ima_template_desc *template) #undef MSG } +static bool ima_validate_rule(struct ima_rule_entry *entry) +{ + /* Ensure that the action is set */ + if (entry->action == UNKNOWN) + return false; + + /* + * Ensure that the hook function is compatible with the other + * components of the rule + */ + switch (entry->func) { + case NONE: + case FILE_CHECK: + case MMAP_CHECK: + case BPRM_CHECK: + case CREDS_CHECK: + case POST_SETATTR: + case MODULE_CHECK: + case FIRMWARE_CHECK: + case KEXEC_KERNEL_CHECK: + case KEXEC_INITRAMFS_CHECK: + case POLICY_CHECK: + /* Validation of these hook functions is in ima_parse_rule() */ + break; + case KEXEC_CMDLINE: + case KEY_CHECK: + if (entry->action & ~(MEASURE | DONT_MEASURE)) + return false; + + break; + default: + return false; + } + + return true; +} + static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) { struct audit_buffer *ab; @@ -1150,7 +1187,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) keyrings_len = strlen(args[0].from) + 1; if ((entry->keyrings) || - (entry->action != MEASURE) || (entry->func != KEY_CHECK) || (keyrings_len < 2)) { result = -EINVAL; @@ -1356,7 +1392,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) break; } } - if (!result && (entry->action == UNKNOWN)) + if (!result && !ima_validate_rule(entry)) result = -EINVAL; else if (entry->action == APPRAISE) temp_ima_appraise |= ima_appraise_flag(entry->func); From db2045f5892a9db7354442bf77f9b03b50ff9ee1 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 9 Jul 2020 01:19:04 -0500 Subject: [PATCH 07/18] ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an invalid cond The KEXEC_CMDLINE hook function only supports the pcr conditional. Make this clear at policy load so that IMA policy authors don't assume that other conditionals are supported. Since KEXEC_CMDLINE's inception, ima_match_rules() has always returned true on any loaded KEXEC_CMDLINE rule without any consideration for other conditionals present in the rule. Make it clear that pcr is the only supported KEXEC_CMDLINE conditional by returning an error during policy load. An example of why this is a problem can be explained with the following rule: dont_measure func=KEXEC_CMDLINE obj_type=foo_t An IMA policy author would have assumed that rule is valid because the parser accepted it but the result was that measurements for all KEXEC_CMDLINE operations would be disabled. Fixes: b0935123a183 ("IMA: Define a new hook to measure the kexec boot command line arguments") Signed-off-by: Tyler Hicks Reviewed-by: Mimi Zohar Reviewed-by: Lakshmi Ramasubramanian Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 40c28f1a6a5a..1c64bd6f1728 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -343,6 +343,17 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry) return 0; } +static bool ima_rule_contains_lsm_cond(struct ima_rule_entry *entry) +{ + int i; + + for (i = 0; i < MAX_LSM_RULES; i++) + if (entry->lsm[i].args_p) + return true; + + return false; +} + /* * The LSM policy can be reloaded, leaving the IMA LSM based rules referring * to the old, stale LSM policy. Update the IMA LSM based rules to reflect @@ -998,6 +1009,16 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) /* Validation of these hook functions is in ima_parse_rule() */ break; case KEXEC_CMDLINE: + if (entry->action & ~(MEASURE | DONT_MEASURE)) + return false; + + if (entry->flags & ~(IMA_FUNC | IMA_PCR)) + return false; + + if (ima_rule_contains_lsm_cond(entry)) + return false; + + break; case KEY_CHECK: if (entry->action & ~(MEASURE | DONT_MEASURE)) return false; From eb624fe214a2e156ddafd9868377cf91499f789d Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 9 Jul 2020 01:19:05 -0500 Subject: [PATCH 08/18] ima: Fail rule parsing when the KEY_CHECK hook is combined with an invalid cond The KEY_CHECK function only supports the uid, pcr, and keyrings conditionals. Make this clear at policy load so that IMA policy authors don't assume that other conditionals are supported. Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys") Signed-off-by: Tyler Hicks Reviewed-by: Lakshmi Ramasubramanian Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 1c64bd6f1728..81da02071d41 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -1023,6 +1023,13 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) if (entry->action & ~(MEASURE | DONT_MEASURE)) return false; + if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR | + IMA_KEYRINGS)) + return false; + + if (ima_rule_contains_lsm_cond(entry)) + return false; + break; default: return false; From 5f3e92657bbfb63ad3109433d843c89996114b03 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 9 Jul 2020 01:19:06 -0500 Subject: [PATCH 09/18] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable Verifying that a file hash is not blacklisted is currently only supported for files with appended signatures (modsig). In the future, this might change. For now, the "appraise_flag" option is only appropriate for appraise actions and its "blacklist" value is only appropriate when CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is only appropriate when "appraise_type=imasig|modsig" is also present. Make this clear at policy load so that IMA policy authors don't assume that other uses of "appraise_flag=blacklist" are supported. Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig") Signed-off-by: Tyler Hicks Reivewed-by: Nayna Jain Tested-by: Nayna Jain Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 81da02071d41..cf3ddb38dfa8 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) return false; } + /* Ensure that combinations of flags are compatible with each other */ + if (entry->flags & IMA_CHECK_BLACKLIST && + !(entry->flags & IMA_MODSIG_ALLOWED)) + return false; + return true; } @@ -1371,9 +1376,17 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) result = -EINVAL; break; case Opt_appraise_flag: + if (entry->action != APPRAISE) { + result = -EINVAL; + break; + } + ima_log_string(ab, "appraise_flag", args[0].from); - if (strstr(args[0].from, "blacklist")) + if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) && + strstr(args[0].from, "blacklist")) entry->flags |= IMA_CHECK_BLACKLIST; + else + result = -EINVAL; break; case Opt_permit_directio: entry->flags |= IMA_PERMIT_DIRECTIO; From 39e5993d0d452b9ef612f2fcf7ca77ff319438f4 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 9 Jul 2020 01:19:07 -0500 Subject: [PATCH 10/18] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements The args_p member is a simple string that is allocated by ima_rule_init(). Shallow copy it like other non-LSM references in ima_rule_entry structs. There are no longer any necessary error path cleanups to do in ima_lsm_copy_rule(). Signed-off-by: Tyler Hicks Signed-off-by: Tyler Hicks Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index cf3ddb38dfa8..86ccd0076e71 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -300,10 +300,13 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) continue; nentry->lsm[i].type = entry->lsm[i].type; - nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p, - GFP_KERNEL); - if (!nentry->lsm[i].args_p) - goto out_err; + 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; security_filter_rule_init(nentry->lsm[i].type, Audit_equal, @@ -311,14 +314,9 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) &nentry->lsm[i].rule); if (!nentry->lsm[i].rule) pr_warn("rule for LSM \'%s\' is undefined\n", - (char *)entry->lsm[i].args_p); + (char *)nentry->lsm[i].args_p); } return nentry; - -out_err: - ima_lsm_free_rule(nentry); - kfree(nentry); - return NULL; } static int ima_lsm_update_rule(struct ima_rule_entry *entry) From aa0c0227d331719052cf14a3c10e99a12818d81b Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 9 Jul 2020 01:19:08 -0500 Subject: [PATCH 11/18] ima: Use correct type for the args_p member of ima_rule_entry.lsm elements Make args_p be of the char pointer type rather than have it be a void pointer that gets casted to char pointer when it is used. It is a simple NUL-terminated string as returned by match_strdup(). Signed-off-by: Tyler Hicks Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 86ccd0076e71..37438bffc62b 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -74,7 +74,7 @@ struct ima_rule_entry { int pcr; struct { void *rule; /* LSM file metadata specific */ - void *args_p; /* audit value */ + char *args_p; /* audit value */ int type; /* audit type */ } lsm[MAX_LSM_RULES]; char *fsname; @@ -314,7 +314,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) &nentry->lsm[i].rule); if (!nentry->lsm[i].rule) pr_warn("rule for LSM \'%s\' is undefined\n", - (char *)nentry->lsm[i].args_p); + nentry->lsm[i].args_p); } return nentry; } @@ -918,7 +918,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry, &entry->lsm[lsm_rule].rule); if (!entry->lsm[lsm_rule].rule) { pr_warn("rule for LSM \'%s\' is undefined\n", - (char *)entry->lsm[lsm_rule].args_p); + entry->lsm[lsm_rule].args_p); if (ima_rules == &ima_default_rules) { kfree(entry->lsm[lsm_rule].args_p); @@ -1684,27 +1684,27 @@ int ima_policy_show(struct seq_file *m, void *v) switch (i) { case LSM_OBJ_USER: seq_printf(m, pt(Opt_obj_user), - (char *)entry->lsm[i].args_p); + entry->lsm[i].args_p); break; case LSM_OBJ_ROLE: seq_printf(m, pt(Opt_obj_role), - (char *)entry->lsm[i].args_p); + entry->lsm[i].args_p); break; case LSM_OBJ_TYPE: seq_printf(m, pt(Opt_obj_type), - (char *)entry->lsm[i].args_p); + entry->lsm[i].args_p); break; case LSM_SUBJ_USER: seq_printf(m, pt(Opt_subj_user), - (char *)entry->lsm[i].args_p); + entry->lsm[i].args_p); break; case LSM_SUBJ_ROLE: seq_printf(m, pt(Opt_subj_role), - (char *)entry->lsm[i].args_p); + entry->lsm[i].args_p); break; case LSM_SUBJ_TYPE: seq_printf(m, pt(Opt_subj_type), - (char *)entry->lsm[i].args_p); + entry->lsm[i].args_p); break; } seq_puts(m, " "); From 30031b0ec8aef903ebede41f43a8d021f0030499 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 9 Jul 2020 01:19:09 -0500 Subject: [PATCH 12/18] ima: Move comprehensive rule validation checks out of the token parser Use ima_validate_rule(), at the end of the token parsing stage, to verify combinations of actions, hooks, and flags. This is useful to increase readability by consolidating such checks into a single function and also because rule conditionals can be specified in arbitrary order making it difficult to do comprehensive rule validation until the entire rule has been parsed. This allows for the check that ties together the "keyrings" conditional with the KEY_CHECK function hook to be moved into the final rule validation. The modsig check no longer needs to compiled conditionally because the token parser will ensure that modsig support is enabled before accepting "imasig|modsig" appraise type values. The final rule validation will ensure that appraise_type and appraise_flag options are only present in appraise rules. Finally, this allows for the check that ties together the "pcr" conditional with the measure action to be moved into the final rule validation. Signed-off-by: Tyler Hicks Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 6 --- security/integrity/ima/ima_modsig.c | 20 ---------- security/integrity/ima/ima_policy.c | 57 +++++++++++++++++++---------- 3 files changed, 37 insertions(+), 46 deletions(-) diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 59ec28f5c117..ea7e77536f3c 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -372,7 +372,6 @@ static inline int ima_read_xattr(struct dentry *dentry, #endif /* CONFIG_IMA_APPRAISE */ #ifdef CONFIG_IMA_APPRAISE_MODSIG -bool ima_hook_supports_modsig(enum ima_hooks func); int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len, struct modsig **modsig); void ima_collect_modsig(struct modsig *modsig, const void *buf, loff_t size); @@ -382,11 +381,6 @@ int ima_get_raw_modsig(const struct modsig *modsig, const void **data, u32 *data_len); void ima_free_modsig(struct modsig *modsig); #else -static inline bool ima_hook_supports_modsig(enum ima_hooks func) -{ - return false; -} - static inline int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len, struct modsig **modsig) { diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c index d106885cc495..fb25723c65bc 100644 --- a/security/integrity/ima/ima_modsig.c +++ b/security/integrity/ima/ima_modsig.c @@ -32,26 +32,6 @@ struct modsig { u8 raw_pkcs7[]; }; -/** - * ima_hook_supports_modsig - can the policy allow modsig for this hook? - * - * modsig is only supported by hooks using ima_post_read_file(), because only - * they preload the contents of the file in a buffer. FILE_CHECK does that in - * some cases, but not when reached from vfs_open(). POLICY_CHECK can support - * it, but it's not useful in practice because it's a text file so deny. - */ -bool ima_hook_supports_modsig(enum ima_hooks func) -{ - switch (func) { - case KEXEC_KERNEL_CHECK: - case KEXEC_INITRAMFS_CHECK: - case MODULE_CHECK: - return true; - default: - return false; - } -} - /* * ima_read_modsig - Read modsig from buf. * diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 37438bffc62b..c679144af042 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -984,10 +984,27 @@ static void check_template_modsig(const struct ima_template_desc *template) static bool ima_validate_rule(struct ima_rule_entry *entry) { - /* Ensure that the action is set */ + /* Ensure that the action is set and is compatible with the flags */ if (entry->action == UNKNOWN) return false; + if (entry->action != MEASURE && entry->flags & IMA_PCR) + return false; + + if (entry->action != APPRAISE && + entry->flags & (IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED | IMA_CHECK_BLACKLIST)) + return false; + + /* + * The IMA_FUNC bit must be set if and only if there's a valid hook + * function specified, and vice versa. Enforcing this property allows + * for the NONE case below to validate a rule without an explicit hook + * function. + */ + if (((entry->flags & IMA_FUNC) && entry->func == NONE) || + (!(entry->flags & IMA_FUNC) && entry->func != NONE)) + return false; + /* * Ensure that the hook function is compatible with the other * components of the rule @@ -999,12 +1016,27 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) case BPRM_CHECK: case CREDS_CHECK: case POST_SETATTR: - case MODULE_CHECK: case FIRMWARE_CHECK: + case POLICY_CHECK: + if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC | + IMA_UID | IMA_FOWNER | IMA_FSUUID | + IMA_INMASK | IMA_EUID | IMA_PCR | + IMA_FSNAME | IMA_DIGSIG_REQUIRED | + IMA_PERMIT_DIRECTIO)) + return false; + + break; + case MODULE_CHECK: case KEXEC_KERNEL_CHECK: case KEXEC_INITRAMFS_CHECK: - case POLICY_CHECK: - /* Validation of these hook functions is in ima_parse_rule() */ + if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC | + IMA_UID | IMA_FOWNER | IMA_FSUUID | + IMA_INMASK | IMA_EUID | IMA_PCR | + IMA_FSNAME | IMA_DIGSIG_REQUIRED | + IMA_PERMIT_DIRECTIO | IMA_MODSIG_ALLOWED | + IMA_CHECK_BLACKLIST)) + return false; + break; case KEXEC_CMDLINE: if (entry->action & ~(MEASURE | DONT_MEASURE)) @@ -1218,7 +1250,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) keyrings_len = strlen(args[0].from) + 1; if ((entry->keyrings) || - (entry->func != KEY_CHECK) || (keyrings_len < 2)) { result = -EINVAL; break; @@ -1358,15 +1389,10 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) AUDIT_SUBJ_TYPE); break; case Opt_appraise_type: - if (entry->action != APPRAISE) { - result = -EINVAL; - break; - } - ima_log_string(ab, "appraise_type", args[0].from); if ((strcmp(args[0].from, "imasig")) == 0) entry->flags |= IMA_DIGSIG_REQUIRED; - else if (ima_hook_supports_modsig(entry->func) && + else if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) && strcmp(args[0].from, "imasig|modsig") == 0) entry->flags |= IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED; @@ -1374,11 +1400,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) result = -EINVAL; break; case Opt_appraise_flag: - if (entry->action != APPRAISE) { - result = -EINVAL; - break; - } - ima_log_string(ab, "appraise_flag", args[0].from); if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) && strstr(args[0].from, "blacklist")) @@ -1390,10 +1411,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) entry->flags |= IMA_PERMIT_DIRECTIO; break; case Opt_pcr: - if (entry->action != MEASURE) { - result = -EINVAL; - break; - } ima_log_string(ab, "pcr", args[0].from); result = kstrtoint(args[0].from, 10, &entry->pcr); From 592b24cbdc12e52bdb5937c0697df9febf41f8d9 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 9 Jul 2020 01:19:10 -0500 Subject: [PATCH 13/18] ima: Use the common function to detect LSM conditionals in a rule Make broader use of ima_rule_contains_lsm_cond() to check if a given rule contains an LSM conditional. This is a code cleanup and has no user-facing change. Signed-off-by: Tyler Hicks Reviewed-by: Mimi Zohar Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index c679144af042..dcd1aaac4ff0 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -360,17 +360,10 @@ static bool ima_rule_contains_lsm_cond(struct ima_rule_entry *entry) static void ima_lsm_update_rules(void) { struct ima_rule_entry *entry, *e; - int i, result, needs_update; + int result; list_for_each_entry_safe(entry, e, &ima_policy_rules, list) { - needs_update = 0; - for (i = 0; i < MAX_LSM_RULES; i++) { - if (entry->lsm[i].args_p) { - needs_update = 1; - break; - } - } - if (!needs_update) + if (!ima_rule_contains_lsm_cond(entry)) continue; result = ima_lsm_update_rule(entry); From 4834177e633258fbf3c5754b1220f01c705b79eb Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 9 Jul 2020 01:19:11 -0500 Subject: [PATCH 14/18] ima: Support additional conditionals in the KEXEC_CMDLINE hook function Take the properties of the kexec kernel's inode and the current task ownership into consideration when matching a KEXEC_CMDLINE operation to the rules in the IMA policy. This allows for some uniformity when writing IMA policy rules for KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK, and KEXEC_CMDLINE operations. Prior to this patch, it was not possible to write a set of rules like this: dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t dont_measure func=KEXEC_CMDLINE obj_type=foo_t measure func=KEXEC_KERNEL_CHECK measure func=KEXEC_INITRAMFS_CHECK measure func=KEXEC_CMDLINE The inode information associated with the kernel being loaded by a kexec_kernel_load(2) syscall can now be included in the decision to measure or not Additonally, the uid, euid, and subj_* conditionals can also now be used in KEXEC_CMDLINE rules. There was no technical reason as to why those conditionals weren't being considered previously other than ima_match_rules() didn't have a valid inode to use so it immediately bailed out for KEXEC_CMDLINE operations rather than going through the full list of conditional comparisons. Signed-off-by: Tyler Hicks Cc: Eric Biederman Cc: kexec@lists.infradead.org Reviewed-by: Lakshmi Ramasubramanian Signed-off-by: Mimi Zohar --- include/linux/ima.h | 4 ++-- kernel/kexec_file.c | 2 +- security/integrity/ima/ima.h | 2 +- security/integrity/ima/ima_api.c | 2 +- security/integrity/ima/ima_appraise.c | 2 +- security/integrity/ima/ima_asymmetric_keys.c | 2 +- security/integrity/ima/ima_main.c | 23 +++++++++++++++----- security/integrity/ima/ima_policy.c | 17 +++++---------- security/integrity/ima/ima_queue_keys.c | 2 +- 9 files changed, 31 insertions(+), 25 deletions(-) diff --git a/include/linux/ima.h b/include/linux/ima.h index 9164e1534ec9..d15100de6cdd 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -25,7 +25,7 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size, enum kernel_read_file_id id); extern void ima_post_path_mknod(struct dentry *dentry); extern int ima_file_hash(struct file *file, char *buf, size_t buf_size); -extern void ima_kexec_cmdline(const void *buf, int size); +extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size); #ifdef CONFIG_IMA_KEXEC extern void ima_add_kexec_buffer(struct kimage *image); @@ -103,7 +103,7 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size) return -EOPNOTSUPP; } -static inline void ima_kexec_cmdline(const void *buf, int size) {} +static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {} #endif /* CONFIG_IMA */ #ifndef CONFIG_IMA_KEXEC diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c index bb05fd52de85..07df431c1f21 100644 --- a/kernel/kexec_file.c +++ b/kernel/kexec_file.c @@ -287,7 +287,7 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd, goto out; } - ima_kexec_cmdline(image->cmdline_buf, + ima_kexec_cmdline(kernel_fd, image->cmdline_buf, image->cmdline_buf_len - 1); } diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index ea7e77536f3c..576ae2c6d418 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -265,7 +265,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file, struct evm_ima_xattr_data *xattr_value, int xattr_len, const struct modsig *modsig, int pcr, struct ima_template_desc *template_desc); -void process_buffer_measurement(const void *buf, int size, +void process_buffer_measurement(struct inode *inode, const void *buf, int size, const char *eventname, enum ima_hooks func, int pcr, const char *keyring); void ima_audit_measurement(struct integrity_iint_cache *iint, diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index bf22de8b7ce0..4f39fb93f278 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -162,7 +162,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename, /** * ima_get_action - appraise & measure decision based on policy. - * @inode: pointer to inode to measure + * @inode: pointer to the inode associated with the object being validated * @cred: pointer to credentials structure to validate * @secid: secid of the task being validated * @mask: contains the permission mask (MAY_READ, MAY_WRITE, MAY_EXEC, diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index a9649b04b9f1..6c52bf7ea7f0 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -328,7 +328,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint, rc = is_binary_blacklisted(digest, digestsize); if ((rc == -EPERM) && (iint->flags & IMA_MEASURE)) - process_buffer_measurement(digest, digestsize, + process_buffer_measurement(NULL, digest, digestsize, "blacklisted-hash", NONE, pcr, NULL); } diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c index aaae80c4e376..1c68c500c26f 100644 --- a/security/integrity/ima/ima_asymmetric_keys.c +++ b/security/integrity/ima/ima_asymmetric_keys.c @@ -58,7 +58,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key, * if the IMA policy is configured to measure a key linked * to the given keyring. */ - process_buffer_measurement(payload, payload_len, + process_buffer_measurement(NULL, payload, payload_len, keyring->description, KEY_CHECK, 0, keyring->description); } diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 8351b2fd48e0..8a91711ca79b 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -726,6 +726,7 @@ int ima_load_data(enum kernel_load_data_id id) /* * process_buffer_measurement - Measure the buffer to ima log. + * @inode: inode associated with the object being measured (NULL for KEY_CHECK) * @buf: pointer to the buffer that needs to be added to the log. * @size: size of buffer(in bytes). * @eventname: event name to be used for the buffer entry. @@ -735,7 +736,7 @@ int ima_load_data(enum kernel_load_data_id id) * * Based on policy, the buffer is measured into the ima log. */ -void process_buffer_measurement(const void *buf, int size, +void process_buffer_measurement(struct inode *inode, const void *buf, int size, const char *eventname, enum ima_hooks func, int pcr, const char *keyring) { @@ -768,7 +769,7 @@ void process_buffer_measurement(const void *buf, int size, */ if (func) { security_task_getsecid(current, &secid); - action = ima_get_action(NULL, current_cred(), secid, 0, func, + action = ima_get_action(inode, current_cred(), secid, 0, func, &pcr, &template, keyring); if (!(action & IMA_MEASURE)) return; @@ -823,16 +824,26 @@ void process_buffer_measurement(const void *buf, int size, /** * ima_kexec_cmdline - measure kexec cmdline boot args + * @kernel_fd: file descriptor of the kexec kernel being loaded * @buf: pointer to buffer * @size: size of buffer * * Buffers can only be measured, not appraised. */ -void ima_kexec_cmdline(const void *buf, int size) +void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) { - if (buf && size != 0) - process_buffer_measurement(buf, size, "kexec-cmdline", - KEXEC_CMDLINE, 0, NULL); + struct fd f; + + if (!buf || !size) + return; + + f = fdget(kernel_fd); + if (!f.file) + return; + + process_buffer_measurement(file_inode(f.file), buf, size, + "kexec-cmdline", KEXEC_CMDLINE, 0, NULL); + fdput(f); } static int __init init_ima(void) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index dcd1aaac4ff0..9284055ee13a 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -443,13 +443,9 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode, { int i; - if ((func == KEXEC_CMDLINE) || (func == KEY_CHECK)) { - if ((rule->flags & IMA_FUNC) && (rule->func == func)) { - if (func == KEY_CHECK) - return ima_match_keyring(rule, keyring, cred); - return true; - } - return false; + if (func == KEY_CHECK) { + return (rule->flags & IMA_FUNC) && (rule->func == func) && + ima_match_keyring(rule, keyring, cred); } if ((rule->flags & IMA_FUNC) && (rule->func != func && func != POST_SETATTR)) @@ -1035,10 +1031,9 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) if (entry->action & ~(MEASURE | DONT_MEASURE)) return false; - if (entry->flags & ~(IMA_FUNC | IMA_PCR)) - return false; - - if (ima_rule_contains_lsm_cond(entry)) + if (entry->flags & ~(IMA_FUNC | IMA_FSMAGIC | IMA_UID | + IMA_FOWNER | IMA_FSUUID | IMA_EUID | + IMA_PCR | IMA_FSNAME)) return false; break; diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c index 56ce24a18b66..69a8626a35c0 100644 --- a/security/integrity/ima/ima_queue_keys.c +++ b/security/integrity/ima/ima_queue_keys.c @@ -158,7 +158,7 @@ void ima_process_queued_keys(void) list_for_each_entry_safe(entry, tmp, &ima_keys, list) { if (!timer_expired) - process_buffer_measurement(entry->payload, + process_buffer_measurement(NULL, entry->payload, entry->payload_len, entry->keyring_name, KEY_CHECK, 0, From b8867eedcf76caef8ae6412da97cd9abfd092ff8 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Fri, 10 Jul 2020 15:37:50 -0500 Subject: [PATCH 15/18] ima: Rename internal filter rule functions Rename IMA's internal filter rule functions from security_filter_rule_*() to ima_filter_rule_*(). This avoids polluting the security_* namespace, which is typically reserved for general security subsystem infrastructure. Signed-off-by: Tyler Hicks Suggested-by: Casey Schaufler [zohar@linux.ibm.com: reword using the term "filter", not "audit"] Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 16 +++++++-------- security/integrity/ima/ima_policy.c | 30 +++++++++++++---------------- 2 files changed, 21 insertions(+), 25 deletions(-) diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 576ae2c6d418..38043074ce5e 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -413,24 +413,24 @@ static inline void ima_free_modsig(struct modsig *modsig) /* LSM based policy rules require audit */ #ifdef CONFIG_IMA_LSM_RULES -#define security_filter_rule_init security_audit_rule_init -#define security_filter_rule_free security_audit_rule_free -#define security_filter_rule_match security_audit_rule_match +#define ima_filter_rule_init security_audit_rule_init +#define ima_filter_rule_free security_audit_rule_free +#define ima_filter_rule_match security_audit_rule_match #else -static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr, - void **lsmrule) +static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr, + void **lsmrule) { return -EINVAL; } -static inline void security_filter_rule_free(void *lsmrule) +static inline void ima_filter_rule_free(void *lsmrule) { } -static inline int security_filter_rule_match(u32 secid, u32 field, u32 op, - void *lsmrule) +static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op, + void *lsmrule) { return -EINVAL; } diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 9284055ee13a..07f033634b27 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -258,7 +258,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry) int i; for (i = 0; i < MAX_LSM_RULES; i++) { - security_filter_rule_free(entry->lsm[i].rule); + ima_filter_rule_free(entry->lsm[i].rule); kfree(entry->lsm[i].args_p); } } @@ -308,10 +308,9 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) */ entry->lsm[i].args_p = NULL; - security_filter_rule_init(nentry->lsm[i].type, - Audit_equal, - nentry->lsm[i].args_p, - &nentry->lsm[i].rule); + ima_filter_rule_init(nentry->lsm[i].type, Audit_equal, + nentry->lsm[i].args_p, + &nentry->lsm[i].rule); if (!nentry->lsm[i].rule) pr_warn("rule for LSM \'%s\' is undefined\n", nentry->lsm[i].args_p); @@ -495,18 +494,16 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode, case LSM_OBJ_ROLE: case LSM_OBJ_TYPE: security_inode_getsecid(inode, &osid); - rc = security_filter_rule_match(osid, - rule->lsm[i].type, - Audit_equal, - rule->lsm[i].rule); + rc = ima_filter_rule_match(osid, rule->lsm[i].type, + Audit_equal, + rule->lsm[i].rule); break; case LSM_SUBJ_USER: case LSM_SUBJ_ROLE: case LSM_SUBJ_TYPE: - rc = security_filter_rule_match(secid, - rule->lsm[i].type, - Audit_equal, - rule->lsm[i].rule); + rc = ima_filter_rule_match(secid, rule->lsm[i].type, + Audit_equal, + rule->lsm[i].rule); default: break; } @@ -901,10 +898,9 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry, return -ENOMEM; entry->lsm[lsm_rule].type = audit_type; - result = security_filter_rule_init(entry->lsm[lsm_rule].type, - Audit_equal, - entry->lsm[lsm_rule].args_p, - &entry->lsm[lsm_rule].rule); + result = ima_filter_rule_init(entry->lsm[lsm_rule].type, Audit_equal, + entry->lsm[lsm_rule].args_p, + &entry->lsm[lsm_rule].rule); if (!entry->lsm[lsm_rule].rule) { pr_warn("rule for LSM \'%s\' is undefined\n", entry->lsm[lsm_rule].args_p); From 1768215a650c612e7e68e779cfb50a8b55c2cf40 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Tue, 23 Jun 2020 18:38:23 -0500 Subject: [PATCH 16/18] ima: AppArmor satisfies the audit rule requirements AppArmor meets all the requirements for IMA in terms of audit rules since commit e79c26d04043 ("apparmor: Add support for audit rule filtering"). Update IMA's Kconfig section for CONFIG_IMA_LSM_RULES to reflect this. Fixes: e79c26d04043 ("apparmor: Add support for audit rule filtering") Signed-off-by: Tyler Hicks Signed-off-by: Mimi Zohar --- security/integrity/ima/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index edde88dbe576..794ebb5cbf74 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -54,7 +54,7 @@ config IMA_MEASURE_PCR_IDX config IMA_LSM_RULES bool - depends on IMA && AUDIT && (SECURITY_SELINUX || SECURITY_SMACK) + depends on IMA && AUDIT && (SECURITY_SELINUX || SECURITY_SMACK || SECURITY_APPARMOR) default y help Disabling this option will disregard LSM based policy rules. From 311aa6aafea446c2f954cc19d66425bfed8c4b0b Mon Sep 17 00:00:00 2001 From: Bruno Meneguele Date: Mon, 13 Jul 2020 13:48:30 -0300 Subject: [PATCH 17/18] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The IMA_APPRAISE_BOOTPARAM config allows enabling different "ima_appraise=" modes - log, fix, enforce - at run time, but not when IMA architecture specific policies are enabled.  This prevents properly labeling the filesystem on systems where secure boot is supported, but not enabled on the platform.  Only when secure boot is actually enabled should these IMA appraise modes be disabled. This patch removes the compile time dependency and makes it a runtime decision, based on the secure boot state of that platform. Test results as follows: -> x86-64 with secure boot enabled [ 0.015637] Kernel command line: <...> ima_policy=appraise_tcb ima_appraise=fix [ 0.015668] ima: Secure boot enabled: ignoring ima_appraise=fix boot parameter option -> powerpc with secure boot disabled [ 0.000000] Kernel command line: <...> ima_policy=appraise_tcb ima_appraise=fix [ 0.000000] Secure boot mode disabled -> Running the system without secure boot and with both options set: CONFIG_IMA_APPRAISE_BOOTPARAM=y CONFIG_IMA_ARCH_POLICY=y Audit prompts "missing-hash" but still allow execution and, consequently, filesystem labeling: type=INTEGRITY_DATA msg=audit(07/09/2020 12:30:27.778:1691) : pid=4976 uid=root auid=root ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=appraise_data cause=missing-hash comm=bash name=/usr/bin/evmctl dev="dm-0" ino=493150 res=no Cc: stable@vger.kernel.org Fixes: d958083a8f64 ("x86/ima: define arch_get_ima_policy() for x86") Signed-off-by: Bruno Meneguele Cc: stable@vger.kernel.org # 5.0 Signed-off-by: Mimi Zohar --- security/integrity/ima/Kconfig | 2 +- security/integrity/ima/ima_appraise.c | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index 794ebb5cbf74..080c53545ff0 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -232,7 +232,7 @@ config IMA_APPRAISE_REQUIRE_POLICY_SIGS config IMA_APPRAISE_BOOTPARAM bool "ima_appraise boot parameter" - depends on IMA_APPRAISE && !IMA_ARCH_POLICY + depends on IMA_APPRAISE default y help This option enables the different "ima_appraise=" modes diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 6c52bf7ea7f0..372d16382960 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -19,6 +19,12 @@ 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; + } + if (strncmp(str, "off", 3) == 0) ima_appraise = 0; else if (strncmp(str, "log", 3) == 0) From 3db0d0c276a752af39beb5ca7424cb659aa005bb Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Wed, 1 Jul 2020 14:46:34 +0100 Subject: [PATCH 18/18] integrity: remove redundant initialization of variable ret The variable ret is being initialized with a value that is never read and it is being updated later with a new value. The initialization is redundant and can be removed. Fixes: eb5798f2e28f ("integrity: convert digsig to akcipher api") Signed-off-by: Colin Ian King Acked-by: James Morris Addresses-Coverity: ("Unused value") Signed-off-by: Mimi Zohar --- security/integrity/digsig_asymmetric.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c index 4e0d6778277e..cfa4127d0518 100644 --- a/security/integrity/digsig_asymmetric.c +++ b/security/integrity/digsig_asymmetric.c @@ -79,7 +79,7 @@ int asymmetric_verify(struct key *keyring, const char *sig, struct public_key_signature pks; struct signature_v2_hdr *hdr = (struct signature_v2_hdr *)sig; struct key *key; - int ret = -ENOMEM; + int ret; if (siglen <= sizeof(*hdr)) return -EBADMSG;