IMA extends Platform Configuration Registers (PCRs) of the TPM to give a
proof to a remote verifier that the measurement list contains all
measurements done by the kernel and that the list was not maliciously
modified by an attacker.
IMA was originally designed to extend PCRs with a SHA1 digest, provided
with the measurement list, and was subsequently updated to extend all PCR
banks in case a TPM 2.0 is used. Non-SHA1 PCR banks are not supposed to be
used for remote attestation, as they are extended with a SHA1 digest padded
with zeros, which does not increase the strength.
This patch set addresses this issue by extending PCRs with the digest of
the measurement entry calculated with the crypto subsystem. The list of
algorithms used to calculate the digest are taken from
ima_tpm_chip->allocated_banks, returned by the TPM driver. The SHA1 digest
is always calculated, as SHA1 still remains the default algorithm for the
template digest in the measurement list.
This patch set also makes two additional modifications related to the usage
of hash algorithms. First, since now the template digest for the default
IMA algorithm is always calculated, this is used for hash collision
detection, to check if there are duplicate measurement entries.
Second, it uses the default IMA hash algorithm to calculate the boot
aggregate, assuming that the corresponding PCR bank is currently allocated.
Otherwise, it finds the first PCR bank for which the crypto ID is known.
IMA initialization fails only if no algorithm known to the crypto subsystem
is found.
This patch set does not yet modify the format of the measurement list to
provide the digests passed to the TPM. However, reconstructing the value of
the quoted PCR is still possible for the verifier by calculating the digest
on measurement data found in binary_runtime_measurements.
The attest-tools library [1] has been updated to verify non-SHA1 PCR
banks [2] and to handle non-SHA1 boot aggregate [3].
[1] https://github.com/euleros/attest-tools/tree/0.2-devel
[2] https://github.com/euleros/attest-tools/commit/282a0b1a5e6d9c87adf21561018528d7bbdc7f38
[3] https://github.com/euleros/attest-tools/commit/3a4c8e250fde7661257aba022d677bf0af5399da
Roberto Sassu (8):
tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST
ima: evaluate error in init_ima()
ima: store template digest directly in ima_template_entry
ima: switch to dynamically allocated buffer for template digests
ima: allocate and initialize tfm for each PCR bank
ima: calculate and extend PCR with digests in ima_template_entry
ima: use ima_hash_algo for collision detection in the measurement list
ima: switch to ima_hash_algo for boot aggregate
drivers/char/tpm/tpm2-cmd.c | 2 +
security/integrity/ima/ima.h | 7 +-
security/integrity/ima/ima_api.c | 20 ++-
security/integrity/ima/ima_crypto.c | 219 ++++++++++++++++++++------
security/integrity/ima/ima_fs.c | 4 +-
security/integrity/ima/ima_init.c | 6 +-
security/integrity/ima/ima_main.c | 6 +
security/integrity/ima/ima_queue.c | 36 +++--
security/integrity/ima/ima_template.c | 22 ++-
9 files changed, 241 insertions(+), 81 deletions(-)
--
2.17.1
This patch modifies ima_calc_field_array_hash() to calculate a template
digest for each allocated PCR bank and SHA1. It also passes the tpm_digest
array of the template entry to ima_pcr_extend() or in case of a violation,
the pre-initialized digests array filled with 0xff.
Padding with zeros is still done if the mapping between TPM algorithm ID
and crypto ID is unknown.
This patch calculates again the template digest when a measurement list is
restored. Copying only the SHA1 digest (due to the limitation of the
current measurement list format) is not sufficient, as hash collision
detection will be done on the digest calculated with the default IMA hash
algorithm.
Signed-off-by: Roberto Sassu <[email protected]>
---
security/integrity/ima/ima_crypto.c | 26 ++++++++++++++++++++++-
security/integrity/ima/ima_queue.c | 30 ++++++++++++++++-----------
security/integrity/ima/ima_template.c | 14 +++++++++++--
3 files changed, 55 insertions(+), 15 deletions(-)
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 63fb4bdf80b0..786340feebbb 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -610,9 +610,33 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
int ima_calc_field_array_hash(struct ima_field_data *field_data,
struct ima_template_entry *entry)
{
- int rc;
+ u16 alg_id;
+ int rc, i;
rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_sha1_idx);
+ if (rc)
+ return rc;
+
+ entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
+
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+ if (i == ima_sha1_idx)
+ continue;
+
+ alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
+ entry->digests[i].alg_id = alg_id;
+
+ if (!ima_algo_array[i].tfm) {
+ memcpy(entry->digests[i].digest,
+ entry->digests[ima_sha1_idx].digest,
+ TPM_DIGEST_SIZE);
+ continue;
+ }
+
+ rc = ima_calc_field_array_hash_tfm(field_data, entry, i);
+ if (rc)
+ return rc;
+ }
return rc;
}
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index bcd99db9722c..7f7509774b85 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -137,18 +137,14 @@ unsigned long ima_get_binary_runtime_size(void)
return binary_runtime_size + sizeof(struct ima_kexec_hdr);
};
-static int ima_pcr_extend(const u8 *hash, int pcr)
+static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
{
int result = 0;
- int i;
if (!ima_tpm_chip)
return result;
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++)
- memcpy(digests[i].digest, hash, TPM_DIGEST_SIZE);
-
- result = tpm_pcr_extend(ima_tpm_chip, pcr, digests);
+ result = tpm_pcr_extend(ima_tpm_chip, pcr, digests_arg);
if (result != 0)
pr_err("Error Communicating to TPM chip, result: %d\n", result);
return result;
@@ -166,7 +162,8 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
const char *op, struct inode *inode,
const unsigned char *filename)
{
- u8 digest[TPM_DIGEST_SIZE];
+ u8 *digest = entry->digests[ima_sha1_idx].digest;
+ struct tpm_digest *digests_arg = entry->digests;
const char *audit_cause = "hash_added";
char tpm_audit_cause[AUDIT_CAUSE_LEN_MAX];
int audit_info = 1;
@@ -174,8 +171,6 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
mutex_lock(&ima_extend_list_mutex);
if (!violation) {
- memcpy(digest, entry->digests[ima_sha1_idx].digest,
- sizeof(digest));
if (ima_lookup_digest_entry(digest, entry->pcr)) {
audit_cause = "hash_exists";
result = -EEXIST;
@@ -191,9 +186,9 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
}
if (violation) /* invalidate pcr */
- memset(digest, 0xff, sizeof(digest));
+ digests_arg = digests;
- tpmresult = ima_pcr_extend(digest, entry->pcr);
+ tpmresult = ima_pcr_extend(digests_arg, entry->pcr);
if (tpmresult != 0) {
snprintf(tpm_audit_cause, AUDIT_CAUSE_LEN_MAX, "TPM_error(%d)",
tpmresult);
@@ -219,6 +214,8 @@ int ima_restore_measurement_entry(struct ima_template_entry *entry)
int __init ima_init_digests(void)
{
+ u16 digest_size;
+ u16 crypto_id;
int i;
if (!ima_tpm_chip)
@@ -229,8 +226,17 @@ int __init ima_init_digests(void)
if (!digests)
return -ENOMEM;
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++)
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
+ digest_size = ima_tpm_chip->allocated_banks[i].digest_size;
+ crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
+
+ /* for unmapped TPM algorithms digest is still a padded SHA1 */
+ if (crypto_id == HASH_ALGO__LAST)
+ digest_size = SHA1_DIGEST_SIZE;
+
+ memset(digests[i].digest, 0xff, digest_size);
+ }
return 0;
}
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 2e9c5d99e70e..a9e1390c8e12 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -356,6 +356,7 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
int ima_restore_measurement_list(loff_t size, void *buf)
{
char template_name[MAX_TEMPLATE_NAME_LEN];
+ unsigned char zero[TPM_DIGEST_SIZE] = { 0 };
struct ima_kexec_hdr *khdr = buf;
struct ima_field_data hdr[HDR__LAST] = {
@@ -455,8 +456,17 @@ int ima_restore_measurement_list(loff_t size, void *buf)
if (ret < 0)
break;
- memcpy(entry->digests[ima_sha1_idx].digest,
- hdr[HDR_DIGEST].data, hdr[HDR_DIGEST].len);
+ if (memcmp(hdr[HDR_DIGEST].data, zero, sizeof(zero))) {
+ ret = ima_calc_field_array_hash(
+ &entry->template_data[0],
+ entry);
+ if (ret < 0) {
+ pr_err("cannot calculate template digest\n");
+ ret = -EINVAL;
+ break;
+ }
+ }
+
entry->pcr = !ima_canonical_fmt ? *(hdr[HDR_PCR].data) :
le32_to_cpu(*(hdr[HDR_PCR].data));
ret = ima_restore_measurement_entry(entry);
--
2.17.1
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.
Signed-off-by: Roberto Sassu <[email protected]>
---
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_api.c | 2 +-
security/integrity/ima/ima_crypto.c | 25 ++++++++++++++++++-------
security/integrity/ima/ima_main.c | 1 +
security/integrity/ima/ima_queue.c | 8 ++++----
security/integrity/ima/ima_template.c | 2 +-
6 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d959dab5bcce..3e1afa5150bc 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_appraise;
extern struct tpm_chip *ima_tpm_chip;
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index ebaf0056735c..a9bb45de6db9 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -51,7 +51,7 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
if (!*entry)
return -ENOMEM;
- (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
+ (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
sizeof(*(*entry)->digests), GFP_NOFS);
if (!(*entry)->digests) {
result = -ENOMEM;
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 786340feebbb..f84dfd8fc5ca 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -93,7 +93,7 @@ static struct crypto_shash *ima_alloc_tfm(enum hash_algo algo)
if (algo == ima_hash_algo)
return tfm;
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++)
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++)
if (ima_algo_array[i].algo == algo && ima_algo_array[i].tfm)
return ima_algo_array[i].tfm;
@@ -116,19 +116,20 @@ int __init ima_init_crypto(void)
if (rc)
return rc;
- ima_algo_array = kmalloc_array(ima_tpm_chip->nr_allocated_banks + 1,
+ ima_algo_array = kmalloc_array(ima_tpm_chip->nr_allocated_banks + 2,
sizeof(*ima_algo_array), GFP_KERNEL);
if (!ima_algo_array) {
rc = -ENOMEM;
goto out;
}
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++) {
ima_algo_array[i].tfm = NULL;
ima_algo_array[i].algo = HASH_ALGO__LAST;
}
ima_sha1_idx = -1;
+ ima_hash_algo_idx = -1;
for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
algo = ima_tpm_chip->allocated_banks[i].crypto_id;
@@ -143,6 +144,7 @@ int __init ima_init_crypto(void)
if (algo == ima_hash_algo) {
ima_algo_array[i].tfm = ima_shash_tfm;
+ ima_hash_algo_idx = i;
continue;
}
@@ -166,12 +168,21 @@ int __init ima_init_crypto(void)
}
ima_sha1_idx = i;
- ima_algo_array[i].algo = HASH_ALGO_SHA1;
+ if (ima_hash_algo == HASH_ALGO_SHA1)
+ ima_hash_algo_idx = i;
+
+ ima_algo_array[i++].algo = HASH_ALGO_SHA1;
+ }
+
+ if (ima_hash_algo_idx < 0) {
+ ima_algo_array[i].tfm = ima_shash_tfm;
+ ima_algo_array[i].algo = ima_hash_algo;
+ ima_hash_algo_idx = i;
}
return 0;
out_array:
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++) {
if (!ima_algo_array[i].tfm ||
ima_algo_array[i].tfm == ima_shash_tfm)
continue;
@@ -190,7 +201,7 @@ static void ima_free_tfm(struct crypto_shash *tfm)
if (tfm == ima_shash_tfm)
return;
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++)
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++)
if (ima_algo_array[i].tfm == tfm)
return;
@@ -619,7 +630,7 @@ int ima_calc_field_array_hash(struct ima_field_data *field_data,
entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
+ for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 2; i++) {
if (i == ima_sha1_idx)
continue;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c068067a0c47..6963d6c31bf1 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -31,6 +31,7 @@
#include "ima.h"
int ima_sha1_idx;
+int ima_hash_algo_idx;
#ifdef CONFIG_IMA_APPRAISE
int ima_appraise = IMA_APPRAISE_ENFORCE;
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];
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index a9e1390c8e12..f71b5ee44179 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -311,7 +311,7 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
if (!*entry)
return -ENOMEM;
- (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
+ (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
sizeof(*(*entry)->digests), GFP_NOFS);
if (!(*entry)->digests) {
kfree(*entry);
--
2.17.1
boot_aggregate is the first entry of IMA measurement list. Its purpose is
to link pre-boot measurements to IMA measurements. As IMA was designed to
work with a TPM 1.2, the SHA1 PCR bank was always selected.
Currently, even if a TPM 2.0 is used, the SHA1 PCR bank is selected.
However, the assumption that the SHA1 PCR bank is always available is not
correct, as PCR banks can be selected with the PCR_Allocate() TPM command.
This patch tries to use ima_hash_algo as hash algorithm for boot_aggregate.
If no PCR bank uses that algorithm, the patch scans the allocated PCR banks
and selects the first for which the mapping between TPM algorithm ID and
crypto algorithm ID is known. If no suitable algorithm is found, an error
is returned.
Signed-off-by: Roberto Sassu <[email protected]>
---
security/integrity/ima/ima_crypto.c | 38 +++++++++++++++++------------
security/integrity/ima/ima_init.c | 6 ++---
2 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index f84dfd8fc5ca..9bf5e69945b7 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -780,25 +780,27 @@ static void __init ima_pcrread(u32 idx, struct tpm_digest *d)
/*
* Calculate the boot aggregate hash
*/
-static int __init ima_calc_boot_aggregate_tfm(char *digest,
- struct crypto_shash *tfm)
+static int __init ima_calc_boot_aggregate_tfm(char *digest, int bank_idx)
{
- struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
+ struct tpm_digest d = { .digest = {0} };
int rc;
u32 i;
- SHASH_DESC_ON_STACK(shash, tfm);
+ SHASH_DESC_ON_STACK(shash, ima_algo_array[bank_idx].tfm);
- shash->tfm = tfm;
+ shash->tfm = ima_algo_array[bank_idx].tfm;
rc = crypto_shash_init(shash);
if (rc != 0)
return rc;
+ d.alg_id = ima_tpm_chip->allocated_banks[bank_idx].alg_id;
+
/* cumulative sha1 over tpm registers 0-7 */
for (i = TPM_PCR0; i < TPM_PCR8; i++) {
ima_pcrread(i, &d);
/* now accumulate with current aggregate */
- rc = crypto_shash_update(shash, d.digest, TPM_DIGEST_SIZE);
+ rc = crypto_shash_update(shash, d.digest,
+ ima_tpm_chip->allocated_banks[bank_idx].digest_size);
}
if (!rc)
crypto_shash_final(shash, digest);
@@ -807,17 +809,21 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
int __init ima_calc_boot_aggregate(struct ima_digest_data *hash)
{
- struct crypto_shash *tfm;
- int rc;
+ int bank_idx = ima_hash_algo_idx;
- tfm = ima_alloc_tfm(hash->algo);
- if (IS_ERR(tfm))
- return PTR_ERR(tfm);
-
- hash->length = crypto_shash_digestsize(tfm);
- rc = ima_calc_boot_aggregate_tfm(hash->digest, tfm);
+ if (bank_idx >= ima_tpm_chip->nr_allocated_banks) {
+ for (bank_idx = 0; bank_idx < ima_tpm_chip->nr_allocated_banks;
+ bank_idx++)
+ if (ima_algo_array[bank_idx].tfm)
+ break;
- ima_free_tfm(tfm);
+ if (bank_idx == ima_tpm_chip->nr_allocated_banks) {
+ pr_err("No suitable algo found for boot aggregate\n");
+ return -ENOENT;
+ }
+ }
- return rc;
+ hash->algo = ima_algo_array[bank_idx].algo;
+ hash->length = crypto_shash_digestsize(ima_algo_array[bank_idx].tfm);
+ return ima_calc_boot_aggregate_tfm(hash->digest, bank_idx);
}
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 195cb4079b2b..b4da190a33ba 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -51,14 +51,14 @@ static int __init ima_add_boot_aggregate(void)
int violation = 0;
struct {
struct ima_digest_data hdr;
- char digest[TPM_DIGEST_SIZE];
+ char digest[SHA512_DIGEST_SIZE];
} hash;
memset(iint, 0, sizeof(*iint));
memset(&hash, 0, sizeof(hash));
iint->ima_hash = &hash.hdr;
- iint->ima_hash->algo = HASH_ALGO_SHA1;
- iint->ima_hash->length = SHA1_DIGEST_SIZE;
+ iint->ima_hash->algo = ima_hash_algo;
+ iint->ima_hash->length = hash_digest_size[ima_hash_algo];
if (ima_tpm_chip) {
result = ima_calc_boot_aggregate(&hash.hdr);
--
2.17.1
This patch dynamically allocates the array of tpm_digest structures in
ima_alloc_init_template() and ima_restore_template_data(). It also
allocates an item in addition to the number of allocated PCR banks, to
store a SHA1 digest when the SHA1 PCR bank is not allocated.
Calculating the SHA1 digest is mandatory, as SHA1 still remains the default
hash algorithm for the measurement list. When IMA will support the Crypto
Agile format, remaining digests will be also provided.
The position in the array of the SHA1 digest is stored in the ima_sha1_idx
global variable and it is determined at IMA initialization time.
Signed-off-by: Roberto Sassu <[email protected]>
---
security/integrity/ima/ima.h | 3 ++-
security/integrity/ima/ima_api.c | 8 ++++++++
security/integrity/ima/ima_crypto.c | 3 ++-
security/integrity/ima/ima_fs.c | 4 ++--
security/integrity/ima/ima_main.c | 2 ++
security/integrity/ima/ima_queue.c | 10 ++++++----
security/integrity/ima/ima_template.c | 12 ++++++++++--
7 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 6238c50ae44e..d959dab5bcce 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -50,6 +50,7 @@ extern int ima_policy_flag;
/* set during initialization */
extern int ima_hash_algo;
+extern int ima_sha1_idx;
extern int ima_appraise;
extern struct tpm_chip *ima_tpm_chip;
@@ -92,7 +93,7 @@ struct ima_template_desc {
struct ima_template_entry {
int pcr;
- u8 digest[TPM_DIGEST_SIZE]; /* sha1 or md5 measurement hash */
+ struct tpm_digest *digests;
struct ima_template_desc *template_desc; /* template descriptor */
u32 template_data_len;
struct ima_field_data template_data[0]; /* template related data */
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 2ef5a40c7ca5..ebaf0056735c 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -27,6 +27,7 @@ void ima_free_template_entry(struct ima_template_entry *entry)
for (i = 0; i < entry->template_desc->num_fields; i++)
kfree(entry->template_data[i].data);
+ kfree(entry->digests);
kfree(entry);
}
@@ -50,6 +51,13 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
if (!*entry)
return -ENOMEM;
+ (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
+ sizeof(*(*entry)->digests), GFP_NOFS);
+ if (!(*entry)->digests) {
+ result = -ENOMEM;
+ goto out;
+ }
+
(*entry)->template_desc = template_desc;
for (i = 0; i < template_desc->num_fields; i++) {
const struct ima_template_field *field =
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index f2d9d4b18499..57b06321d5c8 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -504,7 +504,8 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
}
if (!rc)
- rc = crypto_shash_final(shash, entry->digest);
+ rc = crypto_shash_final(shash,
+ entry->digests[ima_sha1_idx].digest);
return rc;
}
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 2000e8df0301..2b79581d0e25 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -152,7 +152,7 @@ int ima_measurements_show(struct seq_file *m, void *v)
ima_putc(m, &pcr, sizeof(e->pcr));
/* 2nd: template digest */
- ima_putc(m, e->digest, TPM_DIGEST_SIZE);
+ ima_putc(m, e->digests[ima_sha1_idx].digest, TPM_DIGEST_SIZE);
/* 3rd: template name size */
namelen = !ima_canonical_fmt ? strlen(template_name) :
@@ -235,7 +235,7 @@ static int ima_ascii_measurements_show(struct seq_file *m, void *v)
seq_printf(m, "%2d ", e->pcr);
/* 2nd: SHA1 template hash */
- ima_print_digest(m, e->digest, TPM_DIGEST_SIZE);
+ ima_print_digest(m, e->digests[ima_sha1_idx].digest, TPM_DIGEST_SIZE);
/* 3th: template name */
seq_printf(m, " %s", template_name);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2f95b133f246..c068067a0c47 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -30,6 +30,8 @@
#include "ima.h"
+int ima_sha1_idx;
+
#ifdef CONFIG_IMA_APPRAISE
int ima_appraise = IMA_APPRAISE_ENFORCE;
#else
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 1ce8b1701566..bcd99db9722c 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -57,7 +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->digest, digest_value, TPM_DIGEST_SIZE);
+ rc = memcmp(qe->entry->digests[ima_sha1_idx].digest,
+ digest_value, TPM_DIGEST_SIZE);
if ((rc == 0) && (qe->entry->pcr == pcr)) {
ret = qe;
break;
@@ -77,7 +78,7 @@ static int get_binary_runtime_size(struct ima_template_entry *entry)
int size = 0;
size += sizeof(u32); /* pcr */
- size += sizeof(entry->digest);
+ size += TPM_DIGEST_SIZE;
size += sizeof(int); /* template name size field */
size += strlen(entry->template_desc->name);
size += sizeof(entry->template_data_len);
@@ -109,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->digest);
+ key = ima_hash_key(entry->digests[ima_sha1_idx].digest);
hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
}
@@ -173,7 +174,8 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
mutex_lock(&ima_extend_list_mutex);
if (!violation) {
- memcpy(digest, entry->digest, sizeof(digest));
+ memcpy(digest, entry->digests[ima_sha1_idx].digest,
+ sizeof(digest));
if (ima_lookup_digest_entry(digest, entry->pcr)) {
audit_cause = "hash_exists";
result = -EEXIST;
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 6aa6408603e3..2e9c5d99e70e 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -311,11 +311,19 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
if (!*entry)
return -ENOMEM;
+ (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
+ sizeof(*(*entry)->digests), GFP_NOFS);
+ if (!(*entry)->digests) {
+ kfree(*entry);
+ return -ENOMEM;
+ }
+
ret = ima_parse_buf(template_data, template_data + template_data_size,
NULL, template_desc->num_fields,
(*entry)->template_data, NULL, NULL,
ENFORCE_FIELDS | ENFORCE_BUFEND, "template data");
if (ret < 0) {
+ kfree((*entry)->digests);
kfree(*entry);
return ret;
}
@@ -447,8 +455,8 @@ int ima_restore_measurement_list(loff_t size, void *buf)
if (ret < 0)
break;
- memcpy(entry->digest, hdr[HDR_DIGEST].data,
- hdr[HDR_DIGEST].len);
+ memcpy(entry->digests[ima_sha1_idx].digest,
+ hdr[HDR_DIGEST].data, hdr[HDR_DIGEST].len);
entry->pcr = !ima_canonical_fmt ? *(hdr[HDR_PCR].data) :
le32_to_cpu(*(hdr[HDR_PCR].data));
ret = ima_restore_measurement_entry(entry);
--
2.17.1
> -----Original Message-----
> From: Roberto Sassu
> Sent: Monday, January 27, 2020 6:05 PM
> To: [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> Silviu Vlasceanu <[email protected]>; Roberto Sassu
> <[email protected]>
> Subject: [PATCH 6/8] ima: calculate and extend PCR with digests in
> ima_template_entry
>
> This patch modifies ima_calc_field_array_hash() to calculate a template
> digest for each allocated PCR bank and SHA1. It also passes the tpm_digest
> array of the template entry to ima_pcr_extend() or in case of a violation,
> the pre-initialized digests array filled with 0xff.
>
> Padding with zeros is still done if the mapping between TPM algorithm ID
> and crypto ID is unknown.
>
> This patch calculates again the template digest when a measurement list is
> restored. Copying only the SHA1 digest (due to the limitation of the
> current measurement list format) is not sufficient, as hash collision
> detection will be done on the digest calculated with the default IMA hash
> algorithm.
>
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> security/integrity/ima/ima_crypto.c | 26 ++++++++++++++++++++++-
> security/integrity/ima/ima_queue.c | 30 ++++++++++++++++-----------
> security/integrity/ima/ima_template.c | 14 +++++++++++--
> 3 files changed, 55 insertions(+), 15 deletions(-)
>
> diff --git a/security/integrity/ima/ima_crypto.c
> b/security/integrity/ima/ima_crypto.c
> index 63fb4bdf80b0..786340feebbb 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -610,9 +610,33 @@ static int ima_calc_field_array_hash_tfm(struct
> ima_field_data *field_data,
> int ima_calc_field_array_hash(struct ima_field_data *field_data,
> struct ima_template_entry *entry)
> {
> - int rc;
> + u16 alg_id;
> + int rc, i;
>
> rc = ima_calc_field_array_hash_tfm(field_data, entry,
> ima_sha1_idx);
> + if (rc)
> + return rc;
> +
> + entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
> +
> + for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> + if (i == ima_sha1_idx)
> + continue;
> +
> + alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
The line above should be executed for i < ima_tpm_chip->nr_allocated_banks.
I will fix in the next version of the patch set.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> 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.
>
> Signed-off-by: Roberto Sassu <[email protected]>
Definitely needed to protect against a sha1 collision attack.
<snip>
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index ebaf0056735c..a9bb45de6db9 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
> if (!*entry)
> return -ENOMEM;
>
> - (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
> + (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
> sizeof(*(*entry)->digests), GFP_NOFS);
> if (!(*entry)->digests) {
> result = -ENOMEM;
I would prefer not having to allocate and use "nr_allocated_banks + 1"
everywhere, but I understand the need for it. I'm not sure this patch
warrants allocating +2. Perhaps, if a TPM bank doesn't exist for the
IMA default hash algorithm, use a different algorithm or, worst case,
continue using the ima_sha1_idx.
Mimi
Hi Roberto,
On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> IMA extends Platform Configuration Registers (PCRs) of the TPM to give a
> proof to a remote verifier that the measurement list contains all
> measurements done by the kernel and that the list was not maliciously
> modified by an attacker.
>
> IMA was originally designed to extend PCRs with a SHA1 digest, provided
> with the measurement list, and was subsequently updated to extend all PCR
> banks in case a TPM 2.0 is used. Non-SHA1 PCR banks are not supposed to be
> used for remote attestation, as they are extended with a SHA1 digest padded
> with zeros, which does not increase the strength.
>
> This patch set addresses this issue by extending PCRs with the digest of
> the measurement entry calculated with the crypto subsystem. The list of
> algorithms used to calculate the digest are taken from
> ima_tpm_chip->allocated_banks, returned by the TPM driver. The SHA1 digest
> is always calculated, as SHA1 still remains the default algorithm for the
> template digest in the measurement list.
>
> This patch set also makes two additional modifications related to the usage
> of hash algorithms. First, since now the template digest for the default
> IMA algorithm is always calculated, this is used for hash collision
> detection, to check if there are duplicate measurement entries.
>
> Second, it uses the default IMA hash algorithm to calculate the boot
> aggregate, assuming that the corresponding PCR bank is currently allocated.
> Otherwise, it finds the first PCR bank for which the crypto ID is known.
> IMA initialization fails only if no algorithm known to the crypto subsystem
> is found.
>
> This patch set does not yet modify the format of the measurement list to
> provide the digests passed to the TPM. However, reconstructing the value of
> the quoted PCR is still possible for the verifier by calculating the digest
> on measurement data found in binary_runtime_measurements.
Thank you! I'm still reviewing and testing the patches, but it is
really nicely written.
Mimi
> -----Original Message-----
> From: [email protected] [mailto:linux-integrity-
> [email protected]] On Behalf Of Mimi Zohar
> Sent: Thursday, January 30, 2020 11:26 PM
> To: Roberto Sassu <[email protected]>;
> [email protected];
> [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> Silviu Vlasceanu <[email protected]>
> Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in
> the measurement list
>
> On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > 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.
> >
> > Signed-off-by: Roberto Sassu <[email protected]>
>
> Definitely needed to protect against a sha1 collision attack.
>
> <snip>
>
> >
> > diff --git a/security/integrity/ima/ima_api.c
> b/security/integrity/ima/ima_api.c
> > index ebaf0056735c..a9bb45de6db9 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct ima_event_data
> *event_data,
> > if (!*entry)
> > return -ENOMEM;
> >
> > - (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
> > + (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
> > sizeof(*(*entry)->digests), GFP_NOFS);
> > if (!(*entry)->digests) {
> > result = -ENOMEM;
>
> I would prefer not having to allocate and use "nr_allocated_banks + 1"
> everywhere, but I understand the need for it. I'm not sure this patch
> warrants allocating +2. Perhaps, if a TPM bank doesn't exist for the
> IMA default hash algorithm, use a different algorithm or, worst case,
> continue using the ima_sha1_idx.
We could introduce a new option called ima_hash_algo_tpm to specify
the algorithm of an allocated bank. We can use this for boot_aggregate
and hash collision detection.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
On Fri, 2020-01-31 at 14:02 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: [email protected] [mailto:linux-integrity-
> > [email protected]] On Behalf Of Mimi Zohar
> > Sent: Thursday, January 30, 2020 11:26 PM
> > To: Roberto Sassu <[email protected]>;
> > [email protected];
> > [email protected]; [email protected]
> > Cc: [email protected]; [email protected];
> > Silviu Vlasceanu <[email protected]>
> > Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in
> > the measurement list
> >
> > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > 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.
> > >
> > > Signed-off-by: Roberto Sassu <[email protected]>
> >
> > Definitely needed to protect against a sha1 collision attack.
> >
> > <snip>
> >
> > >
> > > diff --git a/security/integrity/ima/ima_api.c
> > b/security/integrity/ima/ima_api.c
> > > index ebaf0056735c..a9bb45de6db9 100644
> > > --- a/security/integrity/ima/ima_api.c
> > > +++ b/security/integrity/ima/ima_api.c
> > > @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct ima_event_data
> > *event_data,
> > > if (!*entry)
> > > return -ENOMEM;
> > >
> > > - (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
> > > + (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
> > > sizeof(*(*entry)->digests), GFP_NOFS);
> > > if (!(*entry)->digests) {
> > > result = -ENOMEM;
> >
> > I would prefer not having to allocate and use "nr_allocated_banks + 1"
> > everywhere, but I understand the need for it. I'm not sure this patch
> > warrants allocating +2. Perhaps, if a TPM bank doesn't exist for the
> > IMA default hash algorithm, use a different algorithm or, worst case,
> > continue using the ima_sha1_idx.
>
> We could introduce a new option called ima_hash_algo_tpm to specify
> the algorithm of an allocated bank. We can use this for boot_aggregate
> and hash collision detection.
I don't think that would work in the case where the IMA default hash
is set to sha256, but the system has a TPM 1.2 chip. We would be left
using SHA1 for the file hash collision detection.
With my suggestion of defining an "extra" variable, I kind of back
tracked here. There are two problems that I'm trying to address -
hard coding the number of additional "banks" and unnecessarily
allocating more memory than necessary. By pre-walking the list,
calculating the "extra" banks, you'll resolve both issues.
Mimi
> -----Original Message-----
> From: Mimi Zohar [mailto:[email protected]]
> Sent: Friday, January 31, 2020 3:22 PM
> To: Roberto Sassu <[email protected]>;
> [email protected];
> [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> Silviu Vlasceanu <[email protected]>
> Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in
> the measurement list
>
> On Fri, 2020-01-31 at 14:02 +0000, Roberto Sassu wrote:
> > > -----Original Message-----
> > > From: [email protected] [mailto:linux-integrity-
> > > [email protected]] On Behalf Of Mimi Zohar
> > > Sent: Thursday, January 30, 2020 11:26 PM
> > > To: Roberto Sassu <[email protected]>;
> > > [email protected];
> > > [email protected]; linux-
> [email protected]
> > > Cc: [email protected]; linux-
> [email protected];
> > > Silviu Vlasceanu <[email protected]>
> > > Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection
> in
> > > the measurement list
> > >
> > > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > > 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.
> > > >
> > > > Signed-off-by: Roberto Sassu <[email protected]>
> > >
> > > Definitely needed to protect against a sha1 collision attack.
> > >
> > > <snip>
> > >
> > > >
> > > > diff --git a/security/integrity/ima/ima_api.c
> > > b/security/integrity/ima/ima_api.c
> > > > index ebaf0056735c..a9bb45de6db9 100644
> > > > --- a/security/integrity/ima/ima_api.c
> > > > +++ b/security/integrity/ima/ima_api.c
> > > > @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct
> ima_event_data
> > > *event_data,
> > > > if (!*entry)
> > > > return -ENOMEM;
> > > >
> > > > - (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
> > > > + (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
> > > > sizeof(*(*entry)->digests), GFP_NOFS);
> > > > if (!(*entry)->digests) {
> > > > result = -ENOMEM;
> > >
> > > I would prefer not having to allocate and use "nr_allocated_banks + 1"
> > > everywhere, but I understand the need for it. I'm not sure this patch
> > > warrants allocating +2. Perhaps, if a TPM bank doesn't exist for the
> > > IMA default hash algorithm, use a different algorithm or, worst case,
> > > continue using the ima_sha1_idx.
> >
> > We could introduce a new option called ima_hash_algo_tpm to specify
> > the algorithm of an allocated bank. We can use this for boot_aggregate
> > and hash collision detection.
>
> I don't think that would work in the case where the IMA default hash
> is set to sha256, but the system has a TPM 1.2 chip. We would be left
> using SHA1 for the file hash collision detection.
I thought that using a stronger algorithm for hash collision detection but
doing remote attestation with the weaker would not bring additional value.
If there is a hash collision on SHA1, an attacker can still replace the data of
one of the two entries in the measurement list with the data of the other
without being detected (without additional countermeasures).
If the verifier additionally checks for duplicate template digests, he could
detect the attack (IMA would not add a new measurement entry with the
same template digest of previous entries).
Ok, I will use ima_hash_algo for hash collision detection.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
On Fri, 2020-01-31 at 14:41 +0000, Roberto Sassu wrote:
> I thought that using a stronger algorithm for hash collision detection but
> doing remote attestation with the weaker would not bring additional value.
>
> If there is a hash collision on SHA1, an attacker can still replace the data of
> one of the two entries in the measurement list with the data of the other
> without being detected (without additional countermeasures).
>
> If the verifier additionally checks for duplicate template digests, he could
> detect the attack (IMA would not add a new measurement entry with the
> same template digest of previous entries).
>
> Ok, I will use ima_hash_algo for hash collision detection.
Thanks!
Mimi
> -----Original Message-----
> From: Roberto Sassu
> Sent: Monday, January 27, 2020 6:05 PM
> To: [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> Silviu Vlasceanu <[email protected]>; Roberto Sassu
> <[email protected]>
> Subject: [PATCH 8/8] ima: switch to ima_hash_algo for boot aggregate
I will remove this patch from the patch set.
[PATCH v3 1/2] ima: support calculating the boot aggregate based on non-SHA1 algorithms
[PATCH v3 2/2] ima: support calculating the boot_aggregate based on different TPM banks
by Mimi will provide similar functionality.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli