[PATCH] ima: drop vla in ima_audit_measurement()

[PATCH] ima: drop vla in ima_audit_measurement()

[Tycho Andersen]

In keeping with the directive to get rid of VLAs [1], let's drop the VLA
from ima_audit_measurement(). We need to adjust the return type of
ima_audit_measurement, because now this function can fail if an allocation
fails.

[1]: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
---
 security/integrity/ima/ima.h      |  4 ++--
 security/integrity/ima/ima_api.c  | 31 +++++++++++++++++++++++--------
 security/integrity/ima/ima_main.c |  7 +++++--
 3 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d52b487ad259..8e2470f72f7f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -201,8 +201,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
 			   const unsigned char *filename,
 			   struct evm_ima_xattr_data *xattr_value,
 			   int xattr_len, int pcr);
-void ima_audit_measurement(struct integrity_iint_cache *iint,
-			   const unsigned char *filename);
+int ima_audit_measurement(struct integrity_iint_cache *iint,
+			  const unsigned char *filename);
 int ima_alloc_init_template(struct ima_event_data *event_data,
 			    struct ima_template_entry **entry);
 int ima_store_template(struct ima_template_entry *entry, int violation,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 08fe405338e1..008d3887ae00 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -304,17 +304,28 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
 		ima_free_template_entry(entry);
 }
 
