[v2,8/8] ima: Use ima_hash_algo for collision detection in the measurement list

Commit Message

Roberto Sassu Feb. 5, 2020, 10:33 a.m. UTC

Before calculating a digest for each PCR bank, collisions were detected
with a SHA1 digest. This patch includes ima_hash_algo among the algorithms
used to calculate the template digest and checks collisions on that digest.

Changelog

v1:
- increment ima_num_template_digests before kcalloc() (suggested by Mimi)
- check if ima_tpm_chip is NULL

Signed-off-by: Roberto Sassu <[e-mail]>
---
 security/integrity/ima/ima.h        |  1 +
 security/integrity/ima/ima_crypto.c | 20 ++++++++++++++++++--
 security/integrity/ima/ima_queue.c  |  8 ++++----
 3 files changed, 23 insertions(+), 6 deletions(-)

Comments

Roberto Sassu March 2, 2020, 9:46 a.m. UTC | #1

> -----Original Message-----
> From: kernel test robot [mailto:[e-mail]]
> Sent: Monday, March 2, 2020 2:22 AM
> To: Roberto Sassu <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; Silviu
> Vlasceanu <[e-mail]>; Roberto Sassu
> <[e-mail]>; [e-mail]
> Subject: [ima] 9165b814d2:
> BUG:kernel_NULL_pointer_dereference,address
> 
> FYI, we noticed the following commit (built with gcc-7):
> 
> commit: 9165b814d2bea8cfeb557505bb206396331e8192 ("[PATCH v2 8/8]
> ima: Use ima_hash_algo for collision detection in the measurement list")
> url: https://github.com/0day-ci/linux/commits/Roberto-Sassu/ima-support-
> stronger-algorithms-for-attestation/20200205-233901
> base: https://git.kernel.org/cgit/linux/kernel/git/zohar/linux-integrity.git
> next-integrity

Hi

thanks for the report.

Yes, version 2 had a bug:

---
		ima_algo_array[i++].algo = HASH_ALGO_SHA1;
	}

	if (ima_hash_algo_idx >= nr_allocated_banks) {
		ima_algo_array[i].tfm = ima_shash_tfm;
		ima_algo_array[i].algo = ima_hash_algo;
	}
---

The code allocated ima_algo_array with size 1 (TPM was not found and
the default algorithm is SHA1).

However, later it initializes ima_algo_array for SHA1 and increments the
i variable. Since the code does not check if the default algorithm is SHA1,
the last part is also executed and causes corruption, because ima_algo_array
has only one element.

I fixed already this bug in version 3 of the patch set.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Patch

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 4843077dc9e8..23d63bb96d2c 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -51,6 +51,7 @@  extern int ima_policy_flag;
 /* set during initialization */
 extern int ima_hash_algo;
 extern int ima_sha1_idx;
+extern int ima_hash_algo_idx;
 extern int ima_num_template_digests;
 extern int ima_appraise;
 extern struct tpm_chip *ima_tpm_chip;
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 1ee813d33bdc..f391ee3412b9 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -65,6 +65,7 @@  struct ima_algo_desc {
 };
 
 int ima_sha1_idx;
+int ima_hash_algo_idx;
 int ima_num_template_digests;
 
 static struct ima_algo_desc *ima_algo_array;
@@ -123,16 +124,26 @@  int __init ima_init_crypto(void)
 		nr_allocated_banks = ima_tpm_chip->nr_allocated_banks;
 
 	ima_sha1_idx = -1;
+	ima_hash_algo_idx = -1;
 	ima_num_template_digests = nr_allocated_banks;
 
 	for (i = 0; i < nr_allocated_banks; i++) {
 		algo = ima_tpm_chip->allocated_banks[i].crypto_id;
 		if (algo == HASH_ALGO_SHA1)
 			ima_sha1_idx = i;
+
+		if (algo == ima_hash_algo)
+			ima_hash_algo_idx = i;
 	}
 
-	if (ima_sha1_idx < 0)
+	if (ima_sha1_idx < 0) {
 		ima_sha1_idx = ima_num_template_digests++;
+		if (ima_hash_algo == HASH_ALGO_SHA1)
+			ima_hash_algo_idx = ima_sha1_idx;
+	}
+
+	if (ima_hash_algo_idx < 0)
+		ima_hash_algo_idx = ima_num_template_digests++;
 
 	ima_algo_array = kcalloc(ima_num_template_digests,
 				 sizeof(*ima_algo_array), GFP_KERNEL);
@@ -173,7 +184,12 @@  int __init ima_init_crypto(void)
 			goto out_array;
 		}
 
-		ima_algo_array[i].algo = HASH_ALGO_SHA1;
+		ima_algo_array[i++].algo = HASH_ALGO_SHA1;
+	}
+
+	if (ima_hash_algo_idx >= nr_allocated_banks) {
+		ima_algo_array[i].tfm = ima_shash_tfm;
+		ima_algo_array[i].algo = ima_hash_algo;
 	}
 
 	return 0;
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 7f7509774b85..58983d0f0214 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -57,8 +57,8 @@  static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
 	key = ima_hash_key(digest_value);
 	rcu_read_lock();
 	hlist_for_each_entry_rcu(qe, &ima_htable.queue[key], hnext) {
-		rc = memcmp(qe->entry->digests[ima_sha1_idx].digest,
-			    digest_value, TPM_DIGEST_SIZE);
+		rc = memcmp(qe->entry->digests[ima_hash_algo_idx].digest,
+			    digest_value, hash_digest_size[ima_hash_algo]);
 		if ((rc == 0) && (qe->entry->pcr == pcr)) {
 			ret = qe;
 			break;
@@ -110,7 +110,7 @@  static int ima_add_digest_entry(struct ima_template_entry *entry,
 
 	atomic_long_inc(&ima_htable.len);
 	if (update_htable) {
-		key = ima_hash_key(entry->digests[ima_sha1_idx].digest);
+		key = ima_hash_key(entry->digests[ima_hash_algo_idx].digest);
 		hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
 	}
 
@@ -162,7 +162,7 @@  int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 			   const char *op, struct inode *inode,
 			   const unsigned char *filename)
 {
-	u8 *digest = entry->digests[ima_sha1_idx].digest;
+	u8 *digest = entry->digests[ima_hash_algo_idx].digest;
 	struct tpm_digest *digests_arg = entry->digests;
 	const char *audit_cause = "hash_added";
 	char tpm_audit_cause[AUDIT_CAUSE_LEN_MAX];