LinuxLists.cc - [PATCH v2 1/5 RFC] added ima hook for buffer, being enabled as a policy

2019-04-24 00:17:08

Subject: [PATCH v2 1/5 RFC] added ima hook for buffer, being enabled as a policy

From: Prakhar Srivastava <[email protected]>

Signed-off-by: Prakhar Srivastava <[email protected]>
---
Currently for soft reboot(kexec_file_load) the kernel file and
signature is measured by IMA. The cmdline args used to load the kernel
is not measured.
The boot aggregate that gets calculated will have no change since the
EFI loader has not been triggered.
Adding the kexec cmdline args measure and kernel version will add some
attestable criteria.

This adds a new ima hook ima_buffer_check and a policy entry BUFFER_CHECK.
This enables buffer has measurements into ima log

Documentation/ABI/testing/ima_policy | 1 +
include/linux/ima.h | 13 +++-
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_main.c | 95 ++++++++++++++++++++++++++++
security/integrity/ima/ima_policy.c | 14 +++-
5 files changed, 122 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index bb0f9a135e21..676088c7ab26 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -28,6 +28,7 @@ Description:
base: func:= [BPRM_CHECK][MMAP_CHECK][FILE_CHECK][MODULE_CHECK]
[FIRMWARE_CHECK]
[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
+ [BUFFER_CHECK]
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 7f6952f8d6aa..733d0cb9dedc 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -14,6 +14,12 @@
#include <linux/kexec.h>
struct linux_binprm;

+enum __buffer_id {
+ KERNEL_VERSION,
+ KEXEC_CMDLINE,
+ MAX_BUFFER_ID = KEXEC_CMDLINE
+} buffer_id;
+
#ifdef CONFIG_IMA
extern int ima_bprm_check(struct linux_binprm *bprm);
extern int ima_file_check(struct file *file, int mask, int opened);
@@ -23,7 +29,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_buffer_check(const void *buff, int size, enum buffer_id id);
#ifdef CONFIG_IMA_KEXEC
extern void ima_add_kexec_buffer(struct kimage *image);
#endif
@@ -65,6 +71,11 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
return;
}

+static inline void ima_buffer_check(const void *buff, int size,
+ enum buffer_id id)
+{
+ return;
+}
#endif /* CONFIG_IMA */

#ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b563fbd4d122..b71f2f6f7421 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -181,6 +181,7 @@ enum ima_hooks {
FIRMWARE_CHECK,
KEXEC_KERNEL_CHECK,
KEXEC_INITRAMFS_CHECK,
+ BUFFER_CHECK,
POLICY_CHECK,
MAX_CHECK
};
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2aebb7984437..6408cadaadbb 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -155,6 +155,84 @@ void ima_file_free(struct file *file)
ima_check_last_writer(iint, inode, file);
}

+/*
+ * process_buffer_measurement - Measure the buffer passed to ima log.
+ * (Instead of using the file hash the buffer hash is used).
+ * @buff - The buffer that needs to be added to the log
+ * @size - size of buffer(in bytes)
+ * @id - buffer id, this is differentiator for the various buffers
+ * that can be measured.
+ *
+ * The buffer passed is added to the ima logs.
+ * If the sig template is used, then the sig field contains the buffer.
+ *
+ * On success return 0.
+ * On error cases surface errors from ima calls.
+ */
+static int process_buffer_measurement(const void *buff, int size,
+ enum buffer_id id)
+{
+ int ret = -EINVAL;
+ 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;
+ char *name = NULL;
+ int violation = 0;
+ int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
+
+ if (!buff || size == 0)
+ goto err_out;
+
+ if (ima_get_action(NULL, 0, BUFFER_CHECK, &pcr) != IMA_MEASURE)
+ goto err_out;
+
+ switch (buffer_id) {
+ case KERNEL_VERSION:
+ name = "Kernel-version";
+ break;
+ case KEXEC_CMDLINE:
+ name = "Kexec-cmdline";
+ break;
+ default:
+ goto err_out;
+ }
+
+ memset(iint, 0, sizeof(*iint));
+ memset(&hash, 0, sizeof(hash));
+
+ event_data.filename = name;
+
+ 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(buff, size, iint->ima_hash);
+ if (ret < 0)
+ goto err_out;
+
+ ret = ima_alloc_init_template(&event_data, &entry);
+ if (ret < 0)
+ goto err_out;
+
+ ret = ima_store_template(entry, violation, NULL,
+ buff, pcr);
+ if (ret < 0) {
+ ima_free_template_entry(entry);
+ goto err_out;
+ }
+
+ return 0;
+
+err_out:
+ pr_err("Error in adding buffer measure: %d\n", ret);
+ return ret;
+}
+
static int process_measurement(struct file *file, char *buf, loff_t size,
int mask, enum ima_hooks func, int opened)
{
@@ -370,6 +448,23 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
return 0;
}

