| Message ID | 20201101222626.6111-6-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IMA: Infrastructure for measurement of critical kernel data | expand |
Hi Tushar, On Sun, 2020-11-01 at 14:26 -0800, Tushar Sugandhi wrote: > Currently, IMA does not restrict random data sources from measuring > their data using ima_measure_critical_data(). Any kernel data source can > call the function, and it's data will get measured as long as the input > event_data_source is part of the IMA policy - CRITICAL_DATA+data_sources. > > To ensure that only data from supported sources are measured, the kernel > subsystem name needs to be added to a compile-time list of supported > sources (an "allowed list of components"). IMA then validates the input > parameter - "event_data_source" passed to ima_measure_critical_data() > against this allowed list at run-time. > > This compile-time list must be updated when kernel subsystems are > updated to measure their data using IMA. > > Provide an infrastructure for kernel data sources to be added to > IMA's supported data sources list at compile-time. Update > ima_measure_critical_data() to validate, at run-time, that the data > source is supported before measuring the data coming from that source. For those interested in limiting which critical data to measure, the "data sources" IMA policy rule option already does that. Why is this needed? thanks, Mimi
On 2020-11-06 6:01 a.m., Mimi Zohar wrote: > Hi Tushar, > > On Sun, 2020-11-01 at 14:26 -0800, Tushar Sugandhi wrote: >> Currently, IMA does not restrict random data sources from measuring >> their data using ima_measure_critical_data(). Any kernel data source can >> call the function, and it's data will get measured as long as the input >> event_data_source is part of the IMA policy - CRITICAL_DATA+data_sources. >> >> To ensure that only data from supported sources are measured, the kernel >> subsystem name needs to be added to a compile-time list of supported >> sources (an "allowed list of components"). IMA then validates the input >> parameter - "event_data_source" passed to ima_measure_critical_data() >> against this allowed list at run-time. >> >> This compile-time list must be updated when kernel subsystems are >> updated to measure their data using IMA. >> >> Provide an infrastructure for kernel data sources to be added to >> IMA's supported data sources list at compile-time. Update >> ima_measure_critical_data() to validate, at run-time, that the data >> source is supported before measuring the data coming from that source. > > For those interested in limiting which critical data to measure, the > "data sources" IMA policy rule option already does that. Why is this > needed? > > thanks, > > Mimi > This wasn’t part of the initial series. And I wasn’t convinced if it was really needed. :) I added it based on the feedback in v2 of this series. (pasted below for reference[1]). Maybe I misunderstood your feedback at that time. *Question* Could you please let me know if you want us to remove this patch? [1] From v2 of this series: https://patchwork.kernel.org/project/linux-integrity/patch/20200821182107.5328-3-tusharsu@linux.microsoft.com/ >>>> "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
Hi Tushar,
On Thu, 2020-11-12 at 14:09 -0800, Tushar Sugandhi wrote:
> Could you please let me know if you want us to remove this patch?
As neither of us are convinced this is necessary, please drop it.
Mimi
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index c1acf88e1b5d..4a35db010d91 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -228,6 +228,35 @@ extern const char *const func_tokens[]; struct modsig; +#define __ima_supported_kernel_data_sources(source) \ + source(MIN_SOURCE, min_source) \ + source(MAX_SOURCE, max_source) + +#define __ima_enum_stringify(ENUM, str) (#str), + +enum ima_supported_kernel_data_sources { + __ima_supported_kernel_data_sources(__ima_hook_enumify) +}; + +static const char * const ima_supported_kernel_data_sources_str[] = { + __ima_supported_kernel_data_sources(__ima_enum_stringify) +}; + +static inline bool ima_kernel_data_source_is_supported(const char *source) +{ + int i; + + if (!source) + return false; + + for (i = MIN_SOURCE + 1; i < MAX_SOURCE; i++) { + if (!strcmp(ima_supported_kernel_data_sources_str[i], source)) + return true; + } + + return false; +} + #ifdef CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS /* * To track keys that need to be measured. diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 6e1b11dcba53..091c2e58f3c7 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -937,6 +937,12 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) * A given kernel subsystem (event_data_source) may send * data (buf) to be measured when the data or the subsystem state changes. * The state/data change can be described by event_name. + * Before the first use of this function by a given kernel subsystem, + * the subsystem name (event_data_source) must be added to the + * compile-time list of data sources being measured - + * i.e. __ima_supported_kernel_data_sources. + * Otherwise, IMA will not measure any data for that event_data_source + * at run-time. * Examples of critical data (buf) could be kernel in-memory r/o structures, * hash of the memory structures, or data that represents subsystem * state change. @@ -954,6 +960,12 @@ void ima_measure_critical_data(const char *event_data_source, return; } + if (!ima_kernel_data_source_is_supported(event_data_source)) { + pr_err("measuring data source %s is not permitted", + event_data_source); + return; + } + process_buffer_measurement(NULL, buf, buf_len, event_name, CRITICAL_DATA, 0, event_data_source, measure_buf_hash);
Currently, IMA does not restrict random data sources from measuring their data using ima_measure_critical_data(). Any kernel data source can call the function, and it's data will get measured as long as the input event_data_source is part of the IMA policy - CRITICAL_DATA+data_sources. To ensure that only data from supported sources are measured, the kernel subsystem name needs to be added to a compile-time list of supported sources (an "allowed list of components"). IMA then validates the input parameter - "event_data_source" passed to ima_measure_critical_data() against this allowed list at run-time. This compile-time list must be updated when kernel subsystems are updated to measure their data using IMA. Provide an infrastructure for kernel data sources to be added to IMA's supported data sources list at compile-time. Update ima_measure_critical_data() to validate, at run-time, that the data source is supported before measuring the data coming from that source. Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> --- security/integrity/ima/ima.h | 29 +++++++++++++++++++++++++++++ security/integrity/ima/ima_main.c | 12 ++++++++++++ 2 files changed, 41 insertions(+)