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

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

[Ilias Apalodimas]

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);
-			break;
+			continue;
 		}
 
 		if (!efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
-- 
2.32.0

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

[Heinrich Schuchardt]

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.

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

For security reasons we should not support EFI_CERT_SHA1_GUID for db.

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

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

Best regards

Heinrich

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

[Ilias Apalodimas]

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