diff mbox series

[RFC] efi_loader: clean up uefi secure boot image verification logic

Message ID 20220124153621.662350-1-ilias.apalodimas@linaro.org
State Superseded
Headers show
Series [RFC] efi_loader: clean up uefi secure boot image verification logic | expand

Commit Message

Ilias Apalodimas Jan. 24, 2022, 3:36 p.m. UTC
From: Ilias Apalodimas <apalos@gmail.com>

We currently distinguish between signed and non signed PE/COFF
executables while trying to authenticate signatures and/or sha256
hashes in db and dbx.  That code duplication can be avoided.
On sha256 hashes we don't really care if the image is signed or
not.  The logic can be implemented in
 1. Is the sha256 of the image in dbx
 2. Is the image signed with a certificate that's found in db and
    not in dbx
 3. The image carries a cert which is signed by a cert in db (and
    not in dbx, and the image can be verified against the former)
 4. Is the sha256 of the image in db
So weed out the 'special' case handling unsigned images.

While at it move the checking of a hash outside the certificate
verification loop which isn't really needed and check for the hash
only once.  Also allow a mix of signatures and hashes in db instead
of explicitly breaking out the sha verification loop if a signature
happens to be added first.

It's worth noting that (2) only works if the certificate is self
signed,  but that canb be fixed in a following patch.

Signed-off-by: Ilias Apalodimas <apalos@gmail.com>
---
 lib/efi_loader/efi_image_loader.c | 84 ++++++-------------------------
 lib/efi_loader/efi_signature.c    |  2 +-
 2 files changed, 15 insertions(+), 71 deletions(-)

Comments

