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

[Heinrich Schuchardt]

On 1/24/22 16:36, Ilias Apalodimas wrote:
> From: Ilias Apalodimas <apalos at 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 at 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)) {