+/**
+ * ima_buffer_check - based on policy, collect & store buffer measurement
+ * @buf: pointer to buffer
+ * @size: size of buffer
+ * @buffer_id: caller identifier
+ *
+ * Buffers can only be measured, not appraised. The buffer identifier
+ * is used as the measurement list entry name (eg. boot_cmdline).
+ */
+void ima_buffer_check(const void *buf, int size, enum buffer_id id)
+{
+ if (buf && size != 0)
+ process_buffer_measurement(buf, size, id);
+
+ return;
+}
+
static int read_idmap[READING_MAX_ID] = {
[READING_FIRMWARE] = FIRMWARE_CHECK,
[READING_MODULE] = MODULE_CHECK,
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 3ab1067db624..cefe1a188f31 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -231,6 +231,12 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
const struct cred *cred = current_cred();
int i;

+ // Incase of BUFFER_CHECK, Inode is NULL
+ if (!inode) {
+ if ((rule->flags & IMA_FUNC) && (rule->func == func))
+ return true;
+ return false;
+ }
if ((rule->flags & IMA_FUNC) &&
(rule->func != func && func != POST_SETATTR))
return false;
@@ -665,6 +671,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
else if (strcmp(args[0].from, "KEXEC_INITRAMFS_CHECK")
== 0)
entry->func = KEXEC_INITRAMFS_CHECK;
+ else if (strcmp(args[0].from, "BUFFER_CHECK") == 0)
+ entry->func = BUFFER_CHECK;
else if (strcmp(args[0].from, "POLICY_CHECK") == 0)
entry->func = POLICY_CHECK;
else
@@ -944,7 +952,7 @@ enum {
func_file = 0, func_mmap, func_bprm,
func_module, func_firmware, func_post,
func_kexec_kernel, func_kexec_initramfs,
- func_policy
+ func_buffer, func_policy
};

static char *func_tokens[] = {
@@ -956,6 +964,7 @@ static char *func_tokens[] = {
"POST_SETATTR",
"KEXEC_KERNEL_CHECK",
"KEXEC_INITRAMFS_CHECK",
+ "BUFFER_CHECK",
"POLICY_CHECK"
};

@@ -1027,6 +1036,9 @@ static void policy_func_show(struct seq_file *m, enum ima_hooks func)
case KEXEC_INITRAMFS_CHECK:
seq_printf(m, pt(Opt_func), ft(func_kexec_initramfs));
break;
+ case BUFFER_CHECK:
+ seq_printf(m, pt(Opt_func), ft(func_buffer));
+ break;
case POLICY_CHECK:
seq_printf(m, pt(Opt_func), ft(func_policy));
break;
--
2.17.1

2019-04-24 00:17:14

by Prakhar Srivastava

[permalink] [raw]

Subject: [PATCH v2 5/5 RFC] add the buffer to the event data in ima free entry data if store_template failed added check in templates for buffer

2019-04-24 00:17:15

by Prakhar Srivastava

[permalink] [raw]

Subject: [PATCH v2 2/5 RFC] use event name instead of enum to make the call generic

2019-04-24 00:18:34

by Prakhar Srivastava

[permalink] [raw]

Subject: [PATCH v2 4/5 RFC] added a buffer_check LSM hook

2019-04-24 00:19:15

by Prakhar Srivastava

[permalink] [raw]

Subject: [PATCH v2 3/5 RFC] since cmdline args can be same for multiple kexec, log entry hash will collide. Prepend the kernel file name to the cmdline args to distinguish between cmdline args passed to subsequent kexec calls

2019-04-25 13:40:20

by Nayna Jain

[permalink] [raw]

Subject: Re: [PATCH v2 2/5 RFC] use event name instead of enum to make the call generic

On 04/23/2019 08:15 PM, Prakhar Srivastava wrote:
> From: Prakhar Srivastava <[email protected]>
>
> Signed-off-by: Prakhar Srivastava <[email protected]>
> ---
>
> Currently for soft reboot(kexec_file_load) the kernel file and
> signature is measured by IMA. The cmdline args used to load the kernel
> is not measured.
> The boot aggregate that gets calculated will have no change since the
> EFI loader has not been triggered.
> Adding the kexec cmdline args measure and kernel version will add some
> attestable criteria.
>

Any reason for including the whole commit message after "---"

Anything after "---" is not included in the patch description when patch
is applied.

This comment applies to all the patches in this patchset.

> remove enums to control type of buffers entries, instead pass the event name to be used.

Is the last statement meant to be a Changelog from v1-> v2 ? Only the
changelog has to be after "---"

Also, If posting more than one patch, it is preferrable to add a
cover-letter.

> include/linux/ima.h | 10 ++--------
> kernel/kexec_file.c | 3 +++
> security/integrity/ima/ima.h | 2 +-
> security/integrity/ima/ima_main.c | 30 ++++++++++--------------------
> 4 files changed, 16 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 733d0cb9dedc..5e41507c57e5 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -14,12 +14,6 @@
> #include <linux/kexec.h>
> struct linux_binprm;
>
> -enum __buffer_id {
> - KERNEL_VERSION,
> - KEXEC_CMDLINE,
> - MAX_BUFFER_ID = KEXEC_CMDLINE
> -} buffer_id;
> -

Is the v2 version created on top of the v1 version that was posted ?

The v2 version has to be on top of the HEAD of the repository itself,
and not on the v1 version. Only the final reviewed and tested version
makes to the upstream.

Btw, which repository and its branch are you using ?

Thanks & Regards,
- Nayna

> #ifdef CONFIG_IMA
> extern int ima_bprm_check(struct linux_binprm *bprm);
> extern int ima_file_check(struct file *file, int mask, int opened);
> @@ -29,7 +23,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_buffer_check(const void *buff, int size, enum buffer_id id);
> +extern void ima_buffer_check(const void *buff, int size, char *eventname);
> #ifdef CONFIG_IMA_KEXEC
> extern void ima_add_kexec_buffer(struct kimage *image);
> #endif
> @@ -72,7 +66,7 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
> }
>
> static inline void ima_buffer_check(const void *buff, int size,
> - enum buffer_id id)
> + char *eventname)
> {
> return;
> }
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index b118735fea9d..2a5234eb4b28 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -182,6 +182,9 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
> ret = -EINVAL;
> goto out;
> }
> +
> + ima_buffer_check(image->cmdline_buf, cmdline_len - 1,
> + "kexec_cmdline");
> }
>
> /* Call arch image load handlers */
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index b71f2f6f7421..fcade3c103ed 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -181,8 +181,8 @@ enum ima_hooks {
> FIRMWARE_CHECK,
> KEXEC_KERNEL_CHECK,
> KEXEC_INITRAMFS_CHECK,
> - BUFFER_CHECK,
> POLICY_CHECK,
> + BUFFER_CHECK,
> MAX_CHECK
> };
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 6408cadaadbb..da82c705a5ed 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -160,8 +160,7 @@ void ima_file_free(struct file *file)
> * (Instead of using the file hash the buffer hash is used).
> * @buff - The buffer that needs to be added to the log
> * @size - size of buffer(in bytes)
> - * @id - buffer id, this is differentiator for the various buffers
> - * that can be measured.
> + * @id - eventname, event name to be used for buffer measurement.
> *
> * The buffer passed is added to the ima logs.
> * If the sig template is used, then the sig field contains the buffer.
> @@ -170,7 +169,7 @@ void ima_file_free(struct file *file)
> * On error cases surface errors from ima calls.
> */
> static int process_buffer_measurement(const void *buff, int size,
> - enum buffer_id id)
> + char *eventname)
> {
> int ret = -EINVAL;
> struct ima_template_entry *entry = NULL;
> @@ -185,23 +184,13 @@ static int process_buffer_measurement(const void *buff, int size,
> int violation = 0;
> int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
>
> - if (!buff || size == 0)
> + if (!buff || size == 0 || !eventname)
> goto err_out;
>
> if (ima_get_action(NULL, 0, BUFFER_CHECK, &pcr) != IMA_MEASURE)
> goto err_out;
>
> - switch (buffer_id) {
> - case KERNEL_VERSION:
> - name = "Kernel-version";
> - break;
> - case KEXEC_CMDLINE:
> - name = "Kexec-cmdline";
> - break;
> - default:
> - goto err_out;
> - }
> -
> + name = eventname;
> memset(iint, 0, sizeof(*iint));
> memset(&hash, 0, sizeof(hash));
>
> @@ -452,15 +441,16 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> * ima_buffer_check - based on policy, collect & store buffer measurement
> * @buf: pointer to buffer
> * @size: size of buffer
> - * @buffer_id: caller identifier
> + * @eventname: caller identifier
> *
> * Buffers can only be measured, not appraised. The buffer identifier
> - * is used as the measurement list entry name (eg. boot_cmdline).
> + * is used as the measurement list entry name (eg. boot_cmdline,
> + * kernel_version).
> */
> -void ima_buffer_check(const void *buf, int size, enum buffer_id id)
> +void ima_buffer_check(const void *buf, int size, char *eventname)
> {
> - if (buf && size != 0)
> - process_buffer_measurement(buf, size, id);
> + if (buf && size != 0 && eventname)
> + process_buffer_measurement(buf, size, eventname);
>
> return;
> }

2019-04-25 19:01:42

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH v2 2/5 RFC] use event name instead of enum to make the call generic

On Thu, Apr 25, 2019 at 10:19 AM prsriva <[email protected]> wrote:
>
> I will fix the comments and send out the patchset with a cover letter.
> Thankyou for pointing this out.

Please trim the emails you reply to rather than quoting everything.

But the real reason I'm reacting to this email is because it was
marked as spam, because your SMTP setup is incorrect, and you don't go
through the proper MS SMTP server. As a result, you don't get the
proper DKIM signature for verification of the microsoft.com email
address, and sane email clients will mark your emails as spam.

Please fix.

Linus

2019-04-25 19:09:04

by Nayna Jain

[permalink] [raw]

Subject: Re: [PATCH v2 2/5 RFC] use event name instead of enum to make the call generic

On 04/25/2019 01:19 PM, prsriva wrote:
> On 2019-04-25 4:48 a.m., Nayna wrote:
>>
>>
>> On 04/23/2019 08:15 PM, Prakhar Srivastava wrote:
>>> From: Prakhar Srivastava <[email protected]>
>>>
>>> Signed-off-by: Prakhar Srivastava <[email protected]>
>>> ---
>>>
>> The v2 version has to be on top of the HEAD of the repository itself,
>> and not on the v1 version. Only the final reviewed and tested version
>> makes to the upstream.
>>
>> Btw, which repository and its branch are you using ?
>>
> I am basing my changes off IMA branch:
> git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git

Ok. Please use either next-integrity branch or James Morris next-general
or next-testing.

Thanks & Regards,
- Nayna

2019-04-25 23:10:30

by James Morris

[permalink] [raw]

Subject: Re: [PATCH v2 2/5 RFC] use event name instead of enum to make the call generic

On Thu, 25 Apr 2019, Linus Torvalds wrote:

> On Thu, Apr 25, 2019 at 10:19 AM prsriva <[email protected]> wrote:
> >
> > I will fix the comments and send out the patchset with a cover letter.
> > Thankyou for pointing this out.
>
> Please trim the emails you reply to rather than quoting everything.
>
> But the real reason I'm reacting to this email is because it was
> marked as spam, because your SMTP setup is incorrect, and you don't go
> through the proper MS SMTP server. As a result, you don't get the
> proper DKIM signature for verification of the microsoft.com email
> address, and sane email clients will mark your emails as spam.
>
> Please fix.

It's the correct SMTP server for linux.microsoft.com.

linux.microsoft.com. 3600 IN TXT "v=spf1 ip4:13.77.154.182 -all"
linux.microsoft.com. 3600 IN TXT "v=DMARC1;p=none;pct=100;rua=mailto:[email protected]"

We don't have DKIM set up yet.

--
James Morris
<[email protected]>

2019-04-25 23:20:48

by James Bottomley

[permalink] [raw]

Subject: Re: [PATCH v2 2/5 RFC] use event name instead of enum to make the call generic

On Fri, 2019-04-26 at 08:34 +1000, James Morris wrote:
> On Thu, 25 Apr 2019, Linus Torvalds wrote:
>
> > On Thu, Apr 25, 2019 at 10:19 AM prsriva <[email protected]
> > om> wrote:
> > >
> > > I will fix the comments and send out the patchset with a cover
> > > letter. Thankyou for pointing this out.
> >
> > Please trim the emails you reply to rather than quoting everything.
> >
> > But the real reason I'm reacting to this email is because it was
> > marked as spam, because your SMTP setup is incorrect, and you don't
> > go through the proper MS SMTP server. As a result, you don't get
> > the proper DKIM signature for verification of the microsoft.com
> > email address, and sane email clients will mark your emails as
> > spam.
> >
> > Please fix.
>
> It's the correct SMTP server for linux.microsoft.com.
>
> linux.microsoft.com. 3600 IN TXT "v=spf1
> ip4:13.77.154.182 -all"
> linux.microsoft.com. 3600 IN TXT "v=DMARC
> 1;p=none;pct=100;rua=mailto:[email protected]"

That's not the correct location: DMARC records should be at the _dmarc.
subdomain. without this you'll inherit the dmarc policy of
_dmarc.microsoft.com

> We don't have DKIM set up yet.

If you advertise DMARC, you're expected to have DKIM working for spam
purposes. On the other hand, if you don't advertise DMARC, google will
probably still bin all your email as spam.

James

2019-04-25 23:23:11

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH v2 2/5 RFC] use event name instead of enum to make the call generic