Heinrich Schuchardt Jan. 24, 2022, 5:17 p.m. UTC | #1
On 1/24/22 16:36, Ilias Apalodimas wrote:
> From: Ilias Apalodimas <apalos@gmail.com>
>
> We currently distinguish between signed and non signed PE/COFF
> executables while trying to authenticate signatures and/or sha256
> hashes in db and dbx.  That code duplication can be avoided.
> On sha256 hashes we don't really care if the image is signed or
> not.  The logic can be implemented in
>   1. Is the sha256 of the image in dbx
>   2. Is the image signed with a certificate that's found in db and
>      not in dbx
>   3. The image carries a cert which is signed by a cert in db (and
>      not in dbx, and the image can be verified against the former)
>   4. Is the sha256 of the image in db
> So weed out the 'special' case handling unsigned images.
>
> While at it move the checking of a hash outside the certificate
> verification loop which isn't really needed and check for the hash
> only once.  Also allow a mix of signatures and hashes in db instead
> of explicitly breaking out the sha verification loop if a signature
> happens to be added first.
>
> It's worth noting that (2) only works if the certificate is self
> signed,  but that canb be fixed in a following patch.
>
> Signed-off-by: Ilias Apalodimas <apalos@gmail.com>
> ---
>   lib/efi_loader/efi_image_loader.c | 84 ++++++-------------------------
>   lib/efi_loader/efi_signature.c    |  2 +-
>   2 files changed, 15 insertions(+), 71 deletions(-)
>
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index 255613eb72ba..53d54ca42f5c 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -516,63 +516,16 @@ err:
>   }
>
>   #ifdef CONFIG_EFI_SECURE_BOOT
> -/**
> - * efi_image_unsigned_authenticate() - authenticate unsigned image with
> - * SHA256 hash
> - * @regs:	List of regions to be verified
> - *
> - * If an image is not signed, it doesn't have a signature. In this case,
> - * its message digest is calculated and it will be compared with one of
> - * hash values stored in signature databases.
> - *
> - * Return:	true if authenticated, false if not
> - */
> -static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs)
> -{
> -	struct efi_signature_store *db = NULL, *dbx = NULL;
> -	bool ret = false;
> -
> -	dbx = efi_sigstore_parse_sigdb(L"dbx");
> -	if (!dbx) {
> -		EFI_PRINT("Getting signature database(dbx) failed\n");
> -		goto out;
> -	}
> -
> -	db = efi_sigstore_parse_sigdb(L"db");
> -	if (!db) {
> -		EFI_PRINT("Getting signature database(db) failed\n");
> -		goto out;
> -	}
> -
> -	/* try black-list first */
> -	if (efi_signature_lookup_digest(regs, dbx)) {
> -		EFI_PRINT("Image is not signed and its digest found in \"dbx\"\n");
> -		goto out;
> -	}
> -
> -	/* try white-list */
> -	if (efi_signature_lookup_digest(regs, db))
> -		ret = true;
> -	else
> -		EFI_PRINT("Image is not signed and its digest not found in \"db\" or \"dbx\"\n");
> -
> -out:
> -	efi_sigstore_free(db);
> -	efi_sigstore_free(dbx);
> -
> -	return ret;
> -}
> -
>   /**
>    * efi_image_authenticate() - verify a signature of signed image
>    * @efi:	Pointer to image
>    * @efi_size:	Size of @efi
>    *
> - * A signed image should have its signature stored in a table of its PE header.
> + * An image should have its signature stored in a table of its PE header.

The signature must be stored in the "Certificate Table". See PE-COFF
specification. The PE header only contains the Optional Header Data
Directory that points to the table.

Best regards

Heinrich

>    * So if an image is signed and only if if its signature is verified using
> - * signature databases, an image is authenticated.
> - * If an image is not signed, its validity is checked by using
> - * efi_image_unsigned_authenticated().
> + * signature databases or if it's sha256 is found in db an image is
> + * authenticated.
> + *
>    * TODO:
>    * When AuditMode==0, if the image's signature is not found in
>    * the authorized database, or is found in the forbidden database,
> @@ -608,14 +561,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>   	if (!efi_image_parse(new_efi, efi_size, &regs, &wincerts,
>   			     &wincerts_len)) {
>   		EFI_PRINT("Parsing PE executable image failed\n");
> -		goto err;
> -	}
> -
> -	if (!wincerts) {
> -		/* The image is not signed */
> -		ret = efi_image_unsigned_authenticate(regs);
> -
> -		goto err;
> +		goto out;
>   	}
>
>   	/*
> @@ -624,18 +570,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>   	db = efi_sigstore_parse_sigdb(L"db");
>   	if (!db) {
>   		EFI_PRINT("Getting signature database(db) failed\n");
> -		goto err;
> +		goto out;
>   	}
>
>   	dbx = efi_sigstore_parse_sigdb(L"dbx");
>   	if (!dbx) {
>   		EFI_PRINT("Getting signature database(dbx) failed\n");
> -		goto err;
> +		goto out;
>   	}
>
>   	if (efi_signature_lookup_digest(regs, dbx)) {
>   		EFI_PRINT("Image's digest was found in \"dbx\"\n");
> -		goto err;
> +		goto out;
>   	}
>
>   	/*
> @@ -729,20 +675,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>   		/* try white-list */
>   		if (efi_signature_verify(regs, msg, db, dbx)) {
>   			ret = true;
> -			break;
> +			goto out;
>   		}
>
>   		EFI_PRINT("Signature was not verified by \"db\"\n");
>
> -		if (efi_signature_lookup_digest(regs, db)) {
> -			ret = true;
> -			break;
> -		}
> -
> -		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
>   	}
>
> -err:
> +	if (efi_signature_lookup_digest(regs, db))
> +		ret = true;
> +	else
> +		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
> +out:
>   	efi_sigstore_free(db);
>   	efi_sigstore_free(dbx);
>   	pkcs7_free_message(msg);
> 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)) {
AKASHI Takahiro Jan. 25, 2022, 2:31 a.m. UTC | #2
Hi Ilias,

On Mon, Jan 24, 2022 at 05:36:20PM +0200, Ilias Apalodimas wrote:
> From: Ilias Apalodimas <apalos@gmail.com>
> 
> We currently distinguish between signed and non signed PE/COFF
> executables while trying to authenticate signatures and/or sha256
> hashes in db and dbx.  That code duplication can be avoided.

Thank you for cleaning up the code.
The change you made here looks good.
I want you to revise some wordings for clarification.

> On sha256 hashes we don't really care if the image is signed or
> not.

The sentence above might sound a bit misleading?

> The logic can be implemented in
>  1. Is the sha256 of the image in dbx

Please explicitly mention that the image will be rejected
in this case unlike the other cases below.

>  2. Is the image signed with a certificate that's found in db and
>     not in dbx
>  3. The image carries a cert which is signed by a cert in db (and
>     not in dbx, and the image can be verified against the former)

(2) and (3) are the rules applied only if the image is signed. So,
   2. If the image is signed,
      2-1. ...
      2-2. ...

In addition, your (3) should be described in a similar way as (2).
For instance,
   3. Is the image signed with a certificate that is carried in
      the image and the cert is signed by another cert in the image
      and so on, or in db (... ).

(although it is difficult to describe the logic precisely.)

>  4. Is the sha256 of the image in db
> So weed out the 'special' case handling unsigned images.
> 
> While at it move the checking of a hash outside the certificate
> verification loop which isn't really needed and check for the hash
> only once.

It might be my preference, but I would like to add
  assert(ret == false);
before efi_signature_lookup_digest(regs, db) to indicate this is the last
resort to authenticate the image.

> Also allow a mix of signatures and hashes in db instead
> of explicitly breaking out the sha verification loop if a signature
> happens to be added first.

I don't think that your change gives us an extra case of "allowing".
Please elaborate if I misunderstand something.

> It's worth noting that (2) only works if the certificate is self
> signed,  but that canb be fixed in a following patch.

Yes, please.

-Takahiro Akashi

> Signed-off-by: Ilias Apalodimas <apalos@gmail.com>
> ---
>  lib/efi_loader/efi_image_loader.c | 84 ++++++-------------------------
>  lib/efi_loader/efi_signature.c    |  2 +-
>  2 files changed, 15 insertions(+), 71 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index 255613eb72ba..53d54ca42f5c 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -516,63 +516,16 @@ err:
>  }
>  
>  #ifdef CONFIG_EFI_SECURE_BOOT
> -/**
> - * efi_image_unsigned_authenticate() - authenticate unsigned image with
> - * SHA256 hash
> - * @regs:	List of regions to be verified
> - *
> - * If an image is not signed, it doesn't have a signature. In this case,
> - * its message digest is calculated and it will be compared with one of
> - * hash values stored in signature databases.
> - *
> - * Return:	true if authenticated, false if not
> - */
> -static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs)
> -{
> -	struct efi_signature_store *db = NULL, *dbx = NULL;
> -	bool ret = false;
> -
> -	dbx = efi_sigstore_parse_sigdb(L"dbx");
> -	if (!dbx) {
> -		EFI_PRINT("Getting signature database(dbx) failed\n");
> -		goto out;
> -	}
> -
> -	db = efi_sigstore_parse_sigdb(L"db");
> -	if (!db) {
> -		EFI_PRINT("Getting signature database(db) failed\n");
> -		goto out;
> -	}
> -
> -	/* try black-list first */
> -	if (efi_signature_lookup_digest(regs, dbx)) {
> -		EFI_PRINT("Image is not signed and its digest found in \"dbx\"\n");
> -		goto out;
> -	}
> -
> -	/* try white-list */
> -	if (efi_signature_lookup_digest(regs, db))
> -		ret = true;
> -	else
> -		EFI_PRINT("Image is not signed and its digest not found in \"db\" or \"dbx\"\n");
> -
> -out:
> -	efi_sigstore_free(db);
> -	efi_sigstore_free(dbx);
> -
> -	return ret;
> -}
> -
>  /**
>   * efi_image_authenticate() - verify a signature of signed image
>   * @efi:	Pointer to image
>   * @efi_size:	Size of @efi
>   *
> - * A signed image should have its signature stored in a table of its PE header.
> + * An image should have its signature stored in a table of its PE header.
>   * So if an image is signed and only if if its signature is verified using
> - * signature databases, an image is authenticated.
> - * If an image is not signed, its validity is checked by using
> - * efi_image_unsigned_authenticated().
> + * signature databases or if it's sha256 is found in db an image is
> + * authenticated.
> + *
>   * TODO:
>   * When AuditMode==0, if the image's signature is not found in
>   * the authorized database, or is found in the forbidden database,
> @@ -608,14 +561,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  	if (!efi_image_parse(new_efi, efi_size, &regs, &wincerts,
>  			     &wincerts_len)) {
>  		EFI_PRINT("Parsing PE executable image failed\n");
> -		goto err;
> -	}
> -
> -	if (!wincerts) {
> -		/* The image is not signed */
> -		ret = efi_image_unsigned_authenticate(regs);
> -
> -		goto err;
> +		goto out;
>  	}
>  
>  	/*
> @@ -624,18 +570,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  	db = efi_sigstore_parse_sigdb(L"db");
>  	if (!db) {
>  		EFI_PRINT("Getting signature database(db) failed\n");
> -		goto err;
> +		goto out;
>  	}
>  
>  	dbx = efi_sigstore_parse_sigdb(L"dbx");
>  	if (!dbx) {
>  		EFI_PRINT("Getting signature database(dbx) failed\n");
> -		goto err;
> +		goto out;
>  	}
>  
>  	if (efi_signature_lookup_digest(regs, dbx)) {
>  		EFI_PRINT("Image's digest was found in \"dbx\"\n");
> -		goto err;
> +		goto out;
>  	}
>  
>  	/*
> @@ -729,20 +675,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  		/* try white-list */
>  		if (efi_signature_verify(regs, msg, db, dbx)) {
>  			ret = true;
> -			break;
> +			goto out;
>  		}
>  
>  		EFI_PRINT("Signature was not verified by \"db\"\n");
>  
> -		if (efi_signature_lookup_digest(regs, db)) {
> -			ret = true;
> -			break;
> -		}
> -
> -		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
>  	}
>  
> -err:
> +	if (efi_signature_lookup_digest(regs, db))
> +		ret = true;
> +	else
> +		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
> +out:
>  	efi_sigstore_free(db);
>  	efi_sigstore_free(dbx);
>  	pkcs7_free_message(msg);
> 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.30.2
>
Ilias Apalodimas Jan. 25, 2022, 7:25 a.m. UTC | #3
Hi Akashi-san, 

