| Message ID | 20200623003236.830149-6-tyhicks@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support | expand |
On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote: > 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> > --- > 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 ee5152ecd3d9..ecc234b956a2 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -979,6 +979,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; > +} > + Good idea. There are a couple of other examples that could be cleaned up as well. For example, for performance reasons "appraise_flag=check_blacklist" is limited to files with appended signatures, like kernel modules and the kexec kernel image (OpenPower). Mimi
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index ee5152ecd3d9..ecc234b956a2 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -979,6 +979,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; @@ -1156,7 +1189,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; @@ -1362,7 +1394,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);
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> --- security/integrity/ima/ima_policy.c | 36 +++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-)