| Message ID | 476DC76E7D1DF2438D32BFADF679FC5649EDCB91@ORSMSX101.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Questions on SHA1 in ima_init | expand |
On Mon, 2020-05-11 at 17:49 +0000, Roberts, William C wrote: > Hello, > > I'm part of the tpm2 users pace tooling and libraries, and I am > trying to track down an issue in where boot aggregate is only > extended in the SHA1 > bank of PCR10. > > You can read the details on the link below, but ill summarize here > - https://lists.01.org/hyperkitty/list/tpm2@lists.01.org/thread/FUB > D3MY5U5YICNUYSF3NE2STO3YAW7Y4/ > > It looks like ima_add_boot_aggregate() is hardcoded to SHA1, our > guess is, that it's so it works between TPM 1.X and TPM2.0 chips. Is > that > correct? > > I was wondering if that synopsis is correct and if there would be > traction to add something like querying the tpm chip and getting the > version And picking SHA256 if its tpm2.0, as a sample to guide the > discussion I included the patch below (totally untested/not- > compiled). The main downside would be leaking TPM versions into IMA > to make a decisions, so it may be better to have a helper in the tpm > code to pick the best default algorithm where it could pick SHA1 for > TPM1.X and SHA256 for TPM2.0. Thoughts? I think you're not tracking the list. The current patch set doing this among other things is here: https://lore.kernel.org/linux-integrity/20200325104712.25694-1-roberto.sassu@huawei.com/ The patch below is too simplistic. If you follow the threads on the list, you'll see we found a Dell with a TPM2 that won't enable the sha256 bank if it's set in bios to sha1 mode, which is why the actual patch here: https://lore.kernel.org/linux-integrity/20200325104712.25694-1-roberto.sassu@huawei.com/ Checks the supported banks and uses sha256 if it's listed. James > diff --git a/security/integrity/ima/ima_init.c > b/security/integrity/ima/ima_init.c > index 567468188a61..d0513bafeebf 100644 > --- a/security/integrity/ima/ima_init.c > +++ b/security/integrity/ima/ima_init.c > @@ -15,6 +15,7 @@ > #include <linux/scatterlist.h> > #include <linux/slab.h> > #include <linux/err.h> > +#include <linux/printk.h> > > #include "ima.h" > > @@ -59,6 +60,16 @@ static int __init ima_add_boot_aggregate(void) > iint->ima_hash->length = SHA1_DIGEST_SIZE; > > if (ima_tpm_chip) { > + result = tpm_is_tpm2(ima_tpm_chip); > + if (result > 0) { > + /* yes it's a TPM2 chipset use sha256 */ > + iint->ima_hash->algo = HASH_ALGO_SHA256; > + iint->ima_hash->length = SHA256_DIGEST_SIZE; > + } else if (result < 0) { > + /* ignore errors here, as we can just move on > with SHA1 */ > + pr_warn("Could not query TPM chip version, > got: %d\n", result); > + } > + > result = ima_calc_boot_aggregate(&hash.hdr); > if (result < 0) { > audit_cause = "hashing_error"; > > > > >
> -----Original Message----- > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity- > owner@vger.kernel.org] On Behalf Of James Bottomley > Sent: Monday, May 11, 2020 2:14 PM > To: Roberts, William C <william.c.roberts@intel.com>; linux- > integrity@vger.kernel.org > Subject: Re: Questions on SHA1 in ima_init > > On Mon, 2020-05-11 at 17:49 +0000, Roberts, William C wrote: > > Hello, > > > > I'm part of the tpm2 users pace tooling and libraries, and I am trying > > to track down an issue in where boot aggregate is only extended in the > > SHA1 bank of PCR10. > > > > You can read the details on the link below, but ill summarize here > > - https://lists.01.org/hyperkitty/list/tpm2@lists.01.org/thread/FUB > > D3MY5U5YICNUYSF3NE2STO3YAW7Y4/ > > > > It looks like ima_add_boot_aggregate() is hardcoded to SHA1, our guess > > is, that it's so it works between TPM 1.X and TPM2.0 chips. Is that > > correct? > > > > I was wondering if that synopsis is correct and if there would be > > traction to add something like querying the tpm chip and getting the > > version And picking SHA256 if its tpm2.0, as a sample to guide the > > discussion I included the patch below (totally untested/not- > > compiled). The main downside would be leaking TPM versions into IMA to > > make a decisions, so it may be better to have a helper in the tpm code > > to pick the best default algorithm where it could pick SHA1 for TPM1.X > > and SHA256 for TPM2.0. Thoughts? > > I think you're not tracking the list. The current patch set doing this among other > things is here: > > https://lore.kernel.org/linux-integrity/20200325104712.25694-1- > roberto.sassu@huawei.com/ > > The patch below is too simplistic. If you follow the threads on the list, you'll see > we found a Dell with a TPM2 that won't enable the > sha256 bank if it's set in bios to sha1 mode, which is why the actual patch here: > > https://lore.kernel.org/linux-integrity/20200325104712.25694-1- > roberto.sassu@huawei.com/ > > Checks the supported banks and uses sha256 if it's listed. > No I am not tracking this list and my grep foo was failing in the archives. This is exactly what I was looking for , thanks! > James > > > diff --git a/security/integrity/ima/ima_init.c > > b/security/integrity/ima/ima_init.c > > index 567468188a61..d0513bafeebf 100644 > > --- a/security/integrity/ima/ima_init.c > > +++ b/security/integrity/ima/ima_init.c > > @@ -15,6 +15,7 @@ > > #include <linux/scatterlist.h> > > #include <linux/slab.h> > > #include <linux/err.h> > > +#include <linux/printk.h> > > > > #include "ima.h" > > > > @@ -59,6 +60,16 @@ static int __init ima_add_boot_aggregate(void) > > iint->ima_hash->length = SHA1_DIGEST_SIZE; > > > > if (ima_tpm_chip) { > > + result = tpm_is_tpm2(ima_tpm_chip); > > + if (result > 0) { > > + /* yes it's a TPM2 chipset use sha256 */ > > + iint->ima_hash->algo = HASH_ALGO_SHA256; > > + iint->ima_hash->length = SHA256_DIGEST_SIZE; > > + } else if (result < 0) { > > + /* ignore errors here, as we can just move on > > with SHA1 */ > > + pr_warn("Could not query TPM chip version, > > got: %d\n", result); > > + } > > + > > result = ima_calc_boot_aggregate(&hash.hdr); > > if (result < 0) { > > audit_cause = "hashing_error"; > > > > > > > > > >
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 567468188a61..d0513bafeebf 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -15,6 +15,7 @@ #include <linux/scatterlist.h> #include <linux/slab.h> #include <linux/err.h> +#include <linux/printk.h> #include "ima.h" @@ -59,6 +60,16 @@ static int __init ima_add_boot_aggregate(void) iint->ima_hash->length = SHA1_DIGEST_SIZE; if (ima_tpm_chip) { + result = tpm_is_tpm2(ima_tpm_chip); + if (result > 0) { + /* yes it's a TPM2 chipset use sha256 */ + iint->ima_hash->algo = HASH_ALGO_SHA256; + iint->ima_hash->length = SHA256_DIGEST_SIZE; + } else if (result < 0) { + /* ignore errors here, as we can just move on with SHA1 */ + pr_warn("Could not query TPM chip version, got: %d\n", result); + } + result = ima_calc_boot_aggregate(&hash.hdr); if (result < 0) { audit_cause = "hashing_error";