On Tue, Jan 25, 2022 at 11:31:07AM +0900, AKASHI Takahiro wrote:
> Hi Ilias,
> 
> On Mon, Jan 24, 2022 at 05:36:20PM +0200, Ilias Apalodimas wrote:
> > From: Ilias Apalodimas <apalos@gmail.com>
> > 
> > We currently distinguish between signed and non signed PE/COFF
> > executables while trying to authenticate signatures and/or sha256
> > hashes in db and dbx.  That code duplication can be avoided.
> 
> Thank you for cleaning up the code.
> The change you made here looks good.
> I want you to revise some wordings for clarification.
> 
> > On sha256 hashes we don't really care if the image is signed or
> > not.
> 
> The sentence above might sound a bit misleading?

I can change that to 'when checking for sha256 hashes in db'  or something
along those lines 

> 
> > The logic can be implemented in
> >  1. Is the sha256 of the image in dbx
> 
> Please explicitly mention that the image will be rejected
> in this case unlike the other cases below.

sure

> 
> >  2. Is the image signed with a certificate that's found in db and
> >     not in dbx
> >  3. The image carries a cert which is signed by a cert in db (and
> >     not in dbx, and the image can be verified against the former)
> 
> (2) and (3) are the rules applied only if the image is signed. So,
>    2. If the image is signed,
>       2-1. ...
>       2-2. ...
> 
> In addition, your (3) should be described in a similar way as (2).
> For instance,
>    3. Is the image signed with a certificate that is carried in
>       the image and the cert is signed by another cert in the image
>       and so on, or in db (... ).

