| Message ID | 20191024034717.70552-6-nayna@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | powerpc: Enabling IMA arch specific secure boot policies | expand |
On 10/23/19 8:47 PM, Nayna Jain wrote: Hi Nayna, > +void process_buffer_measurement(const void *buf, int size, > + const char *eventname, enum ima_hooks func, > + int pcr) > { > int ret = 0; > struct ima_template_entry *entry = NULL; > + if (func) { > + security_task_getsecid(current, &secid); > + action = ima_get_action(NULL, current_cred(), secid, 0, func, > + &pcr, &template); > + if (!(action & IMA_MEASURE)) > + return; > + } In your change set process_buffer_measurement is called with NONE for the parameter func. So ima_get_action (the above if block) will not be executed. Wouldn't it better to update ima_get_action (and related functions) to handle the ima policy (func param)? thanks, -lakshmi
On 10/24/19 10:20 AM, Lakshmi Ramasubramanian wrote: > On 10/23/19 8:47 PM, Nayna Jain wrote: > > Hi Nayna, > >> +void process_buffer_measurement(const void *buf, int size, >> + const char *eventname, enum ima_hooks func, >> + int pcr) >> { >> int ret = 0; >> struct ima_template_entry *entry = NULL; > >> + if (func) { >> + security_task_getsecid(current, &secid); >> + action = ima_get_action(NULL, current_cred(), secid, 0, func, >> + &pcr, &template); >> + if (!(action & IMA_MEASURE)) >> + return; >> + } > > In your change set process_buffer_measurement is called with NONE for > the parameter func. So ima_get_action (the above if block) will not be > executed. > > Wouldn't it better to update ima_get_action (and related functions) to > handle the ima policy (func param)? The idea is to use ima-buf template for the auxiliary measurement record. The auxiliary measurement record is an additional record to the one already created based on the existing policy. When func is passed as NONE, it represents it is an additional record. I am not sure what you mean by updating ima_get_action, it is already handling the ima policy. Thanks & Regards, - Nayna
On 10/25/2019 10:24 AM, Nayna Jain wrote: > > On 10/24/19 10:20 AM, Lakshmi Ramasubramanian wrote: >> On 10/23/19 8:47 PM, Nayna Jain wrote: >> >> Hi Nayna, >> >>> +void process_buffer_measurement(const void *buf, int size, >>> + const char *eventname, enum ima_hooks func, >>> + int pcr) >>> { >>> int ret = 0; >>> struct ima_template_entry *entry = NULL; >> >>> + if (func) { >>> + security_task_getsecid(current, &secid); >>> + action = ima_get_action(NULL, current_cred(), secid, 0, func, >>> + &pcr, &template); >>> + if (!(action & IMA_MEASURE)) >>> + return; >>> + } >> >> In your change set process_buffer_measurement is called with NONE for >> the parameter func. So ima_get_action (the above if block) will not be >> executed. >> >> Wouldn't it better to update ima_get_action (and related functions) to >> handle the ima policy (func param)? > > > The idea is to use ima-buf template for the auxiliary measurement > record. The auxiliary measurement record is an additional record to the > one already created based on the existing policy. When func is passed as > NONE, it represents it is an additional record. I am not sure what you > mean by updating ima_get_action, it is already handling the ima policy. > I was referring to using "func" in process_buffer_measurement to determine ima action. In my opinion, process_buffer_measurement should be generic. ima_get_action() should instead determine the required ima action, template, pcr, etc. based on "func" passed to it. thanks, -lakshmi
On Fri, 2019-10-25 at 10:32 -0700, Lakshmi Ramasubramanian wrote: > > On 10/25/2019 10:24 AM, Nayna Jain wrote: > > > > On 10/24/19 10:20 AM, Lakshmi Ramasubramanian wrote: > >> On 10/23/19 8:47 PM, Nayna Jain wrote: > >> > >> Hi Nayna, > >> > >>> +void process_buffer_measurement(const void *buf, int size, > >>> + const char *eventname, enum ima_hooks func, > >>> + int pcr) > >>> { > >>> int ret = 0; > >>> struct ima_template_entry *entry = NULL; > >> > >>> + if (func) { Let's comment this line. Perhaps something like /*Unnecessary for auxiliary buffer measurements */ > >>> + security_task_getsecid(current, &secid); > >>> + action = ima_get_action(NULL, current_cred(), secid, 0, func, > >>> + &pcr, &template); > >>> + if (!(action & IMA_MEASURE)) > >>> + return; > >>> + } > >> > >> In your change set process_buffer_measurement is called with NONE for > >> the parameter func. So ima_get_action (the above if block) will not be > >> executed. > >> > >> Wouldn't it better to update ima_get_action (and related functions) to > >> handle the ima policy (func param)? > > > > > > The idea is to use ima-buf template for the auxiliary measurement > > record. The auxiliary measurement record is an additional record to the > > one already created based on the existing policy. When func is passed as > > NONE, it represents it is an additional record. I am not sure what you > > mean by updating ima_get_action, it is already handling the ima policy. > > > > I was referring to using "func" in process_buffer_measurement to > determine ima action. In my opinion, process_buffer_measurement should > be generic. > > ima_get_action() should instead determine the required ima action, > template, pcr, etc. based on "func" passed to it. Nayna's original patch moved ima_get_action() into the caller, but that resulted in code duplication in each of the callers. This solution differentiates between the initial, which requires calling ima_get_action(), and auxiliary buffer measurement records. Mimi
On 10/23/19 8:47 PM, Nayna Jain wrote: Hi Nayna, > process_buffer_measurement() is limited to measuring the kexec boot > command line. This patch makes process_buffer_measurement() more > generic, allowing it to measure other types of buffer data (e.g. > blacklisted binary hashes or key hashes). Now that process_buffer_measurement() is being made generic to measure any buffer, it would be good to add a tag to indicate what type of buffer is being measured. For example, if the buffer is kexec command line the log could look like: "kexec_cmdline: <command line data>" Similarly, if the buffer is blacklisted binary hash: "blacklist hash: <data>". If the buffer is key hash: "<name of the keyring>: key data". This would greatly help the consumer of the IMA log to know the type of data represented in each IMA log entry. thanks, -lakshmi
On Wed, 2019-10-30 at 08:22 -0700, Lakshmi Ramasubramanian wrote: > On 10/23/19 8:47 PM, Nayna Jain wrote: > > Hi Nayna, > > > process_buffer_measurement() is limited to measuring the kexec boot > > command line. This patch makes process_buffer_measurement() more > > generic, allowing it to measure other types of buffer data (e.g. > > blacklisted binary hashes or key hashes). > > Now that process_buffer_measurement() is being made generic to measure > any buffer, it would be good to add a tag to indicate what type of > buffer is being measured. > > For example, if the buffer is kexec command line the log could look like: > > "kexec_cmdline: <command line data>" > > Similarly, if the buffer is blacklisted binary hash: > > "blacklist hash: <data>". > > If the buffer is key hash: > > "<name of the keyring>: key data". > > This would greatly help the consumer of the IMA log to know the type of > data represented in each IMA log entry. Both the existing kexec command line and the new blacklist buffer measurement pass that information in the eventname. The [PATCH 7/8] "ima: check against blacklisted hashes for files with modsig" patch description includes an example. Mimi
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 3689081aaf38..a65772ffa427 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -217,6 +217,9 @@ 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, + const char *eventname, enum ima_hooks func, + int pcr); void ima_audit_measurement(struct integrity_iint_cache *iint, const unsigned char *filename); int ima_alloc_init_template(struct ima_event_data *event_data, diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 60027c643ecd..fe0b704ffdeb 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -626,14 +626,14 @@ int ima_load_data(enum kernel_load_data_id id) * @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. - * @cred: a pointer to a credentials structure for user validation. - * @secid: the secid of the task to be validated. + * @func: IMA hook + * @pcr: pcr to extend the measurement * * Based on policy, the buffer is measured into the ima log. */ -static void process_buffer_measurement(const void *buf, int size, - const char *eventname, - const struct cred *cred, u32 secid) +void process_buffer_measurement(const void *buf, int size, + const char *eventname, enum ima_hooks func, + int pcr) { int ret = 0; struct ima_template_entry *entry = NULL; @@ -642,19 +642,38 @@ static void process_buffer_measurement(const void *buf, int size, .filename = eventname, .buf = buf, .buf_len = size}; - struct ima_template_desc *template_desc = NULL; + struct ima_template_desc *template = NULL; struct { struct ima_digest_data hdr; char digest[IMA_MAX_DIGEST_SIZE]; } hash = {}; int violation = 0; - int pcr = CONFIG_IMA_MEASURE_PCR_IDX; int action = 0; + u32 secid; - action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr, - &template_desc); - if (!(action & IMA_MEASURE)) - return; + if (func) { + security_task_getsecid(current, &secid); + action = ima_get_action(NULL, current_cred(), secid, 0, func, + &pcr, &template); + if (!(action & IMA_MEASURE)) + return; + } + + if (!pcr) + pcr = CONFIG_IMA_MEASURE_PCR_IDX; + + if (!template) { + template = lookup_template_desc("ima-buf"); + ret = template_desc_init_fields(template->fmt, + &(template->fields), + &(template->num_fields)); + if (ret < 0) { + pr_err("template %s init failed, result: %d\n", + (strlen(template->name) ? + template->name : template->fmt), ret); + return; + } + } iint.ima_hash = &hash.hdr; iint.ima_hash->algo = ima_hash_algo; @@ -664,7 +683,7 @@ static void process_buffer_measurement(const void *buf, int size, if (ret < 0) goto out; - ret = ima_alloc_init_template(&event_data, &entry, template_desc); + ret = ima_alloc_init_template(&event_data, &entry, template); if (ret < 0) goto out; @@ -686,13 +705,9 @@ static void process_buffer_measurement(const void *buf, int size, */ void ima_kexec_cmdline(const void *buf, int size) { - u32 secid; - - if (buf && size != 0) { - security_task_getsecid(current, &secid); + if (buf && size != 0) process_buffer_measurement(buf, size, "kexec-cmdline", - current_cred(), secid); - } + KEXEC_CMDLINE, 0); } static int __init init_ima(void)
process_buffer_measurement() is limited to measuring the kexec boot command line. This patch makes process_buffer_measurement() more generic, allowing it to measure other types of buffer data (e.g. blacklisted binary hashes or key hashes). process_buffer_measurement() may be called directly from an IMA hook or as an auxiliary measurement record. In both cases the buffer measurement is based on policy. This patch modifies the function to conditionally retrieve the policy defined PCR and template for the IMA hook case. Signed-off-by: Nayna Jain <nayna@linux.ibm.com> --- security/integrity/ima/ima.h | 3 ++ security/integrity/ima/ima_main.c | 51 ++++++++++++++++++++----------- 2 files changed, 36 insertions(+), 18 deletions(-)