Questions on SHA1 in ima_init

Questions on SHA1 in ima_init

[Roberts, William C]

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/FUBD3MY5U5YICNUYSF3NE2STO3YAW7Y4/

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?

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";

Re: Questions on SHA1 in ima_init

[James Bottomley]

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";
> 
> 
> 
> 
> 

RE: Questions on SHA1 in ima_init

[Roberts, William C]

> -----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";
> >
> >
> >
> >
> >