On Thu, Apr 25, 2019 at 3:34 PM James Morris <[email protected]> wrote:
> >
> > Please fix.
>
> It's the correct SMTP server for linux.microsoft.com.

In that case, it seems that your SMTP server is misconfigured, and
shouldn't be used.

> linux.microsoft.com. 3600 IN TXT "v=spf1 ip4:13.77.154.182 -all"
> linux.microsoft.com. 3600 IN TXT "v=DMARC1;p=none;pct=100;rua=mailto:[email protected]"
>
> We don't have DKIM set up yet.

I get

dmarc=fail (p=REJECT sp=REJECT dis=QUARANTINE) header.from=microsoft.com

because the microsoft.com DMARC rules will be triggered before the
"linux.microsoft.com" rules are.

Linus

2019-04-26 01:40:06

by James Morris

[permalink] [raw]

Subject: Re: [PATCH v2 2/5 RFC] use event name instead of enum to make the call generic

On Thu, 25 Apr 2019, James Bottomley wrote:

> That's not the correct location: DMARC records should be at the _dmarc.
> subdomain. without this you'll inherit the dmarc policy of
> _dmarc.microsoft.com
>

Thanks.

> > We don't have DKIM set up yet.
>
> If you advertise DMARC, you're expected to have DKIM working for spam
> purposes. On the other hand, if you don't advertise DMARC, google will
> probably still bin all your email as spam.

Working on it.

--
James Morris
<[email protected]>