Doesn't the 'signed against the former' describes the same thing here?

> 
> (although it is difficult to describe the logic precisely.)
> 
> >  4. Is the sha256 of the image in db
> > So weed out the 'special' case handling unsigned images.
> > 
> > While at it move the checking of a hash outside the certificate
> > verification loop which isn't really needed and check for the hash
> > only once.
> 
> It might be my preference, but I would like to add
>   assert(ret == false);
> before efi_signature_lookup_digest(regs, db) to indicate this is the last
> resort to authenticate the image.

I am not a fan of asserts myself.  How about a comment or an EFI_PRINT?
> 
> > Also allow a mix of signatures and hashes in db instead
> > of explicitly breaking out the sha verification loop if a signature
> > happens to be added first.
> 
> I don't think that your change gives us an extra case of "allowing".
> Please elaborate if I misunderstand something.

This might actually be better of in a separated patch. 
The patch for efi_signature.c changes the logic a bit.
Right now when we try to verify a hash and find a non supported algorithm
we stop processing db.  But that assumes that the hashes are added first
and the signatures follow.
IOW if you add in db <sha256 hash, esl cert> everything works fine when you
try to authenticate an image with it's hash.  If the order is reversed the
code rejects the image (while it shouldn't)

> 
> > It's worth noting that (2) only works if the certificate is self
> > signed,  but that canb be fixed in a following patch.
> 
> Yes, please.
> 
> -Takahiro Akashi

Thanks
/Ilias

> 
> > Signed-off-by: Ilias Apalodimas <apalos@gmail.com>
> > ---
> >  lib/efi_loader/efi_image_loader.c | 84 ++++++-------------------------
> >  lib/efi_loader/efi_signature.c    |  2 +-
> >  2 files changed, 15 insertions(+), 71 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> > index 255613eb72ba..53d54ca42f5c 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -516,63 +516,16 @@ err:
> >  }
> >  
> >  #ifdef CONFIG_EFI_SECURE_BOOT
> > -/**
> > - * efi_image_unsigned_authenticate() - authenticate unsigned image with
> > - * SHA256 hash
> > - * @regs:	List of regions to be verified
> > - *
> > - * If an image is not signed, it doesn't have a signature. In this case,
> > - * its message digest is calculated and it will be compared with one of
> > - * hash values stored in signature databases.
> > - *
> > - * Return:	true if authenticated, false if not
> > - */
> > -static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs)
> > -{
> > -	struct efi_signature_store *db = NULL, *dbx = NULL;
> > -	bool ret = false;
> > -
> > -	dbx = efi_sigstore_parse_sigdb(L"dbx");
> > -	if (!dbx) {
> > -		EFI_PRINT("Getting signature database(dbx) failed\n");
> > -		goto out;
> > -	}
> > -
> > -	db = efi_sigstore_parse_sigdb(L"db");
> > -	if (!db) {
> > -		EFI_PRINT("Getting signature database(db) failed\n");
> > -		goto out;
> > -	}
> > -
> > -	/* try black-list first */
> > -	if (efi_signature_lookup_digest(regs, dbx)) {
> > -		EFI_PRINT("Image is not signed and its digest found in \"dbx\"\n");
> > -		goto out;
> > -	}
> > -
> > -	/* try white-list */
> > -	if (efi_signature_lookup_digest(regs, db))
> > -		ret = true;
> > -	else
> > -		EFI_PRINT("Image is not signed and its digest not found in \"db\" or \"dbx\"\n");
> > -
> > -out:
> > -	efi_sigstore_free(db);
> > -	efi_sigstore_free(dbx);
> > -
> > -	return ret;
> > -}
> > -
> >  /**
> >   * efi_image_authenticate() - verify a signature of signed image
> >   * @efi:	Pointer to image
> >   * @efi_size:	Size of @efi
> >   *
> > - * A signed image should have its signature stored in a table of its PE header.
> > + * An image should have its signature stored in a table of its PE header.
> >   * So if an image is signed and only if if its signature is verified using
> > - * signature databases, an image is authenticated.
> > - * If an image is not signed, its validity is checked by using
> > - * efi_image_unsigned_authenticated().
> > + * signature databases or if it's sha256 is found in db an image is
> > + * authenticated.
> > + *
> >   * TODO:
> >   * When AuditMode==0, if the image's signature is not found in
> >   * the authorized database, or is found in the forbidden database,
> > @@ -608,14 +561,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  	if (!efi_image_parse(new_efi, efi_size, &regs, &wincerts,
> >  			     &wincerts_len)) {
> >  		EFI_PRINT("Parsing PE executable image failed\n");
> > -		goto err;
> > -	}
> > -
> > -	if (!wincerts) {
> > -		/* The image is not signed */
> > -		ret = efi_image_unsigned_authenticate(regs);
> > -
> > -		goto err;
> > +		goto out;
> >  	}
> >  
> >  	/*
> > @@ -624,18 +570,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  	db = efi_sigstore_parse_sigdb(L"db");
> >  	if (!db) {
> >  		EFI_PRINT("Getting signature database(db) failed\n");
> > -		goto err;
> > +		goto out;
> >  	}
> >  
> >  	dbx = efi_sigstore_parse_sigdb(L"dbx");
> >  	if (!dbx) {
> >  		EFI_PRINT("Getting signature database(dbx) failed\n");
> > -		goto err;
> > +		goto out;
> >  	}
> >  
> >  	if (efi_signature_lookup_digest(regs, dbx)) {
> >  		EFI_PRINT("Image's digest was found in \"dbx\"\n");
> > -		goto err;
> > +		goto out;
> >  	}
> >  
> >  	/*
> > @@ -729,20 +675,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  		/* try white-list */
> >  		if (efi_signature_verify(regs, msg, db, dbx)) {
> >  			ret = true;
> > -			break;
> > +			goto out;
> >  		}
> >  
> >  		EFI_PRINT("Signature was not verified by \"db\"\n");
> >  
> > -		if (efi_signature_lookup_digest(regs, db)) {
> > -			ret = true;
> > -			break;
> > -		}
> > -
> > -		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
> >  	}
> >  
> > -err:
> > +	if (efi_signature_lookup_digest(regs, db))
> > +		ret = true;
> > +	else
> > +		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
> > +out:
> >  	efi_sigstore_free(db);
> >  	efi_sigstore_free(dbx);
> >  	pkcs7_free_message(msg);
> > 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.30.2
> >
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index 255613eb72ba..53d54ca42f5c 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -516,63 +516,16 @@  err:
 }
 
 #ifdef CONFIG_EFI_SECURE_BOOT
