Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Wed, Jan 19, 2022 at 09:07:04AM +0200, Ilias Apalodimas wrote:
> Hi Akashi-san,
> 
> 
> On Wed, 19 Jan 2022 at 06:47, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > On Tue, Jan 18, 2022 at 08:12:22PM +0200, Ilias Apalodimas wrote:
> > > Hi Heinrich,
> > >
> > > On Tue, 18 Jan 2022 at 18:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >
> > > > On 1/18/22 15:03, Ilias Apalodimas wrote:
> > > > > Hi Heinrich,
> > > > >
> > > > >>>>> -         info.checksum = image_get_checksum_algo("sha256,rsa2048");
> > > > >
> > > > > [...]
> > > > >
> > > > >>>>> -         info.name = "sha256,rsa2048";
> > > > >>>>> - } else {
> > > > >>>>> -         pr_warn("unknown msg digest algo: %s\n", sig->hash_algo);
> > > > >>>>> + if (strcmp(sig->pkey_algo, "rsa")) {
> > > > >>>>> +         pr_err("Encryption is not RSA: %s\n", sig->pkey_algo);
> > > > >>>>>                   return -ENOPKG;
> > > > >>>>>           }
> > > > >>>>> + ret = snprintf(algo, sizeof(algo), "%s,%s%d", sig->hash_algo,
> > > > >>>>> +                sig->pkey_algo, sig->s_size * 8);
> > > > >>
> > > > >> How do we ensure that the unsafe SHA1 algorithm is not used?
> > > > >
> > > > > We don't,  but the current code allows it as well.  Should we enforce this
> > > > > from U-Boot  though?  The spec doesn't forbid it as far as I remember
> > > >
> > > > Collisions for SHA1 have been first created successfully in 2017.
> > > >
> > > > It is feasible to create two different EFI binaries with the same SHA1.
> > > > One will be reviewed and signed. After copying the signature to the
> > > > other one it will happily boot on U-Boot. Ouch. This is exactly what
> > > > signatures are meant to avoid.
> > > >
> > > > We must not accept SHA1 for signatures.
> > >
> > > Right, but is this the right place to do it? This is function to
> > > verify signatures.  Isn't it better to keep this as is and then
> > > explicitly deny adding sha1 hashed keys into db?
> >
> > If you don't want to trust SHA1, just disable it with !CONFIG_SHA1.
> 
> No that's not doable.  Things like EFI_TCG2 protocol needs that since
> we use a sha1 in the tcg eventlog.

I simply wonder why you can trust SHA1 in PCR/event log while you don't
trust it in secure boot.

-Takahiro Akashi

> I've looked at the code a bit more
> and not adding in db looks either bad or hard to reason about, since
> we do have different storage backends(i.e efi variables in RPMB via
> standaloneMM).  So one way to do this without affecting the generic
> crypto code is
> 
> bool efi_signature_verify(struct efi_image_regions *regs,
>                 if (ret < 0 || !signer)
>                         goto out;
> 
> +               if (!strcmp(signer->sig->hash_algo, "sha1")) {
> +                       pr_err("SHA1 support is disabled for EFI\n");
> +                       goto out;
> +               }
> +
>                 if (sinfo->blacklisted)
>                         goto out;
> 
> Cheers
> /Ilias
> 
> > -Takahiro Akashi
> >
> > > Cheers
> > > /Ilias
> > > >
> > > > Best regards
> > > >
> > > > Heinrich
> > > >
> > > > >
> > > > > Regards
> > > > > /Ilias
> > > > >>
> > > > >> Best regards
> > > > >>
> > > > >> Heinrich
> > > > >>
> > > > >>>>
> > > > >>>> I'm not sure that this naming rule, in particular the latter part, will
> > > > >>>> always hold in the future while all the existing algo's observe it.
> > > > >>>> (Maybe we need some note somewhere?)
> > > > >>>
> > > > >>> The if a few lines below will shield us and return -EINVAL.  How about
> > > > >>> adding an error message there?
> > > > >>>
> > > > >>> Cheers
> > > > >>> /Ilias
> > > > >>>>
> > > > >>>> -Takahiro Akashi
> > > > >>>>
> > > > >>>>> +
> > > > >>>>> + if (ret >= sizeof(algo))
> > > > >>>>> +         return -EINVAL;
> > > > >>>>> +
> > > > >>>>> + info.checksum = image_get_checksum_algo((const char *)algo);
> > > > >>>>> + info.name = (const char *)algo;
> > > > >>>>>           info.crypto = image_get_crypto_algo(info.name);
> > > > >>>>> - if (IS_ERR(info.checksum) || IS_ERR(info.crypto))
> > > > >>>>> + if (!info.checksum || !info.crypto)
> > > > >>>>>                   return -ENOPKG;
> > > > >>>>>
> > > > >>>>>           info.key = pkey->key;
> > > > >>>>> --
> > > > >>>>> 2.30.2
> > > > >>>>>
> > > > >>
> > > >