| Message ID | 20201211235807.30815-5-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IMA: support for measuring kernel integrity critical data | expand |
On 2020-12-11 15:58:03, Tushar Sugandhi wrote: > A new IMA policy rule is needed for the IMA hook > ima_measure_critical_data() and the corresponding func CRITICAL_DATA for > measuring the input buffer. The policy rule should ensure the buffer > would get measured only when the policy rule allows the action. The > policy rule should also support the necessary constraints (flags etc.) > for integrity critical buffer data measurements. > > Add a policy rule to define the constraints for restricting integrity > critical data measurements. > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > --- > Documentation/ABI/testing/ima_policy | 2 +- > security/integrity/ima/ima_policy.c | 34 ++++++++++++++++++++++++---- > 2 files changed, 31 insertions(+), 5 deletions(-) > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy > index e35263f97fc1..6ec7daa87cba 100644 > --- a/Documentation/ABI/testing/ima_policy > +++ b/Documentation/ABI/testing/ima_policy > @@ -32,7 +32,7 @@ Description: > func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK]MODULE_CHECK] > [FIRMWARE_CHECK] > [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK] > - [KEXEC_CMDLINE] [KEY_CHECK] > + [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA] > mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND] > [[^]MAY_EXEC] > fsmagic:= hex value > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index a09d1a41a290..07116ff35c25 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -85,6 +85,7 @@ struct ima_rule_entry { > } lsm[MAX_LSM_RULES]; > char *fsname; > struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */ > + struct ima_rule_opt_list *data_source; /* Measure data from this source */ Argh, there are still some more instances of data_source sneaking into this patch too early instead of waiting until the next patch. > struct ima_template_desc *template; > }; > > @@ -479,6 +480,12 @@ static bool ima_match_rule_data(struct ima_rule_entry *rule, > > opt_list = rule->keyrings; > break; > + case CRITICAL_DATA: > + if (!rule->data_source) > + return true; > + > + opt_list = rule->data_source; > + break; I guess this case should unconditionally return true in this patch and then the include this additional logic in the next patch. Sorry, I missed these on my last review. Tyler > default: > return false; > } > @@ -515,13 +522,19 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode, > { > int i; > > - if (func == KEY_CHECK) { > - return (rule->flags & IMA_FUNC) && (rule->func == func) && > - ima_match_rule_data(rule, func_data, cred); > - } > if ((rule->flags & IMA_FUNC) && > (rule->func != func && func != POST_SETATTR)) > return false; > + > + switch (func) { > + case KEY_CHECK: > + case CRITICAL_DATA: > + return ((rule->func == func) && > + ima_match_rule_data(rule, func_data, cred)); > + default: > + break; > + } > + > if ((rule->flags & IMA_MASK) && > (rule->mask != mask && func != POST_SETATTR)) > return false; > @@ -1116,6 +1129,17 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > if (ima_rule_contains_lsm_cond(entry)) > return false; > > + break; > + case CRITICAL_DATA: > + if (entry->action & ~(MEASURE | DONT_MEASURE)) > + return false; > + > + if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR)) > + return false; > + > + if (ima_rule_contains_lsm_cond(entry)) > + return false; > + > break; > default: > return false; > @@ -1248,6 +1272,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) > else if (IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) && > strcmp(args[0].from, "KEY_CHECK") == 0) > entry->func = KEY_CHECK; > + else if (strcmp(args[0].from, "CRITICAL_DATA") == 0) > + entry->func = CRITICAL_DATA; > else > result = -EINVAL; > if (!result) > -- > 2.17.1 >
On 2020-12-11 4:25 p.m., Tyler Hicks wrote: > On 2020-12-11 15:58:03, Tushar Sugandhi wrote: >> A new IMA policy rule is needed for the IMA hook >> ima_measure_critical_data() and the corresponding func CRITICAL_DATA for >> measuring the input buffer. The policy rule should ensure the buffer >> would get measured only when the policy rule allows the action. The >> policy rule should also support the necessary constraints (flags etc.) >> for integrity critical buffer data measurements. >> >> Add a policy rule to define the constraints for restricting integrity >> critical data measurements. >> >> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >> --- >> Documentation/ABI/testing/ima_policy | 2 +- >> security/integrity/ima/ima_policy.c | 34 ++++++++++++++++++++++++---- >> 2 files changed, 31 insertions(+), 5 deletions(-) >> >> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy >> index e35263f97fc1..6ec7daa87cba 100644 >> --- a/Documentation/ABI/testing/ima_policy >> +++ b/Documentation/ABI/testing/ima_policy >> @@ -32,7 +32,7 @@ Description: >> func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK]MODULE_CHECK] >> [FIRMWARE_CHECK] >> [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK] >> - [KEXEC_CMDLINE] [KEY_CHECK] >> + [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA] >> mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND] >> [[^]MAY_EXEC] >> fsmagic:= hex value >> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c >> index a09d1a41a290..07116ff35c25 100644 >> --- a/security/integrity/ima/ima_policy.c >> +++ b/security/integrity/ima/ima_policy.c >> @@ -85,6 +85,7 @@ struct ima_rule_entry { >> } lsm[MAX_LSM_RULES]; >> char *fsname; >> struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */ >> + struct ima_rule_opt_list *data_source; /* Measure data from this source */ > > Argh, there are still some more instances of data_source sneaking into > this patch too early instead of waiting until the next patch. > I kept it purposefully in this patch so that the "case CRITICAL_DATA:" could be properly defined. Also, my impression was rule->data_source is not part of the user facing policy. Whereas IMA_DATA_SOURCE, Opt_data_source, data_source=%s are. That's why they are part of Patch #5. Patch #5 IMA: limit critical data measurement based on a label >> struct ima_template_desc *template; >> }; >> >> @@ -479,6 +480,12 @@ static bool ima_match_rule_data(struct ima_rule_entry *rule, >> >> opt_list = rule->keyrings; >> break; >> + case CRITICAL_DATA: >> + if (!rule->data_source) >> + return true; >> + >> + opt_list = rule->data_source; >> + break; > > I guess this case should unconditionally return true in this patch and > then the include this additional logic in the next patch. > > Sorry, I missed these on my last review. > No worries. As I mentioned above, I kept it purposefully in this patch since my impression was rule->data_source is not part of the user facing policy. But I can simply return true here as you suggested, and move the logic to the next patch. + case CRITICAL_DATA: + if (!rule->data_source) + return true; + + opt_list = rule->data_source; + break; ~Tushar
On 2020-12-11 17:17:22, Tushar Sugandhi wrote: > > > On 2020-12-11 4:25 p.m., Tyler Hicks wrote: > > On 2020-12-11 15:58:03, Tushar Sugandhi wrote: > > > A new IMA policy rule is needed for the IMA hook > > > ima_measure_critical_data() and the corresponding func CRITICAL_DATA for > > > measuring the input buffer. The policy rule should ensure the buffer > > > would get measured only when the policy rule allows the action. The > > > policy rule should also support the necessary constraints (flags etc.) > > > for integrity critical buffer data measurements. > > > > > > Add a policy rule to define the constraints for restricting integrity > > > critical data measurements. > > > > > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > > > --- > > > Documentation/ABI/testing/ima_policy | 2 +- > > > security/integrity/ima/ima_policy.c | 34 ++++++++++++++++++++++++---- > > > 2 files changed, 31 insertions(+), 5 deletions(-) > > > > > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy > > > index e35263f97fc1..6ec7daa87cba 100644 > > > --- a/Documentation/ABI/testing/ima_policy > > > +++ b/Documentation/ABI/testing/ima_policy > > > @@ -32,7 +32,7 @@ Description: > > > func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK]MODULE_CHECK] > > > [FIRMWARE_CHECK] > > > [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK] > > > - [KEXEC_CMDLINE] [KEY_CHECK] > > > + [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA] > > > mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND] > > > [[^]MAY_EXEC] > > > fsmagic:= hex value > > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > > > index a09d1a41a290..07116ff35c25 100644 > > > --- a/security/integrity/ima/ima_policy.c > > > +++ b/security/integrity/ima/ima_policy.c > > > @@ -85,6 +85,7 @@ struct ima_rule_entry { > > > } lsm[MAX_LSM_RULES]; > > > char *fsname; > > > struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */ > > > + struct ima_rule_opt_list *data_source; /* Measure data from this source */ > > > > Argh, there are still some more instances of data_source sneaking into > > this patch too early instead of waiting until the next patch. > > > I kept it purposefully in this patch so that the > "case CRITICAL_DATA:" could be properly defined. > > Also, my impression was rule->data_source is not part of the user facing > policy. > > Whereas IMA_DATA_SOURCE, Opt_data_source, data_source=%s are. > That's why they are part of Patch #5. > > Patch #5 IMA: limit critical data measurement based on a label > > > > struct ima_template_desc *template; > > > }; > > > @@ -479,6 +480,12 @@ static bool ima_match_rule_data(struct ima_rule_entry *rule, > > > opt_list = rule->keyrings; > > > break; > > > + case CRITICAL_DATA: > > > + if (!rule->data_source) > > > + return true; > > > + > > > + opt_list = rule->data_source; > > > + break; > > > > I guess this case should unconditionally return true in this patch and > > then the include this additional logic in the next patch. > > > > Sorry, I missed these on my last review. > > > No worries. > > As I mentioned above, I kept it purposefully in this patch since > my impression was rule->data_source is not part of the user facing > policy. > > But I can simply return true here as you suggested, and move the logic to > the next patch. I understand the thinking that it isn't harmful in this patch but I think it is a bit cleaner to introduce the data_source policy language element and all of its backend support in the same patch. Please move it to the next patch. Thanks! Tyler > > + case CRITICAL_DATA: > + if (!rule->data_source) > + return true; > + > + opt_list = rule->data_source; > + break; > > > ~Tushar >
>>>> + case CRITICAL_DATA: >>>> + if (!rule->data_source) >>>> + return true; >>>> + >>>> + opt_list = rule->data_source; >>>> + break; >>> >>> I guess this case should unconditionally return true in this patch and >>> then the include this additional logic in the next patch. >>> >>> Sorry, I missed these on my last review. >>> >> No worries. >> >> As I mentioned above, I kept it purposefully in this patch since >> my impression was rule->data_source is not part of the user facing >> policy. >> >> But I can simply return true here as you suggested, and move the logic to >> the next patch. > > I understand the thinking that it isn't harmful in this patch but I > think it is a bit cleaner to introduce the data_source policy language > element and all of its backend support in the same patch. Please move it > to the next patch. Thanks! > > Tyler > Will do. Thanks a lot Tyler for a detailed review. Appreciate it. ~Tushar
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index e35263f97fc1..6ec7daa87cba 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -32,7 +32,7 @@ Description: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK]MODULE_CHECK] [FIRMWARE_CHECK] [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK] - [KEXEC_CMDLINE] [KEY_CHECK] + [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA] mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND] [[^]MAY_EXEC] fsmagic:= hex value diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index a09d1a41a290..07116ff35c25 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -85,6 +85,7 @@ struct ima_rule_entry { } lsm[MAX_LSM_RULES]; char *fsname; struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */ + struct ima_rule_opt_list *data_source; /* Measure data from this source */ struct ima_template_desc *template; }; @@ -479,6 +480,12 @@ static bool ima_match_rule_data(struct ima_rule_entry *rule, opt_list = rule->keyrings; break; + case CRITICAL_DATA: + if (!rule->data_source) + return true; + + opt_list = rule->data_source; + break; default: return false; } @@ -515,13 +522,19 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode, { int i; - if (func == KEY_CHECK) { - return (rule->flags & IMA_FUNC) && (rule->func == func) && - ima_match_rule_data(rule, func_data, cred); - } if ((rule->flags & IMA_FUNC) && (rule->func != func && func != POST_SETATTR)) return false; + + switch (func) { + case KEY_CHECK: + case CRITICAL_DATA: + return ((rule->func == func) && + ima_match_rule_data(rule, func_data, cred)); + default: + break; + } + if ((rule->flags & IMA_MASK) && (rule->mask != mask && func != POST_SETATTR)) return false; @@ -1116,6 +1129,17 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) if (ima_rule_contains_lsm_cond(entry)) return false; + break; + case CRITICAL_DATA: + if (entry->action & ~(MEASURE | DONT_MEASURE)) + return false; + + if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR)) + return false; + + if (ima_rule_contains_lsm_cond(entry)) + return false; + break; default: return false; @@ -1248,6 +1272,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) else if (IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) && strcmp(args[0].from, "KEY_CHECK") == 0) entry->func = KEY_CHECK; + else if (strcmp(args[0].from, "CRITICAL_DATA") == 0) + entry->func = CRITICAL_DATA; else result = -EINVAL; if (!result)
A new IMA policy rule is needed for the IMA hook ima_measure_critical_data() and the corresponding func CRITICAL_DATA for measuring the input buffer. The policy rule should ensure the buffer would get measured only when the policy rule allows the action. The policy rule should also support the necessary constraints (flags etc.) for integrity critical buffer data measurements. Add a policy rule to define the constraints for restricting integrity critical data measurements. Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> --- Documentation/ABI/testing/ima_policy | 2 +- security/integrity/ima/ima_policy.c | 34 ++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 5 deletions(-)