[PATCH 0/6] ima: fixes for the new template management mechanism

[PATCH 0/6] ima: fixes for the new template management mechanism

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 1813 bytes --]

Hi everyone

this patch set fixes some issues in the new template management mechanism.
In particular, first four patches are simple bug fixes, explained in the patch
description, while last two restore the original IMA behavior when producing
a measurement entry with the old 'ima' template'. In respect to the behavior
adopted for newly introduced templates ('ima-ng' and 'ima-sig'), where
the total template length and the field length are sent through the
'binary_runtime_measurements' interface and the latter information is included
in the calculation of the template digest, for the old 'ima' template it is
necessary to handle the following exceptions:

 - the event digest field length is NOT sent through the userspace interface
   and is NOT included in the template digest calculation;
 - the event name field length is sent through the userspace interface
   but is NOT included in the template digest calculation.


Regards

Roberto Sassu


Roberto Sassu (6):
  ima: change the default hash algorithm to SHA1 in
    ima_eventdigest_ng_init()
  ima: pass HASH_ALGO__LAST as hash algo in ima_eventdigest_init()
  ima: remove unneeded size_limit argument from
    ima_eventdigest_init_common()
  ima: check result of crypto_shash_update() in
    ima_calc_field_array_hash_tfm
  ima: do not include field length in template digest calc for ima
    template
  ima: do not send field length to userspace for digest of ima template

 security/integrity/ima/ima.h              |  6 ++++--
 security/integrity/ima/ima_api.c          |  1 +
 security/integrity/ima/ima_crypto.c       | 17 ++++++++++++-----
 security/integrity/ima/ima_fs.c           | 14 +++++++++++---
 security/integrity/ima/ima_template_lib.c | 24 +++++++++++++-----------
 5 files changed, 41 insertions(+), 21 deletions(-)

-- 
1.8.1.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

[PATCH 1/6] ima: change the default hash algorithm to SHA1 in ima_eventdigest_ng_init()

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 976 bytes --]

