| Message ID | 1580140919-6127-2-git-send-email-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ima: use the IMA configured hash algo to calculate the boot aggregate | expand |
On 1/27/2020 8:01 AM, Mimi Zohar wrote: > + > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) { > + if (ima_tpm_chip->allocated_banks[i].alg_id == d.alg_id) > + break; > + } > + > + if (i == ima_tpm_chip->nr_allocated_banks) > + d.alg_id = ima_tpm_chip->allocated_banks[0].alg_id; > + Can the number of allocated banks (ima_tpm_chip->nr_allocated_banks) be zero? Should that be checked before accessing "allocated_banks"? -lakshmi
On Mon, 2020-01-27 at 08:50 -0800, Lakshmi Ramasubramanian wrote: > On 1/27/2020 8:01 AM, Mimi Zohar wrote: > > > + > > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) { > > + if (ima_tpm_chip->allocated_banks[i].alg_id == d.alg_id) > > + break; > > + } > > + > > + if (i == ima_tpm_chip->nr_allocated_banks) > > + d.alg_id = ima_tpm_chip->allocated_banks[0].alg_id; > > + > > Can the number of allocated banks (ima_tpm_chip->nr_allocated_banks) be > zero? Should that be checked before accessing "allocated_banks"? Yes, that might be the true, but I think the solution is not fixing the problem here, but when ima_tpm_chip is set in ima_init(). tpm_default_chip() should be modified to return a TPM with at least one bank enabled; and ima_init() needs to go into TPM-bypass mode if there isn't. Can anyone look into this please? Mimi
On 1/27/2020 11:50 AM, Lakshmi Ramasubramanian wrote: > Can the number of allocated banks (ima_tpm_chip->nr_allocated_banks) be > zero? Should that be checked before accessing "allocated_banks"? Summary: It's unlikely that Linux on a PC will encounter a TPM without PCR 10. It is likely that PCR 10 will be only SHA-256, that there will be no SHA-1 PCR 10. ~~ In theory: Yes, one could have a TPM with no allocated banks. In practice: A PC Client TPM must have at least one bank with PCR 0 and PCR 17. Some other TPMs, like automotive or embedded, may be different. Most platforms will be designed to meet Windows requirements, which will have AFAIK at least one bank of 24 PCRs. The TPM specification permits allocation of partial banks. In theory, one could encounter a TPM with e.g., PCR 0-7 but not PCR 10. In practice, AFAIK the hardware TPMs implement only full banks. Platform firmware allocates full banks.
> -----Original Message----- > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity- > owner@vger.kernel.org] On Behalf Of Mimi Zohar > Sent: Monday, January 27, 2020 5:02 PM > To: linux-integrity@vger.kernel.org > Cc: Jerry Snitselaar <jsnitsel@redhat.com>; James Bottomley > <James.Bottomley@HansenPartnership.com>; linux- > kernel@vger.kernel.org; Mimi Zohar <zohar@linux.ibm.com> > Subject: [PATCH 2/2] ima: support calculating the boot_aggregate based on > different TPM banks > > Calculating the boot_aggregate attempts to read the TPM SHA1 bank, > assuming it is always enabled. With TPM 2.0 hash agility, TPM chips > could support multiple TPM PCR banks, allowing firmware to configure and > enable different banks. > > Instead of hard coding the TPM 2.0 bank hash algorithm used for calculating > the boot-aggregate, see if the configured IMA_DEFAULT_HASH algorithm is > an allocated TPM bank, otherwise use the first allocated TPM bank. > > For TPM 1.2 SHA1 is the only supported hash algorithm. > > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > --- > security/integrity/ima/ima_crypto.c | 37 > ++++++++++++++++++++++++++++++++++++- > 1 file changed, 36 insertions(+), 1 deletion(-) > > diff --git a/security/integrity/ima/ima_crypto.c > b/security/integrity/ima/ima_crypto.c > index 7967a6904851..b1b26d61f174 100644 > --- a/security/integrity/ima/ima_crypto.c > +++ b/security/integrity/ima/ima_crypto.c > @@ -656,8 +656,25 @@ static void __init ima_pcrread(u32 idx, struct > tpm_digest *d) > pr_err("Error Communicating to TPM chip\n"); > } > > +/* tpm2_hash_map is the same as defined in tpm2-cmd.c and > trusted_tpm2.c */ > +static struct tpm2_hash tpm2_hash_map[] = { > + {HASH_ALGO_SHA1, TPM_ALG_SHA1}, > + {HASH_ALGO_SHA256, TPM_ALG_SHA256}, > + {HASH_ALGO_SHA384, TPM_ALG_SHA384}, > + {HASH_ALGO_SHA512, TPM_ALG_SHA512}, > + {HASH_ALGO_SM3_256, TPM_ALG_SM3_256}, > +}; > + > /* > - * Calculate the boot aggregate hash > + * The boot_aggregate is a cumulative hash over TPM registers 0 - 7. With > + * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks, > + * allowing firmware to configure and enable different banks. > + * > + * Instead of hard coding the TPM bank hash algorithm used for calculating > + * the boot-aggregate, see if the configured IMA_DEFAULT_HASH > algorithm is > + * an allocated TPM bank, otherwise use the first allocated TPM bank. > + * > + * For TPM 1.2 SHA1 is the only hash algorithm. > */ > static int __init ima_calc_boot_aggregate_tfm(char *digest, > struct crypto_shash *tfm) > @@ -673,6 +690,24 @@ static int __init ima_calc_boot_aggregate_tfm(char > *digest, > if (rc != 0) > return rc; > > + for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) { > + if (tpm2_hash_map[i].crypto_id == ima_hash_algo) { It is not necessary to define a new map. ima_tpm_chip->allocated_banks has a crypto_id field. > + d.alg_id = tpm2_hash_map[i].tpm_id; > + break; > + } > + } > + > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) { > + if (ima_tpm_chip->allocated_banks[i].alg_id == d.alg_id) > + break; > + } > + > + if (i == ima_tpm_chip->nr_allocated_banks) > + d.alg_id = ima_tpm_chip->allocated_banks[0].alg_id; This code assumes that the algorithm used to calculate boot_aggregate and the algorithm of the PCR bank can be different. I don't know if it is possible to communicate to the verifier which bank has been selected (it depends on the local configuration). In my opinion the safest approach would be to use the same algorithm for the digest and the PCR bank. If you agree to this, then the code above must be moved to ima_calc_boot_aggregate() so that the algorithm of the selected PCR bank can be passed to ima_alloc_tfm(). The selected PCR bank might be not the first, if the algorithm is unknown to the crypto subsystem. > + pr_info("Calculating the boot-aggregregate, reading TPM PCR Typo. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
On Tue, 2020-01-28 at 14:19 +0000, Roberto Sassu wrote: > > -----Original Message----- > > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity- > > owner@vger.kernel.org] On Behalf Of Mimi Zohar > > Sent: Monday, January 27, 2020 5:02 PM > > To: linux-integrity@vger.kernel.org > > Cc: Jerry Snitselaar <jsnitsel@redhat.com>; James Bottomley > > <James.Bottomley@HansenPartnership.com>; linux- > > kernel@vger.kernel.org; Mimi Zohar <zohar@linux.ibm.com> > > Subject: [PATCH 2/2] ima: support calculating the boot_aggregate based on > > different TPM banks > > > > Calculating the boot_aggregate attempts to read the TPM SHA1 bank, > > assuming it is always enabled. With TPM 2.0 hash agility, TPM chips > > could support multiple TPM PCR banks, allowing firmware to configure and > > enable different banks. > > > > Instead of hard coding the TPM 2.0 bank hash algorithm used for calculating > > the boot-aggregate, see if the configured IMA_DEFAULT_HASH algorithm is > > an allocated TPM bank, otherwise use the first allocated TPM bank. > > > > For TPM 1.2 SHA1 is the only supported hash algorithm. > > > > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com> > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > --- > > security/integrity/ima/ima_crypto.c | 37 > > ++++++++++++++++++++++++++++++++++++- > > 1 file changed, 36 insertions(+), 1 deletion(-) > > > > diff --git a/security/integrity/ima/ima_crypto.c > > b/security/integrity/ima/ima_crypto.c > > index 7967a6904851..b1b26d61f174 100644 > > --- a/security/integrity/ima/ima_crypto.c > > +++ b/security/integrity/ima/ima_crypto.c > > @@ -656,8 +656,25 @@ static void __init ima_pcrread(u32 idx, struct > > tpm_digest *d) > > pr_err("Error Communicating to TPM chip\n"); > > } > > > > +/* tpm2_hash_map is the same as defined in tpm2-cmd.c and > > trusted_tpm2.c */ > > +static struct tpm2_hash tpm2_hash_map[] = { > > + {HASH_ALGO_SHA1, TPM_ALG_SHA1}, > > + {HASH_ALGO_SHA256, TPM_ALG_SHA256}, > > + {HASH_ALGO_SHA384, TPM_ALG_SHA384}, > > + {HASH_ALGO_SHA512, TPM_ALG_SHA512}, > > + {HASH_ALGO_SM3_256, TPM_ALG_SM3_256}, > > +}; > > + > > /* > > - * Calculate the boot aggregate hash > > + * The boot_aggregate is a cumulative hash over TPM registers 0 - 7. With > > + * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks, > > + * allowing firmware to configure and enable different banks. > > + * > > + * Instead of hard coding the TPM bank hash algorithm used for calculating > > + * the boot-aggregate, see if the configured IMA_DEFAULT_HASH > > algorithm is > > + * an allocated TPM bank, otherwise use the first allocated TPM bank. > > + * > > + * For TPM 1.2 SHA1 is the only hash algorithm. > > */ > > static int __init ima_calc_boot_aggregate_tfm(char *digest, > > struct crypto_shash *tfm) > > @@ -673,6 +690,24 @@ static int __init ima_calc_boot_aggregate_tfm(char > > *digest, > > if (rc != 0) > > return rc; > > > > + for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) { > > + if (tpm2_hash_map[i].crypto_id == ima_hash_algo) { > > It is not necessary to define a new map. ima_tpm_chip->allocated_banks > has a crypto_id field. Ok, thanks. > > > + d.alg_id = tpm2_hash_map[i].tpm_id; > > + break; > > + } > > + } > > + > > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) { > > + if (ima_tpm_chip->allocated_banks[i].alg_id == d.alg_id) > > + break; > > + } > > + > > + if (i == ima_tpm_chip->nr_allocated_banks) > > + d.alg_id = ima_tpm_chip->allocated_banks[0].alg_id; > > This code assumes that the algorithm used to calculate boot_aggregate and > the algorithm of the PCR bank can be different. I don't know if it is possible to > communicate to the verifier which bank has been selected (it depends on > the local configuration). Agreed, but defaulting to the first bank would only happen if the IMA default hash algorithm is not a configured TPM algorithm. > > In my opinion the safest approach would be to use the same algorithm for the > digest and the PCR bank. If you agree to this, then the code above must be > moved to ima_calc_boot_aggregate() so that the algorithm of the selected > PCR bank can be passed to ima_alloc_tfm(). Using the same hash algorithm, preferably the IMA hash default algorithm, for reading the TPM PCR bank and calculating the boot_aggregate makes sense. > > The selected PCR bank might be not the first, if the algorithm is unknown to > the crypto subsystem. It sounds like you're suggesting finding a common configured hash algorithm between the TPM and the kernel. > > > + pr_info("Calculating the boot-aggregregate, reading TPM PCR > > Typo. thanks Mimi
> -----Original Message----- > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Tuesday, January 28, 2020 4:41 PM > To: Roberto Sassu <roberto.sassu@huawei.com>; linux- > integrity@vger.kernel.org > Cc: Jerry Snitselaar <jsnitsel@redhat.com>; James Bottomley > <James.Bottomley@HansenPartnership.com>; linux- > kernel@vger.kernel.org; Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com> > Subject: Re: [PATCH 2/2] ima: support calculating the boot_aggregate based > on different TPM banks > > On Tue, 2020-01-28 at 14:19 +0000, Roberto Sassu wrote: > > > -----Original Message----- > > > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity- > > > owner@vger.kernel.org] On Behalf Of Mimi Zohar > > > Sent: Monday, January 27, 2020 5:02 PM > > > To: linux-integrity@vger.kernel.org > > > Cc: Jerry Snitselaar <jsnitsel@redhat.com>; James Bottomley > > > <James.Bottomley@HansenPartnership.com>; linux- > > > kernel@vger.kernel.org; Mimi Zohar <zohar@linux.ibm.com> > > > Subject: [PATCH 2/2] ima: support calculating the boot_aggregate based > on > > > different TPM banks > > > > > > Calculating the boot_aggregate attempts to read the TPM SHA1 bank, > > > assuming it is always enabled. With TPM 2.0 hash agility, TPM chips > > > could support multiple TPM PCR banks, allowing firmware to configure > and > > > enable different banks. > > > > > > Instead of hard coding the TPM 2.0 bank hash algorithm used for > calculating > > > the boot-aggregate, see if the configured IMA_DEFAULT_HASH > algorithm is > > > an allocated TPM bank, otherwise use the first allocated TPM bank. > > > > > > For TPM 1.2 SHA1 is the only supported hash algorithm. > > > > > > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > > --- > > > security/integrity/ima/ima_crypto.c | 37 > > > ++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 36 insertions(+), 1 deletion(-) > > > > > > diff --git a/security/integrity/ima/ima_crypto.c > > > b/security/integrity/ima/ima_crypto.c > > > index 7967a6904851..b1b26d61f174 100644 > > > --- a/security/integrity/ima/ima_crypto.c > > > +++ b/security/integrity/ima/ima_crypto.c > > > @@ -656,8 +656,25 @@ static void __init ima_pcrread(u32 idx, struct > > > tpm_digest *d) > > > pr_err("Error Communicating to TPM chip\n"); > > > } > > > > > > +/* tpm2_hash_map is the same as defined in tpm2-cmd.c and > > > trusted_tpm2.c */ > > > +static struct tpm2_hash tpm2_hash_map[] = { > > > + {HASH_ALGO_SHA1, TPM_ALG_SHA1}, > > > + {HASH_ALGO_SHA256, TPM_ALG_SHA256}, > > > + {HASH_ALGO_SHA384, TPM_ALG_SHA384}, > > > + {HASH_ALGO_SHA512, TPM_ALG_SHA512}, > > > + {HASH_ALGO_SM3_256, TPM_ALG_SM3_256}, > > > +}; > > > + > > > /* > > > - * Calculate the boot aggregate hash > > > + * The boot_aggregate is a cumulative hash over TPM registers 0 - 7. > With > > > + * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR > banks, > > > + * allowing firmware to configure and enable different banks. > > > + * > > > + * Instead of hard coding the TPM bank hash algorithm used for > calculating > > > + * the boot-aggregate, see if the configured IMA_DEFAULT_HASH > > > algorithm is > > > + * an allocated TPM bank, otherwise use the first allocated TPM bank. > > > + * > > > + * For TPM 1.2 SHA1 is the only hash algorithm. > > > */ > > > static int __init ima_calc_boot_aggregate_tfm(char *digest, > > > struct crypto_shash *tfm) > > > @@ -673,6 +690,24 @@ static int __init > ima_calc_boot_aggregate_tfm(char > > > *digest, > > > if (rc != 0) > > > return rc; > > > > > > + for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) { > > > + if (tpm2_hash_map[i].crypto_id == ima_hash_algo) { > > > > It is not necessary to define a new map. ima_tpm_chip->allocated_banks > > has a crypto_id field. > > Ok, thanks. When you send the new version, please include patch 1/8 of my patch set, that sets crypto_id to HASH_ALGO__LAST if there is no mapping between TPM ID and crypto ID. Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
On Tue, 2020-01-28 at 10:40 -0500, Mimi Zohar wrote: > > This code assumes that the algorithm used to calculate boot_aggregate and > > the algorithm of the PCR bank can be different. I don't know if it is possible to > > communicate to the verifier which bank has been selected (it depends on > > the local configuration). > > Agreed, but defaulting to the first bank would only happen if the IMA > default hash algorithm is not a configured TPM algorithm. > > > > > In my opinion the safest approach would be to use the same algorithm for the > > digest and the PCR bank. If you agree to this, then the code above must be > > moved to ima_calc_boot_aggregate() so that the algorithm of the selected > > PCR bank can be passed to ima_alloc_tfm(). > > Using the same hash algorithm, preferably the IMA hash default > algorithm, for reading the TPM PCR bank and calculating the > boot_aggregate makes sense. > > > > > The selected PCR bank might be not the first, if the algorithm is unknown to > > the crypto subsystem. > > It sounds like you're suggesting finding a common configured hash > algorithm between the TPM and the kernel. I'd like to clarify the logic for the case when a common algorithm does not exist. None of the TPM allocated banks are known by the kernel. If the hash algorithm of the boot_aggregate represents not only that of the digest included in the measurement list, but also of the TPM PCR bank read, what should happen? Should the boot aggregate be 0's, like in the case when there isn't a TPM? thanks, Mimi
On Wed, 2020-01-29 at 18:20 -0500, Mimi Zohar wrote: > On Tue, 2020-01-28 at 10:40 -0500, Mimi Zohar wrote: > > > This code assumes that the algorithm used to calculate > > > boot_aggregate and the algorithm of the PCR bank can be > > > different. I don't know if it is possible to communicate to the > > > verifier which bank has been selected (it depends on the local > > > configuration). > > > > Agreed, but defaulting to the first bank would only happen if the > > IMA default hash algorithm is not a configured TPM algorithm. > > > > > > > > In my opinion the safest approach would be to use the same > > > algorithm for the digest and the PCR bank. If you agree to this, > > > then the code above must be moved to ima_calc_boot_aggregate() so > > > that the algorithm of the selected PCR bank can be passed to > > > ima_alloc_tfm(). > > > > Using the same hash algorithm, preferably the IMA hash default > > algorithm, for reading the TPM PCR bank and calculating the > > boot_aggregate makes sense. > > > > > > > > The selected PCR bank might be not the first, if the algorithm is > > > unknown to the crypto subsystem. > > > > It sounds like you're suggesting finding a common configured hash > > algorithm between the TPM and the kernel. > > I'd like to clarify the logic for the case when a common algorithm > does not exist. I'm not sure we need to twist ourselves in knots over this. The TCG client platform for 2.0 mandates sha256, so we should be able to count on it being present. I'd be happy to treat the failure to find sha256 on TPM 2.0 as a fatal error, and the same for the failure to find sha1 on TPM 1.2. > None of the TPM allocated banks are known by the kernel. If the > hash algorithm of the boot_aggregate represents not only that of the > digest included in the measurement list, but also of the TPM PCR bank > read, what should happen? Should the boot aggregate be 0's, like in > the case when there isn't a TPM? For TPM 1.2 sha1 is required and for TPM2 sha256 is. I'd just force search for those, based on the TPM version, and fail to use the TPM if they're not found. There should be a boot time and config option for weird hashes which might be the only ones on BRIC embedded devices, like Streebog or ZM2, but the clear default should be the TCG mandates and it should be up to anything weird to cope with their own induced problems and not make it part of the standard setup. James
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index 7967a6904851..b1b26d61f174 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -656,8 +656,25 @@ static void __init ima_pcrread(u32 idx, struct tpm_digest *d) pr_err("Error Communicating to TPM chip\n"); } +/* tpm2_hash_map is the same as defined in tpm2-cmd.c and trusted_tpm2.c */ +static struct tpm2_hash tpm2_hash_map[] = { + {HASH_ALGO_SHA1, TPM_ALG_SHA1}, + {HASH_ALGO_SHA256, TPM_ALG_SHA256}, + {HASH_ALGO_SHA384, TPM_ALG_SHA384}, + {HASH_ALGO_SHA512, TPM_ALG_SHA512}, + {HASH_ALGO_SM3_256, TPM_ALG_SM3_256}, +}; + /* - * Calculate the boot aggregate hash + * The boot_aggregate is a cumulative hash over TPM registers 0 - 7. With + * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks, + * allowing firmware to configure and enable different banks. + * + * Instead of hard coding the TPM bank hash algorithm used for calculating + * the boot-aggregate, see if the configured IMA_DEFAULT_HASH algorithm is + * an allocated TPM bank, otherwise use the first allocated TPM bank. + * + * For TPM 1.2 SHA1 is the only hash algorithm. */ static int __init ima_calc_boot_aggregate_tfm(char *digest, struct crypto_shash *tfm) @@ -673,6 +690,24 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest, if (rc != 0) return rc; + for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) { + if (tpm2_hash_map[i].crypto_id == ima_hash_algo) { + d.alg_id = tpm2_hash_map[i].tpm_id; + break; + } + } + + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) { + if (ima_tpm_chip->allocated_banks[i].alg_id == d.alg_id) + break; + } + + if (i == ima_tpm_chip->nr_allocated_banks) + d.alg_id = ima_tpm_chip->allocated_banks[0].alg_id; + + pr_info("Calculating the boot-aggregregate, reading TPM PCR bank: %04x", + d.alg_id); + /* cumulative sha1 over tpm registers 0-7 */ for (i = TPM_PCR0; i < TPM_PCR8; i++) { ima_pcrread(i, &d);
Calculating the boot_aggregate attempts to read the TPM SHA1 bank, assuming it is always enabled. With TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks, allowing firmware to configure and enable different banks. Instead of hard coding the TPM 2.0 bank hash algorithm used for calculating the boot-aggregate, see if the configured IMA_DEFAULT_HASH algorithm is an allocated TPM bank, otherwise use the first allocated TPM bank. For TPM 1.2 SHA1 is the only supported hash algorithm. Reported-by: Jerry Snitselaar <jsnitsel@redhat.com> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> --- security/integrity/ima/ima_crypto.c | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-)