-void ima_audit_measurement(struct integrity_iint_cache *iint,
-			   const unsigned char *filename)
+int ima_audit_measurement(struct integrity_iint_cache *iint,
+			  const unsigned char *filename)
 {
 	struct audit_buffer *ab;
-	char hash[(iint->ima_hash->length * 2) + 1];
+	char *hash, *algo_hash;
 	const char *algo_name = hash_algo_name[iint->ima_hash->algo];
-	char algo_hash[sizeof(hash) + strlen(algo_name) + 2];
-	int i;
+	int i, hash_len, algo_hash_len;
 
 	if (iint->flags & IMA_AUDITED)
-		return;
+		return 0;
+
+	hash_len = (iint->ima_hash->length * 2) + 1;
+	hash = kzalloc(hash_len, GFP_KERNEL);
+	if (!hash)
+		return -ENOMEM;
+
+	algo_hash_len = hash_len + strlen(algo_name) + 2;
+	algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
+	if (!algo_hash) {
+		kfree(hash);
+		return -ENOMEM;
+	}
 
 	for (i = 0; i < iint->ima_hash->length; i++)
 		hex_byte_pack(hash + (i * 2), iint->ima_hash->digest[i]);
@@ -323,18 +334,22 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
 	ab = audit_log_start(current->audit_context, GFP_KERNEL,
 			     AUDIT_INTEGRITY_RULE);
 	if (!ab)
-		return;
+		goto out;
 
 	audit_log_format(ab, "file=");
 	audit_log_untrustedstring(ab, filename);
 	audit_log_format(ab, " hash=");
-	snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
+	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
 	audit_log_untrustedstring(ab, algo_hash);
 
 	audit_log_task_info(ab, current);
 	audit_log_end(ab);
 
 	iint->flags |= IMA_AUDITED;
+out:
+	kfree(hash);
+	kfree(algo_hash);
+	return 0;
 }
 
 /*
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2cfb0c714967..356faae6f09c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -288,8 +288,11 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 					      xattr_value, xattr_len, opened);
 		inode_unlock(inode);
 	}
-	if (action & IMA_AUDIT)
-		ima_audit_measurement(iint, pathname);
+	if (action & IMA_AUDIT) {
+		rc = ima_audit_measurement(iint, pathname);
+		if (rc < 0)
+			goto out_locked;
+	}
 
 	if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO))
 		rc = 0;
-- 
2.14.1

Re: [PATCH] ima: drop vla in ima_audit_measurement()

[Andy Shevchenko]

On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> from ima_audit_measurement(). We need to adjust the return type of
> ima_audit_measurement, because now this function can fail if an allocation
> fails.



> +       algo_hash_len = hash_len + strlen(algo_name) + 2;
> +       algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);

> -       snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> +       snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);

kasprintf() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] ima: drop vla in ima_audit_measurement()

[Tycho Andersen]

On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> > from ima_audit_measurement(). We need to adjust the return type of
> > ima_audit_measurement, because now this function can fail if an allocation
> > fails.
> 
> 
> 
> > +       algo_hash_len = hash_len + strlen(algo_name) + 2;
> > +       algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
> 
> > -       snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> > +       snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> 
> kasprintf() ?

Sure, in fact I think we could just do:

-	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
-	audit_log_untrustedstring(ab, algo_hash);
+	audit_log_untrustedstring(ab, algo_name);
+	audit_log_format(ab, ":");
+	audit_log_untrustedstring(ab, hash);

and get rid of the allocation entirely. I'll test and make sure it
works and then re-send.

Cheers,

Tycho

Re: [PATCH] ima: drop vla in ima_audit_measurement()

[Mimi Zohar]

On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote:
> On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > > In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> > > from ima_audit_measurement(). We need to adjust the return type of
> > > ima_audit_measurement, because now this function can fail if an allocation
> > > fails.
> > 
> > 
> > 
> > > +       algo_hash_len = hash_len + strlen(algo_name) + 2;
> > > +       algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
> > 
> > > -       snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> > > +       snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > 
> > kasprintf() ?
> 
> Sure, in fact I think we could just do:
> 
> -	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> -	audit_log_untrustedstring(ab, algo_hash);
> +	audit_log_untrustedstring(ab, algo_name);
> +	audit_log_format(ab, ":");
> +	audit_log_untrustedstring(ab, hash);
> 
> and get rid of the allocation entirely. I'll test and make sure it
> works and then re-send.

The hash algorithm name is an enumeration that comes from the kernel.
 It's defined in crypto/hash_info.c: hash_algo_name.  Why do we need
to use audit_log_untrustedstring()?

Mimi

Re: [PATCH] ima: drop vla in ima_audit_measurement()

[Mimi Zohar]

On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote:
> On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > > In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> > > from ima_audit_measurement(). We need to adjust the return type of
> > > ima_audit_measurement, because now this function can fail if an allocation
> > > fails.
> > 
> > 
> > 
> > > +       algo_hash_len = hash_len + strlen(algo_name) + 2;
> > > +       algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
> > 
> > > -       snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> > > +       snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > 
> > kasprintf() ?
> 
> Sure, in fact I think we could just do:
> 
> -	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> -	audit_log_untrustedstring(ab, algo_hash);
> +	audit_log_untrustedstring(ab, algo_name);
> +	audit_log_format(ab, ":");
> +	audit_log_untrustedstring(ab, hash);
> 
> and get rid of the allocation entirely. I'll test and make sure it
> works and then re-send.

The hash algorithm name is an enumeration that comes from the kernel.
 It's defined in crypto/hash_info.c: hash_algo_name.  Why do we need
to use audit_log_untrustedstring()?

Mimi

Re: [PATCH] ima: drop vla in ima_audit_measurement()

[Tycho Andersen]

On Thu, Mar 08, 2018 at 01:50:30PM -0500, Mimi Zohar wrote:
> On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote:
> > On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > > > In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> > > > from ima_audit_measurement(). We need to adjust the return type of
> > > > ima_audit_measurement, because now this function can fail if an allocation
> > > > fails.
> > > 
> > > 
> > > 
> > > > +       algo_hash_len = hash_len + strlen(algo_name) + 2;
> > > > +       algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
> > > 
> > > > -       snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> > > > +       snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > 
> > > kasprintf() ?
> > 
> > Sure, in fact I think we could just do:
> > 
> > -	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > -	audit_log_untrustedstring(ab, algo_hash);
> > +	audit_log_untrustedstring(ab, algo_name);
> > +	audit_log_format(ab, ":");
> > +	audit_log_untrustedstring(ab, hash);
> > 
> > and get rid of the allocation entirely. I'll test and make sure it
> > works and then re-send.
> 
> The hash algorithm name is an enumeration that comes from the kernel.
>  It's defined in crypto/hash_info.c: hash_algo_name.  Why do we need
> to use audit_log_untrustedstring()?

Yes, I suppose we don't need it for the hash either, since we're
generating that and we know it's just hex digits and not any audit
control characters or "s or anything.

It looks like we could get rid of the other allocation too by just
using audit_log_n_hex, but that uses hex_byte_pack_upper, vs. the
hex_byte_pack that's currently in use in this function. Is that too
much of a breakage?

Tycho

Re: [PATCH] ima: drop vla in ima_audit_measurement()

[Tycho Andersen]

On Thu, Mar 08, 2018 at 01:50:30PM -0500, Mimi Zohar wrote:
> On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote:
> > On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > > > In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> > > > from ima_audit_measurement(). We need to adjust the return type of
> > > > ima_audit_measurement, because now this function can fail if an allocation
> > > > fails.
> > > 
> > > 
> > > 
> > > > +       algo_hash_len = hash_len + strlen(algo_name) + 2;
> > > > +       algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
> > > 
> > > > -       snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> > > > +       snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > 
> > > kasprintf() ?
> > 
> > Sure, in fact I think we could just do:
> > 
> > -	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > -	audit_log_untrustedstring(ab, algo_hash);
> > +	audit_log_untrustedstring(ab, algo_name);
> > +	audit_log_format(ab, ":");
> > +	audit_log_untrustedstring(ab, hash);
> > 
> > and get rid of the allocation entirely. I'll test and make sure it
> > works and then re-send.
> 
> The hash algorithm name is an enumeration that comes from the kernel.
>  It's defined in crypto/hash_info.c: hash_algo_name.  Why do we need
> to use audit_log_untrustedstring()?

Yes, I suppose we don't need it for the hash either, since we're
generating that and we know it's just hex digits and not any audit
control characters or "s or anything.

It looks like we could get rid of the other allocation too by just
using audit_log_n_hex, but that uses hex_byte_pack_upper, vs. the
hex_byte_pack that's currently in use in this function. Is that too
much of a breakage?

Tycho

Re: [PATCH] ima: drop vla in ima_audit_measurement()

[Mimi Zohar]

On Thu, 2018-03-08 at 12:04 -0700, Tycho Andersen wrote:
> On Thu, Mar 08, 2018 at 01:50:30PM -0500, Mimi Zohar wrote:
> > On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote:
> > > On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > > > > In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> > > > > from ima_audit_measurement(). We need to adjust the return type of
> > > > > ima_audit_measurement, because now this function can fail if an allocation
> > > > > fails.
> > > > 
> > > > 
> > > > 
> > > > > +       algo_hash_len = hash_len + strlen(algo_name) + 2;
> > > > > +       algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
> > > > 
> > > > > -       snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> > > > > +       snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > > 
> > > > kasprintf() ?
> > > 
> > > Sure, in fact I think we could just do:
> > > 
> > > -	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > -	audit_log_untrustedstring(ab, algo_hash);
> > > +	audit_log_untrustedstring(ab, algo_name);
> > > +	audit_log_format(ab, ":");
> > > +	audit_log_untrustedstring(ab, hash);
> > > 
> > > and get rid of the allocation entirely. I'll test and make sure it
> > > works and then re-send.
> > 
> > The hash algorithm name is an enumeration that comes from the kernel.
> >  It's defined in crypto/hash_info.c: hash_algo_name.  Why do we need
> > to use audit_log_untrustedstring()?
> 
> Yes, I suppose we don't need it for the hash either, since we're
> generating that and we know it's just hex digits and not any audit
> control characters or "s or anything.
> 
> It looks like we could get rid of the other allocation too by just
> using audit_log_n_hex, but that uses hex_byte_pack_upper, vs. the
> hex_byte_pack that's currently in use in this function. Is that too
> much of a breakage?

Based on the discussion with Richard Briggs, we need to differentiate
between the ima_audit_measurement() and the ima_parse_rule() usage of
AUDIT_INTEGRITY_RULE.  The ima_parse_rule() will continue to use
AUDIT_INTEGRITY_RULE.  ima_audit_measurement() will need to define and
use a new number.  Auidt name suggestions would be appreciated.

When we make that sort of change, any other changes are insignificant.
How different are the two formats?

Mimi

Re: [PATCH] ima: drop vla in ima_audit_measurement()

[Mimi Zohar]

On Thu, 2018-03-08 at 12:04 -0700, Tycho Andersen wrote:
> On Thu, Mar 08, 2018 at 01:50:30PM -0500, Mimi Zohar wrote:
> > On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote:
> > > On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > > > > In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> > > > > from ima_audit_measurement(). We need to adjust the return type of
> > > > > ima_audit_measurement, because now this function can fail if an allocation
> > > > > fails.
> > > > 
> > > > 
> > > > 
> > > > > +       algo_hash_len = hash_len + strlen(algo_name) + 2;
> > > > > +       algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
> > > > 
> > > > > -       snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> > > > > +       snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > > 
> > > > kasprintf() ?
> > > 
> > > Sure, in fact I think we could just do:
> > > 
> > > -	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > -	audit_log_untrustedstring(ab, algo_hash);
> > > +	audit_log_untrustedstring(ab, algo_name);
> > > +	audit_log_format(ab, ":");
> > > +	audit_log_untrustedstring(ab, hash);
> > > 
> > > and get rid of the allocation entirely. I'll test and make sure it
> > > works and then re-send.
> > 
> > The hash algorithm name is an enumeration that comes from the kernel.
> >  It's defined in crypto/hash_info.c: hash_algo_name.  Why do we need
> > to use audit_log_untrustedstring()?
> 
> Yes, I suppose we don't need it for the hash either, since we're
> generating that and we know it's just hex digits and not any audit
> control characters or "s or anything.
> 
> It looks like we could get rid of the other allocation too by just
> using audit_log_n_hex, but that uses hex_byte_pack_upper, vs. the
> hex_byte_pack that's currently in use in this function. Is that too
> much of a breakage?

Based on the discussion with Richard Briggs, we need to differentiate
between the ima_audit_measurement() and the ima_parse_rule() usage of
AUDIT_INTEGRITY_RULE.  The ima_parse_rule() will continue to use
AUDIT_INTEGRITY_RULE.  ima_audit_measurement() will need to define and
use a new number.  Auidt name suggestions would be appreciated.

When we make that sort of change, any other changes are insignificant.
How different are the two formats?

Mimi

Re: [PATCH] ima: drop vla in ima_audit_measurement()

[Tycho Andersen]

On Thu, Mar 08, 2018 at 02:20:17PM -0500, Mimi Zohar wrote:
> On Thu, 2018-03-08 at 12:04 -0700, Tycho Andersen wrote:
> > On Thu, Mar 08, 2018 at 01:50:30PM -0500, Mimi Zohar wrote:
> > > On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote:
> > > > On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > > > > > In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> > > > > > from ima_audit_measurement(). We need to adjust the return type of
> > > > > > ima_audit_measurement, because now this function can fail if an allocation
> > > > > > fails.
> > > > > 
> > > > > 
> > > > > 
> > > > > > +       algo_hash_len = hash_len + strlen(algo_name) + 2;
> > > > > > +       algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
> > > > > 
> > > > > > -       snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> > > > > > +       snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > > > 
> > > > > kasprintf() ?
> > > > 
> > > > Sure, in fact I think we could just do:
> > > > 
> > > > -	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > > -	audit_log_untrustedstring(ab, algo_hash);
> > > > +	audit_log_untrustedstring(ab, algo_name);
> > > > +	audit_log_format(ab, ":");
> > > > +	audit_log_untrustedstring(ab, hash);
> > > > 
> > > > and get rid of the allocation entirely. I'll test and make sure it
> > > > works and then re-send.
> > > 
> > > The hash algorithm name is an enumeration that comes from the kernel.
> > >  It's defined in crypto/hash_info.c: hash_algo_name.  Why do we need
> > > to use audit_log_untrustedstring()?
> > 
> > Yes, I suppose we don't need it for the hash either, since we're
> > generating that and we know it's just hex digits and not any audit
> > control characters or "s or anything.
> > 
> > It looks like we could get rid of the other allocation too by just
> > using audit_log_n_hex, but that uses hex_byte_pack_upper, vs. the
> > hex_byte_pack that's currently in use in this function. Is that too
> > much of a breakage?
> 
> Based on the discussion with Richard Briggs, we need to differentiate
> between the ima_audit_measurement() and the ima_parse_rule() usage of
> AUDIT_INTEGRITY_RULE.  The ima_parse_rule() will continue to use
> AUDIT_INTEGRITY_RULE.  ima_audit_measurement() will need to define and
> use a new number.  Auidt name suggestions would be appreciated.
> 
> When we make that sort of change, any other changes are insignificant.
> How different are the two formats?

It's just uppercase and lowercase in the hash value, so:

Mar  8 16:56:46 ima kernel: [  104.922927] audit: type=1805 audit(1520528206.082:53): file="/bin/cat" hash="sha1:79e52322102f073684e2dd0ab7653c7c6fcc49b4" ppid=2049 pid=2123 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="bash" exe="/bin/bash"

vs.

Mar  8 19:45:12 ima kernel: [  207.124383] audit: type=1805 audit(1520538312.740:239): file="/root/.viminfo" hash="sha1:3322BE0C00190AB0D20C47574575842EC3020BF5" ppid=2045 pid=2195 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="vi" exe="/usr/bin/vim.basic"

I'm happy to do either way.

Tycho

Re: [PATCH] ima: drop vla in ima_audit_measurement()

[Tycho Andersen]

On Thu, Mar 08, 2018 at 02:20:17PM -0500, Mimi Zohar wrote:
> On Thu, 2018-03-08 at 12:04 -0700, Tycho Andersen wrote:
> > On Thu, Mar 08, 2018 at 01:50:30PM -0500, Mimi Zohar wrote:
> > > On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote:
> > > > On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > > > > > In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> > > > > > from ima_audit_measurement(). We need to adjust the return type of
> > > > > > ima_audit_measurement, because now this function can fail if an allocation
> > > > > > fails.
> > > > > 
> > > > > 
> > > > > 
> > > > > > +       algo_hash_len = hash_len + strlen(algo_name) + 2;
> > > > > > +       algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
> > > > > 
> > > > > > -       snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> > > > > > +       snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > > > 
> > > > > kasprintf() ?
> > > > 
> > > > Sure, in fact I think we could just do:
> > > > 
> > > > -	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > > -	audit_log_untrustedstring(ab, algo_hash);
> > > > +	audit_log_untrustedstring(ab, algo_name);
> > > > +	audit_log_format(ab, ":");
> > > > +	audit_log_untrustedstring(ab, hash);
> > > > 
> > > > and get rid of the allocation entirely. I'll test and make sure it
> > > > works and then re-send.
> > > 
> > > The hash algorithm name is an enumeration that comes from the kernel.
> > >  It's defined in crypto/hash_info.c: hash_algo_name.  Why do we need
> > > to use audit_log_untrustedstring()?
> > 
> > Yes, I suppose we don't need it for the hash either, since we're
> > generating that and we know it's just hex digits and not any audit
> > control characters or "s or anything.
> > 
> > It looks like we could get rid of the other allocation too by just
> > using audit_log_n_hex, but that uses hex_byte_pack_upper, vs. the
> > hex_byte_pack that's currently in use in this function. Is that too
> > much of a breakage?
> 
> Based on the discussion with Richard Briggs, we need to differentiate
> between the ima_audit_measurement() and the ima_parse_rule() usage of
> AUDIT_INTEGRITY_RULE.  The ima_parse_rule() will continue to use
> AUDIT_INTEGRITY_RULE.  ima_audit_measurement() will need to define and
> use a new number.  Auidt name suggestions would be appreciated.
> 
> When we make that sort of change, any other changes are insignificant.
> How different are the two formats?

It's just uppercase and lowercase in the hash value, so:

Mar  8 16:56:46 ima kernel: [  104.922927] audit: type=1805 audit(1520528206.082:53): file="/bin/cat" hash="sha1:79e52322102f073684e2dd0ab7653c7c6fcc49b4" ppid=2049 pid=2123 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="bash" exe="/bin/bash"

vs.

Mar  8 19:45:12 ima kernel: [  207.124383] audit: type=1805 audit(1520538312.740:239): file="/root/.viminfo" hash="sha1:3322BE0C00190AB0D20C47574575842EC3020BF5" ppid=2045 pid=2195 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="vi" exe="/usr/bin/vim.basic"

I'm happy to do either way.

Tycho

Re: [PATCH] ima: drop vla in ima_audit_measurement()

[Mimi Zohar]

On Thu, 2018-03-08 at 12:47 -0700, Tycho Andersen wrote:
> On Thu, Mar 08, 2018 at 02:20:17PM -0500, Mimi Zohar wrote:
> > On Thu, 2018-03-08 at 12:04 -0700, Tycho Andersen wrote:
> > > On Thu, Mar 08, 2018 at 01:50:30PM -0500, Mimi Zohar wrote:
> > > > On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote:
> > > > > On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > > > > > > In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> > > > > > > from ima_audit_measurement(). We need to adjust the return type of
> > > > > > > ima_audit_measurement, because now this function can fail if an allocation
> > > > > > > fails.
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > +       algo_hash_len = hash_len + strlen(algo_name) + 2;
> > > > > > > +       algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
> > > > > > 
> > > > > > > -       snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> > > > > > > +       snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > > > > 
> > > > > > kasprintf() ?
> > > > > 
> > > > > Sure, in fact I think we could just do:
> > > > > 
> > > > > -	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > > > -	audit_log_untrustedstring(ab, algo_hash);
> > > > > +	audit_log_untrustedstring(ab, algo_name);
> > > > > +	audit_log_format(ab, ":");
> > > > > +	audit_log_untrustedstring(ab, hash);
> > > > > 
> > > > > and get rid of the allocation entirely. I'll test and make sure it
> > > > > works and then re-send.
> > > > 
> > > > The hash algorithm name is an enumeration that comes from the kernel.
> > > >  It's defined in crypto/hash_info.c: hash_algo_name.  Why do we need
> > > > to use audit_log_untrustedstring()?
> > > 
> > > Yes, I suppose we don't need it for the hash either, since we're
> > > generating that and we know it's just hex digits and not any audit
> > > control characters or "s or anything.
> > > 
> > > It looks like we could get rid of the other allocation too by just
> > > using audit_log_n_hex, but that uses hex_byte_pack_upper, vs. the
> > > hex_byte_pack that's currently in use in this function. Is that too
> > > much of a breakage?
> > 
> > Based on the discussion with Richard Briggs, we need to differentiate
> > between the ima_audit_measurement() and the ima_parse_rule() usage of
> > AUDIT_INTEGRITY_RULE.  The ima_parse_rule() will continue to use
> > AUDIT_INTEGRITY_RULE.  ima_audit_measurement() will need to define and
> > use a new number.  Auidt name suggestions would be appreciated.
> > 
> > When we make that sort of change, any other changes are insignificant.
> > How different are the two formats?
> 
> It's just uppercase and lowercase in the hash value, so:
> 
> Mar  8 16:56:46 ima kernel: [  104.922927] audit: type=1805 audit(1520528206.082:53): file="/bin/cat" hash="sha1:79e52322102f073684e2dd0ab7653c7c6fcc49b4" ppid=2049 pid=2123 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="bash" exe="/bin/bash"
> 
> vs.
> 
> Mar  8 19:45:12 ima kernel: [  207.124383] audit: type=1805 audit(1520538312.740:239): file="/root/.viminfo" hash="sha1:3322BE0C00190AB0D20C47574575842EC3020BF5" ppid=2045 pid=2195 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="vi" exe="/usr/bin/vim.basic"
> 
> I'm happy to do either way.

If you're willing to wait until the container-id issue is
resolved/upstreamed, then either way is fine.  If you want the change
to go in sooner, then keep it as it currently is.

Mimi

Re: [PATCH] ima: drop vla in ima_audit_measurement()

[Mimi Zohar]

On Thu, 2018-03-08 at 12:47 -0700, Tycho Andersen wrote:
> On Thu, Mar 08, 2018 at 02:20:17PM -0500, Mimi Zohar wrote:
> > On Thu, 2018-03-08 at 12:04 -0700, Tycho Andersen wrote:
> > > On Thu, Mar 08, 2018 at 01:50:30PM -0500, Mimi Zohar wrote:
> > > > On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote:
> > > > > On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > > > > > > In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> > > > > > > from ima_audit_measurement(). We need to adjust the return type of
> > > > > > > ima_audit_measurement, because now this function can fail if an allocation
> > > > > > > fails.
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > +       algo_hash_len = hash_len + strlen(algo_name) + 2;
> > > > > > > +       algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
> > > > > > 
> > > > > > > -       snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> > > > > > > +       snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > > > > 
> > > > > > kasprintf() ?
> > > > > 
> > > > > Sure, in fact I think we could just do:
> > > > > 
> > > > > -	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > > > -	audit_log_untrustedstring(ab, algo_hash);
> > > > > +	audit_log_untrustedstring(ab, algo_name);
> > > > > +	audit_log_format(ab, ":");
> > > > > +	audit_log_untrustedstring(ab, hash);
> > > > > 
> > > > > and get rid of the allocation entirely. I'll test and make sure it
> > > > > works and then re-send.
> > > > 
> > > > The hash algorithm name is an enumeration that comes from the kernel.
> > > >  It's defined in crypto/hash_info.c: hash_algo_name.  Why do we need
> > > > to use audit_log_untrustedstring()?
> > > 
> > > Yes, I suppose we don't need it for the hash either, since we're
> > > generating that and we know it's just hex digits and not any audit
> > > control characters or "s or anything.
> > > 
> > > It looks like we could get rid of the other allocation too by just
> > > using audit_log_n_hex, but that uses hex_byte_pack_upper, vs. the
> > > hex_byte_pack that's currently in use in this function. Is that too
> > > much of a breakage?
> > 
> > Based on the discussion with Richard Briggs, we need to differentiate
> > between the ima_audit_measurement() and the ima_parse_rule() usage of
> > AUDIT_INTEGRITY_RULE.  The ima_parse_rule() will continue to use
> > AUDIT_INTEGRITY_RULE.  ima_audit_measurement() will need to define and
> > use a new number.  Auidt name suggestions would be appreciated.
> > 
> > When we make that sort of change, any other changes are insignificant.
> > How different are the two formats?
> 
> It's just uppercase and lowercase in the hash value, so:
> 
> Mar  8 16:56:46 ima kernel: [  104.922927] audit: type=1805 audit(1520528206.082:53): file="/bin/cat" hash="sha1:79e52322102f073684e2dd0ab7653c7c6fcc49b4" ppid=2049 pid=2123 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="bash" exe="/bin/bash"
> 
> vs.
> 
> Mar  8 19:45:12 ima kernel: [  207.124383] audit: type=1805 audit(1520538312.740:239): file="/root/.viminfo" hash="sha1:3322BE0C00190AB0D20C47574575842EC3020BF5" ppid=2045 pid=2195 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="vi" exe="/usr/bin/vim.basic"
> 
> I'm happy to do either way.

If you're willing to wait until the container-id issue is
resolved/upstreamed, then either way is fine.  If you want the change
to go in sooner, then keep it as it currently is.

Mimi

Re: [PATCH] ima: drop vla in ima_audit_measurement()

[Tycho Andersen]

Whoops, seems the IMA list has moved since I last sent a patch. I'll
resend shortly.

[PATCH] ima: drop vla in ima_audit_measurement()

[Tycho Andersen]

In keeping with the directive to get rid of VLAs [1], let's drop the VLA
from ima_audit_measurement(). We need to adjust the return type of
ima_audit_measurement, because now this function can fail if an allocation
fails.

[1]: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
---
 security/integrity/ima/ima.h      |  4 ++--
 security/integrity/ima/ima_api.c  | 31 +++++++++++++++++++++++--------
 security/integrity/ima/ima_main.c |  7 +++++--
 3 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d52b487ad259..8e2470f72f7f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -201,8 +201,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
 			   const unsigned char *filename,
 			   struct evm_ima_xattr_data *xattr_value,
 			   int xattr_len, int pcr);
-void ima_audit_measurement(struct integrity_iint_cache *iint,
-			   const unsigned char *filename);
+int ima_audit_measurement(struct integrity_iint_cache *iint,
+			  const unsigned char *filename);
 int ima_alloc_init_template(struct ima_event_data *event_data,
 			    struct ima_template_entry **entry);
 int ima_store_template(struct ima_template_entry *entry, int violation,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 08fe405338e1..008d3887ae00 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -304,17 +304,28 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
 		ima_free_template_entry(entry);
 }
 
-void ima_audit_measurement(struct integrity_iint_cache *iint,
-			   const unsigned char *filename)
+int ima_audit_measurement(struct integrity_iint_cache *iint,
+			  const unsigned char *filename)
 {
 	struct audit_buffer *ab;
-	char hash[(iint->ima_hash->length * 2) + 1];
+	char *hash, *algo_hash;
 	const char *algo_name = hash_algo_name[iint->ima_hash->algo];
-	char algo_hash[sizeof(hash) + strlen(algo_name) + 2];
-	int i;
+	int i, hash_len, algo_hash_len;
 
 	if (iint->flags & IMA_AUDITED)
-		return;
+		return 0;
+
+	hash_len = (iint->ima_hash->length * 2) + 1;
+	hash = kzalloc(hash_len, GFP_KERNEL);
+	if (!hash)
+		return -ENOMEM;
+
+	algo_hash_len = hash_len + strlen(algo_name) + 2;
+	algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
+	if (!algo_hash) {
+		kfree(hash);
+		return -ENOMEM;
+	}
 
 	for (i = 0; i < iint->ima_hash->length; i++)
 		hex_byte_pack(hash + (i * 2), iint->ima_hash->digest[i]);
@@ -323,18 +334,22 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
 	ab = audit_log_start(current->audit_context, GFP_KERNEL,
 			     AUDIT_INTEGRITY_RULE);
 	if (!ab)
-		return;
+		goto out;
 
 	audit_log_format(ab, "file=");
 	audit_log_untrustedstring(ab, filename);
 	audit_log_format(ab, " hash=");
-	snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
+	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
 	audit_log_untrustedstring(ab, algo_hash);
 
 	audit_log_task_info(ab, current);
 	audit_log_end(ab);
 
 	iint->flags |= IMA_AUDITED;
+out:
+	kfree(hash);
+	kfree(algo_hash);
+	return 0;
 }
 
 /*
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2cfb0c714967..356faae6f09c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -288,8 +288,11 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 					      xattr_value, xattr_len, opened);
 		inode_unlock(inode);
 	}
-	if (action & IMA_AUDIT)
-		ima_audit_measurement(iint, pathname);
+	if (action & IMA_AUDIT) {
+		rc = ima_audit_measurement(iint, pathname);
+		if (rc < 0)
+			goto out_locked;
+	}
 
 	if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO))
 		rc = 0;
-- 
2.14.1