Replace HASH_ALGO__LAST with HASH_ALGO_SHA1 as the initial value of
the hash algorithm so that the prefix 'sha1:' is added to violation
digests.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_template_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 6d66ad6..c2ba481 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -251,7 +251,7 @@ int ima_eventdigest_ng_init(struct integrity_iint_cache *iint,
 			    struct evm_ima_xattr_data *xattr_value,
 			    int xattr_len, struct ima_field_data *field_data)
 {
-	u8 *cur_digest = NULL, hash_algo = HASH_ALGO__LAST;
+	u8 *cur_digest = NULL, hash_algo = HASH_ALGO_SHA1;
 	u32 cur_digestsize = 0;
 
 	/* If iint is NULL, we are recording a violation. */
-- 
1.8.1.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

[PATCH 2/6] ima: pass HASH_ALGO__LAST as hash algo in ima_eventdigest_init()

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

Replace the '-1' value with HASH_ALGO__LAST in ima_eventdigest_init()
as the called function ima_eventdigest_init_common() expects an unsigned
char.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_template_lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index c2ba481..4752a53 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -239,8 +239,8 @@ int ima_eventdigest_init(struct integrity_iint_cache *iint, struct file *file,
 	cur_digest = hash.hdr.digest;
 	cur_digestsize = hash.hdr.length;
 out:
-	return ima_eventdigest_init_common(cur_digest, cur_digestsize, -1,
-					   field_data, true);
+	return ima_eventdigest_init_common(cur_digest, cur_digestsize,
+					   HASH_ALGO__LAST, field_data, true);
 }
 
 /*
-- 
1.8.1.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

[PATCH 3/6] ima: remove unneeded size_limit argument from ima_eventdigest_init_common()

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 2323 bytes --]

This patch removes the 'size_limit' argument
from ima_eventdigest_init_common(). Since the 'd' field will never include
the hash algorithm as prefix and the 'd-ng' will always have it, we can
use the hash algorithm to differentiate the two cases in the modified
function (it is equal to HASH_ALGO__LAST in the first case, the opposite
in the second).

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_template_lib.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 4752a53..6d01c69 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -158,8 +158,7 @@ void ima_show_template_sig(struct seq_file *m, enum ima_show_type show,
 }
 
 static int ima_eventdigest_init_common(u8 *digest, u32 digestsize, u8 hash_algo,
-				       struct ima_field_data *field_data,
-				       bool size_limit)
+				       struct ima_field_data *field_data)
 {
 	/*
 	 * digest formats:
@@ -172,11 +171,10 @@ static int ima_eventdigest_init_common(u8 *digest, u32 digestsize, u8 hash_algo,
 	enum data_formats fmt = DATA_FMT_DIGEST;
 	u32 offset = 0;
 
-	if (!size_limit) {
+	if (hash_algo < HASH_ALGO__LAST) {
 		fmt = DATA_FMT_DIGEST_WITH_ALGO;
-		if (hash_algo < HASH_ALGO__LAST)
-			offset += snprintf(buffer, CRYPTO_MAX_ALG_NAME + 1,
-					   "%s", hash_algo_name[hash_algo]);
+		offset += snprintf(buffer, CRYPTO_MAX_ALG_NAME + 1, "%s",
+				   hash_algo_name[hash_algo]);
 		buffer[offset] = ':';
 		offset += 2;
 	}
@@ -240,7 +238,7 @@ int ima_eventdigest_init(struct integrity_iint_cache *iint, struct file *file,
 	cur_digestsize = hash.hdr.length;
 out:
 	return ima_eventdigest_init_common(cur_digest, cur_digestsize,
-					   HASH_ALGO__LAST, field_data, true);
+					   HASH_ALGO__LAST, field_data);
 }
 
 /*
@@ -264,7 +262,7 @@ int ima_eventdigest_ng_init(struct integrity_iint_cache *iint,
 	hash_algo = iint->ima_hash->algo;
 out:
 	return ima_eventdigest_init_common(cur_digest, cur_digestsize,
-					   hash_algo, field_data, false);
+					   hash_algo, field_data);
 }
 
 static int ima_eventname_init_common(struct integrity_iint_cache *iint,
-- 
1.8.1.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

[PATCH 4/6] ima: check result of crypto_shash_update() in ima_calc_field_array_hash_tfm

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

This patch adds a check of the result from the first crypto_shash_update()
in ima_calc_field_array_hash_tfm().

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_crypto.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 676e029..e22708b 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -163,6 +163,9 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
 		rc = crypto_shash_update(&desc.shash,
 					 (const u8 *) &field_data[i].len,
 					 sizeof(field_data[i].len));
+		if (rc)
+			break;
+
 		rc = crypto_shash_update(&desc.shash, field_data[i].data,
 					 field_data[i].len);
 		if (rc)
-- 
1.8.1.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

[PATCH 5/6] ima: do not include field length in template digest calc for ima template

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 3690 bytes --]

To maintain compatibility with userspace tools, the field length must not
be included in the template digest calculation for the 'ima' template.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/ima.h        |  3 ++-
 security/integrity/ima/ima_api.c    |  1 +
 security/integrity/ima/ima_crypto.c | 20 ++++++++++++--------
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index bf03c6a..a21cf70 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -97,7 +97,8 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 			   const char *op, struct inode *inode,
 			   const unsigned char *filename);
 int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash);
-int ima_calc_field_array_hash(struct ima_field_data *field_data, int num_fields,
+int ima_calc_field_array_hash(struct ima_field_data *field_data,
+			      struct ima_template_desc *desc, int num_fields,
 			      struct ima_digest_data *hash);
 int __init ima_calc_boot_aggregate(struct ima_digest_data *hash);
 void ima_add_violation(struct file *file, const unsigned char *filename,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 0e75408..8037484 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -94,6 +94,7 @@ int ima_store_template(struct ima_template_entry *entry,
 		/* this function uses default algo */
 		hash.hdr.algo = HASH_ALGO_SHA1;
 		result = ima_calc_field_array_hash(&entry->template_data[0],
+						   entry->template_desc,
 						   num_fields, &hash.hdr);
 		if (result < 0) {
 			integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode,
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index e22708b..fdf60de 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -140,6 +140,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
  * Calculate the hash of template data
  */
 static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
+					 struct ima_template_desc *td,
 					 int num_fields,
 					 struct ima_digest_data *hash,
 					 struct crypto_shash *tfm)
@@ -160,12 +161,13 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
 		return rc;
 
 	for (i = 0; i < num_fields; i++) {
-		rc = crypto_shash_update(&desc.shash,
-					 (const u8 *) &field_data[i].len,
-					 sizeof(field_data[i].len));
-		if (rc)
-			break;
-
+		if (strcmp(td->name, IMA_TEMPLATE_IMA_NAME) != 0) {
+			rc = crypto_shash_update(&desc.shash,
+						(const u8 *) &field_data[i].len,
+						sizeof(field_data[i].len));
+			if (rc)
+				break;
+		}
 		rc = crypto_shash_update(&desc.shash, field_data[i].data,
 					 field_data[i].len);
 		if (rc)
@@ -178,7 +180,8 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
 	return rc;
 }
 
-int ima_calc_field_array_hash(struct ima_field_data *field_data, int num_fields,
+int ima_calc_field_array_hash(struct ima_field_data *field_data,
+			      struct ima_template_desc *desc, int num_fields,
 			      struct ima_digest_data *hash)
 {
 	struct crypto_shash *tfm;
@@ -188,7 +191,8 @@ int ima_calc_field_array_hash(struct ima_field_data *field_data, int num_fields,
 	if (IS_ERR(tfm))
 		return PTR_ERR(tfm);
 
-	rc = ima_calc_field_array_hash_tfm(field_data, num_fields, hash, tfm);
+	rc = ima_calc_field_array_hash_tfm(field_data, desc, num_fields,
+					   hash, tfm);
 
 	ima_free_tfm(tfm);
 
-- 
1.8.1.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

[PATCH 6/6] ima: do not send field length to userspace for digest of ima template

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 3320 bytes --]

This patch defines a new value for the 'ima_show_type' enumerator
(IMA_SHOW_BINARY_NO_FIELD_LEN) to prevent that the field length
is transmitted through the 'binary_runtime_measurements' interface
for the digest field of the 'ima' template.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/ima.h              |  3 ++-
 security/integrity/ima/ima_fs.c           | 14 +++++++++++---
 security/integrity/ima/ima_template_lib.c |  6 +++++-
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index a21cf70..9636e17 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -26,7 +26,8 @@
 
 #include "../integrity.h"
 
-enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_ASCII };
+enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN,
+		     IMA_SHOW_ASCII };
 enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
 
 /* digest size for IMA, fits SHA1 or MD5 */
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index d47a7c8..db01125 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -120,6 +120,7 @@ static int ima_measurements_show(struct seq_file *m, void *v)
 	struct ima_template_entry *e;
 	int namelen;
 	u32 pcr = CONFIG_IMA_MEASURE_PCR_IDX;
+	bool is_ima_template = false;
 	int i;
 
 	/* get entry */
@@ -145,14 +146,21 @@ static int ima_measurements_show(struct seq_file *m, void *v)
 	ima_putc(m, e->template_desc->name, namelen);
 
 	/* 5th:  template length (except for 'ima' template) */
-	if (strcmp(e->template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
+	if (strcmp(e->template_desc->name, IMA_TEMPLATE_IMA_NAME) == 0)
+		is_ima_template = true;
+
+	if (!is_ima_template)
 		ima_putc(m, &e->template_data_len,
 			 sizeof(e->template_data_len));
 
 	/* 6th:  template specific data */
 	for (i = 0; i < e->template_desc->num_fields; i++) {
-		e->template_desc->fields[i]->field_show(m, IMA_SHOW_BINARY,
-							&e->template_data[i]);
+		enum ima_show_type show = IMA_SHOW_BINARY;
+		struct ima_template_field *field = e->template_desc->fields[i];
+
+		if (is_ima_template && strcmp(field->field_id, "d") == 0)
+			show = IMA_SHOW_BINARY_NO_FIELD_LEN;
+		field->field_show(m, show, &e->template_data[i]);
 	}
 	return 0;
 }
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 6d01c69..1683bbf 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -109,9 +109,12 @@ static void ima_show_template_data_binary(struct seq_file *m,
 					  enum data_formats datafmt,
 					  struct ima_field_data *field_data)
 {
-	ima_putc(m, &field_data->len, sizeof(u32));
+	if (show != IMA_SHOW_BINARY_NO_FIELD_LEN)
+		ima_putc(m, &field_data->len, sizeof(u32));
+
 	if (!field_data->len)
 		return;
+
 	ima_putc(m, field_data->data, field_data->len);
 }
 
@@ -125,6 +128,7 @@ static void ima_show_template_field_data(struct seq_file *m,
 		ima_show_template_data_ascii(m, show, datafmt, field_data);
 		break;
 	case IMA_SHOW_BINARY:
+	case IMA_SHOW_BINARY_NO_FIELD_LEN:
 		ima_show_template_data_binary(m, show, datafmt, field_data);
 		break;
 	default:
-- 
1.8.1.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

Re: [PATCH 5/6] ima: do not include field length in template digest calc for ima template

[Dmitry Kasatkin]

On Fri, Nov 15, 2013 at 3:45 PM, Roberto Sassu <roberto.sassu@polito.it> wrote:
> To maintain compatibility with userspace tools, the field length must not
> be included in the template digest calculation for the 'ima' template.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
>  security/integrity/ima/ima.h        |  3 ++-
>  security/integrity/ima/ima_api.c    |  1 +
>  security/integrity/ima/ima_crypto.c | 20 ++++++++++++--------
>  3 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index bf03c6a..a21cf70 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -97,7 +97,8 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>                            const char *op, struct inode *inode,
>                            const unsigned char *filename);
>  int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash);
> -int ima_calc_field_array_hash(struct ima_field_data *field_data, int num_fields,
> +int ima_calc_field_array_hash(struct ima_field_data *field_data,
> +                             struct ima_template_desc *desc, int num_fields,
>                               struct ima_digest_data *hash);
>  int __init ima_calc_boot_aggregate(struct ima_digest_data *hash);
>  void ima_add_violation(struct file *file, const unsigned char *filename,
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 0e75408..8037484 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -94,6 +94,7 @@ int ima_store_template(struct ima_template_entry *entry,
>                 /* this function uses default algo */
>                 hash.hdr.algo = HASH_ALGO_SHA1;
>                 result = ima_calc_field_array_hash(&entry->template_data[0],
> +                                                  entry->template_desc,
>                                                    num_fields, &hash.hdr);
>                 if (result < 0) {
>                         integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode,
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index e22708b..fdf60de 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -140,6 +140,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>   * Calculate the hash of template data
>   */
>  static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
> +                                        struct ima_template_desc *td,
>                                          int num_fields,
>                                          struct ima_digest_data *hash,
>                                          struct crypto_shash *tfm)
> @@ -160,12 +161,13 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
>                 return rc;
>
>         for (i = 0; i < num_fields; i++) {
> -               rc = crypto_shash_update(&desc.shash,
> -                                        (const u8 *) &field_data[i].len,
> -                                        sizeof(field_data[i].len));
> -               if (rc)
> -                       break;
> -
> +               if (strcmp(td->name, IMA_TEMPLATE_IMA_NAME) != 0) {
> +                       rc = crypto_shash_update(&desc.shash,
> +                                               (const u8 *) &field_data[i].len,
> +                                               sizeof(field_data[i].len));
> +                       if (rc)
> +                               break;
> +               }

What was actually the point in including field length in the hash calculation?
Does it really make it cryptographically stronger?
If not then remove it at all...

- Dmitry


>                 rc = crypto_shash_update(&desc.shash, field_data[i].data,
>                                          field_data[i].len);
>                 if (rc)
> @@ -178,7 +180,8 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
>         return rc;
>  }
>
> -int ima_calc_field_array_hash(struct ima_field_data *field_data, int num_fields,
> +int ima_calc_field_array_hash(struct ima_field_data *field_data,
> +                             struct ima_template_desc *desc, int num_fields,
>                               struct ima_digest_data *hash)
>  {
>         struct crypto_shash *tfm;
> @@ -188,7 +191,8 @@ int ima_calc_field_array_hash(struct ima_field_data *field_data, int num_fields,
>         if (IS_ERR(tfm))
>                 return PTR_ERR(tfm);
>
> -       rc = ima_calc_field_array_hash_tfm(field_data, num_fields, hash, tfm);
> +       rc = ima_calc_field_array_hash_tfm(field_data, desc, num_fields,
> +                                          hash, tfm);
>
>         ima_free_tfm(tfm);
>
> --
> 1.8.1.4
>



-- 
Thanks,
Dmitry

Re: [PATCH 5/6] ima: do not include field length in template digest calc for ima template

[Mimi Zohar]

On Mon, 2013-11-18 at 17:30 +0200, Dmitry Kasatkin wrote:
> On Fri, Nov 15, 2013 at 3:45 PM, Roberto Sassu <roberto.sassu@polito.it> wrote:

> > @@ -160,12 +161,13 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
> >                 return rc;
> >
> >         for (i = 0; i < num_fields; i++) {
> > -               rc = crypto_shash_update(&desc.shash,
> > -                                        (const u8 *) &field_data[i].len,
> > -                                        sizeof(field_data[i].len));
> > -               if (rc)
> > -                       break;
> > -
> > +               if (strcmp(td->name, IMA_TEMPLATE_IMA_NAME) != 0) {
> > +                       rc = crypto_shash_update(&desc.shash,
> > +                                               (const u8 *) &field_data[i].len,
> > +                                               sizeof(field_data[i].len));
> > +                       if (rc)
> > +                               break;
> > +               }
> 
> What was actually the point in including field length in the hash calculation?
> Does it really make it cryptographically stronger?
> If not then remove it at all...

We should be able to walk the measurement list without needing to
understand template specific data.  All of the template data, including
the field lengths, needs to be included in the template hash.

Mimi

Re: [PATCH 5/6] ima: do not include field length in template digest calc for ima template

[Dmitry Kasatkin]

On Mon, Nov 18, 2013 at 5:50 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Mon, 2013-11-18 at 17:30 +0200, Dmitry Kasatkin wrote:
>> On Fri, Nov 15, 2013 at 3:45 PM, Roberto Sassu <roberto.sassu@polito.it> wrote:
>
>> > @@ -160,12 +161,13 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
>> >                 return rc;
>> >
>> >         for (i = 0; i < num_fields; i++) {
>> > -               rc = crypto_shash_update(&desc.shash,
>> > -                                        (const u8 *) &field_data[i].len,
>> > -                                        sizeof(field_data[i].len));
>> > -               if (rc)
>> > -                       break;
>> > -
>> > +               if (strcmp(td->name, IMA_TEMPLATE_IMA_NAME) != 0) {
>> > +                       rc = crypto_shash_update(&desc.shash,
>> > +                                               (const u8 *) &field_data[i].len,
>> > +                                               sizeof(field_data[i].len));
>> > +                       if (rc)
>> > +                               break;
>> > +               }
>>
>> What was actually the point in including field length in the hash calculation?
>> Does it really make it cryptographically stronger?
>> If not then remove it at all...
>
> We should be able to walk the measurement list without needing to
> understand template specific data.  All of the template data, including
> the field lengths, needs to be included in the template hash.
>

Sorry, how adding field length helps walking measurement list?
It makes it even more tricky...

Previously, I could calculate the hash over the whole template data
from binary_measurement_list
to get template hash.
Now every field must be processed separately to get template hash...


- Dmitry


> Mimi
>



-- 
Thanks,
Dmitry

Re: [PATCH 5/6] ima: do not include field length in template digest calc for ima template

[Mimi Zohar]

On Mon, 2013-11-18 at 21:40 +0200, Dmitry Kasatkin wrote:
> On Mon, Nov 18, 2013 at 5:50 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Mon, 2013-11-18 at 17:30 +0200, Dmitry Kasatkin wrote:
> >> On Fri, Nov 15, 2013 at 3:45 PM, Roberto Sassu <roberto.sassu@polito.it> wrote:
> >
> >> > @@ -160,12 +161,13 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
> >> >                 return rc;
> >> >
> >> >         for (i = 0; i < num_fields; i++) {
> >> > -               rc = crypto_shash_update(&desc.shash,
> >> > -                                        (const u8 *) &field_data[i].len,
> >> > -                                        sizeof(field_data[i].len));
> >> > -               if (rc)
> >> > -                       break;
> >> > -
> >> > +               if (strcmp(td->name, IMA_TEMPLATE_IMA_NAME) != 0) {
> >> > +                       rc = crypto_shash_update(&desc.shash,
> >> > +                                               (const u8 *) &field_data[i].len,
> >> > +                                               sizeof(field_data[i].len));
> >> > +                       if (rc)
> >> > +                               break;
> >> > +               }
> >>
> >> What was actually the point in including field length in the hash calculation?
> >> Does it really make it cryptographically stronger?
> >> If not then remove it at all...
> >
> > We should be able to walk the measurement list without needing to
> > understand template specific data.  All of the template data, including
> > the field lengths, needs to be included in the template hash.
> >
> 
> Sorry, how adding field length helps walking measurement list?
> It makes it even more tricky...

The new template architecture adds sending the field length.

> Previously, I could calculate the hash over the whole template data
> from binary_measurement_list to get template hash.

> Now every field must be processed separately to get template hash...

You still calculate the template hash based on the entire template data,
including the lengths.  Without including the lengths in the hash
calculation, you would need to parse the individual template field
data. 

Mimi

Re: [PATCH 5/6] ima: do not include field length in template digest calc for ima template

[Dmitry Kasatkin]

On Mon, Nov 18, 2013 at 10:05 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Mon, 2013-11-18 at 21:40 +0200, Dmitry Kasatkin wrote:
>> On Mon, Nov 18, 2013 at 5:50 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> > On Mon, 2013-11-18 at 17:30 +0200, Dmitry Kasatkin wrote:
>> >> On Fri, Nov 15, 2013 at 3:45 PM, Roberto Sassu <roberto.sassu@polito.it> wrote:
>> >
>> >> > @@ -160,12 +161,13 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
>> >> >                 return rc;
>> >> >
>> >> >         for (i = 0; i < num_fields; i++) {
>> >> > -               rc = crypto_shash_update(&desc.shash,
>> >> > -                                        (const u8 *) &field_data[i].len,
>> >> > -                                        sizeof(field_data[i].len));
>> >> > -               if (rc)
>> >> > -                       break;
>> >> > -
>> >> > +               if (strcmp(td->name, IMA_TEMPLATE_IMA_NAME) != 0) {
>> >> > +                       rc = crypto_shash_update(&desc.shash,
>> >> > +                                               (const u8 *) &field_data[i].len,
>> >> > +                                               sizeof(field_data[i].len));
>> >> > +                       if (rc)
>> >> > +                               break;
>> >> > +               }
>> >>
>> >> What was actually the point in including field length in the hash calculation?
>> >> Does it really make it cryptographically stronger?
>> >> If not then remove it at all...
>> >
>> > We should be able to walk the measurement list without needing to
>> > understand template specific data.  All of the template data, including
>> > the field lengths, needs to be included in the template hash.
>> >
>>
>> Sorry, how adding field length helps walking measurement list?
>> It makes it even more tricky...
>
> The new template architecture adds sending the field length.
>
>> Previously, I could calculate the hash over the whole template data
>> from binary_measurement_list to get template hash.
>
>> Now every field must be processed separately to get template hash...
>
> You still calculate the template hash based on the entire template data,
> including the lengths.  Without including the lengths in the hash
> calculation, you would need to parse the individual template field
> data.

Right.. Exactly reverse.. Yes, binary measurement list includes length field..
My mistake here...

- Dmitry

>
> Mimi
>



-- 
Thanks,
Dmitry