| Message ID | 20190510223744.10154-2-prsriva02@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Kexec cmdline bufffer measure | expand |
On Fri, 2019-05-10 at 15:37 -0700, Prakhar Srivastava wrote: > +/* > + * process_buffer_measurement - Measure the buffer passed to ima log. "passed to ima log" is unnecessary. > + * (Instead of using the file hash use the buffer hash). This comment, if needed, belongs in the text description area below, not here. > + * @buf - The buffer that needs to be added to the log > + * @size - size of buffer(in bytes) > + * @eventname - event name to be used for buffer. Missing are the other fields. > + * > + * The buffer passed is added to the ima log. > + * > + * On success return 0. > + * On error cases surface errors from ima calls. Only IMA-appraise returns errors to the caller. There's no point in returning an error. > + */ > +static int process_buffer_measurement(const void *buf, int size, > + const char *eventname, const struct cred *cred, > + u32 secid) This should be "static void". > +{ > + int ret = 0; > + struct ima_template_entry *entry = NULL; > + struct integrity_iint_cache tmp_iint, *iint = &tmp_iint; > + struct ima_event_data event_data = {iint, NULL, NULL, > + NULL, 0, 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; > + > + action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr); > + if (!(action & IMA_AUDIT) && !(action & IMA_MEASURE)) > + goto out; > + > + memset(iint, 0, sizeof(*iint)); > + memset(&hash, 0, sizeof(hash)); > + > + event_data.filename = eventname; > + > + iint->ima_hash = &hash.hdr; > + iint->ima_hash->algo = ima_hash_algo; > + iint->ima_hash->length = hash_digest_size[ima_hash_algo]; > + > + ret = ima_calc_buffer_hash(buf, size, iint->ima_hash); > + if (ret < 0) > + goto out; > + > + ret = ima_alloc_init_template(&event_data, &entry); > + if (ret < 0) > + goto out; > + > + if (action & IMA_MEASURE) > + ret = ima_store_template(entry, violation, NULL, buf, pcr); > + > + if (action & IMA_AUDIT) > + ima_audit_measurement(iint, event_data.filename); The cover letter and patch description say this patch set is limited to measuring the boot command line - IMA-measurement. ima_audit_measurement() adds file hashes in the audit log, which can be used for security analytics and/or forensics. This is part of IMA- audit. The call to ima_audit_measurement() is inappropriate. Mimi > + > + if (ret < 0) { > + ima_free_template_entry(entry); > + goto out; > + } > + > +out: > + return ret; > +} > +
On Mon, May 13, 2019 at 9:56 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Fri, 2019-05-10 at 15:37 -0700, Prakhar Srivastava wrote: > > > +/* > > + * process_buffer_measurement - Measure the buffer passed to ima log. > > "passed to ima log" is unnecessary. > > > + * (Instead of using the file hash use the buffer hash). > > This comment, if needed, belongs in the text description area below, > not here. > > > + * @buf - The buffer that needs to be added to the log > > + * @size - size of buffer(in bytes) > > + * @eventname - event name to be used for buffer. > > Missing are the other fields. > > > + * > > + * The buffer passed is added to the ima log. > > + * > > + * On success return 0. > > + * On error cases surface errors from ima calls. > > Only IMA-appraise returns errors to the caller. There's no point in > returning an error. > > > > + */ > > +static int process_buffer_measurement(const void *buf, int size, > > + const char *eventname, const struct cred *cred, > > + u32 secid) > > This should be "static void". > > > +{ > > + > > + if (action & IMA_MEASURE) > > + ret = ima_store_template(entry, violation, NULL, buf, pcr); > > + > > + if (action & IMA_AUDIT) > > + ima_audit_measurement(iint, event_data.filename); > > The cover letter and patch description say this patch set is limited > to measuring the boot command line - IMA-measurement. > ima_audit_measurement() adds file hashes in the audit log, which can > be used for security analytics and/or forensics. This is part of IMA- > audit. The call to ima_audit_measurement() is inappropriate. > To clarify, in one of the previous versions you mentioned it might be helpful to add audit. I might have misunderstood you, but i will remove the audit_measurement and make other corrections. Thankyou for your feedback. - Thanks, Prakhar Srivastava Mimi
> > > +{ > > > + > > > + if (action & IMA_MEASURE) > > > + ret = ima_store_template(entry, violation, NULL, buf, pcr); > > > + > > > + if (action & IMA_AUDIT) > > > + ima_audit_measurement(iint, event_data.filename); > > > > The cover letter and patch description say this patch set is limited > > to measuring the boot command line - IMA-measurement. > > ima_audit_measurement() adds file hashes in the audit log, which can > > be used for security analytics and/or forensics. This is part of IMA- > > audit. The call to ima_audit_measurement() is inappropriate. > > > To clarify, in one of the previous versions you mentioned it > might be helpful to add audit. The original question was whether the kexec command line should ONLY be measured. That decision impacts whether a new function (process_buffer_measurement) is needed or whether you should still call process_measurement(). > I might have misunderstood you, but i will remove the > audit_measurement and make other corrections. > Thankyou for your feedback. Even if it was agreed that you were adding the ability to measure and audit the kexec boot command line, the cover letter, the patch descriptions and the patches themselves would need to reflect that. The call to ima_audit_measurement() would not be hidden like this, but included as a separate patch. Mimi
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index 74c6702de74e..62e7cd687e9c 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -29,6 +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] mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND] [[^]MAY_EXEC] fsmagic:= hex value diff --git a/include/linux/ima.h b/include/linux/ima.h index dc12fbcf484c..2e2c77280be8 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -26,6 +26,7 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id); extern int ima_post_read_file(struct file *file, void *buf, loff_t size, enum kernel_read_file_id id); extern void ima_post_path_mknod(struct dentry *dentry); +extern void ima_kexec_cmdline(const void *buf, int size); #ifdef CONFIG_IMA_KEXEC extern void ima_add_kexec_buffer(struct kimage *image); @@ -92,6 +93,7 @@ static inline void ima_post_path_mknod(struct dentry *dentry) return; } +static inline void ima_kexec_cmdline(const void *buf, int size) {} #endif /* CONFIG_IMA */ #ifndef CONFIG_IMA_KEXEC diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index d213e835c498..226a26d8de09 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -184,6 +184,7 @@ static inline unsigned long ima_hash_key(u8 *digest) hook(KEXEC_KERNEL_CHECK) \ hook(KEXEC_INITRAMFS_CHECK) \ hook(POLICY_CHECK) \ + hook(KEXEC_CMDLINE) \ hook(MAX_CHECK) #define __ima_hook_enumify(ENUM) ENUM, diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index c7505fb122d4..800d965232e5 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -169,6 +169,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 * mask: contains the permission mask * fsmagic: hex value * diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 357edd140c09..1d186bda25fe 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -576,6 +576,90 @@ int ima_load_data(enum kernel_load_data_id id) return 0; } +/* + * process_buffer_measurement - Measure the buffer passed to ima log. + * (Instead of using the file hash use the buffer hash). + * @buf - The buffer that needs to be added to the log + * @size - size of buffer(in bytes) + * @eventname - event name to be used for buffer. + * + * The buffer passed is added to the ima log. + * + * On success return 0. + * On error cases surface errors from ima calls. + */ +static int process_buffer_measurement(const void *buf, int size, + const char *eventname, const struct cred *cred, + u32 secid) +{ + int ret = 0; + struct ima_template_entry *entry = NULL; + struct integrity_iint_cache tmp_iint, *iint = &tmp_iint; + struct ima_event_data event_data = {iint, NULL, NULL, + NULL, 0, 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; + + action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr); + if (!(action & IMA_AUDIT) && !(action & IMA_MEASURE)) + goto out; + + memset(iint, 0, sizeof(*iint)); + memset(&hash, 0, sizeof(hash)); + + event_data.filename = eventname; + + iint->ima_hash = &hash.hdr; + iint->ima_hash->algo = ima_hash_algo; + iint->ima_hash->length = hash_digest_size[ima_hash_algo]; + + ret = ima_calc_buffer_hash(buf, size, iint->ima_hash); + if (ret < 0) + goto out; + + ret = ima_alloc_init_template(&event_data, &entry); + if (ret < 0) + goto out; + + if (action & IMA_MEASURE) + ret = ima_store_template(entry, violation, NULL, buf, pcr); + + if (action & IMA_AUDIT) + ima_audit_measurement(iint, event_data.filename); + + if (ret < 0) { + ima_free_template_entry(entry); + goto out; + } + +out: + return ret; +} + +/** + * ima_kexec_cmdline - based on policy, store kexec cmdline args + * @buf: pointer to buffer + * @size: size of buffer + * + * Buffers can only be measured, not appraised. + */ +void ima_kexec_cmdline(const void *buf, int size) +{ + u32 secid; + + if (buf && size != 0) { + security_task_getsecid(current, &secid); + process_buffer_measurement(buf, size, "kexec-cmdline", + current_cred(), secid); + } +} + + static int __init init_ima(void) { int error; diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index e0cc323f948f..413e5921b248 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -291,6 +291,13 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode, { int i; + /* only incase of KEXEC_CMDLINE, inode is NULL */ + if (func == KEXEC_CMDLINE) { + if ((rule->flags & IMA_FUNC) && + (rule->func == func) && (!inode)) + return true; + return false; + } if ((rule->flags & IMA_FUNC) && (rule->func != func && func != POST_SETATTR)) return false; @@ -869,6 +876,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) entry->func = KEXEC_INITRAMFS_CHECK; else if (strcmp(args[0].from, "POLICY_CHECK") == 0) entry->func = POLICY_CHECK; + else if (strcmp(args[0].from, "KEXEC_CMDLINE") == 0) + entry->func = KEXEC_CMDLINE; else result = -EINVAL; if (!result)