Re: [PATCH 4/5] efi_loader: image_loader: add a missing digest verification for signed PE image

Heinrich,

On Tue, Jul 05, 2022 at 05:29:07PM +0200, Heinrich Schuchardt wrote:

On 7/5/22 07:48, AKASHI Takahiro wrote:

...

At the last step of PE image authentication, an image's hash value must be compared with a message digest stored as the content (of SpcPeImageData type) of pkcs7's contentInfo.

Fixes: commit 4540dabdcaca ("efi_loader: image_loader: support image authentication") Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

---

lib/efi_loader/Kconfig | 1 + lib/efi_loader/efi_image_loader.c | 62 ++++++++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index e2a1a5a69a24..e3f2402d0e8e 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -366,6 +366,7 @@ config EFI_SECURE_BOOT select X509_CERTIFICATE_PARSER select PKCS7_MESSAGE_PARSER select PKCS7_VERIFY

• select MSCODE_PARSER select EFI_SIGNATURE_SUPPORT help Select this option to enable EFI secure boot support.

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index fe8e4a89082c..eaf75a5803d4 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -16,6 +16,7 @@ #include <malloc.h> #include <pe.h> #include <sort.h> +#include <crypto/mscode.h> #include <crypto/pkcs7_parser.h> #include <linux/err.h>

@@ -516,6 +517,51 @@ err: }

#ifdef CONFIG_EFI_SECURE_BOOT +/**

• • efi_image_verify_digest - verify image's message digest
• • @regs: Array of memory regions to digest
• • @msg: Signature in pkcs7 structure
• • @regs contains all the data in a PE image to digest. Calculate
• • a hash value based on @regs and compare it with a messaged digest
• • in the content (SpcPeImageData) of @msg's contentInfo.
• • Return: true if verified, false if not
• */

+static bool efi_image_verify_digest(struct efi_image_regions *regs,

• 		    struct pkcs7_message *msg)
  

+{

• struct pefile_context ctx;
• void *hash;
• int hash_len, ret;
• const void *data;
• size_t data_len;
• size_t asn1hdrlen;
• /* get pkcs7's contentInfo */
• ret = pkcs7_get_content_data(msg, &data, &data_len, &asn1hdrlen);
• if (ret < 0 || !data)

• return false;
  

• /* parse data and retrieve a message digest into ctx */
• ret = mscode_parse(&ctx, data, data_len, asn1hdrlen);
• if (ret < 0)

• return false;
  

• /* calculate a hash value of PE image */
• hash = NULL;
• if (!efi_hash_regions(regs->reg, regs->num, &hash, ctx.digest_algo,

• 	      &hash_len))
  

• return false;
  

• /* match the digest */
• if (ctx.digest_len != hash_len || memcmp(ctx.digest, hash, hash_len))

• return false;
  

• return true;

+}

• /**
  • efi_image_authenticate() - verify a signature of signed image
  • @efi: Pointer to image

@@ -645,6 +691,9 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) }

/*

•  * verify signatures in pkcs7's signedInfos which are
  

•  * to authenticate the integrity of pkcs7's contentInfo.
  

•  *
  

  • NOTE:
  • UEFI specification defines two signature types possible
  • in signature database:

@@ -677,12 +726,21 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) }

/* try white-list */

• if (efi_signature_verify(regs, msg, db, dbx)) {
  

• if (!efi_signature_verify(regs, msg, db, dbx)) {
  

• 	log_debug("Signature was not verified by \"db\"\n");
  

Shouldn't this be log_err()?

I think that I have already explained my idea behind here in my previous reply.

...

• 	continue;
  

• }
  

• /*
  

•  * now calculate an image's hash value and compare it with
  

•  * a messaged digest embedded in pkcs7's contentInfo
  

•  */
  

• if (efi_image_verify_digest(regs, msg)) {
  ret = true;
  continue;
  

  }

• log_debug("Signature was not verified by \"db\"\n");
  

ditto?

Or does the caller write an error message somewhere?

Yes: efi_load_pe() ... /* Authenticate an image */ if (efi_image_authenticate(efi, efi_size)) { handle->auth_status = EFI_IMAGE_AUTH_PASSED; } else { handle->auth_status = EFI_IMAGE_AUTH_FAILED; log_err("Image not authenticated\n"); }

-Takahiro Akashi

Best regards

Heinrich

...

• log_debug("Message digest doesn't match\n");
  

  }