Re: [PATCH] efi_loader: correctly handle mixed hashes and signatures in db

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

On Fri, Jan 28, 2022 at 08:30:12AM +0100, Heinrich Schuchardt wrote:
> On 1/27/22 23:36, Ilias Apalodimas wrote:
> > A mix of signatures and hashes in db doesn't always work as intended.
> > Currently if the digest algorithm is not supported we stop walking the
> > security database and reject the image.
> > That's problematic in case we find and try to check a signature before
> > inspecting the sha256 hash.  If the image is unsigned we will reject it
> > even if the digest matches
> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   lib/efi_loader/efi_signature.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> > index 3243e2c60de0..8fa82f8cea4c 100644
> > --- a/lib/efi_loader/efi_signature.c
> > +++ b/lib/efi_loader/efi_signature.c
> > @@ -176,7 +176,7 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
> >   		if (guidcmp(&siglist->sig_type, &efi_guid_sha256)) {
> >   			EFI_PRINT("Digest algorithm is not supported: %pUs\n",
> >   				  &siglist->sig_type);
> 
> According to the commit message siglist->sig_type could be
> EFI_CERT_RSA2048_SHA256_GUID which does not relate to a 'Digest
> algorithm'. So the debug message text is wrong. As we expect guidcmp()
> to report a mismatch we could remove the message.

Ok

> 
> > -			break;
> > +			continue;
> 
> This still is not correct:
> 
> dbx containing a signature type that we do not support must disable the
> loading of any image.
> 
> The UEFI specification defines EFI_CERT_SHA1_GUID, EFI_CERT_SHA384_GUID
> and EFI_CERT_SHA512_GUID. We should support all of these for dbx.
> 

I don't really think we should go and support all of those.  I have doubts
about why we support time based revocation to begin with. 

> For security reasons we should not support EFI_CERT_SHA1_GUID for db.

Nor dbx, sha1 should be completely ignored imho.

> 
> The function lacks an argument indicating if we are dealing with db or
> dbx which have to be treated differently.

How about making it a bit more generic?  We can add the default value of
'ret'.   So we can easily handle cases were the algorithm requested is
not supported.

> 
> >   		}
> > 
> >   		if (!efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
> 
> The subsequent code has a performance issue:
> 
> We should not hash the image once per entry in db but once per hash
> algorithm.

Yea we seem to have this kind of issue in other parts as well.  I'll fix
that along the way

Thanks
/Ilias