lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Aug 2020 16:23:06 -0700 From: Tushar Sugandhi <tusharsu@...ux.microsoft.com> To: Mimi Zohar <zohar@...ux.ibm.com>, stephen.smalley.work@...il.com, casey@...aufler-ca.com, agk@...hat.com, snitzer@...hat.com, gmazyland@...il.com Cc: tyhicks@...ux.microsoft.com, sashal@...nel.org, jmorris@...ei.org, nramas@...ux.microsoft.com, linux-integrity@...r.kernel.org, selinux@...r.kernel.org, linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org, dm-devel@...hat.com Subject: Re: [PATCH v2 2/3] IMA: add policy to support measuring critical data from kernel components On 2020-08-25 1:43 p.m., Mimi Zohar wrote: > On Tue, 2020-08-25 at 10:32 -0700, Tushar Sugandhi wrote: >> >> On 2020-08-24 3:46 p.m., Mimi Zohar wrote: >>> On Fri, 2020-08-21 at 11:21 -0700, Tushar Sugandhi wrote: >>>> There would be several candidate kernel components suitable for IMA >>>> measurement. Not all of them would have support for IMA measurement. >>>> Also, system administrators may not want to measure data for all of >>>> them, even when they support IMA measurement. An IMA policy specific >>>> to various kernel components is needed to measure their respective >>>> critical data. >>>> >>>> Add a new IMA policy CRITICAL_DATA+data_sources to support measuring >>>> various critical kernel components. This policy would enable the >>>> system administrators to limit the measurement to the components, >>>> if the components support IMA measurement. >>>> >>>> Signed-off-by: Tushar Sugandhi <tusharsu@...ux.microsoft.com> >>>> --- >>>> Documentation/ABI/testing/ima_policy | 6 ++- >>>> security/integrity/ima/ima.h | 1 + >>>> security/integrity/ima/ima_api.c | 2 +- >>>> security/integrity/ima/ima_policy.c | 62 +++++++++++++++++++++++++--- >>>> 4 files changed, 63 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy >>>> index cd572912c593..a0dd0f108555 100644 >>>> --- a/Documentation/ABI/testing/ima_policy >>>> +++ b/Documentation/ABI/testing/ima_policy >>>> @@ -29,7 +29,7 @@ Description: >>>> base: 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 >>>> @@ -125,3 +125,7 @@ Description: >>>> keys added to .builtin_trusted_keys or .ima keyring: >>>> >>>> measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima >>>> + >>>> + Example of measure rule using CRITICAL_DATA to measure critical data >>>> + >>>> + measure func=CRITICAL_DATA data_sources=selinux|apparmor|dm-crypt >>> >>> This example uses "data_sources" without first defining it in the >>> "option:" section. Defining two new options is an indication that this >> Thanks. I will define "data_sources" first in "option:" section. >>> patch should be split up. One which defines the "CRITICAL_DATA" and >>> another one which defines the new key value pair. The term >> I intentionally kept the "CRITICAL_DATA" and "data_sources" in the same >> patch. >> >> CRITICAL_DATA is different than KEY_CHECK because in case of KEY_CHECK, >> "keyrings=" is optional. If "keyrings=" is not specified, then we >> measure all keyrings. >> >> Where for CRITICAL_DATA, "data_sources=" is mandatory. >> >> Because the data sources would be diverse and orthogonal to each other, >> (unlike "keyrings=") - not specifying "data_sources=" shouldn't result >> in IMA blindly measuring all data sources. > > Good point. >> >> Since CRITICAL_DATA, and "data_sources=" go hand in hand, I wanted them >> to be part of the same patch. > > Separating them will help clarify the patch description. There's no > harm in defining the critical data source first. > I will put func=CRITICAL_DATA into one patch, and "data_sources=" into the next patch. Coding wise, the reverse order of patches (where "data_sources=" goes in the first patch, before func=CRITICAL_DATA) doesn't make sense. Because ima_match_rules() etc. have switch cases built around func=CRITICAL_DATA etc. >>> "data_sources" is pretty generic. Perhaps constrain it a bit by re- >>> naming it "critical_data=". Or was such using a generic name >>> intentional? >>> >> We intentionally kept the name generic because the data to be measured >> could be coming from any kernel component with any granularity (from a >> single bool to megabytes of data). The kernel component is also loosely >> defined here. It could be an LSM (like SELinux), or a broader base layer >> (like device-mapper), or a specific module (like dm-crypt), or it could >> be different parts of a single module. >> >> Also, we didn't want to name "data_sources" as "critical_data" to avoid >> confusion with func "CRITICAL_DATA". > > The point is that you're measuring critical data, not just any data > from any source. Whatever term is used, it needs to be added to the > Documentation/ABI/testing/ima_policy. I think something that is self > describing will help. See what makes the most sense. Fair enough. Does "critical_kernel_data_sources=" sound ok? > >>> Normally "CRITICAL_DATA" would be defined with the critical data hook, >>> but that seems to be defined in patch 3/3 "IMA: define IMA hook to >>> measure critical data from kernel components". >>> >> I can make the "CRITICAL_DATA" and the hook as part of the same patch. >> That would mean combining patch 2 and 3 into a single one. >> >> Does it sound ok? > > In the other thread, we discussed separating out "measure_payload_hash"from other changes. The end result you want one logical change per patch. Each patch builds upon the previous one. (Look at how Tyler does it.) Will do. > >>>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h >>>> index 8875085db689..0f4209a92bfb 100644 >>>> --- a/security/integrity/ima/ima.h >>>> +++ b/security/integrity/ima/ima.h >>>> @@ -200,6 +200,7 @@ static inline unsigned int ima_hash_key(u8 *digest) >>>> hook(POLICY_CHECK, policy) \ >>>> hook(KEXEC_CMDLINE, kexec_cmdline) \ >>>> hook(KEY_CHECK, key) \ >>>> + hook(CRITICAL_DATA, critical_data) \ >>>> hook(MAX_CHECK, none) >>>> >>>> #define __ima_hook_enumify(ENUM, str) ENUM, >>>> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c >>>> index af218babd198..9917e1730cb6 100644 >>>> --- a/security/integrity/ima/ima_api.c >>>> +++ b/security/integrity/ima/ima_api.c >>>> @@ -176,7 +176,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename, >>>> * subj=, obj=, type=, func=, mask=, fsmagic= >>>> * subj,obj, and type: are LSM specific. >>>> * func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK >>>> - * | KEXEC_CMDLINE | KEY_CHECK >>>> + * | KEXEC_CMDLINE | KEY_CHECK | CRITICAL_DATA >>>> * mask: contains the permission mask >>>> * fsmagic: hex value >>>> * >>>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c >>>> index 8866e84d0062..7b649095ac7a 100644 >>>> --- a/security/integrity/ima/ima_policy.c >>>> +++ b/security/integrity/ima/ima_policy.c >>>> @@ -33,6 +33,7 @@ >>>> #define IMA_PCR 0x0100 >>>> #define IMA_FSNAME 0x0200 >>>> #define IMA_KEYRINGS 0x0400 >>>> +#define IMA_DATA_SOURCES 0x0800 >>>> >>>> #define UNKNOWN 0 >>>> #define MEASURE 0x0001 /* same as IMA_MEASURE */ >>>> @@ -84,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_sources; /* Measure data from these sources */ >>>> struct ima_template_desc *template; >>>> }; >>>> >>>> @@ -508,14 +510,23 @@ 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, rule->keyrings, func_data, >>>> - true, cred); >>>> - } >>>> if ((rule->flags & IMA_FUNC) && >>>> (rule->func != func && func != POST_SETATTR)) >>>> return false; >>>> + >>>> + switch (func) { >>>> + case KEY_CHECK: >>>> + return ((rule->func == func) && >>>> + ima_match_rule_data(rule, rule->keyrings, >>>> + func_data, true, cred)); >>>> + case CRITICAL_DATA: >>>> + return ((rule->func == func) && >>>> + ima_match_rule_data(rule, rule->data_sources, >>>> + func_data, false, cred)); >>>> + default: >>>> + break; >>>> + } >>>> + >>>> if ((rule->flags & IMA_MASK) && >>>> (rule->mask != mask && func != POST_SETATTR)) >>>> return false; >>>> @@ -911,7 +922,7 @@ enum { >>>> Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt, >>>> Opt_appraise_type, Opt_appraise_flag, >>>> Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings, >>>> - Opt_err >>>> + Opt_data_sources, Opt_err >>>> }; >>>> >>>> static const match_table_t policy_tokens = { >>>> @@ -948,6 +959,7 @@ static const match_table_t policy_tokens = { >>>> {Opt_pcr, "pcr=%s"}, >>>> {Opt_template, "template=%s"}, >>>> {Opt_keyrings, "keyrings=%s"}, >>>> + {Opt_data_sources, "data_sources=%s"}, >>>> {Opt_err, NULL} >>>> }; >>>> >>>> @@ -1110,6 +1122,19 @@ 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_DATA_SOURCES) || >>>> + (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR | >>>> + IMA_DATA_SOURCES))) >>>> + return false; >>> >>> Requiring IMA_FUNC and IMA_DATA_SOURCES makes sense, but why are >>> IMA_UID and IMA_PCR required? >>> >> Since the data to be measured could be for any scenario, I didn't want >> to restrict the kernel components from choosing UID to measure the data >> for, or restrict them from choosing PCR to store the measurements in. >> But as the consumers are kernel components, perhaps support for IMA_UID >> is not required. But we should still support IMA_PCR. >> Please let me know what do you think, and I can update the logic >> accordingly. > > I think I misinterpreted this code. As long as IMA_UID and IMA_PCR > aren't required, then it is fine. Yes, IMA_UID and IMA_PCR are not mandatory. Only IMA_DATA_SOURCES is. I will keep both of them. Thanks for confirming. > >>>> + >>>> + if (ima_rule_contains_lsm_cond(entry)) >>>> + return false; >>>> + >>>> break; >>>> default: >>>> return false; >>>> @@ -1242,6 +1267,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) >>>> @@ -1312,6 +1339,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) >>>> >>>> entry->flags |= IMA_KEYRINGS; >>>> break; >>>> + case Opt_data_sources: >>>> + ima_log_string(ab, "data_sources", args[0].from); >>>> + >>>> + if (entry->data_sources) { >>>> + result = -EINVAL; >>>> + break; >>>> + } >>>> + >>>> + entry->data_sources = ima_alloc_rule_opt_list(args); >>>> + if (IS_ERR(entry->data_sources)) { >>>> + result = PTR_ERR(entry->data_sources); >>>> + entry->data_sources = NULL; >>>> + break; >>>> + } >>>> + >>> >>> "keyrings=" isn't bounded because keyrings can be created by userspace. >>> Perhaps keyring names has a minimum/maximum length. IMA isn't >>> measuring userspace construsts. Shouldn't the list of critical data >>> being measured be bounded and verified? >> The comment is not entirely clear. >> Do you mean there should be some sort of allow_list in IMA, against >> which the values in "data_sources=" should be vetted? And if the >> value is present in the IMA allow_list, then only the measurements for >> that data source are allowed? >> >> Or do you mean something else? > > Yes, something along those lines. Does the list of critical data need > to be vetted? And if so, against what? I am thinking of having an enum and string array - just like ima_hooks and ima_hooks_measure_str in ima.h. And any new kernel component that would support generic IMA measurements in future would have to add itself to the enum/array. And the param *event_data_source in ima_measure_critical_data() will be vetted against the above enum/string array. I will implement it in the next iteration, and hopefully the vetting workflow will be more clear. ~Tushar > > Mimi > >>> >>>> + entry->flags |= IMA_DATA_SOURCES; >>>> + break; >>>> case Opt_fsuuid: >>>> ima_log_string(ab, "fsuuid", args[0].from); >>>> >>>> @@ -1692,6 +1736,12 @@ int ima_policy_show(struct seq_file *m, void *v) >>>> seq_puts(m, " "); >>>> } >>>> >>>> + if (entry->flags & IMA_DATA_SOURCES) { >>>> + seq_puts(m, "data_sources="); >>>> + ima_show_rule_opt_list(m, entry->data_sources); >>>> + seq_puts(m, " "); >>>> + } >>>> + >>>> if (entry->flags & IMA_PCR) { >>>> snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr); >>>> seq_printf(m, pt(Opt_pcr), tbuf); >
Powered by blists - more mailing lists