diff mbox series

[V10,2/3] IMA: Define a new template field buf

Message ID 20190624062331.388-3-prsriva02@gmail.com (mailing list archive)
State New, archived
Headers show
Series Add support for measuring the boot command line during kexec_file_load | expand

Commit Message

Prakhar Srivastava June 24, 2019, 6:23 a.m. UTC 
A buffer(kexec boot command line arguments) measured into IMA
measuremnt list cannot be appraised, without already being
aware of the buffer contents. Since hashes are non-reversible,
raw buffer is needed for validation or regenerating hash for
appraisal/attestation.

Add support to store/read the buffer contents in HEX.
The kexec cmdline hash is stored in the "d-ng" field of the
template data,it can be verified using
sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements |
  grep  kexec-cmdline | cut -d' ' -f 6 | xxd -r -p | sha256sum

- Add two new fields to ima_event_data to hold the buf and
buf_len [Suggested by Roberto]
- Add a new temaplte field 'buf' to be used to store/read
the buffer data.[Suggested by Mimi]
- Updated process_buffer_meaurement to add the buffer to
ima_event_data. process_buffer_measurement added in
"Define a new IMA hook to measure the boot command line
 arguments"
- Add a new template policy name ima-buf to represent
'd-ng|n-ng|buf'

Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
---
 Documentation/security/IMA-templates.rst  |  7 ++++---
 security/integrity/ima/ima.h              |  2 ++
 security/integrity/ima/ima_main.c         |  4 +++-
 security/integrity/ima/ima_template.c     |  3 +++
 security/integrity/ima/ima_template_lib.c | 21 +++++++++++++++++++++
 security/integrity/ima/ima_template_lib.h |  4 ++++
 6 files changed, 37 insertions(+), 4 deletions(-)

Comments

Thiago Jung Bauermann June 24, 2019, 10:03 p.m. UTC | #1 
Hello Prakhar,

Prakhar Srivastava <prsriva02@gmail.com> writes:

> diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
> index 00dd5a434689..a01a17e5c581 100644
> --- a/security/integrity/ima/ima_template.c
> +++ b/security/integrity/ima/ima_template.c
> @@ -26,6 +26,7 @@ static struct ima_template_desc builtin_templates[] = {
>  	{.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
>  	{.name = "ima-ng", .fmt = "d-ng|n-ng"},
>  	{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
> +	{.name = "ima-buf", .fmt = "d-ng|n-ng|buf"},
>  	{.name = "", .fmt = ""},	/* placeholder for a custom format */
>  };
>
> @@ -43,6 +44,8 @@ static const struct ima_template_field supported_fields[] = {
>  	 .field_show = ima_show_template_string},
>  	{.field_id = "sig", .field_init = ima_eventsig_init,
>  	 .field_show = ima_show_template_sig},
> +	{.field_id = "buf", .field_init = ima_eventbuf_init,
> +	 .field_show = ima_show_template_buf},
>  };
>  #define MAX_TEMPLATE_NAME_LEN 15

Currently, MAX_TEMPLATE_NAME_LEN is the length of a template that
contains all valid fields. It may make sense to increase it since
there's a new field being added.

I suggest using a sizeof() to show where the number comes from (and
which can be visually shown to be correct):

#define MAX_TEMPLATE_NAME_LEN sizeof("d|n|d-ng|n-ng|sig|buf")

The sizeof() is calculated at compile time.

--
Thiago Jung Bauermann
IBM Linux Technology Center
Mimi Zohar June 27, 2019, 3:08 p.m. UTC | #2 
On Mon, 2019-06-24 at 19:03 -0300, Thiago Jung Bauermann wrote:
> Hello Prakhar,
> 
> Prakhar Srivastava <prsriva02@gmail.com> writes:
> 
> > diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
> > index 00dd5a434689..a01a17e5c581 100644
> > --- a/security/integrity/ima/ima_template.c
> > +++ b/security/integrity/ima/ima_template.c
> > @@ -26,6 +26,7 @@ static struct ima_template_desc builtin_templates[] = {
> >  	{.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
> >  	{.name = "ima-ng", .fmt = "d-ng|n-ng"},
> >  	{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
> > +	{.name = "ima-buf", .fmt = "d-ng|n-ng|buf"},
> >  	{.name = "", .fmt = ""},	/* placeholder for a custom format */
> >  };
> >
> > @@ -43,6 +44,8 @@ static const struct ima_template_field supported_fields[] = {
> >  	 .field_show = ima_show_template_string},
> >  	{.field_id = "sig", .field_init = ima_eventsig_init,
> >  	 .field_show = ima_show_template_sig},
> > +	{.field_id = "buf", .field_init = ima_eventbuf_init,
> > +	 .field_show = ima_show_template_buf},
> >  };
> >  #define MAX_TEMPLATE_NAME_LEN 15
> 
> Currently, MAX_TEMPLATE_NAME_LEN is the length of a template that
> contains all valid fields. It may make sense to increase it since
> there's a new field being added.
> 
> I suggest using a sizeof() to show where the number comes from (and
> which can be visually shown to be correct):
> 
> #define MAX_TEMPLATE_NAME_LEN sizeof("d|n|d-ng|n-ng|sig|buf")
> 
> The sizeof() is calculated at compile time.

MAX_TEMPLATE_NAME_LEN is used when restoring measurements carried over
from a kexec.  'd' and 'd-ng' should not both be defined in the
template description, nor should 'n' and 'n-ng'.  Even without the
duplication, the MAX_TEPLATE_NAME_LEN is greater than the current 15.

Thiago, could you address this as a separate patch?

thanks!

Mimi
Thiago Jung Bauermann June 27, 2019, 11:27 p.m. UTC | #3 
Mimi Zohar <zohar@linux.ibm.com> writes:

> On Mon, 2019-06-24 at 19:03 -0300, Thiago Jung Bauermann wrote:
>> Hello Prakhar,
>> 
>> Prakhar Srivastava <prsriva02@gmail.com> writes:
>> 
>> > diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
>> > index 00dd5a434689..a01a17e5c581 100644
>> > --- a/security/integrity/ima/ima_template.c
>> > +++ b/security/integrity/ima/ima_template.c
>> > @@ -26,6 +26,7 @@ static struct ima_template_desc builtin_templates[] = {
>> >  	{.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
>> >  	{.name = "ima-ng", .fmt = "d-ng|n-ng"},
>> >  	{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
>> > +	{.name = "ima-buf", .fmt = "d-ng|n-ng|buf"},
>> >  	{.name = "", .fmt = ""},	/* placeholder for a custom format */
>> >  };
>> >
>> > @@ -43,6 +44,8 @@ static const struct ima_template_field supported_fields[] = {
>> >  	 .field_show = ima_show_template_string},
>> >  	{.field_id = "sig", .field_init = ima_eventsig_init,
>> >  	 .field_show = ima_show_template_sig},
>> > +	{.field_id = "buf", .field_init = ima_eventbuf_init,
>> > +	 .field_show = ima_show_template_buf},
>> >  };
>> >  #define MAX_TEMPLATE_NAME_LEN 15
>> 
>> Currently, MAX_TEMPLATE_NAME_LEN is the length of a template that
>> contains all valid fields. It may make sense to increase it since
>> there's a new field being added.
>> 
>> I suggest using a sizeof() to show where the number comes from (and
>> which can be visually shown to be correct):
>> 
>> #define MAX_TEMPLATE_NAME_LEN sizeof("d|n|d-ng|n-ng|sig|buf")
>> 
>> The sizeof() is calculated at compile time.
>
> MAX_TEMPLATE_NAME_LEN is used when restoring measurements carried over
> from a kexec.  'd' and 'd-ng' should not both be defined in the
> template description, nor should 'n' and 'n-ng'.

Ah, makes sense. Thanks for that information. 

> Even without the
> duplication, the MAX_TEPLATE_NAME_LEN is greater than the current 15.
>
> Thiago, could you address this as a separate patch?

Yes, I just sent a patch.
diff mbox series

Patch

diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
index 2cd0e273cc9a..3d1cca287aa4 100644
--- a/Documentation/security/IMA-templates.rst
+++ b/Documentation/security/IMA-templates.rst
@@ -69,15 +69,16 @@  descriptors by adding their identifier to the format string
    algorithm (field format: [<hash algo>:]digest, where the digest
    prefix is shown only if the hash algorithm is not SHA1 or MD5);
  - 'n-ng': the name of the event, without size limitations;
- - 'sig': the file signature.
+ - 'sig': the file signature;
+ - 'buf': the buffer data that was used to generate the hash without size limitations;
 
 
 Below, there is the list of defined template descriptors:
 
  - "ima": its format is ``d|n``;
  - "ima-ng" (default): its format is ``d-ng|n-ng``;
- - "ima-sig": its format is ``d-ng|n-ng|sig``.
-
+ - "ima-sig": its format is ``d-ng|n-ng|sig``;
+ - "ima-buf": its format is ``d-ng|n-ng|buf``;
 
 
 Use
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index bdca641f9e51..6aa28ab53d27 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -65,6 +65,8 @@  struct ima_event_data {
 	struct evm_ima_xattr_data *xattr_value;
 	int xattr_len;
 	const char *violation;
+	const void *buf;
+	int buf_len;
 };
 
 /* IMA template field data definition */
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2507bee1b762..317c4b6f2c18 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -627,7 +627,9 @@  static void process_buffer_measurement(const void *buf, int size,
 	struct ima_template_entry *entry = NULL;
 	struct integrity_iint_cache iint = {};
 	struct ima_event_data event_data = {.iint = &iint,
-					    .filename = eventname};
+					    .filename = eventname,
+					    .buf = buf,
+					    .buf_len = size};
 	struct ima_template_desc *template_desc = NULL;
 	struct {
 		struct ima_digest_data hdr;
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 00dd5a434689..a01a17e5c581 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -26,6 +26,7 @@  static struct ima_template_desc builtin_templates[] = {
 	{.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
 	{.name = "ima-ng", .fmt = "d-ng|n-ng"},
 	{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
+	{.name = "ima-buf", .fmt = "d-ng|n-ng|buf"},
 	{.name = "", .fmt = ""},	/* placeholder for a custom format */
 };
 
@@ -43,6 +44,8 @@  static const struct ima_template_field supported_fields[] = {
 	 .field_show = ima_show_template_string},
 	{.field_id = "sig", .field_init = ima_eventsig_init,
 	 .field_show = ima_show_template_sig},
+	{.field_id = "buf", .field_init = ima_eventbuf_init,
+	 .field_show = ima_show_template_buf},
 };
 #define MAX_TEMPLATE_NAME_LEN 15
 
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 513b457ae900..baf4de45c5aa 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -162,6 +162,12 @@  void ima_show_template_sig(struct seq_file *m, enum ima_show_type show,
 	ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data);
 }
 
+void ima_show_template_buf(struct seq_file *m, enum ima_show_type show,
+			   struct ima_field_data *field_data)
+{
+	ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data);
+}
+
 /**
  * ima_parse_buf() - Parses lengths and data from an input buffer
  * @bufstartp:       Buffer start address.
@@ -389,3 +395,18 @@  int ima_eventsig_init(struct ima_event_data *event_data,
 	return ima_write_template_field_data(xattr_value, event_data->xattr_len,
 					     DATA_FMT_HEX, field_data);
 }
+
+/*
+ *  ima_eventbuf_init - include the buffer(kexec-cmldine) as part of the
+ *  template data.
+ */
+int ima_eventbuf_init(struct ima_event_data *event_data,
+		      struct ima_field_data *field_data)
+{
+	if ((!event_data->buf) || (event_data->buf_len == 0))
+		return 0;
+
+	return ima_write_template_field_data(event_data->buf,
+					     event_data->buf_len, DATA_FMT_HEX,
+					     field_data);
+}
diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
index 6a3d8b831deb..12f1a8578b31 100644
--- a/security/integrity/ima/ima_template_lib.h
+++ b/security/integrity/ima/ima_template_lib.h
@@ -29,6 +29,8 @@  void ima_show_template_string(struct seq_file *m, enum ima_show_type show,
 			      struct ima_field_data *field_data);
 void ima_show_template_sig(struct seq_file *m, enum ima_show_type show,
 			   struct ima_field_data *field_data);
+void ima_show_template_buf(struct seq_file *m, enum ima_show_type show,
+			   struct ima_field_data *field_data);
 int ima_parse_buf(void *bufstartp, void *bufendp, void **bufcurp,
 		  int maxfields, struct ima_field_data *fields, int *curfields,
 		  unsigned long *len_mask, int enforce_mask, char *bufname);
@@ -42,4 +44,6 @@  int ima_eventname_ng_init(struct ima_event_data *event_data,
 			  struct ima_field_data *field_data);
 int ima_eventsig_init(struct ima_event_data *event_data,
 		      struct ima_field_data *field_data);
+int ima_eventbuf_init(struct ima_event_data *event_data,
+		      struct ima_field_data *field_data);
 #endif /* __LINUX_IMA_TEMPLATE_LIB_H */