-/**
- * efi_image_unsigned_authenticate() - authenticate unsigned image with
- * SHA256 hash
- * @regs:	List of regions to be verified
- *
- * If an image is not signed, it doesn't have a signature. In this case,
- * its message digest is calculated and it will be compared with one of
- * hash values stored in signature databases.
- *
- * Return:	true if authenticated, false if not
- */
-static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs)
-{
-	struct efi_signature_store *db = NULL, *dbx = NULL;
-	bool ret = false;
-
-	dbx = efi_sigstore_parse_sigdb(L"dbx");
-	if (!dbx) {
-		EFI_PRINT("Getting signature database(dbx) failed\n");
-		goto out;
-	}
-
-	db = efi_sigstore_parse_sigdb(L"db");
-	if (!db) {
-		EFI_PRINT("Getting signature database(db) failed\n");
-		goto out;
-	}
-
-	/* try black-list first */
-	if (efi_signature_lookup_digest(regs, dbx)) {
-		EFI_PRINT("Image is not signed and its digest found in \"dbx\"\n");
-		goto out;
-	}
-
-	/* try white-list */
-	if (efi_signature_lookup_digest(regs, db))
-		ret = true;
-	else
-		EFI_PRINT("Image is not signed and its digest not found in \"db\" or \"dbx\"\n");
-
-out:
-	efi_sigstore_free(db);
-	efi_sigstore_free(dbx);
-
-	return ret;
-}
-
 /**
  * efi_image_authenticate() - verify a signature of signed image
  * @efi:	Pointer to image
  * @efi_size:	Size of @efi
  *
- * A signed image should have its signature stored in a table of its PE header.
+ * An image should have its signature stored in a table of its PE header.
  * So if an image is signed and only if if its signature is verified using
- * signature databases, an image is authenticated.
- * If an image is not signed, its validity is checked by using
- * efi_image_unsigned_authenticated().
+ * signature databases or if it's sha256 is found in db an image is
+ * authenticated.
+ *
  * TODO:
  * When AuditMode==0, if the image's signature is not found in
  * the authorized database, or is found in the forbidden database,
@@ -608,14 +561,7 @@  static bool efi_image_authenticate(void *efi, size_t efi_size)
 	if (!efi_image_parse(new_efi, efi_size, &regs, &wincerts,
 			     &wincerts_len)) {
 		EFI_PRINT("Parsing PE executable image failed\n");
-		goto err;
-	}
-
-	if (!wincerts) {
-		/* The image is not signed */
-		ret = efi_image_unsigned_authenticate(regs);
-
-		goto err;
+		goto out;
 	}
 
 	/*
@@ -624,18 +570,18 @@  static bool efi_image_authenticate(void *efi, size_t efi_size)
 	db = efi_sigstore_parse_sigdb(L"db");
 	if (!db) {
 		EFI_PRINT("Getting signature database(db) failed\n");
-		goto err;
+		goto out;
 	}
 
 	dbx = efi_sigstore_parse_sigdb(L"dbx");
 	if (!dbx) {
 		EFI_PRINT("Getting signature database(dbx) failed\n");
-		goto err;
+		goto out;
 	}
 
 	if (efi_signature_lookup_digest(regs, dbx)) {
 		EFI_PRINT("Image's digest was found in \"dbx\"\n");
-		goto err;
+		goto out;
 	}
 
 	/*
@@ -729,20 +675,18 @@  static bool efi_image_authenticate(void *efi, size_t efi_size)
 		/* try white-list */
 		if (efi_signature_verify(regs, msg, db, dbx)) {
 			ret = true;
-			break;
+			goto out;
 		}
 
 		EFI_PRINT("Signature was not verified by \"db\"\n");
 
-		if (efi_signature_lookup_digest(regs, db)) {
-			ret = true;
-			break;
-		}
-
-		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
 	}
 
-err:
+	if (efi_signature_lookup_digest(regs, db))
+		ret = true;
+	else
+		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
+out:
 	efi_sigstore_free(db);
 	efi_sigstore_free(dbx);
 	pkcs7_free_message(msg);
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)) {