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

[Ilias Apalodimas]

Hi Akashi-san,


On Wed, 19 Jan 2022 at 06:47, AKASHI Takahiro
<takahiro.akashi at 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 at 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'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
> > > >>>>>
> > > >>
> > >