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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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");
> >   	}
> > 
> > 
>