Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 0/8] ima: support stronger algorithms for attestation
@ 2020-02-10 10:00 Roberto Sassu
  2020-02-10 10:00 ` [PATCH v3 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST Roberto Sassu
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Roberto Sassu @ 2020-02-10 10:00 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu

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 SHA256 PCR bank (mandatory for TPM 2.0 in TCG PC
Client specification). Lastly, if that bank was not found, it selects the
first PCR bank. If the TPM algorithm ID of the first PCR bank is not mapped
to a crypto ID, boot_aggregate is set to zero.

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.

attest-tools (https://github.com/euleros/attest-tools, branch 0.2-devel)
has the ability to parse the BIOS and IMA event logs, and to compare
boot_aggregate with the digest of final PCR values obtained by performing
in software the PCR extend operation with digests in the BIOS event log.

To perform the test, it is necessary to have a complete BIOS event log and
to apply the boot_aggregate patches. It would be possible to use qemu and
swtpm from Stefan Berger, but at the moment it is necessary to change the
ACPI parser in drivers/char/tpm/event_log/acpi.c to accept TPM 2.0 and to
return EFI_TCG2_EVENT_LOG_FORMAT_TCG_2.

Create req.json with this content:
---
{
  "reqs":{
    "dummy|verify":"",
    "ima_boot_aggregate|verify":""
  }
}
---

With the requirements above, attest-tools verifies boot_aggregate and
accepts any other entry in the event logs.

On server side run:
# attest_ra_server -p 10 -r req.json -s -i

-s disables TPM signature verification
-i allows IMA violations

To enable TPM signature verification it is necessary to have a valid AK
certificate. It can be obtained by following the instructions at:

https://github.com/euleros/attest-tools/blob/0.2-devel/README

On client side run:
# echo test > aik_cert.pem
# echo aik_cert.pem > list_privacy_ca
# attest_ra_client -A

The commands above generate an AK and tell attest-tools to use a dummy AK
certificate.

# attest_ra_client -s <server IP> -q -p 10 -P <PCR algo> -b -i

The command above sends the TPM quote and the event logs to the RA server
and gets the response (successful/failed verification).

-b includes the BIOS event log from securityfs
-i includes the IMA event log from securityfs

To check that IMA extends non-SHA1 PCR banks with an appropriate digest,
use -P sha256, so that attest_ra_client selects the SHA256 PCR bank. To
check that boot_aggregate is calculated properly, set ima_hash=sha256 in
the kernel command line.

Changelog

v2:
- add NR_BANKS macro to return zero if ima_tpm_chip is NULL
- replace ima_num_template_digests with
  NR_BANKS(ima_tpm_chip) + ima_extra_slots (suggested by Mimi)
- add __ro_after_init to declaration of ima_sha1_idx ima_hash_algo_idx and
  ima_extra_slots (suggested by Mimi)
- declare ima_init_ima_crypto() as static (reported by kbuild test robot)
- use ima_sha1_idx and ima_hash_algo_idx to access ima_algo_array elements
  in ima_init_crypto()

v1:
- move ima_sha1_idx and ima_hash_algo_idx to ima_crypto.c
- introduce ima_num_template_digests (suggested by Mimi)
- determine ima_num_template_digests before allocating ima_algo_array
  (suggested by Mimi)
- replace kmalloc_array() with kcalloc() in ima_init_crypto() (suggested by
  Mimi)
- check if ima_tpm_chip is NULL

Roberto Sassu (8):
  tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST
  ima: Switch to ima_hash_algo for boot aggregate
  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

 drivers/char/tpm/tpm2-cmd.c           |   2 +
 security/integrity/ima/ima.h          |  10 +-
 security/integrity/ima/ima_api.c      |  20 +--
 security/integrity/ima/ima_crypto.c   | 244 ++++++++++++++++++++++----
 security/integrity/ima/ima_fs.c       |   4 +-
 security/integrity/ima/ima_init.c     |  22 ++-
 security/integrity/ima/ima_main.c     |   3 +
 security/integrity/ima/ima_queue.c    |  36 ++--
 security/integrity/ima/ima_template.c |  22 ++-
 9 files changed, 290 insertions(+), 73 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST
  2020-02-10 10:00 [PATCH v3 0/8] ima: support stronger algorithms for attestation Roberto Sassu
@ 2020-02-10 10:00 ` Roberto Sassu
  2020-02-10 16:00   ` Jarkko Sakkinen
  2020-02-10 10:00 ` [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate Roberto Sassu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Roberto Sassu @ 2020-02-10 10:00 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu, stable

chip->allocated_banks, an array of tpm_bank_info structures, contains the
list of TPM algorithm IDs of allocated PCR banks. It also contains the
corresponding ID of the crypto subsystem, so that users of the TPM driver
can calculate a digest for a PCR extend operation.

However, if there is no mapping between TPM algorithm ID and crypto ID, the
crypto_id field of tpm_bank_info remains set to zero (the array is
allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
Zero should not be used as value for unknown mappings, as it is a valid
crypto ID (HASH_ALGO_MD4).

Thus, initialize crypto_id to HASH_ALGO__LAST.

Cc: stable@vger.kernel.org # 5.1.x
Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
 drivers/char/tpm/tpm2-cmd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 13696deceae8..760329598b99 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -525,6 +525,8 @@ static int tpm2_init_bank_info(struct tpm_chip *chip, u32 bank_index)
 		return 0;
 	}
 
+	bank->crypto_id = HASH_ALGO__LAST;
+
 	return tpm2_pcr_read(chip, 0, &digest, &bank->digest_size);
 }
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-02-10 10:00 [PATCH v3 0/8] ima: support stronger algorithms for attestation Roberto Sassu
  2020-02-10 10:00 ` [PATCH v3 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST Roberto Sassu
@ 2020-02-10 10:00 ` Roberto Sassu
  2020-02-10 22:23   ` Mimi Zohar
  2020-02-10 10:00 ` [PATCH v3 3/8] ima: Evaluate error in init_ima() Roberto Sassu
  2020-02-10 10:00 ` [PATCH v3 4/8] ima: Store template digest directly in ima_template_entry Roberto Sassu
  3 siblings, 1 reply; 13+ messages in thread
From: Roberto Sassu @ 2020-02-10 10:00 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu, stable

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 tries to find the SHA256 PCR
bank (which is mandatory in the TCG PC Client specification). If also this
bank is not found, the patch selects the first one. If the TPM algorithm
of that bank is not mapped to a crypto ID, boot_aggregate is set to zero.

Changelog

v1:
- add Mimi's comments
- if there is no PCR bank for the IMA default algorithm use SHA256
  (suggested by James Bottomley)

Cc: stable@vger.kernel.org # 5.1.x
Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_crypto.c | 45 +++++++++++++++++++++++++----
 security/integrity/ima/ima_init.c   | 22 ++++++++++----
 2 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 73044fc6a952..f2f41a2bc3d4 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -655,18 +655,29 @@ static void __init ima_pcrread(u32 idx, struct tpm_digest *d)
 }
 
 /*
- * Calculate the boot aggregate hash
+ * The boot_aggregate is a cumulative hash over TPM registers 0 - 7.  With
+ * TPM 1.2 the boot_aggregate was based on reading the SHA1 PCRs, but with
+ * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks,
+ * allowing firmware to configure and enable different banks.
+ *
+ * Knowing which TPM bank is read to calculate the boot_aggregate digest
+ * needs to be conveyed to a verifier.  For this reason, use the same
+ * hash algorithm for reading the TPM PCRs as for calculating the boot
+ * aggregate digest as stored in the measurement list.
  */
-static int __init ima_calc_boot_aggregate_tfm(char *digest,
+static int __init ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
 					      struct crypto_shash *tfm)
 {
-	struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
+	struct tpm_digest d = { .alg_id = alg_id, .digest = {0} };
 	int rc;
 	u32 i;
 	SHASH_DESC_ON_STACK(shash, tfm);
 
 	shash->tfm = tfm;
 
+	pr_devel("calculating the boot-aggregate based on TPM bank: %04x\n",
+		 d.alg_id);
+
 	rc = crypto_shash_init(shash);
 	if (rc != 0)
 		return rc;
@@ -675,7 +686,8 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
 	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,
+					 crypto_shash_digestsize(tfm));
 	}
 	if (!rc)
 		crypto_shash_final(shash, digest);
@@ -685,14 +697,35 @@ 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;
+	u16 crypto_id, alg_id;
+	int rc, i, bank_idx = 0;
+
+	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
+		crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
+		if (crypto_id == hash->algo) {
+			bank_idx = i;
+			break;
+		}
+
+		if (crypto_id == HASH_ALGO_SHA256)
+			bank_idx = i;
+	}
+
+	if (bank_idx == 0 &&
+	    ima_tpm_chip->allocated_banks[0].crypto_id == HASH_ALGO__LAST) {
+		pr_err("No suitable TPM algorithm for boot aggregate\n");
+		return 0;
+	}
+
+	hash->algo = ima_tpm_chip->allocated_banks[bank_idx].crypto_id;
 
 	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);
+	alg_id = ima_tpm_chip->allocated_banks[bank_idx].alg_id;
+	rc = ima_calc_boot_aggregate_tfm(hash->digest, alg_id, tfm);
 
 	ima_free_tfm(tfm);
 
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 5d55ade5f3b9..fbd7a8e28a6b 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -27,7 +27,7 @@ struct tpm_chip *ima_tpm_chip;
 /* Add the boot aggregate to the IMA measurement list and extend
  * the PCR register.
  *
- * Calculate the boot aggregate, a SHA1 over tpm registers 0-7,
+ * Calculate the boot aggregate, a hash over tpm registers 0-7,
  * assuming a TPM chip exists, and zeroes if the TPM chip does not
  * exist.  Add the boot aggregate measurement to the measurement
  * list and extend the PCR register.
@@ -51,15 +51,27 @@ static int __init ima_add_boot_aggregate(void)
 	int violation = 0;
 	struct {
 		struct ima_digest_data hdr;
-		char digest[TPM_DIGEST_SIZE];
+		char digest[TPM_MAX_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];
+
+	/*
+	 * With TPM 2.0 hash agility, TPM chips could support multiple TPM
+	 * PCR banks, allowing firmware to configure and enable different
+	 * banks.  The SHA1 bank is not necessarily enabled.
+	 *
+	 * Use the same hash algorithm for reading the TPM PCRs as for
+	 * calculating the boot aggregate digest.  Preference is given to
+	 * the configured IMA default hash algorithm.  Otherwise, use the
+	 * TPM required banks - SHA256 for TPM 2.0, SHA1 for TPM 1.2.  If
+	 * SHA256 is not available, use the first PCR bank and if that is
+	 * not mapped to a crypto ID, set boot_aggregate to zero.
+	 */
 	if (ima_tpm_chip) {
 		result = ima_calc_boot_aggregate(&hash.hdr);
 		if (result < 0) {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 3/8] ima: Evaluate error in init_ima()
  2020-02-10 10:00 [PATCH v3 0/8] ima: support stronger algorithms for attestation Roberto Sassu
  2020-02-10 10:00 ` [PATCH v3 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST Roberto Sassu
  2020-02-10 10:00 ` [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate Roberto Sassu
@ 2020-02-10 10:00 ` Roberto Sassu
  2020-02-10 10:00 ` [PATCH v3 4/8] ima: Store template digest directly in ima_template_entry Roberto Sassu
  3 siblings, 0 replies; 13+ messages in thread
From: Roberto Sassu @ 2020-02-10 10:00 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu, stable

Evaluate error in init_ima() before register_blocking_lsm_notifier() and
return if not zero.

Cc: stable@vger.kernel.org # 5.3.x
Fixes: b16942455193 ("ima: use the lsm policy update notifier")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index d7e987baf127..a16c148ed90d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -738,6 +738,9 @@ static int __init init_ima(void)
 		error = ima_init();
 	}
 
+	if (error)
+		return error;
+
 	error = register_blocking_lsm_notifier(&ima_lsm_policy_notifier);
 	if (error)
 		pr_warn("Couldn't register LSM notifier, error %d\n", error);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 4/8] ima: Store template digest directly in ima_template_entry
  2020-02-10 10:00 [PATCH v3 0/8] ima: support stronger algorithms for attestation Roberto Sassu
                   ` (2 preceding siblings ...)
  2020-02-10 10:00 ` [PATCH v3 3/8] ima: Evaluate error in init_ima() Roberto Sassu
@ 2020-02-10 10:00 ` Roberto Sassu
  3 siblings, 0 replies; 13+ messages in thread
From: Roberto Sassu @ 2020-02-10 10:00 UTC (permalink / raw)
  To: zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu

In preparation for the patch that calculates a digest for each allocated
PCR bank, this patch passes to ima_calc_field_array_hash() the
ima_template_entry structure, so that digests can be directly stored in
that structure instead of ima_digest_data.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima.h        |  3 +--
 security/integrity/ima/ima_api.c    | 12 +-----------
 security/integrity/ima/ima_crypto.c | 18 +++++++-----------
 3 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index df4ca482fb53..2f380fb92a7a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -138,8 +138,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash);
 int ima_calc_buffer_hash(const void *buf, loff_t len,
 			 struct ima_digest_data *hash);
 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 ima_template_entry *entry);
 int __init ima_calc_boot_aggregate(struct ima_digest_data *hash);
 void ima_add_violation(struct file *file, const unsigned char *filename,
 		       struct integrity_iint_cache *iint,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 610759fe63b8..51f562111864 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -96,26 +96,16 @@ int ima_store_template(struct ima_template_entry *entry,
 	static const char audit_cause[] = "hashing_error";
 	char *template_name = entry->template_desc->name;
 	int result;
-	struct {
-		struct ima_digest_data hdr;
-		char digest[TPM_DIGEST_SIZE];
-	} hash;
 
 	if (!violation) {
-		int num_fields = entry->template_desc->num_fields;
-
-		/* 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);
+						   entry);
 		if (result < 0) {
 			integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode,
 					    template_name, op,
 					    audit_cause, result, 0);
 			return result;
 		}
-		memcpy(entry->digest, hash.hdr.digest, hash.hdr.length);
 	}
 	entry->pcr = pcr;
 	result = ima_add_template_entry(entry, violation, op, inode, filename);
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index f2f41a2bc3d4..2d356ae8e823 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -464,18 +464,16 @@ 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 ima_template_entry *entry,
 					 struct crypto_shash *tfm)
 {
 	SHASH_DESC_ON_STACK(shash, tfm);
+	struct ima_template_desc *td = entry->template_desc;
+	int num_fields = entry->template_desc->num_fields;
 	int rc, i;
 
 	shash->tfm = tfm;
 
-	hash->length = crypto_shash_digestsize(tfm);
-
 	rc = crypto_shash_init(shash);
 	if (rc != 0)
 		return rc;
@@ -504,24 +502,22 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
 	}
 
 	if (!rc)
-		rc = crypto_shash_final(shash, hash->digest);
+		rc = crypto_shash_final(shash, entry->digest);
 
 	return rc;
 }
 
 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 ima_template_entry *entry)
 {
 	struct crypto_shash *tfm;
 	int rc;
 
-	tfm = ima_alloc_tfm(hash->algo);
+	tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
 	if (IS_ERR(tfm))
 		return PTR_ERR(tfm);
 
-	rc = ima_calc_field_array_hash_tfm(field_data, desc, num_fields,
-					   hash, tfm);
+	rc = ima_calc_field_array_hash_tfm(field_data, entry, tfm);
 
 	ima_free_tfm(tfm);
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST
  2020-02-10 10:00 ` [PATCH v3 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST Roberto Sassu
@ 2020-02-10 16:00   ` Jarkko Sakkinen
  0 siblings, 0 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2020-02-10 16:00 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, James.Bottomley, linux-integrity, linux-security-module,
	linux-kernel, silviu.vlasceanu, stable

On Mon, Feb 10, 2020 at 11:00:41AM +0100, Roberto Sassu wrote:
> chip->allocated_banks, an array of tpm_bank_info structures, contains the
> list of TPM algorithm IDs of allocated PCR banks. It also contains the
> corresponding ID of the crypto subsystem, so that users of the TPM driver
> can calculate a digest for a PCR extend operation.
> 
> However, if there is no mapping between TPM algorithm ID and crypto ID, the
> crypto_id field of tpm_bank_info remains set to zero (the array is
> allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
> Zero should not be used as value for unknown mappings, as it is a valid
> crypto ID (HASH_ALGO_MD4).
> 
> Thus, initialize crypto_id to HASH_ALGO__LAST.
> 
> Cc: stable@vger.kernel.org # 5.1.x
> Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-02-10 10:00 ` [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate Roberto Sassu
@ 2020-02-10 22:23   ` Mimi Zohar
  2020-02-11 10:09     ` Roberto Sassu
  0 siblings, 1 reply; 13+ messages in thread
From: Mimi Zohar @ 2020-02-10 22:23 UTC (permalink / raw)
  To: Roberto Sassu, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, stable

On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote:
> 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 tries to find the SHA256 PCR
> bank (which is mandatory in the TCG PC Client specification). 

Up to here, the patch description matches the code.
> If also this
> bank is not found, the patch selects the first one. If the TPM algorithm
> of that bank is not mapped to a crypto ID, boot_aggregate is set to zero.

This comment and the one inline are left over from previous version.

> 
> Changelog
> 
> v1:
> - add Mimi's comments
> - if there is no PCR bank for the IMA default algorithm use SHA256
>   (suggested by James Bottomley)
> 
> Cc: stable@vger.kernel.org # 5.1.x
> Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Thanks, Roberto.  This patch is dependent on 1/8.  As soon as there's
a topic branch, I'll queue this, removing the extraneous comments.

Mimi

> ---
>  security/integrity/ima/ima_crypto.c | 45 +++++++++++++++++++++++++----
>  security/integrity/ima/ima_init.c   | 22 ++++++++++----
>  2 files changed, 56 insertions(+), 11 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 73044fc6a952..f2f41a2bc3d4 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -655,18 +655,29 @@ static void __init ima_pcrread(u32 idx, struct tpm_digest *d)
>  }
>  
>  /*
> - * Calculate the boot aggregate hash
> + * The boot_aggregate is a cumulative hash over TPM registers 0 - 7.  With
> + * TPM 1.2 the boot_aggregate was based on reading the SHA1 PCRs, but with
> + * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks,
> + * allowing firmware to configure and enable different banks.
> + *
> + * Knowing which TPM bank is read to calculate the boot_aggregate digest
> + * needs to be conveyed to a verifier.  For this reason, use the same
> + * hash algorithm for reading the TPM PCRs as for calculating the boot
> + * aggregate digest as stored in the measurement list.
>   */
> -static int __init ima_calc_boot_aggregate_tfm(char *digest,
> +static int __init ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
>  					      struct crypto_shash *tfm)
>  {
> -	struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
> +	struct tpm_digest d = { .alg_id = alg_id, .digest = {0} };
>  	int rc;
>  	u32 i;
>  	SHASH_DESC_ON_STACK(shash, tfm);
>  
>  	shash->tfm = tfm;
>  
> +	pr_devel("calculating the boot-aggregate based on TPM bank: %04x\n",
> +		 d.alg_id);
> +
>  	rc = crypto_shash_init(shash);
>  	if (rc != 0)
>  		return rc;
> @@ -675,7 +686,8 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
>  	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,
> +					 crypto_shash_digestsize(tfm));
>  	}
>  	if (!rc)
>  		crypto_shash_final(shash, digest);
> @@ -685,14 +697,35 @@ 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;
> +	u16 crypto_id, alg_id;
> +	int rc, i, bank_idx = 0;
> +
> +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> +		crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
> +		if (crypto_id == hash->algo) {
> +			bank_idx = i;
> +			break;
> +		}
> +
> +		if (crypto_id == HASH_ALGO_SHA256)
> +			bank_idx = i;
> +	}
> +
> +	if (bank_idx == 0 &&
> +	    ima_tpm_chip->allocated_banks[0].crypto_id == HASH_ALGO__LAST) {
> +		pr_err("No suitable TPM algorithm for boot aggregate\n");
> +		return 0;
> +	}
> +
> +	hash->algo = ima_tpm_chip->allocated_banks[bank_idx].crypto_id;
>  
>  	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);
> +	alg_id = ima_tpm_chip->allocated_banks[bank_idx].alg_id;
> +	rc = ima_calc_boot_aggregate_tfm(hash->digest, alg_id, tfm);
>  
>  	ima_free_tfm(tfm);
>  
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index 5d55ade5f3b9..fbd7a8e28a6b 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -27,7 +27,7 @@ struct tpm_chip *ima_tpm_chip;
>  /* Add the boot aggregate to the IMA measurement list and extend
>   * the PCR register.
>   *
> - * Calculate the boot aggregate, a SHA1 over tpm registers 0-7,
> + * Calculate the boot aggregate, a hash over tpm registers 0-7,
>   * assuming a TPM chip exists, and zeroes if the TPM chip does not
>   * exist.  Add the boot aggregate measurement to the measurement
>   * list and extend the PCR register.
> @@ -51,15 +51,27 @@ static int __init ima_add_boot_aggregate(void)
>  	int violation = 0;
>  	struct {
>  		struct ima_digest_data hdr;
> -		char digest[TPM_DIGEST_SIZE];
> +		char digest[TPM_MAX_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];
> +
> +	/*
> +	 * With TPM 2.0 hash agility, TPM chips could support multiple TPM
> +	 * PCR banks, allowing firmware to configure and enable different
> +	 * banks.  The SHA1 bank is not necessarily enabled.
> +	 *
> +	 * Use the same hash algorithm for reading the TPM PCRs as for
> +	 * calculating the boot aggregate digest.  Preference is given to
> +	 * the configured IMA default hash algorithm.  Otherwise, use the
> +	 * TPM required banks - SHA256 for TPM 2.0, SHA1 for TPM 1.2. If
> +	 * SHA256 is not available, use the first PCR bank and if that is
> +	 * not mapped to a crypto ID, set boot_aggregate to zero.

The last line of the above comment is left over from the previous
version.

> +	 */
>  	if (ima_tpm_chip) {
>  		result = ima_calc_boot_aggregate(&hash.hdr);
>  		if (result < 0) {


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-02-10 22:23   ` Mimi Zohar
@ 2020-02-11 10:09     ` Roberto Sassu
  2020-03-02 13:41       ` Mimi Zohar
  0 siblings, 1 reply; 13+ messages in thread
From: Roberto Sassu @ 2020-02-11 10:09 UTC (permalink / raw)
  To: Mimi Zohar, James.Bottomley, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel,
	Silviu Vlasceanu, stable

> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, February 10, 2020 11:24 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
> 
> On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote:
> > 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 tries to find the SHA256 PCR
> > bank (which is mandatory in the TCG PC Client specification).
> 
> Up to here, the patch description matches the code.
> > If also this
> > bank is not found, the patch selects the first one. If the TPM algorithm
> > of that bank is not mapped to a crypto ID, boot_aggregate is set to zero.
> 
> This comment and the one inline are left over from previous version.

Hi Mimi

actually the code does what is described above. bank_idx is initially
set to zero and remains as it is if there is no PCR bank for the default
IMA algorithm or SHA256.

Roberto

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

> > Changelog
> >
> > v1:
> > - add Mimi's comments
> > - if there is no PCR bank for the IMA default algorithm use SHA256
> >   (suggested by James Bottomley)
> >
> > Cc: stable@vger.kernel.org # 5.1.x
> > Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms
> with PCR read")
> > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > Suggested-by: James Bottomley
> <James.Bottomley@HansenPartnership.com>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Thanks, Roberto.  This patch is dependent on 1/8.  As soon as there's
> a topic branch, I'll queue this, removing the extraneous comments.
> 
> Mimi
> 
> > ---
> >  security/integrity/ima/ima_crypto.c | 45
> +++++++++++++++++++++++++----
> >  security/integrity/ima/ima_init.c   | 22 ++++++++++----
> >  2 files changed, 56 insertions(+), 11 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_crypto.c
> b/security/integrity/ima/ima_crypto.c
> > index 73044fc6a952..f2f41a2bc3d4 100644
> > --- a/security/integrity/ima/ima_crypto.c
> > +++ b/security/integrity/ima/ima_crypto.c
> > @@ -655,18 +655,29 @@ static void __init ima_pcrread(u32 idx, struct
> tpm_digest *d)
> >  }
> >
> >  /*
> > - * Calculate the boot aggregate hash
> > + * The boot_aggregate is a cumulative hash over TPM registers 0 - 7.
> With
> > + * TPM 1.2 the boot_aggregate was based on reading the SHA1 PCRs, but
> with
> > + * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks,
> > + * allowing firmware to configure and enable different banks.
> > + *
> > + * Knowing which TPM bank is read to calculate the boot_aggregate
> digest
> > + * needs to be conveyed to a verifier.  For this reason, use the same
> > + * hash algorithm for reading the TPM PCRs as for calculating the boot
> > + * aggregate digest as stored in the measurement list.
> >   */
> > -static int __init ima_calc_boot_aggregate_tfm(char *digest,
> > +static int __init ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
> >  					      struct crypto_shash *tfm)
> >  {
> > -	struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
> > +	struct tpm_digest d = { .alg_id = alg_id, .digest = {0} };
> >  	int rc;
> >  	u32 i;
> >  	SHASH_DESC_ON_STACK(shash, tfm);
> >
> >  	shash->tfm = tfm;
> >
> > +	pr_devel("calculating the boot-aggregate based on TPM
> bank: %04x\n",
> > +		 d.alg_id);
> > +
> >  	rc = crypto_shash_init(shash);
> >  	if (rc != 0)
> >  		return rc;
> > @@ -675,7 +686,8 @@ static int __init ima_calc_boot_aggregate_tfm(char
> *digest,
> >  	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,
> > +					 crypto_shash_digestsize(tfm));
> >  	}
> >  	if (!rc)
> >  		crypto_shash_final(shash, digest);
> > @@ -685,14 +697,35 @@ 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;
> > +	u16 crypto_id, alg_id;
> > +	int rc, i, bank_idx = 0;
> > +
> > +	for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> > +		crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
> > +		if (crypto_id == hash->algo) {
> > +			bank_idx = i;
> > +			break;
> > +		}
> > +
> > +		if (crypto_id == HASH_ALGO_SHA256)
> > +			bank_idx = i;
> > +	}
> > +
> > +	if (bank_idx == 0 &&
> > +	    ima_tpm_chip->allocated_banks[0].crypto_id ==
> HASH_ALGO__LAST) {
> > +		pr_err("No suitable TPM algorithm for boot aggregate\n");
> > +		return 0;
> > +	}
> > +
> > +	hash->algo = ima_tpm_chip->allocated_banks[bank_idx].crypto_id;
> >
> >  	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);
> > +	alg_id = ima_tpm_chip->allocated_banks[bank_idx].alg_id;
> > +	rc = ima_calc_boot_aggregate_tfm(hash->digest, alg_id, tfm);
> >
> >  	ima_free_tfm(tfm);
> >
> > diff --git a/security/integrity/ima/ima_init.c
> b/security/integrity/ima/ima_init.c
> > index 5d55ade5f3b9..fbd7a8e28a6b 100644
> > --- a/security/integrity/ima/ima_init.c
> > +++ b/security/integrity/ima/ima_init.c
> > @@ -27,7 +27,7 @@ struct tpm_chip *ima_tpm_chip;
> >  /* Add the boot aggregate to the IMA measurement list and extend
> >   * the PCR register.
> >   *
> > - * Calculate the boot aggregate, a SHA1 over tpm registers 0-7,
> > + * Calculate the boot aggregate, a hash over tpm registers 0-7,
> >   * assuming a TPM chip exists, and zeroes if the TPM chip does not
> >   * exist.  Add the boot aggregate measurement to the measurement
> >   * list and extend the PCR register.
> > @@ -51,15 +51,27 @@ static int __init ima_add_boot_aggregate(void)
> >  	int violation = 0;
> >  	struct {
> >  		struct ima_digest_data hdr;
> > -		char digest[TPM_DIGEST_SIZE];
> > +		char digest[TPM_MAX_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];
> > +
> > +	/*
> > +	 * With TPM 2.0 hash agility, TPM chips could support multiple TPM
> > +	 * PCR banks, allowing firmware to configure and enable different
> > +	 * banks.  The SHA1 bank is not necessarily enabled.
> > +	 *
> > +	 * Use the same hash algorithm for reading the TPM PCRs as for
> > +	 * calculating the boot aggregate digest.  Preference is given to
> > +	 * the configured IMA default hash algorithm.  Otherwise, use the
> > +	 * TPM required banks - SHA256 for TPM 2.0, SHA1 for TPM 1.2. If
> > +	 * SHA256 is not available, use the first PCR bank and if that is
> > +	 * not mapped to a crypto ID, set boot_aggregate to zero.
> 
> The last line of the above comment is left over from the previous
> version.
> 
> > +	 */
> >  	if (ima_tpm_chip) {
> >  		result = ima_calc_boot_aggregate(&hash.hdr);
> >  		if (result < 0) {


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-02-11 10:09     ` Roberto Sassu
@ 2020-03-02 13:41       ` Mimi Zohar
  2020-03-02 14:28         ` Roberto Sassu
  0 siblings, 1 reply; 13+ messages in thread
From: Mimi Zohar @ 2020-03-02 13:41 UTC (permalink / raw)
  To: Roberto Sassu, James.Bottomley, jarkko.sakkinen, Dmitry Kasatkin
  Cc: linux-integrity, linux-security-module, linux-kernel,
	Silviu Vlasceanu, stable

On Tue, 2020-02-11 at 10:09 +0000, Roberto Sassu wrote:
> > -----Original Message-----

Please find/use a mailer that doesn't include this junk.

> > On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote:
> > > 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 tries to find the SHA256 PCR
> > > bank (which is mandatory in the TCG PC Client specification).
> > 
> > Up to here, the patch description matches the code.
> > > If also this
> > > bank is not found, the patch selects the first one. If the TPM algorithm
> > > of that bank is not mapped to a crypto ID, boot_aggregate is set to zero.
> > 
> > This comment and the one inline are left over from previous version.
> 
> Hi Mimi
> 
> actually the code does what is described above. bank_idx is initially
> set to zero and remains as it is if there is no PCR bank for the default
> IMA algorithm or SHA256.

Sorry for the delay in continuing to review this patch set.  It took a
while to write ima-evm-utils regression tests for it.

Dmitry and you were the ones that initiated ima-evm-utils, saying
there should a single package for signing files and integrity testing.
 The features in ima-evm-utils should reflect what is actually
upstreamed in the kernel.  (Currently there are a few experimental
features which were never upstreamed.  I'd like to remove them, but am
a bit concerned that they are being used.)  I'd appreciate your help
in keeping ima-evm-utils up to date.  It will help simplify
upstreaming new kernel features.

My initial patch attempted to use any common TPM and kernel hash
algorithm to calculate the boot_aggregate.  The discussion with James
was pretty clear, which you even stated in the Changelog.  Either we
use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for TPM
1.2 for the boot-aggregate.

thanks,

Mimi


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-03-02 13:41       ` Mimi Zohar
@ 2020-03-02 14:28         ` Roberto Sassu
  2020-03-02 14:46           ` Mimi Zohar
  0 siblings, 1 reply; 13+ messages in thread
From: Roberto Sassu @ 2020-03-02 14:28 UTC (permalink / raw)
  To: Mimi Zohar, James.Bottomley, jarkko.sakkinen, Dmitry Kasatkin
  Cc: linux-integrity, linux-security-module, linux-kernel,
	Silviu Vlasceanu, stable

> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, March 2, 2020 2:42 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com; Dmitry Kasatkin
> <dmitry.kasatkin@gmail.com>
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
> 
> On Tue, 2020-02-11 at 10:09 +0000, Roberto Sassu wrote:
> > > -----Original Message-----
> 
> Please find/use a mailer that doesn't include this junk.

I will do. I didn't have the time yet.

> > > On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote:
> > > > 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 tries to find the SHA256
> PCR
> > > > bank (which is mandatory in the TCG PC Client specification).
> > >
> > > Up to here, the patch description matches the code.
> > > > If also this
> > > > bank is not found, the patch selects the first one. If the TPM algorithm
> > > > of that bank is not mapped to a crypto ID, boot_aggregate is set to
> zero.
> > >
> > > This comment and the one inline are left over from previous version.
> >
> > Hi Mimi
> >
> > actually the code does what is described above. bank_idx is initially
> > set to zero and remains as it is if there is no PCR bank for the default
> > IMA algorithm or SHA256.
> 
> Sorry for the delay in continuing to review this patch set.  It took a
> while to write ima-evm-utils regression tests for it.
> 
> Dmitry and you were the ones that initiated ima-evm-utils, saying
> there should a single package for signing files and integrity testing.
>  The features in ima-evm-utils should reflect what is actually
> upstreamed in the kernel.  (Currently there are a few experimental
> features which were never upstreamed.  I'd like to remove them, but am
> a bit concerned that they are being used.)  I'd appreciate your help
> in keeping ima-evm-utils up to date.  It will help simplify
> upstreaming new kernel features.
> 
> My initial patch attempted to use any common TPM and kernel hash
> algorithm to calculate the boot_aggregate.  The discussion with James
> was pretty clear, which you even stated in the Changelog.  Either we
> use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for TPM
> 1.2 for the boot-aggregate.

Ok, I didn't understand fully. I thought we should use the default IMA
algorithm and select SHA256 as fallback choice for TPM 2.0 if there is no
PCR bank for default algorithm. I additionally implemented the logic to
select the first PCR bank if the SHA256 PCR bank is not available but I can
remove it.

SHA256 should be the minimum requirement for boot aggregate. The
advantage of using the default IMA algorithm is that it will be possible to
select stronger algorithms when they are supported by the TPM. We might
introduce a new option to specify only the algorithm for boot aggregate,
like James suggested to support embedded systems. Let me know which
option you prefer.

Thanks

Roberto

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-03-02 14:28         ` Roberto Sassu
@ 2020-03-02 14:46           ` Mimi Zohar
  2020-03-02 15:11             ` Roberto Sassu
  0 siblings, 1 reply; 13+ messages in thread
From: Mimi Zohar @ 2020-03-02 14:46 UTC (permalink / raw)
  To: Roberto Sassu, James.Bottomley, jarkko.sakkinen, Dmitry Kasatkin
  Cc: linux-integrity, linux-security-module, linux-kernel,
	Silviu Vlasceanu, stable


> > > > On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote: 
> > My initial patch attempted to use any common TPM and kernel hash
> > algorithm to calculate the boot_aggregate.  The discussion with James
> > was pretty clear, which you even stated in the Changelog.  Either we
> > use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for TPM
> > 1.2 for the boot-aggregate.
> 
> Ok, I didn't understand fully. I thought we should use the default IMA
> algorithm and select SHA256 as fallback choice for TPM 2.0 if there is no
> PCR bank for default algorithm.

Yes, preference is given to the IMA default algorithm, but it should
fall back to using SHA256 or SHA1, based on the TPM.

> I additionally implemented the logic to
> select the first PCR bank if the SHA256 PCR bank is not available but I can
> remove it.
> 
> SHA256 should be the minimum requirement for boot aggregate. The
> advantage of using the default IMA algorithm is that it will be possible to
> select stronger algorithms when they are supported by the TPM. We might
> introduce a new option to specify only the algorithm for boot aggregate,
> like James suggested to support embedded systems. Let me know which
> option you prefer.

I don't remember James saying that, but if the community really wants
that support, then it should be upstreamed independently, as a
separate patch.  Let's first get the basics working.

thanks,

Mimi


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-03-02 14:46           ` Mimi Zohar
@ 2020-03-02 15:11             ` Roberto Sassu
  2020-03-02 17:33               ` Mimi Zohar
  0 siblings, 1 reply; 13+ messages in thread
From: Roberto Sassu @ 2020-03-02 15:11 UTC (permalink / raw)
  To: Mimi Zohar, James.Bottomley, jarkko.sakkinen, Dmitry Kasatkin
  Cc: linux-integrity, linux-security-module, linux-kernel,
	Silviu Vlasceanu, stable

> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, March 2, 2020 3:47 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com; Dmitry Kasatkin
> <dmitry.kasatkin@gmail.com>
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
> 
> 
> > > > > On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote:
> > > My initial patch attempted to use any common TPM and kernel hash
> > > algorithm to calculate the boot_aggregate.  The discussion with James
> > > was pretty clear, which you even stated in the Changelog.  Either we
> > > use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for
> TPM
> > > 1.2 for the boot-aggregate.
> >
> > Ok, I didn't understand fully. I thought we should use the default IMA
> > algorithm and select SHA256 as fallback choice for TPM 2.0 if there is no
> > PCR bank for default algorithm.
> 
> Yes, preference is given to the IMA default algorithm, but it should
> fall back to using SHA256 or SHA1, based on the TPM.

Ok. The patch already does it even if the TPM version is not checked.
For TPM 1.2, if the default algorithm is not SHA1 the patch will select
the first PCR bank (SHA1).

Should I send a new patch which explicitly checks the TPM version?

> > I additionally implemented the logic to
> > select the first PCR bank if the SHA256 PCR bank is not available but I can
> > remove it.
> >
> > SHA256 should be the minimum requirement for boot aggregate. The
> > advantage of using the default IMA algorithm is that it will be possible to
> > select stronger algorithms when they are supported by the TPM. We
> might
> > introduce a new option to specify only the algorithm for boot aggregate,
> > like James suggested to support embedded systems. Let me know which
> > option you prefer.
> 
> I don't remember James saying that, but if the community really wants
> that support, then it should be upstreamed independently, as a
> separate patch.  Let's first get the basics working.

Ok.

Thanks

Roberto

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
  2020-03-02 15:11             ` Roberto Sassu
@ 2020-03-02 17:33               ` Mimi Zohar
  0 siblings, 0 replies; 13+ messages in thread
From: Mimi Zohar @ 2020-03-02 17:33 UTC (permalink / raw)
  To: Roberto Sassu, James.Bottomley, jarkko.sakkinen, Dmitry Kasatkin
  Cc: linux-integrity, linux-security-module, linux-kernel,
	Silviu Vlasceanu, stable

On Mon, 2020-03-02 at 15:11 +0000, Roberto Sassu wrote:


> > Yes, preference is given to the IMA default algorithm, but it should
> > fall back to using SHA256 or SHA1, based on the TPM.
> 
> Ok. The patch already does it even if the TPM version is not checked.
> For TPM 1.2, if the default algorithm is not SHA1 the patch will select
> the first PCR bank (SHA1).
> 
> Should I send a new patch which explicitly checks the TPM version?

Checking the TPM version shouldn't be necessary.  The code currently
sets bank_idx to the HASH_ALGO_SHA256.  If instead of initializing
bank_idx to 0, initialize it to the nr_allocated_banks or -1.  As long
as the bank_idx value is the same as the initialized value, set the
bank_idx to HASH_ALGO_SHA1.

The subsequent bank_idx would then be limited to testing for the
initialized value.

Mimi


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-10 10:00 [PATCH v3 0/8] ima: support stronger algorithms for attestation Roberto Sassu
2020-02-10 10:00 ` [PATCH v3 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST Roberto Sassu
2020-02-10 16:00   ` Jarkko Sakkinen
2020-02-10 10:00 ` [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate Roberto Sassu
2020-02-10 22:23   ` Mimi Zohar
2020-02-11 10:09     ` Roberto Sassu
2020-03-02 13:41       ` Mimi Zohar
2020-03-02 14:28         ` Roberto Sassu
2020-03-02 14:46           ` Mimi Zohar
2020-03-02 15:11             ` Roberto Sassu
2020-03-02 17:33               ` Mimi Zohar
2020-02-10 10:00 ` [PATCH v3 3/8] ima: Evaluate error in init_ima() Roberto Sassu
2020-02-10 10:00 ` [PATCH v3 4/8] ima: Store template digest directly in ima_template_entry Roberto Sassu

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git