[PATCH 0/5] efi_loader: fix a verification process issue in secure boot

[PATCH 0/5] efi_loader: fix a verification process issue in secure boot

[AKASHI Takahiro]

In the commit 4540dabdcaca ("efi_loader: image_loader: support image
authentication"), U-Boot implementation of UEFI secure boot was
introduced.
It was reported by a Siemens engineer, however, that the verification
process is not fully compliant with MicroSoft's authenticode specification
and it is possible to exploit the code in a signed PE image without deep
knowledge.

This patch series fixes this security issue and, in addition, adds a test
case.

patch#1-3: preparatory patches
patch#4: add a missing step in signature verification process
patch#5: a new test case under pytest

AKASHI Takahiro (5):
  lib: crypto: add mscode_parser
  efi_loader: signature: export efi_hash_regions()
  efi_loader: image_loader: replace EFI_PRINT with log macros
  efi_loader: image_loader: add a missing digest verification for signed
    PE image
  test/py: efi_secboot: add a test for a forged signed image

 include/crypto/mscode.h                       |  43 ++++++
 include/efi_loader.h                          |   2 +
 lib/crypto/Kconfig                            |   9 ++
 lib/crypto/Makefile                           |  12 ++
 lib/crypto/mscode.asn1                        |  28 ++++
 lib/crypto/mscode_parser.c                    | 135 ++++++++++++++++++
 lib/efi_loader/Kconfig                        |   1 +
 lib/efi_loader/efi_image_loader.c             | 114 +++++++++++----
 lib/efi_loader/efi_signature.c                |   4 +-
 test/py/tests/test_efi_secboot/conftest.py    |   3 +
 test/py/tests/test_efi_secboot/forge_image.sh |   5 +
 test/py/tests/test_efi_secboot/test_signed.py |  35 +++++
 12 files changed, 361 insertions(+), 30 deletions(-)
 create mode 100644 include/crypto/mscode.h
 create mode 100644 lib/crypto/mscode.asn1
 create mode 100644 lib/crypto/mscode_parser.c
 create mode 100644 test/py/tests/test_efi_secboot/forge_image.sh

-- 
2.36.1

[PATCH 1/5] lib: crypto: add mscode_parser

[AKASHI Takahiro]

In MS authenticode, pkcs7 should have data in its contentInfo field.
This data is tagged with SpcIndirectData type and, for a signed PE image,
provides a image's message digest as SpcPeImageData.

This parser is used in image authentication to parse the field and
retrieve a message digest.

Imported from linux v5.19-rc, crypto/asymmetric_keys/mscode*.
Checkpatch.pl generates tones of warnings, but those are not fixed
for the sake of maintainability (importing from another source).

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/crypto/mscode.h    |  43 ++++++++++++
 lib/crypto/Kconfig         |   9 +++
 lib/crypto/Makefile        |  12 ++++
 lib/crypto/mscode.asn1     |  28 ++++++++
 lib/crypto/mscode_parser.c | 135 +++++++++++++++++++++++++++++++++++++
 5 files changed, 227 insertions(+)
 create mode 100644 include/crypto/mscode.h
 create mode 100644 lib/crypto/mscode.asn1
 create mode 100644 lib/crypto/mscode_parser.c

diff --git a/include/crypto/mscode.h b/include/crypto/mscode.h
new file mode 100644
index 000000000000..551058b96e60
--- /dev/null
+++ b/include/crypto/mscode.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* PE Binary parser bits
+ *
+ * Copyright (C) 2014 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+
+#include <crypto/pkcs7.h>
+#ifndef __UBOOT__
+#include <crypto/hash_info.h>
+#endif
+
+struct pefile_context {
+#ifndef __UBOOT__
+	unsigned	header_size;
+	unsigned	image_checksum_offset;
+	unsigned	cert_dirent_offset;
+	unsigned	n_data_dirents;
+	unsigned	n_sections;
+	unsigned	certs_size;
+	unsigned	sig_offset;
+	unsigned	sig_len;
+	const struct section_header *secs;
+#endif
+
+	/* PKCS#7 MS Individual Code Signing content */
+	const void	*digest;		/* Digest */
+	unsigned	digest_len;		/* Digest length */
+	const char	*digest_algo;		/* Digest algorithm */
+};
+
+#ifndef __UBOOT__
+#define kenter(FMT, ...)					\
+	pr_devel("==> %s("FMT")\n", __func__, ##__VA_ARGS__)
+#define kleave(FMT, ...) \
+	pr_devel("<== %s()"FMT"\n", __func__, ##__VA_ARGS__)
+#endif
+
+/*
+ * mscode_parser.c
+ */
+extern int mscode_parse(void *_ctx, const void *content_data, size_t data_len,
+			size_t asn1hdrlen);
diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig
index 1c04a7ec5f48..c3f563b2e174 100644
--- a/lib/crypto/Kconfig
+++ b/lib/crypto/Kconfig
@@ -82,4 +82,13 @@ config PKCS7_MESSAGE_PARSER
 config PKCS7_VERIFY
 	bool
 
+config MSCODE_PARSER
+	bool "MS authenticode parser"
+	select ASN1_DECODER
+	select ASN1_COMPILER
+	select OID_REGISTRY
+	help
+	  This option provides support for parsing MicroSoft's Authenticode
+	  in pkcs7 message.
+
 endif # ASYMMETRIC_KEY_TYPE
diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile
index 6792b1d4f007..bec1bc95a658 100644
--- a/lib/crypto/Makefile
+++ b/lib/crypto/Makefile
@@ -55,3 +55,15 @@ obj-$(CONFIG_$(SPL_)PKCS7_VERIFY) += pkcs7_verify.o
 
 $(obj)/pkcs7_parser.o: $(obj)/pkcs7.asn1.h
 $(obj)/pkcs7.asn1.o: $(obj)/pkcs7.asn1.c $(obj)/pkcs7.asn1.h
+
+#
+# Signed PE binary-wrapped key handling
+#
+obj-$(CONFIG_$(SPL_)MSCODE_PARSER) += mscode.o
+
+mscode-y := \
+	mscode_parser.o \
+	mscode.asn1.o
+
+$(obj)/mscode_parser.o: $(obj)/mscode.asn1.h $(obj)/mscode.asn1.h
+$(obj)/mscode.asn1.o: $(obj)/mscode.asn1.c $(obj)/mscode.asn1.h
diff --git a/lib/crypto/mscode.asn1 b/lib/crypto/mscode.asn1
new file mode 100644
index 000000000000..6d09ba48c41c
--- /dev/null
+++ b/lib/crypto/mscode.asn1
@@ -0,0 +1,28 @@
+--- Microsoft individual code signing data blob parser
+---
+--- Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+--- Written by David Howells (dhowells@redhat.com)
+---
+--- This program is free software; you can redistribute it and/or
+--- modify it under the terms of the GNU General Public Licence
+--- as published by the Free Software Foundation; either version
+--- 2 of the Licence, or (at your option) any later version.
+---
+
+MSCode ::= SEQUENCE {
+	type			SEQUENCE {
+		contentType	ContentType,
+		parameters	ANY
+	},
+	content			SEQUENCE {
+		digestAlgorithm	DigestAlgorithmIdentifier,
+		digest		OCTET STRING ({ mscode_note_digest })
+	}
+}
+
+ContentType ::= OBJECT IDENTIFIER ({ mscode_note_content_type })
+
+DigestAlgorithmIdentifier ::= SEQUENCE {
+	algorithm   OBJECT IDENTIFIER ({ mscode_note_digest_algo }),
+	parameters  ANY OPTIONAL
+}
diff --git a/lib/crypto/mscode_parser.c b/lib/crypto/mscode_parser.c
new file mode 100644
index 000000000000..90d5b37a6cf2
--- /dev/null
+++ b/lib/crypto/mscode_parser.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* Parse a Microsoft Individual Code Signing blob
+ *
+ * Copyright (C) 2014 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+
+#define pr_fmt(fmt) "MSCODE: "fmt
+#include <linux/kernel.h>
+#ifndef __UBOOT__
+#include <linux/slab.h>
+#endif
+#include <linux/err.h>
+#include <linux/oid_registry.h>
+#include <crypto/pkcs7.h>
+#ifdef __UBOOT__
+#include <crypto/mscode.h>
+#else
+#include "verify_pefile.h"
+#endif
+#include "mscode.asn1.h"
+
+/*
+ * Parse a Microsoft Individual Code Signing blob
+ */
+int mscode_parse(void *_ctx, const void *content_data, size_t data_len,
+		 size_t asn1hdrlen)
+{
+	struct pefile_context *ctx = _ctx;
+
+	content_data -= asn1hdrlen;
+	data_len += asn1hdrlen;
+	pr_devel("Data: %zu [%*ph]\n", data_len, (unsigned)(data_len),
+		 content_data);
+
+	return asn1_ber_decoder(&mscode_decoder, ctx, content_data, data_len);
+}
+
+/*
+ * Check the content type OID
+ */
+int mscode_note_content_type(void *context, size_t hdrlen,
+			     unsigned char tag,
+			     const void *value, size_t vlen)
+{
+	enum OID oid;
+
+	oid = look_up_OID(value, vlen);
+	if (oid == OID__NR) {
+		char buffer[50];
+
+		sprint_oid(value, vlen, buffer, sizeof(buffer));
+		pr_err("Unknown OID: %s\n", buffer);
+		return -EBADMSG;
+	}
+
+	/*
+	 * pesign utility had a bug where it was putting
+	 * OID_msIndividualSPKeyPurpose instead of OID_msPeImageDataObjId
+	 * So allow both OIDs.
+	 */
+	if (oid != OID_msPeImageDataObjId &&
+	    oid != OID_msIndividualSPKeyPurpose) {
+		pr_err("Unexpected content type OID %u\n", oid);
+		return -EBADMSG;
+	}
+
+	return 0;
+}
+
+/*
+ * Note the digest algorithm OID
+ */
+int mscode_note_digest_algo(void *context, size_t hdrlen,
+			    unsigned char tag,
+			    const void *value, size_t vlen)
+{
+	struct pefile_context *ctx = context;
+	char buffer[50];
+	enum OID oid;
+
+	oid = look_up_OID(value, vlen);
+	switch (oid) {
+	case OID_md4:
+		ctx->digest_algo = "md4";
+		break;
+	case OID_md5:
+		ctx->digest_algo = "md5";
+		break;
+	case OID_sha1:
+		ctx->digest_algo = "sha1";
+		break;
+	case OID_sha256:
+		ctx->digest_algo = "sha256";
+		break;
+	case OID_sha384:
+		ctx->digest_algo = "sha384";
+		break;
+	case OID_sha512:
+		ctx->digest_algo = "sha512";
+		break;
+	case OID_sha224:
+		ctx->digest_algo = "sha224";
+		break;
+
+	case OID__NR:
+		sprint_oid(value, vlen, buffer, sizeof(buffer));
+		pr_err("Unknown OID: %s\n", buffer);
+		return -EBADMSG;
+
+	default:
+		pr_err("Unsupported content type: %u\n", oid);
+		return -ENOPKG;
+	}
+
+	return 0;
+}
+
+/*
+ * Note the digest we're guaranteeing with this certificate
+ */
+int mscode_note_digest(void *context, size_t hdrlen,
+		       unsigned char tag,
+		       const void *value, size_t vlen)
+{
+	struct pefile_context *ctx = context;
+
+	ctx->digest = kmemdup(value, vlen, GFP_KERNEL);
+	if (!ctx->digest)
+		return -ENOMEM;
+
+	ctx->digest_len = vlen;
+
+	return 0;
+}
-- 
2.36.1

[PATCH 2/5] efi_loader: signature: export efi_hash_regions()

[AKASHI Takahiro]

This function is used to calculate a message digest as part of
authentication process in a later patch.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_loader.h           | 2 ++
 lib/efi_loader/efi_signature.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index c1e00ebac398..11930fbea838 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -931,6 +931,8 @@ struct efi_signature_store {
 struct x509_certificate;
 struct pkcs7_message;
 
+bool efi_hash_regions(struct image_region *regs, int count,
+		      void **hash, const char *hash_algo, int *len);
 bool efi_signature_lookup_digest(struct efi_image_regions *regs,
 				 struct efi_signature_store *db,
 				 bool dbx);
diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index ddac751d128e..742d8919402c 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -125,8 +125,8 @@ struct pkcs7_message *efi_parse_pkcs7_header(const void *buf,
  *
  * Return:	true on success, false on error
  */
-static bool efi_hash_regions(struct image_region *regs, int count,
-			     void **hash, const char *hash_algo, int *len)
+bool efi_hash_regions(struct image_region *regs, int count,
+		      void **hash, const char *hash_algo, int *len)
 {
 	int ret, hash_len;
 
-- 
2.36.1

[PATCH 3/5] efi_loader: image_loader: replace EFI_PRINT with log macros

[AKASHI Takahiro]

Now We are migrating from EFI_PRINT() to log macro's.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_image_loader.c | 54 +++++++++++++++----------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index 961139888504..fe8e4a89082c 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -238,7 +238,7 @@ efi_status_t efi_image_region_add(struct efi_image_regions *regs,
 	int i, j;
 
 	if (regs->num >= regs->max) {
-		EFI_PRINT("%s: no more room for regions\n", __func__);
+		log_err("%s: no more room for regions\n", __func__);
 		return EFI_OUT_OF_RESOURCES;
 	}
 
@@ -263,7 +263,7 @@ efi_status_t efi_image_region_add(struct efi_image_regions *regs,
 		}
 
 		/* new data overlapping registered region */
-		EFI_PRINT("%s: new region already part of another\n", __func__);
+		log_err("%s: new region already part of another\n", __func__);
 		return EFI_INVALID_PARAMETER;
 	}
 
@@ -434,8 +434,8 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 		bytes_hashed = opt->SizeOfHeaders;
 		align = opt->FileAlignment;
 	} else {
-		EFI_PRINT("%s: Invalid optional header magic %x\n", __func__,
-			  nt->OptionalHeader.Magic);
+		log_err("%s: Invalid optional header magic %x\n", __func__,
+			nt->OptionalHeader.Magic);
 		goto err;
 	}
 
@@ -445,7 +445,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 			    nt->FileHeader.SizeOfOptionalHeader);
 	sorted = calloc(sizeof(IMAGE_SECTION_HEADER *), num_sections);
 	if (!sorted) {
-		EFI_PRINT("%s: Out of memory\n", __func__);
+		log_err("%s: Out of memory\n", __func__);
 		goto err;
 	}
 
@@ -464,7 +464,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 		efi_image_region_add(regs, efi + sorted[i]->PointerToRawData,
 				     efi + sorted[i]->PointerToRawData + size,
 				     0);
-		EFI_PRINT("section[%d](%s): raw: 0x%x-0x%x, virt: %x-%x\n",
+		log_debug("section[%d](%s): raw: 0x%x-0x%x, virt: %x-%x\n",
 			  i, sorted[i]->Name,
 			  sorted[i]->PointerToRawData,
 			  sorted[i]->PointerToRawData + size,
@@ -478,7 +478,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 
 	/* 3. Extra data excluding Certificates Table */
 	if (bytes_hashed + authsz < len) {
-		EFI_PRINT("extra data for hash: %zu\n",
+		log_debug("extra data for hash: %zu\n",
 			  len - (bytes_hashed + authsz));
 		efi_image_region_add(regs, efi + bytes_hashed,
 				     efi + len - authsz, 0);
@@ -487,18 +487,18 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 	/* Return Certificates Table */
 	if (authsz) {
 		if (len < authoff + authsz) {
-			EFI_PRINT("%s: Size for auth too large: %u >= %zu\n",
-				  __func__, authsz, len - authoff);
+			log_err("%s: Size for auth too large: %u >= %zu\n",
+				__func__, authsz, len - authoff);
 			goto err;
 		}
 		if (authsz < sizeof(*auth)) {
-			EFI_PRINT("%s: Size for auth too small: %u < %zu\n",
-				  __func__, authsz, sizeof(*auth));
+			log_err("%s: Size for auth too small: %u < %zu\n",
+				__func__, authsz, sizeof(*auth));
 			goto err;
 		}
 		*auth = efi + authoff;
 		*auth_len = authsz;
-		EFI_PRINT("WIN_CERTIFICATE: 0x%x, size: 0x%x\n", authoff,
+		log_debug("WIN_CERTIFICATE: 0x%x, size: 0x%x\n", authoff,
 			  authsz);
 	} else {
 		*auth = NULL;
@@ -549,7 +549,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 	size_t auth_size;
 	bool ret = false;
 
-	EFI_PRINT("%s: Enter, %d\n", __func__, ret);
+	log_debug("%s: Enter, %d\n", __func__, ret);
 
 	if (!efi_secure_boot_enabled())
 		return true;
@@ -560,7 +560,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");
+		log_err("Parsing PE executable image failed\n");
 		goto out;
 	}
 
@@ -569,18 +569,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 	 */
 	db = efi_sigstore_parse_sigdb(u"db");
 	if (!db) {
-		EFI_PRINT("Getting signature database(db) failed\n");
+		log_err("Getting signature database(db) failed\n");
 		goto out;
 	}
 
 	dbx = efi_sigstore_parse_sigdb(u"dbx");
 	if (!dbx) {
-		EFI_PRINT("Getting signature database(dbx) failed\n");
+		log_err("Getting signature database(dbx) failed\n");
 		goto out;
 	}
 
 	if (efi_signature_lookup_digest(regs, dbx, true)) {
-		EFI_PRINT("Image's digest was found in \"dbx\"\n");
+		log_debug("Image's digest was found in \"dbx\"\n");
 		goto out;
 	}
 
@@ -602,12 +602,12 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 			break;
 
 		if (wincert->dwLength <= sizeof(*wincert)) {
-			EFI_PRINT("dwLength too small: %u < %zu\n",
+			log_debug("dwLength too small: %u < %zu\n",
 				  wincert->dwLength, sizeof(*wincert));
 			continue;
 		}
 
-		EFI_PRINT("WIN_CERTIFICATE_TYPE: 0x%x\n",
+		log_debug("WIN_CERTIFICATE_TYPE: 0x%x\n",
 			  wincert->wCertificateType);
 
 		auth = (u8 *)wincert + sizeof(*wincert);
@@ -617,12 +617,12 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 				break;
 
 			if (auth_size <= sizeof(efi_guid_t)) {
-				EFI_PRINT("dwLength too small: %u < %zu\n",
+				log_debug("dwLength too small: %u < %zu\n",
 					  wincert->dwLength, sizeof(*wincert));
 				continue;
 			}
 			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
-				EFI_PRINT("Certificate type not supported: %pUs\n",
+				log_debug("Certificate type not supported: %pUs\n",
 					  auth);
 				ret = false;
 				goto out;
@@ -632,14 +632,14 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 			auth_size -= sizeof(efi_guid_t);
 		} else if (wincert->wCertificateType
 				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
-			EFI_PRINT("Certificate type not supported\n");
+			log_debug("Certificate type not supported\n");
 			ret = false;
 			goto out;
 		}
 
 		msg = pkcs7_parse_message(auth, auth_size);
 		if (IS_ERR(msg)) {
-			EFI_PRINT("Parsing image's signature failed\n");
+			log_err("Parsing image's signature failed\n");
 			msg = NULL;
 			continue;
 		}
@@ -666,13 +666,13 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 		/* try black-list first */
 		if (efi_signature_verify_one(regs, msg, dbx)) {
 			ret = false;
-			EFI_PRINT("Signature was rejected by \"dbx\"\n");
+			log_debug("Signature was rejected by \"dbx\"\n");
 			goto out;
 		}
 
 		if (!efi_signature_check_signers(msg, dbx)) {
 			ret = false;
-			EFI_PRINT("Signer(s) in \"dbx\"\n");
+			log_debug("Signer(s) in \"dbx\"\n");
 			goto out;
 		}
 
@@ -682,7 +682,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 			continue;
 		}
 
-		EFI_PRINT("Signature was not verified by \"db\"\n");
+		log_debug("Signature was not verified by \"db\"\n");
 	}
 
 
@@ -698,7 +698,7 @@ out:
 	if (new_efi != efi)
 		free(new_efi);
 
-	EFI_PRINT("%s: Exit, %d\n", __func__, ret);
+	log_debug("%s: Exit, %d\n", __func__, ret);
 	return ret;
 }
 #else
-- 
2.36.1

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

[AKASHI Takahiro]

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");
+			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");
+		log_debug("Message digest doesn't match\n");
 	}
 
 
-- 
2.36.1

[PATCH 5/5] test/py: efi_secboot: add a test for a forged signed image

[AKASHI Takahiro]

In this test case, a image binary, helloworld.efi.signed, is willfully
modified to print a corrupted message while the signature itself is
unchanged.

This binary must be rejected under secure boot mode.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 test/py/tests/test_efi_secboot/conftest.py    |  3 ++
 test/py/tests/test_efi_secboot/forge_image.sh |  5 +++
 test/py/tests/test_efi_secboot/test_signed.py | 35 +++++++++++++++++++
 3 files changed, 43 insertions(+)
 create mode 100644 test/py/tests/test_efi_secboot/forge_image.sh

diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
index 8a53dabe5414..db6b8d301f85 100644
--- a/test/py/tests/test_efi_secboot/conftest.py
+++ b/test/py/tests/test_efi_secboot/conftest.py
@@ -105,6 +105,9 @@ def efi_boot_env(request, u_boot_config):
         # Sign already-signed image with another key
         check_call('cd %s; sbsign --key db1.key --cert db1.crt --output helloworld.efi.signed_2sigs helloworld.efi.signed'
                    % mnt_point, shell=True)
+        # Create a corrupted signed image
+        check_call('cd %s; sh %s/test/py/tests/test_efi_secboot/forge_image.sh helloworld.efi.signed helloworld_forged.efi.signed'
+                   % (mnt_point, u_boot_config.source_dir), shell=True)
         # Digest image
         check_call('cd %s; %shash-to-efi-sig-list helloworld.efi db_hello.hash; %ssign-efi-sig-list -t "2020-04-07" -c KEK.crt -k KEK.key db db_hello.hash db_hello.auth'
                    % (mnt_point, EFITOOLS_PATH, EFITOOLS_PATH),
diff --git a/test/py/tests/test_efi_secboot/forge_image.sh b/test/py/tests/test_efi_secboot/forge_image.sh
new file mode 100644
index 000000000000..2465d10fa7b8
--- /dev/null
+++ b/test/py/tests/test_efi_secboot/forge_image.sh
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+#!/bin/sh
+
+replace_exp="s/H\0e\0l\0l\0o\0/h\0E\0L\0L\0O\0/g"
+perl -p -e ${replace_exp} < $1 > $2
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
index 30b3fa4e701e..ca52e853d8f8 100644
--- a/test/py/tests/test_efi_secboot/test_signed.py
+++ b/test/py/tests/test_efi_secboot/test_signed.py
@@ -334,3 +334,38 @@ class TestEfiSignedImage(object):
                 'efidebug test bootmgr'])
             assert '\'HELLO\' failed' in ''.join(output)
             assert 'efi_start_image() returned: 26' in ''.join(output)
+
+    def test_efi_signed_image_auth8(self, u_boot_console, efi_boot_env):
+        """
+        Test Case 8 - Secure boot is in force,
+                      Same as Test Case 2 but the image binary to be loaded
+                      was willfully modified (forged)
+                      Must be rejected.
+        """
+        u_boot_console.restart_uboot()
+        disk_img = efi_boot_env
+        with u_boot_console.log.section('Test Case 8a'):
+            # Test Case 8a, Secure boot is not yet forced
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % disk_img,
+                'efidebug boot add -b 1 HELLO1 host 0:1 /helloworld_forged.efi.signed -s ""',
+                'efidebug boot next 1',
+                'efidebug test bootmgr'])
+            assert('hELLO, world!' in ''.join(output))
+
+        with u_boot_console.log.section('Test Case 8b'):
+            # Test Case 8b, Install signature database and verify the image
+            output = u_boot_console.run_command_list([
+                'fatload host 0:1 4000000 db.auth',
+                'setenv -e -nv -bs -rt -at -i 4000000:$filesize db',
+                'fatload host 0:1 4000000 KEK.auth',
+                'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK',
+                'fatload host 0:1 4000000 PK.auth',
+                'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK'])
+            assert 'Failed to set EFI variable' not in ''.join(output)
+            output = u_boot_console.run_command_list([
+                'efidebug boot next 1',
+                'efidebug test bootmgr'])
+            assert(not 'hELLO, world!' in ''.join(output))
+            assert('\'HELLO1\' failed' in ''.join(output))
+            assert('efi_start_image() returned: 26' in ''.join(output))
-- 
2.36.1

Re: [PATCH 1/5] lib: crypto: add mscode_parser

[Jason A. Donenfeld]

On Tue, Jul 05, 2022 at 02:48:11PM +0900, AKASHI Takahiro wrote:
> +	  This option provides support for parsing MicroSoft's Authenticode
> +	  in pkcs7 message.

I chuckled when I saw "MicroSoft" in the cover letter, thinking it was a
wink, but here too... haha ummm. We could change it to "MikeRoweSoft"
instead in honor of the Belmont High School student. But... I think
"Microsoft" is what you're after here.

> +	pr_devel("Data: %zu [%*ph]\n", data_len, (unsigned)(data_len),
> +		 content_data);

That's a weird cast around (data_len), but are you sure you want to keep
that print line in there?
 
Jason

Re: [PATCH 3/5] efi_loader: image_loader: replace EFI_PRINT with log macros

[Heinrich Schuchardt]

On 7/5/22 07:48, AKASHI Takahiro wrote:
> Now We are migrating from EFI_PRINT() to log macro's.

I don't understand your logic:

log_err("Parsing image's signature failed\n");
log_debug("Signature was rejected by \"dbx\"\n");

Shouldn't everything leading to a signature being rejected be treated
the same (i.e. use log_err())?

Best regards

Heinrich

>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   lib/efi_loader/efi_image_loader.c | 54 +++++++++++++++----------------
>   1 file changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index 961139888504..fe8e4a89082c 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -238,7 +238,7 @@ efi_status_t efi_image_region_add(struct efi_image_regions *regs,
>   	int i, j;
>
>   	if (regs->num >= regs->max) {
> -		EFI_PRINT("%s: no more room for regions\n", __func__);
> +		log_err("%s: no more room for regions\n", __func__);
>   		return EFI_OUT_OF_RESOURCES;
>   	}
>
> @@ -263,7 +263,7 @@ efi_status_t efi_image_region_add(struct efi_image_regions *regs,
>   		}
>
>   		/* new data overlapping registered region */
> -		EFI_PRINT("%s: new region already part of another\n", __func__);
> +		log_err("%s: new region already part of another\n", __func__);
>   		return EFI_INVALID_PARAMETER;
>   	}
>
> @@ -434,8 +434,8 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>   		bytes_hashed = opt->SizeOfHeaders;
>   		align = opt->FileAlignment;
>   	} else {
> -		EFI_PRINT("%s: Invalid optional header magic %x\n", __func__,
> -			  nt->OptionalHeader.Magic);
> +		log_err("%s: Invalid optional header magic %x\n", __func__,
> +			nt->OptionalHeader.Magic);
>   		goto err;
>   	}
>
> @@ -445,7 +445,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>   			    nt->FileHeader.SizeOfOptionalHeader);
>   	sorted = calloc(sizeof(IMAGE_SECTION_HEADER *), num_sections);
>   	if (!sorted) {
> -		EFI_PRINT("%s: Out of memory\n", __func__);
> +		log_err("%s: Out of memory\n", __func__);
>   		goto err;
>   	}
>
> @@ -464,7 +464,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>   		efi_image_region_add(regs, efi + sorted[i]->PointerToRawData,
>   				     efi + sorted[i]->PointerToRawData + size,
>   				     0);
> -		EFI_PRINT("section[%d](%s): raw: 0x%x-0x%x, virt: %x-%x\n",
> +		log_debug("section[%d](%s): raw: 0x%x-0x%x, virt: %x-%x\n",
>   			  i, sorted[i]->Name,
>   			  sorted[i]->PointerToRawData,
>   			  sorted[i]->PointerToRawData + size,
> @@ -478,7 +478,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>
>   	/* 3. Extra data excluding Certificates Table */
>   	if (bytes_hashed + authsz < len) {
> -		EFI_PRINT("extra data for hash: %zu\n",
> +		log_debug("extra data for hash: %zu\n",
>   			  len - (bytes_hashed + authsz));
>   		efi_image_region_add(regs, efi + bytes_hashed,
>   				     efi + len - authsz, 0);
> @@ -487,18 +487,18 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>   	/* Return Certificates Table */
>   	if (authsz) {
>   		if (len < authoff + authsz) {
> -			EFI_PRINT("%s: Size for auth too large: %u >= %zu\n",
> -				  __func__, authsz, len - authoff);
> +			log_err("%s: Size for auth too large: %u >= %zu\n",
> +				__func__, authsz, len - authoff);
>   			goto err;
>   		}
>   		if (authsz < sizeof(*auth)) {
> -			EFI_PRINT("%s: Size for auth too small: %u < %zu\n",
> -				  __func__, authsz, sizeof(*auth));
> +			log_err("%s: Size for auth too small: %u < %zu\n",
> +				__func__, authsz, sizeof(*auth));
>   			goto err;
>   		}
>   		*auth = efi + authoff;
>   		*auth_len = authsz;
> -		EFI_PRINT("WIN_CERTIFICATE: 0x%x, size: 0x%x\n", authoff,
> +		log_debug("WIN_CERTIFICATE: 0x%x, size: 0x%x\n", authoff,
>   			  authsz);
>   	} else {
>   		*auth = NULL;
> @@ -549,7 +549,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>   	size_t auth_size;
>   	bool ret = false;
>
> -	EFI_PRINT("%s: Enter, %d\n", __func__, ret);
> +	log_debug("%s: Enter, %d\n", __func__, ret);
>
>   	if (!efi_secure_boot_enabled())
>   		return true;
> @@ -560,7 +560,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");
> +		log_err("Parsing PE executable image failed\n");
>   		goto out;
>   	}
>
> @@ -569,18 +569,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>   	 */
>   	db = efi_sigstore_parse_sigdb(u"db");
>   	if (!db) {
> -		EFI_PRINT("Getting signature database(db) failed\n");
> +		log_err("Getting signature database(db) failed\n");
>   		goto out;
>   	}
>
>   	dbx = efi_sigstore_parse_sigdb(u"dbx");
>   	if (!dbx) {
> -		EFI_PRINT("Getting signature database(dbx) failed\n");
> +		log_err("Getting signature database(dbx) failed\n");
>   		goto out;
>   	}
>
>   	if (efi_signature_lookup_digest(regs, dbx, true)) {
> -		EFI_PRINT("Image's digest was found in \"dbx\"\n");
> +		log_debug("Image's digest was found in \"dbx\"\n");
>   		goto out;
>   	}
>
> @@ -602,12 +602,12 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>   			break;
>
>   		if (wincert->dwLength <= sizeof(*wincert)) {
> -			EFI_PRINT("dwLength too small: %u < %zu\n",
> +			log_debug("dwLength too small: %u < %zu\n",
>   				  wincert->dwLength, sizeof(*wincert));
>   			continue;
>   		}
>
> -		EFI_PRINT("WIN_CERTIFICATE_TYPE: 0x%x\n",
> +		log_debug("WIN_CERTIFICATE_TYPE: 0x%x\n",
>   			  wincert->wCertificateType);
>
>   		auth = (u8 *)wincert + sizeof(*wincert);
> @@ -617,12 +617,12 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>   				break;
>
>   			if (auth_size <= sizeof(efi_guid_t)) {
> -				EFI_PRINT("dwLength too small: %u < %zu\n",
> +				log_debug("dwLength too small: %u < %zu\n",
>   					  wincert->dwLength, sizeof(*wincert));
>   				continue;
>   			}
>   			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
> -				EFI_PRINT("Certificate type not supported: %pUs\n",
> +				log_debug("Certificate type not supported: %pUs\n",
>   					  auth);
>   				ret = false;
>   				goto out;
> @@ -632,14 +632,14 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>   			auth_size -= sizeof(efi_guid_t);
>   		} else if (wincert->wCertificateType
>   				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> -			EFI_PRINT("Certificate type not supported\n");
> +			log_debug("Certificate type not supported\n");
>   			ret = false;
>   			goto out;
>   		}
>
>   		msg = pkcs7_parse_message(auth, auth_size);
>   		if (IS_ERR(msg)) {
> -			EFI_PRINT("Parsing image's signature failed\n");
> +			log_err("Parsing image's signature failed\n");
>   			msg = NULL;
>   			continue;
>   		}
> @@ -666,13 +666,13 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>   		/* try black-list first */
>   		if (efi_signature_verify_one(regs, msg, dbx)) {
>   			ret = false;
> -			EFI_PRINT("Signature was rejected by \"dbx\"\n");
> +			log_debug("Signature was rejected by \"dbx\"\n");
>   			goto out;
>   		}
>
>   		if (!efi_signature_check_signers(msg, dbx)) {
>   			ret = false;
> -			EFI_PRINT("Signer(s) in \"dbx\"\n");
> +			log_debug("Signer(s) in \"dbx\"\n");
>   			goto out;
>   		}
>
> @@ -682,7 +682,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>   			continue;
>   		}
>
> -		EFI_PRINT("Signature was not verified by \"db\"\n");
> +		log_debug("Signature was not verified by \"db\"\n");
>   	}
>
>
> @@ -698,7 +698,7 @@ out:
>   	if (new_efi != efi)
>   		free(new_efi);
>
> -	EFI_PRINT("%s: Exit, %d\n", __func__, ret);
> +	log_debug("%s: Exit, %d\n", __func__, ret);
>   	return ret;
>   }
>   #else

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

[Heinrich Schuchardt]

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()?

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

Best regards

Heinrich

> +		log_debug("Message digest doesn't match\n");
>   	}
>
>

Re: [PATCH 1/5] lib: crypto: add mscode_parser

[AKASHI Takahiro]

Hi,

On Tue, Jul 05, 2022 at 03:13:17PM +0200, Jason A. Donenfeld wrote:
> On Tue, Jul 05, 2022 at 02:48:11PM +0900, AKASHI Takahiro wrote:
> > +	  This option provides support for parsing MicroSoft's Authenticode
> > +	  in pkcs7 message.
> 
> I chuckled when I saw "MicroSoft" in the cover letter, thinking it was a
> wink, but here too... haha ummm. We could change it to "MikeRoweSoft"
> instead in honor of the Belmont High School student. But... I think

I have never heard of his name, but

> "Microsoft" is what you're after here.

If so, yes.

> > +	pr_devel("Data: %zu [%*ph]\n", data_len, (unsigned)(data_len),
> > +		 content_data);
> 
> That's a weird cast around (data_len), but are you sure you want to keep
> that print line in there?

My basic policy in importing a file form Linux, as far as lib/crypto/*.c is
concerned, is not to modify the original code unless it's harmful but to add
"#ifndef __UBOOT__" to exclude useless or never-used code so that someone else
other than me can easily synchronize files again in the future.

So even if it looks weird (and checkpatch.pl, which ironically comes from
Linux as well, might raise warnings), I'd like to leave it as it is.

Having said that, if maintainers have a different policy, I don't hesitate
to follow it.

Thanks,
-Takahiro Akashi

> Jason

Re: [PATCH 3/5] efi_loader: image_loader: replace EFI_PRINT with log macros

[AKASHI Takahiro]

Heinrich,

On Tue, Jul 05, 2022 at 05:26:58PM +0200, Heinrich Schuchardt wrote:
> On 7/5/22 07:48, AKASHI Takahiro wrote:
> > Now We are migrating from EFI_PRINT() to log macro's.
> 
> I don't understand your logic:
> 
> log_err("Parsing image's signature failed\n");
> log_debug("Signature was rejected by \"dbx\"\n");

I distinctively use those two macro's.
I use log_err() when such an error is *normally* NOT expected.
I think that users should always be notified of these cases.
(Say, a signing tool generates a incompatible format of image, or
a image itself is corrupted.)

On the other hand, I use log_debug() to show the detailed reason
of why the signature verification process failed.
I always enable those messages when I debug image authentication code.

Please note there is always log_err("Image not authenticated\n") if
efi_image_authenticate() fails.

-Takahiro Akashi

> Shouldn't everything leading to a signature being rejected be treated
> the same (i.e. use log_err())?
> 
> Best regards
> 
> Heinrich
> 
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >   lib/efi_loader/efi_image_loader.c | 54 +++++++++++++++----------------
> >   1 file changed, 27 insertions(+), 27 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> > index 961139888504..fe8e4a89082c 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -238,7 +238,7 @@ efi_status_t efi_image_region_add(struct efi_image_regions *regs,
> >   	int i, j;
> > 
> >   	if (regs->num >= regs->max) {
> > -		EFI_PRINT("%s: no more room for regions\n", __func__);
> > +		log_err("%s: no more room for regions\n", __func__);
> >   		return EFI_OUT_OF_RESOURCES;
> >   	}
> > 
> > @@ -263,7 +263,7 @@ efi_status_t efi_image_region_add(struct efi_image_regions *regs,
> >   		}
> > 
> >   		/* new data overlapping registered region */
> > -		EFI_PRINT("%s: new region already part of another\n", __func__);
> > +		log_err("%s: new region already part of another\n", __func__);
> >   		return EFI_INVALID_PARAMETER;
> >   	}
> > 
> > @@ -434,8 +434,8 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> >   		bytes_hashed = opt->SizeOfHeaders;
> >   		align = opt->FileAlignment;
> >   	} else {
> > -		EFI_PRINT("%s: Invalid optional header magic %x\n", __func__,
> > -			  nt->OptionalHeader.Magic);
> > +		log_err("%s: Invalid optional header magic %x\n", __func__,
> > +			nt->OptionalHeader.Magic);
> >   		goto err;
> >   	}
> > 
> > @@ -445,7 +445,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> >   			    nt->FileHeader.SizeOfOptionalHeader);
> >   	sorted = calloc(sizeof(IMAGE_SECTION_HEADER *), num_sections);
> >   	if (!sorted) {
> > -		EFI_PRINT("%s: Out of memory\n", __func__);
> > +		log_err("%s: Out of memory\n", __func__);
> >   		goto err;
> >   	}
> > 
> > @@ -464,7 +464,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> >   		efi_image_region_add(regs, efi + sorted[i]->PointerToRawData,
> >   				     efi + sorted[i]->PointerToRawData + size,
> >   				     0);
> > -		EFI_PRINT("section[%d](%s): raw: 0x%x-0x%x, virt: %x-%x\n",
> > +		log_debug("section[%d](%s): raw: 0x%x-0x%x, virt: %x-%x\n",
> >   			  i, sorted[i]->Name,
> >   			  sorted[i]->PointerToRawData,
> >   			  sorted[i]->PointerToRawData + size,
> > @@ -478,7 +478,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> > 
> >   	/* 3. Extra data excluding Certificates Table */
> >   	if (bytes_hashed + authsz < len) {
> > -		EFI_PRINT("extra data for hash: %zu\n",
> > +		log_debug("extra data for hash: %zu\n",
> >   			  len - (bytes_hashed + authsz));
> >   		efi_image_region_add(regs, efi + bytes_hashed,
> >   				     efi + len - authsz, 0);
> > @@ -487,18 +487,18 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> >   	/* Return Certificates Table */
> >   	if (authsz) {
> >   		if (len < authoff + authsz) {
> > -			EFI_PRINT("%s: Size for auth too large: %u >= %zu\n",
> > -				  __func__, authsz, len - authoff);
> > +			log_err("%s: Size for auth too large: %u >= %zu\n",
> > +				__func__, authsz, len - authoff);
> >   			goto err;
> >   		}
> >   		if (authsz < sizeof(*auth)) {
> > -			EFI_PRINT("%s: Size for auth too small: %u < %zu\n",
> > -				  __func__, authsz, sizeof(*auth));
> > +			log_err("%s: Size for auth too small: %u < %zu\n",
> > +				__func__, authsz, sizeof(*auth));
> >   			goto err;
> >   		}
> >   		*auth = efi + authoff;
> >   		*auth_len = authsz;
> > -		EFI_PRINT("WIN_CERTIFICATE: 0x%x, size: 0x%x\n", authoff,
> > +		log_debug("WIN_CERTIFICATE: 0x%x, size: 0x%x\n", authoff,
> >   			  authsz);
> >   	} else {
> >   		*auth = NULL;
> > @@ -549,7 +549,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >   	size_t auth_size;
> >   	bool ret = false;
> > 
> > -	EFI_PRINT("%s: Enter, %d\n", __func__, ret);
> > +	log_debug("%s: Enter, %d\n", __func__, ret);
> > 
> >   	if (!efi_secure_boot_enabled())
> >   		return true;
> > @@ -560,7 +560,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");
> > +		log_err("Parsing PE executable image failed\n");
> >   		goto out;
> >   	}
> > 
> > @@ -569,18 +569,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >   	 */
> >   	db = efi_sigstore_parse_sigdb(u"db");
> >   	if (!db) {
> > -		EFI_PRINT("Getting signature database(db) failed\n");
> > +		log_err("Getting signature database(db) failed\n");
> >   		goto out;
> >   	}
> > 
> >   	dbx = efi_sigstore_parse_sigdb(u"dbx");
> >   	if (!dbx) {
> > -		EFI_PRINT("Getting signature database(dbx) failed\n");
> > +		log_err("Getting signature database(dbx) failed\n");
> >   		goto out;
> >   	}
> > 
> >   	if (efi_signature_lookup_digest(regs, dbx, true)) {
> > -		EFI_PRINT("Image's digest was found in \"dbx\"\n");
> > +		log_debug("Image's digest was found in \"dbx\"\n");
> >   		goto out;
> >   	}
> > 
> > @@ -602,12 +602,12 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >   			break;
> > 
> >   		if (wincert->dwLength <= sizeof(*wincert)) {
> > -			EFI_PRINT("dwLength too small: %u < %zu\n",
> > +			log_debug("dwLength too small: %u < %zu\n",
> >   				  wincert->dwLength, sizeof(*wincert));
> >   			continue;
> >   		}
> > 
> > -		EFI_PRINT("WIN_CERTIFICATE_TYPE: 0x%x\n",
> > +		log_debug("WIN_CERTIFICATE_TYPE: 0x%x\n",
> >   			  wincert->wCertificateType);
> > 
> >   		auth = (u8 *)wincert + sizeof(*wincert);
> > @@ -617,12 +617,12 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >   				break;
> > 
> >   			if (auth_size <= sizeof(efi_guid_t)) {
> > -				EFI_PRINT("dwLength too small: %u < %zu\n",
> > +				log_debug("dwLength too small: %u < %zu\n",
> >   					  wincert->dwLength, sizeof(*wincert));
> >   				continue;
> >   			}
> >   			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
> > -				EFI_PRINT("Certificate type not supported: %pUs\n",
> > +				log_debug("Certificate type not supported: %pUs\n",
> >   					  auth);
> >   				ret = false;
> >   				goto out;
> > @@ -632,14 +632,14 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >   			auth_size -= sizeof(efi_guid_t);
> >   		} else if (wincert->wCertificateType
> >   				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> > -			EFI_PRINT("Certificate type not supported\n");
> > +			log_debug("Certificate type not supported\n");
> >   			ret = false;
> >   			goto out;
> >   		}
> > 
> >   		msg = pkcs7_parse_message(auth, auth_size);
> >   		if (IS_ERR(msg)) {
> > -			EFI_PRINT("Parsing image's signature failed\n");
> > +			log_err("Parsing image's signature failed\n");
> >   			msg = NULL;
> >   			continue;
> >   		}
> > @@ -666,13 +666,13 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >   		/* try black-list first */
> >   		if (efi_signature_verify_one(regs, msg, dbx)) {
> >   			ret = false;
> > -			EFI_PRINT("Signature was rejected by \"dbx\"\n");
> > +			log_debug("Signature was rejected by \"dbx\"\n");
> >   			goto out;
> >   		}
> > 
> >   		if (!efi_signature_check_signers(msg, dbx)) {
> >   			ret = false;
> > -			EFI_PRINT("Signer(s) in \"dbx\"\n");
> > +			log_debug("Signer(s) in \"dbx\"\n");
> >   			goto out;
> >   		}
> > 
> > @@ -682,7 +682,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >   			continue;
> >   		}
> > 
> > -		EFI_PRINT("Signature was not verified by \"db\"\n");
> > +		log_debug("Signature was not verified by \"db\"\n");
> >   	}
> > 
> > 
> > @@ -698,7 +698,7 @@ out:
> >   	if (new_efi != efi)
> >   		free(new_efi);
> > 
> > -	EFI_PRINT("%s: Exit, %d\n", __func__, ret);
> > +	log_debug("%s: Exit, %d\n", __func__, ret);
> >   	return ret;
> >   }
> >   #else
> 

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

[AKASHI Takahiro]

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