This series ultimately extends the supported IMA rule conditionals for the KEXEC_CMDLINE hook function. As of today, there's an imbalance in IMA language conditional support for KEXEC_CMDLINE rules in comparison to KEXEC_KERNEL_CHECK and KEXEC_INITRAMFS_CHECK rules. The KEXEC_CMDLINE rules do not support *any* conditionals so you cannot have a sequence 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 Instead, KEXEC_CMDLINE rules can only be measured or not measured and there's no additional flexibility in today's implementation of the KEXEC_CMDLINE hook function. With this series, the above sequence of rules becomes valid and any calls to kexec_file_load() with a kernel and initramfs inode type of foo_t will not be measured (that includes the kernel cmdline buffer) while all other objects given to a kexec_file_load() syscall will be measured. There's obviously not an inode directly associated with the kernel cmdline buffer but this patch series ties the inode based decision making for KEXEC_CMDLINE to the kernel's inode. I think this will be intuitive to policy authors. While reading IMA code and preparing to make this change, I realized that the buffer based hook functions (KEXEC_CMDLINE and KEY_CHECK) are quite special in comparison to longer standing hook functions. These buffer based hook functions can only support measure actions and there are some restrictions on the conditionals that they support. However, the rule parser isn't enforcing any of those restrictions and IMA policy authors wouldn't have any immediate way of knowing that the policy that they wrote is invalid. For example, the sequence of rules above parses successfully in today's kernel but the "dont_measure func=KEXEC_CMDLINE ..." rule is incorrectly handled in ima_match_rules(). The dont_measure rule is *always* considered to be a match so, surprisingly, no KEXEC_CMDLINE measurements are made. While making the rule parser more strict, I realized that the parser does not correctly free all of the allocated memory associated with an ima_rule_entry when going down some error paths. Invalid policy loaded by the policy administrator could result in small memory leaks. I envision patches 1-6 going to stable. The series is ordered in a way that has all the fixes up front, followed by cleanups, followed by the feature patch. The breakdown of patches looks like so: Memory leak fixes: 1-3 Parser strictness fixes: 4-6 Code cleanups made possible by the fixes: 7-10 Extend KEXEC_CMDLINE rule support: 11 Perhaps the most logical ordering for code review is: 1, 2, 3, 7, 8, 4, 5, 6, 9, 10, 11 If you'd like me to re-order or split up the series, just let me know. Thanks for considering these patches! * Series-wide v2 changes - Rebased onto next-integrity-testing - Squashed patches 2 and 3 from v1 + Updated this cover letter to account for changes to patch index changes + See patch 2 for specific code changes Tyler Tyler Hicks (11): ima: Have the LSM free its audit rule ima: Free the entire rule when deleting a list of rules ima: Free the entire rule if it fails to parse ima: Fail rule parsing when buffer hook functions have an invalid action ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an invalid cond ima: Fail rule parsing when the KEY_CHECK hook is combined with an invalid cond ima: Shallow copy the args_p member of ima_rule_entry.lsm elements ima: Use correct type for the args_p member of ima_rule_entry.lsm elements ima: Move validation of the keyrings conditional into ima_validate_rule() ima: Use the common function to detect LSM conditionals in a rule ima: Support additional conditionals in the KEXEC_CMDLINE hook function include/linux/ima.h | 4 +- kernel/kexec_file.c | 2 +- security/integrity/ima/ima.h | 7 +- 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 | 161 ++++++++++++++----- security/integrity/ima/ima_queue_keys.c | 2 +- 9 files changed, 148 insertions(+), 57 deletions(-) -- 2.25.1
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 <tyhicks@linux.microsoft.com> Cc: Janne Karhunen <janne.karhunen@gmail.com> Cc: Casey Schaufler <casey@schaufler-ca.com> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> --- * v2 - Fixed build warning by dropping the 'return -EINVAL' from the stubbed out security_filter_rule_free() since it has a void return type - Added Mimi's Reviewed-by - Developed a follow-on patch to rename security_filter_rule_*() functions, to address Casey's request, but I'll submit it independently of this patch series since it is somewhat unrelated 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); -- 2.25.1
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 <tyhicks@linux.microsoft.com> --- * v2 - Collapsed patch #2 from v1 of this series, into this patch. This patch now introduces ima_free_rule(). - Existing callers of ima_lsm_free_rule() are doing so to free rules after a successful or failed ima_lsm_copy_rule() and those callers continue to directly call ima_lsm_copy_rule() rather than doing explicit reference ownership and calling ima_free_rule(). - The kfree(entry) of ima_lsm_free_rule() was removed from that function to make it focused on freeing the LSM references. Direct callers of ima_lsm_free_rule() must now call kfree(entry) after ima_lsm_free_rule(). - A comment was added in ima_lsm_update_rule() to clarify why ima_free_rule() isn't being used. 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); } } -- 2.25.1
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 <tyhicks@linux.microsoft.com> --- * v2 - No change 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); -- 2.25.1
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 <tyhicks@linux.microsoft.com> --- * v2 - No change security/integrity/ima/ima_policy.c | 36 +++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index e458cd47c099..166124d67774 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -973,6 +973,39 @@ static void check_template_modsig(const struct ima_template_desc *template) #undef MSG } +static bool ima_validate_rule(struct ima_rule_entry *entry) +{ + if (entry->action == UNKNOWN) + return false; + + if (entry->flags & IMA_FUNC) { + 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: + 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 +1183,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 +1388,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); -- 2.25.1
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 <tyhicks@linux.microsoft.com> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> --- * v2 - Added Mimi's Reviewed-by 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 166124d67774..676d5557af1a 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 @@ -993,6 +1004,16 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) case POLICY_CHECK: 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; -- 2.25.1
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 <tyhicks@linux.microsoft.com> --- * v2 - No change 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 676d5557af1a..f9b1bdb897da 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -1018,6 +1018,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; -- 2.25.1
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 <tyhicks@linux.microsoft.com> --- * v2 - Adjusted context to account for ima_lsm_copy_rule() directly calling ima_lsm_free_rule() and the lack of explicit reference ownership transfers - Added comment to ima_lsm_copy_rule() to document the args_p reference ownership transfer security/integrity/ima/ima_policy.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index f9b1bdb897da..ef69c54266c6 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, @@ -314,11 +317,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) (char *)entry->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) -- 2.25.1
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 <tyhicks@linux.microsoft.com> --- * v2 - No change 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 ef69c54266c6..8cdca2399d59 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 *)entry->lsm[i].args_p); + entry->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); @@ -1667,27 +1667,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, " "); -- 2.25.1
Use ima_validate_rule() to ensure that the combination of a hook function and the keyrings conditional is valid and that the keyrings conditional is not specified without an explicit KEY_CHECK func conditional. This is a code cleanup and has no user-facing change. Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> --- * v2 - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO, IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be present in the rule entry flags for non-buffer hook functions. security/integrity/ima/ima_policy.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 8cdca2399d59..43d49ad958fb 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) case KEXEC_KERNEL_CHECK: case KEXEC_INITRAMFS_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 | + IMA_MODSIG_ALLOWED | + IMA_CHECK_BLACKLIST)) + return false; + break; case KEXEC_CMDLINE: if (entry->action & ~(MEASURE | DONT_MEASURE)) @@ -1027,7 +1036,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) default: return false; } - } + } else if (entry->flags & IMA_KEYRINGS) + return false; return true; } @@ -1209,7 +1219,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; -- 2.25.1
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 <tyhicks@linux.microsoft.com> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> --- * v2 - No change 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 43d49ad958fb..5eb14b567a31 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); -- 2.25.1
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 <tyhicks@linux.microsoft.com> Cc: Eric Biederman <ebiederm@xmission.com> Cc: kexec@lists.infradead.org --- * v2 - Moved the inode parameter of process_buffer_measurement() to be the first parameter so that it more closely matches process_masurement() 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 59ec28f5c117..ff2bf57ff0c7 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 5eb14b567a31..294323b36d06 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)) @@ -1007,10 +1003,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, -- 2.25.1
On 6/26/20 3:38 PM, Tyler Hicks wrote:
> 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 <tyhicks@linux.microsoft.com>
> ---
>
> * v2
> - No change
>
> 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 676d5557af1a..f9b1bdb897da 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1018,6 +1018,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;
>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
On 6/26/20 3:38 PM, Tyler Hicks wrote:
> 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 <tyhicks@linux.microsoft.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>
> * v2
> - Added Mimi's Reviewed-by
>
> 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 166124d67774..676d5557af1a 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
> @@ -993,6 +1004,16 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> case POLICY_CHECK:
> 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;
>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
On 6/26/20 3:38 PM, Tyler Hicks wrote: > Use ima_validate_rule() to ensure that the combination of a hook > function and the keyrings conditional is valid and that the keyrings > conditional is not specified without an explicit KEY_CHECK func > conditional. This is a code cleanup and has no user-facing change. In addition to checking for func=KEY_CHECK and the keyrings conditional, the patch also validates the flags for other IMA hooks (such as KEXEC_KERNEL_CHECK, POLICY_CHECK, etc.) Would be good to mention that in the patch description. -lakshmi > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > --- > > * v2 > - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO, > IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be > present in the rule entry flags for non-buffer hook functions. > > security/integrity/ima/ima_policy.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index 8cdca2399d59..43d49ad958fb 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > case KEXEC_KERNEL_CHECK: > case KEXEC_INITRAMFS_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 | > + IMA_MODSIG_ALLOWED | > + IMA_CHECK_BLACKLIST)) > + return false; > + > break; > case KEXEC_CMDLINE: > if (entry->action & ~(MEASURE | DONT_MEASURE)) > @@ -1027,7 +1036,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > default: > return false; > } > - } > + } else if (entry->flags & IMA_KEYRINGS) > + return false; > > return true; > } > @@ -1209,7 +1219,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; >
On 6/26/20 3:39 PM, Tyler Hicks wrote:
> 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 <tyhicks@linux.microsoft.com>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: kexec@lists.infradead.org
> ---
>
> * v2
> - Moved the inode parameter of process_buffer_measurement() to be the
> first parameter so that it more closely matches process_masurement()
>
> 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(-)
>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
On 2020-06-27 16:49:46, Lakshmi Ramasubramanian wrote: > On 6/26/20 3:38 PM, Tyler Hicks wrote: > > > Use ima_validate_rule() to ensure that the combination of a hook > > function and the keyrings conditional is valid and that the keyrings > > conditional is not specified without an explicit KEY_CHECK func > > conditional. This is a code cleanup and has no user-facing change. > > In addition to checking for func=KEY_CHECK and the keyrings conditional, the > patch also validates the flags for other IMA hooks (such as > KEXEC_KERNEL_CHECK, POLICY_CHECK, etc.) Would be good to mention that in the > patch description. It actually doesn't do any additional validation of other IMA hooks at this time. That check on entry->flags is an allowlist of every possible conditional flag except IMA_KEYRINGS. The ima_parse_rule() function is already validating all of these conditional flags. Tyler > > -lakshmi > > > > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > > --- > > > > * v2 > > - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO, > > IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be > > present in the rule entry flags for non-buffer hook functions. > > > > security/integrity/ima/ima_policy.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > > index 8cdca2399d59..43d49ad958fb 100644 > > --- a/security/integrity/ima/ima_policy.c > > +++ b/security/integrity/ima/ima_policy.c > > @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > > case KEXEC_KERNEL_CHECK: > > case KEXEC_INITRAMFS_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 | > > + IMA_MODSIG_ALLOWED | > > + IMA_CHECK_BLACKLIST)) > > + return false; > > + > > break; > > case KEXEC_CMDLINE: > > if (entry->action & ~(MEASURE | DONT_MEASURE)) > > @@ -1027,7 +1036,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > > default: > > return false; > > } > > - } > > + } else if (entry->flags & IMA_KEYRINGS) > > + return false; > > return true; > > } > > @@ -1209,7 +1219,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; > >
On Fri, 2020-06-26 at 17:38 -0500, Tyler Hicks wrote: > Use ima_validate_rule() to ensure that the combination of a hook > function and the keyrings conditional is valid and that the keyrings > conditional is not specified without an explicit KEY_CHECK func > conditional. This is a code cleanup and has no user-facing change. > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > --- > > * v2 > - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO, > IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be > present in the rule entry flags for non-buffer hook functions. > > security/integrity/ima/ima_policy.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index 8cdca2399d59..43d49ad958fb 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > case KEXEC_KERNEL_CHECK: > case KEXEC_INITRAMFS_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 | > + IMA_MODSIG_ALLOWED | > + IMA_CHECK_BLACKLIST)) Other than KEYRINGS, this patch should continue to behave the same. However, this list gives the impressions that all of these flags are permitted on all of the above flags, which isn't true. For example, both IMA_MODSIG_ALLOWED & IMA_CHECK_BLACKLIST are limited to appended signatures, meaning KERNEL_CHECK and KEXEC_KERNEL_CHECK. Both should only be allowed on APPRAISE action rules. IMA_PCR should be limited to MEASURE action rules. IMA_DIGSIG_REQUIRED should be limited to APPRAISE action rules. > + return false; > + > break; > case KEXEC_CMDLINE: > if (entry->action & ~(MEASURE | DONT_MEASURE)) > @@ -1027,7 +1036,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > default: > return false; > } > - } > + } else if (entry->flags & IMA_KEYRINGS) > + return false; IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST need to be added here as well. Mimi > > return true; > } > @@ -1209,7 +1219,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;
On Fri, 2020-06-26 at 17:38 -0500, Tyler Hicks wrote:
> This series ultimately extends the supported IMA rule conditionals for
> the KEXEC_CMDLINE hook function. As of today, there's an imbalance in
> IMA language conditional support for KEXEC_CMDLINE rules in comparison
> to KEXEC_KERNEL_CHECK and KEXEC_INITRAMFS_CHECK rules. The KEXEC_CMDLINE
> rules do not support *any* conditionals so you cannot have a sequence 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
>
> Instead, KEXEC_CMDLINE rules can only be measured or not measured and
> there's no additional flexibility in today's implementation of the
> KEXEC_CMDLINE hook function.
>
> With this series, the above sequence of rules becomes valid and any
> calls to kexec_file_load() with a kernel and initramfs inode type of
> foo_t will not be measured (that includes the kernel cmdline buffer)
> while all other objects given to a kexec_file_load() syscall will be
> measured. There's obviously not an inode directly associated with the
> kernel cmdline buffer but this patch series ties the inode based
> decision making for KEXEC_CMDLINE to the kernel's inode. I think this
> will be intuitive to policy authors.
>
> While reading IMA code and preparing to make this change, I realized
> that the buffer based hook functions (KEXEC_CMDLINE and KEY_CHECK) are
> quite special in comparison to longer standing hook functions. These
> buffer based hook functions can only support measure actions and there
> are some restrictions on the conditionals that they support. However,
> the rule parser isn't enforcing any of those restrictions and IMA policy
> authors wouldn't have any immediate way of knowing that the policy that
> they wrote is invalid. For example, the sequence of rules above parses
> successfully in today's kernel but the
> "dont_measure func=KEXEC_CMDLINE ..." rule is incorrectly handled in
> ima_match_rules(). The dont_measure rule is *always* considered to be a
> match so, surprisingly, no KEXEC_CMDLINE measurements are made.
>
> While making the rule parser more strict, I realized that the parser
> does not correctly free all of the allocated memory associated with an
> ima_rule_entry when going down some error paths. Invalid policy loaded
> by the policy administrator could result in small memory leaks.
>
> I envision patches 1-6 going to stable. The series is ordered in a way
> that has all the fixes up front, followed by cleanups, followed by the
> feature patch. The breakdown of patches looks like so:
>
> Memory leak fixes: 1-3
> Parser strictness fixes: 4-6
> Code cleanups made possible by the fixes: 7-10
> Extend KEXEC_CMDLINE rule support: 11
>
> Perhaps the most logical ordering for code review is:
>
> 1, 2, 3, 7, 8, 4, 5, 6, 9, 10, 11
>
> If you'd like me to re-order or split up the series, just let me know.
> Thanks for considering these patches!
>
> * Series-wide v2 changes
> - Rebased onto next-integrity-testing
> - Squashed patches 2 and 3 from v1
> + Updated this cover letter to account for changes to patch index
> changes
> + See patch 2 for specific code changes
Other than the comment on 9/11 the patch set looks good.
thanks!
Mimi
Hi,
On 06/26/20 at 05:39pm, Tyler Hicks wrote:
> 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 <tyhicks@linux.microsoft.com>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: kexec@lists.infradead.org
> ---
>
> * v2
> - Moved the inode parameter of process_buffer_measurement() to be the
> first parameter so that it more closely matches process_masurement()
>
> 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 59ec28f5c117..ff2bf57ff0c7 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 5eb14b567a31..294323b36d06 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))
> @@ -1007,10 +1003,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,
> --
> 2.25.1
>
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>
Although I still do not understand the deep knowledge of IMA, I
still wonder to know what is the effect to the behavior changes end user
visible. Does it work with a kernel built-in commandline? eg no
cmdlien passed at all.
Thanks
Dave
On 2020-07-01 16:04:16, Dave Young wrote: > Hi, > On 06/26/20 at 05:39pm, Tyler Hicks wrote: > > 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 <tyhicks@linux.microsoft.com> > > Cc: Eric Biederman <ebiederm@xmission.com> > > Cc: kexec@lists.infradead.org > > --- > > > > * v2 > > - Moved the inode parameter of process_buffer_measurement() to be the > > first parameter so that it more closely matches process_masurement() > > > > 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 59ec28f5c117..ff2bf57ff0c7 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 5eb14b567a31..294323b36d06 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)) > > @@ -1007,10 +1003,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, > > -- > > 2.25.1 > > > > > > _______________________________________________ > > kexec mailing list > > kexec@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/kexec > > > > Although I still do not understand the deep knowledge of IMA, I > still wonder to know what is the effect to the behavior changes end user > visible. Does it work with a kernel built-in commandline? eg no > cmdlien passed at all. Ah, very good question. This IMA hook (KEXEC_CMDLINE) only measures the string passed to the cmdline argument of the kexec_file_load(2) syscall. However, kernel commandline options injected into a kernel with the CONFIG_CMDLINE or CONFIG_CMDLINE_EXTEND Kconfig options would still be measured, as part of the vmlinux as a whole, by the KEXEC_KERNEL_CHECK IMA hook. Tyler > > Thanks > Dave
On 2020-06-30 19:07:29, Mimi Zohar wrote: > On Fri, 2020-06-26 at 17:38 -0500, Tyler Hicks wrote: > > Use ima_validate_rule() to ensure that the combination of a hook > > function and the keyrings conditional is valid and that the keyrings > > conditional is not specified without an explicit KEY_CHECK func > > conditional. This is a code cleanup and has no user-facing change. > > > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > > --- > > > > * v2 > > - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO, > > IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be > > present in the rule entry flags for non-buffer hook functions. > > > > security/integrity/ima/ima_policy.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > > index 8cdca2399d59..43d49ad958fb 100644 > > --- a/security/integrity/ima/ima_policy.c > > +++ b/security/integrity/ima/ima_policy.c > > @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > > case KEXEC_KERNEL_CHECK: > > case KEXEC_INITRAMFS_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 | > > + IMA_MODSIG_ALLOWED | > > + IMA_CHECK_BLACKLIST)) > > Other than KEYRINGS, this patch should continue to behave the same. > However, this list gives the impressions that all of these flags are > permitted on all of the above flags, which isn't true. > > For example, both IMA_MODSIG_ALLOWED & IMA_CHECK_BLACKLIST are limited > to appended signatures, meaning KERNEL_CHECK and KEXEC_KERNEL_CHECK. Just to clarify, are both IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST limited to KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK, and MODULE_CHECK? That's what ima_hook_supports_modsig() suggests. > Both should only be allowed on APPRAISE action rules. For completeness, it looks like DONT_APPRAISE should not be allowed. > IMA_PCR should be limited to MEASURE action rules. It looks like DONT_MEASURE should not be allowed. > IMA_DIGSIG_REQUIRED should be limited to APPRAISE action rules. It looks like DONT_APPRAISE should not be allowed. > > > + return false; > > + > > break; > > case KEXEC_CMDLINE: > > if (entry->action & ~(MEASURE | DONT_MEASURE)) > > @@ -1027,7 +1036,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > > default: > > return false; > > } > > - } > > + } else if (entry->flags & IMA_KEYRINGS) > > + return false; > > IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST need to be added here as > well. That makes sense. Tyler > > Mimi > > > > > return true; > > } > > @@ -1209,7 +1219,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;
On Thu, 2020-07-02 at 17:16 -0500, Tyler Hicks wrote: > On 2020-06-30 19:07:29, Mimi Zohar wrote: > > On Fri, 2020-06-26 at 17:38 -0500, Tyler Hicks wrote: > > > Use ima_validate_rule() to ensure that the combination of a hook > > > function and the keyrings conditional is valid and that the keyrings > > > conditional is not specified without an explicit KEY_CHECK func > > > conditional. This is a code cleanup and has no user-facing change. > > > > > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > > > --- > > > > > > * v2 > > > - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO, > > > IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be > > > present in the rule entry flags for non-buffer hook functions. > > > > > > security/integrity/ima/ima_policy.c | 13 +++++++++++-- > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > > > index 8cdca2399d59..43d49ad958fb 100644 > > > --- a/security/integrity/ima/ima_policy.c > > > +++ b/security/integrity/ima/ima_policy.c > > > @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > > > case KEXEC_KERNEL_CHECK: > > > case KEXEC_INITRAMFS_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 | > > > + IMA_MODSIG_ALLOWED | > > > + IMA_CHECK_BLACKLIST)) > > > > Other than KEYRINGS, this patch should continue to behave the same. > > However, this list gives the impressions that all of these flags are > > permitted on all of the above flags, which isn't true. > > > > For example, both IMA_MODSIG_ALLOWED & IMA_CHECK_BLACKLIST are limited > > to appended signatures, meaning KERNEL_CHECK and KEXEC_KERNEL_CHECK. > > Just to clarify, are both IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST > limited to KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK, and MODULE_CHECK? > That's what ima_hook_supports_modsig() suggests. Theoretically that is true, but I have no idea how you would append a signature to the kexec boot command line. The only users of appended signatures are currently kernel modules and the kexec'ed kernel image. > > > Both should only be allowed on APPRAISE action rules. > > For completeness, it looks like DONT_APPRAISE should not be allowed. Good point. > > > IMA_PCR should be limited to MEASURE action rules. > > It looks like DONT_MEASURE should not be allowed. The TPM PCR isn't a file attribute. > > > IMA_DIGSIG_REQUIRED should be limited to APPRAISE action rules. > > It looks like DONT_APPRAISE should not be allowed. Right, in all of these cases the DONT_XXXX isn't applicable. Mimi
On 2020-07-03 10:15:32, Mimi Zohar wrote: > On Thu, 2020-07-02 at 17:16 -0500, Tyler Hicks wrote: > > On 2020-06-30 19:07:29, Mimi Zohar wrote: > > > On Fri, 2020-06-26 at 17:38 -0500, Tyler Hicks wrote: > > > > Use ima_validate_rule() to ensure that the combination of a hook > > > > function and the keyrings conditional is valid and that the keyrings > > > > conditional is not specified without an explicit KEY_CHECK func > > > > conditional. This is a code cleanup and has no user-facing change. > > > > > > > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > > > > --- > > > > > > > > * v2 > > > > - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO, > > > > IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be > > > > present in the rule entry flags for non-buffer hook functions. > > > > > > > > security/integrity/ima/ima_policy.c | 13 +++++++++++-- > > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > > > > index 8cdca2399d59..43d49ad958fb 100644 > > > > --- a/security/integrity/ima/ima_policy.c > > > > +++ b/security/integrity/ima/ima_policy.c > > > > @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > > > > case KEXEC_KERNEL_CHECK: > > > > case KEXEC_INITRAMFS_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 | > > > > + IMA_MODSIG_ALLOWED | > > > > + IMA_CHECK_BLACKLIST)) > > > > > > Other than KEYRINGS, this patch should continue to behave the same. > > > However, this list gives the impressions that all of these flags are > > > permitted on all of the above flags, which isn't true. > > > > > > For example, both IMA_MODSIG_ALLOWED & IMA_CHECK_BLACKLIST are limited > > > to appended signatures, meaning KERNEL_CHECK and KEXEC_KERNEL_CHECK. > > > > Just to clarify, are both IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST > > limited to KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK, and MODULE_CHECK? > > That's what ima_hook_supports_modsig() suggests. > > Theoretically that is true, but I have no idea how you would append a > signature to the kexec boot command line. The only users of appended > signatures are currently kernel modules and the kexec'ed kernel image. The discrepancy was with KEXEC_INITRAMFS_CHECK, not KEXEC_CMDLINE. I now see that there's no support for initramfs signature verification in the kexec code so I'll assume that ima_hook_supports_modsig() is wrong and limit IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST to the KEXEC_KERNEL_CHECK and MODULE_CHECK actions, as you originally suggested. Tyler > > > > > > Both should only be allowed on APPRAISE action rules. > > > > For completeness, it looks like DONT_APPRAISE should not be allowed. > > Good point. > > > > > > IMA_PCR should be limited to MEASURE action rules. > > > > It looks like DONT_MEASURE should not be allowed. > > The TPM PCR isn't a file attribute. > > > > > > IMA_DIGSIG_REQUIRED should be limited to APPRAISE action rules. > > > > It looks like DONT_APPRAISE should not be allowed. > > Right, in all of these cases the DONT_XXXX isn't applicable. > > Mimi
On Mon, 2020-07-06 at 08:18 -0500, Tyler Hicks wrote:
> On 2020-07-03 10:15:32, Mimi Zohar wrote:
> > On Thu, 2020-07-02 at 17:16 -0500, Tyler Hicks wrote:
> > > On 2020-06-30 19:07:29, Mimi Zohar wrote:
> > > > On Fri, 2020-06-26 at 17:38 -0500, Tyler Hicks wrote:
> > > > > Use ima_validate_rule() to ensure that the combination of a hook
> > > > > function and the keyrings conditional is valid and that the keyrings
> > > > > conditional is not specified without an explicit KEY_CHECK func
> > > > > conditional. This is a code cleanup and has no user-facing change.
> > > > >
> > > > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > > > > ---
> > > > >
> > > > > * v2
> > > > > - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO,
> > > > > IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be
> > > > > present in the rule entry flags for non-buffer hook functions.
> > > > >
> > > > > security/integrity/ima/ima_policy.c | 13 +++++++++++--
> > > > > 1 file changed, 11 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > > > > index 8cdca2399d59..43d49ad958fb 100644
> > > > > --- a/security/integrity/ima/ima_policy.c
> > > > > +++ b/security/integrity/ima/ima_policy.c
> > > > > @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> > > > > case KEXEC_KERNEL_CHECK:
> > > > > case KEXEC_INITRAMFS_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 |
> > > > > + IMA_MODSIG_ALLOWED |
> > > > > + IMA_CHECK_BLACKLIST))
> > > >
> > > > Other than KEYRINGS, this patch should continue to behave the same.
> > > > However, this list gives the impressions that all of these flags are
> > > > permitted on all of the above flags, which isn't true.
> > > >
> > > > For example, both IMA_MODSIG_ALLOWED & IMA_CHECK_BLACKLIST are limited
> > > > to appended signatures, meaning KERNEL_CHECK and KEXEC_KERNEL_CHECK.
> > >
> > > Just to clarify, are both IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST
> > > limited to KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK, and MODULE_CHECK?
> > > That's what ima_hook_supports_modsig() suggests.
> >
> > Theoretically that is true, but I have no idea how you would append a
> > signature to the kexec boot command line. The only users of appended
> > signatures are currently kernel modules and the kexec'ed kernel image.
>
> The discrepancy was with KEXEC_INITRAMFS_CHECK, not KEXEC_CMDLINE. I now
> see that there's no support for initramfs signature verification in the
> kexec code so I'll assume that ima_hook_supports_modsig() is wrong and
> limit IMA_MODSIG_ALLOWED and IMA_CHECK_BLACKLIST to the
> KEXEC_KERNEL_CHECK and MODULE_CHECK actions, as you originally
> suggested.
My mistake. Yes, both the kexec kernel image and the initramfs read
the respective file into memory and can be signed either with an
imasig or modsig. Refer to kernel_read_file_from_fd().
Mimi