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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Heinrich, 


On Wed, Jan 19, 2022 at 03:22:53PM +0100, Heinrich Schuchardt wrote:
> On 1/18/22 19:12, 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?
> 
> You must assume that PK, KEK, db are preexisting or are seeded from a
> file. So the check should be done when loading an image.

I've already replied on this and sent a v2. Can you have a look at that ?
It is happening during loading.  The call chain here is 
efi_load_pe() -> 
  efi_image_authenticate() -> 
    efi_signature_verify()

> 
> To be more precise:
> 
> An image should not be validated based on an SHA1 signature or SHA1 hash
> but it must be possible to reject an image based on an SHA1 hash.

That's two different things and imho should go into completely
different patchsets. 

The v2 I've sent only deals with certs.  If you want to kick out sha1 in 
general there's efi_signature_lookup_digest() we should go and change. 
But the purpose of this patchset is fix rsa4096 encrypted signatures,  not
remove the sha1 overall.  So can we review the v2 and I'll send a different
patchset for sha1 hashes.

Thanks
/Ilias
> 
> Best regards
> 
> Heinrich
> 
> > 
> > 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
> > > > > > > > 
> > > > > 
> > > 
>