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

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

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