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

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

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.

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