[PATCH 00/13] efi_loader: rework/improve UEFI secure boot code

[PATCH 00/13] efi_loader: rework/improve UEFI secure boot code

[AKASHI Takahiro]

Summary
=======
I'm currently working on reworking UEFI secure boot, aiming to add
"intermediate certificates" support. In this effort, I found a couple
of issues that should immediately be fixed or useful improvements even
without intermediate certificates support.

Each commit in this patch series has self-explained description of
the issue to be addressed.
While they are independent in terms of functionality, they are compiled
in a set since the one may depend on the other in terms of code change
overlap. All the changes can and should be merged at once for best
convenience.

I hope that I will post intermediate certificates support sometime
in the next week.

Patch structure
===============
Patch#1,#5: rather preliminary patches
Patch#2-#4,#6,#7: main commits
Patch#8-#13: pytests
  Patch#11,#12 for Patch#6
  Patch#13 for Patch#7

Test
====
- Travis CI is still running with the latest
  (it passed with the previous one, including new pytests added here)

AKASHI Takahiro (13):
  efi_loader: signature: move efi_guid_cert_type_pkcs7 to
    efi_signature.c
  efi_loader: image_loader: add a check against certificate type of
    authenticode
  efi_loader: image_loader: retrieve authenticode only if it exists
  efi_loader: signature: fix a size check against revocation list
  efi_loader: signature: make efi_hash_regions more generic
  efi_loader: image_loader: verification for all signatures should pass
  efi_loader: image_loader: add digest-based verification for signed
    image
  test/py: efi_secboot: remove all "re.search"
  test/py: efi_secboot: fix test case 1g of test_authvar
  test/py: efi_secboot: split "signed image" test case-1 into two cases
  test/py: efi_secboot: add a test against certificate revocation
  test/py: efi_secboot: add a test for multiple signatures
  test/py: efi_secboot: add a test for verifying with digest of signed
    image

 include/efi_loader.h                          |  12 +-
 lib/efi_loader/efi_image_loader.c             | 142 ++++--
 lib/efi_loader/efi_signature.c                | 426 ++++++++----------
 lib/efi_loader/efi_variable.c                 |   1 -
 test/py/tests/test_efi_secboot/conftest.py    |  20 +-
 .../py/tests/test_efi_secboot/test_authvar.py |  83 ++--
 test/py/tests/test_efi_secboot/test_signed.py | 236 +++++++---
 .../tests/test_efi_secboot/test_unsigned.py   |  32 +-
 8 files changed, 563 insertions(+), 389 deletions(-)

-- 
2.25.2

[PATCH 01/13] efi_loader: signature: move efi_guid_cert_type_pkcs7 to efi_signature.c

[AKASHI Takahiro]

The global variable, efi_guid_cert_type_pkcs7, will also be used in
efi_image_loader.c in a succeeding patch so as to correctly handle
a signature type of authenticode in signed image.

Meanwhile, it is currently defined in efi_variable.c. Once some secure
storage solution for UEFI variables is introduced, efi_variable.c may
not always be compiled in.

So move the definition to efi_signature.c as a common place.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_signature.c | 1 +
 lib/efi_loader/efi_variable.c  | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index e386d65e170c..be6491c6e255 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -23,6 +23,7 @@ const efi_guid_t efi_guid_sha256 = EFI_CERT_SHA256_GUID;
 const efi_guid_t efi_guid_cert_rsa2048 = EFI_CERT_RSA2048_GUID;
 const efi_guid_t efi_guid_cert_x509 = EFI_CERT_X509_GUID;
 const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID;
+const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
 
 #ifdef CONFIG_EFI_SECURE_BOOT
 
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 0a43db56788a..e097670e2832 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -26,7 +26,6 @@ enum efi_secure_mode {
 	EFI_MODE_DEPLOYED,
 };
 
-const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
 static bool efi_secure_boot;
 static int efi_secure_mode;
 static u8 efi_vendor_keys;
-- 
2.25.2

[PATCH 01/13] efi_loader: signature: move efi_guid_cert_type_pkcs7 to efi_signature.c

[Heinrich Schuchardt]

On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> The global variable, efi_guid_cert_type_pkcs7, will also be used in
> efi_image_loader.c in a succeeding patch so as to correctly handle
> a signature type of authenticode in signed image.
>
> Meanwhile, it is currently defined in efi_variable.c. Once some secure
> storage solution for UEFI variables is introduced, efi_variable.c may
> not always be compiled in.
>
> So move the definition to efi_signature.c as a common place.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[PATCH 02/13] efi_loader: image_loader: add a check against certificate type of authenticode

[AKASHI Takahiro]

UEFI specification requires that we shall support three type of
certificates of authenticode in PE image:
  WIN_CERT_TYPE_EFI_GUID with the guid, EFI_CERT_TYPE_PCKS7_GUID
  WIN_CERT_TYPE_PKCS_SIGNED_DATA
  WIN_CERT_TYPE_EFI_PKCS1_15

As EDK2 does, we will support the first two that are pkcs7 SignedData.

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

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index 7f35b1e58fe5..a13ba3692ec2 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -375,7 +375,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 	}
 
 	/* Return Certificates Table */
-	if (authsz) {
+	if (authsz > 0) {
 		if (len < authoff + authsz) {
 			debug("%s: Size for auth too large: %u >= %zu\n",
 			      __func__, authsz, len - authoff);
@@ -480,8 +480,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 	struct pkcs7_message *msg = NULL;
 	struct efi_signature_store *db = NULL, *dbx = NULL;
 	struct x509_certificate *cert = NULL;
-	void *new_efi = NULL;
-	size_t new_efi_size;
+	void *new_efi = NULL, *auth, *wincerts_end;
+	size_t new_efi_size, auth_size;
 	bool ret = false;
 
 	if (!efi_secure_boot_enabled())
@@ -530,20 +530,51 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 	}
 
 	/* go through WIN_CERTIFICATE list */
-	for (wincert = wincerts;
-	     (void *)wincert < (void *)wincerts + wincerts_len;
+	for (wincert = wincerts, wincerts_end = (void *)wincerts + wincerts_len;
+	     (void *)wincert < wincerts_end;
 	     wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
-		if (wincert->dwLength < sizeof(*wincert)) {
-			debug("%s: dwLength too small: %u < %zu\n",
-			      __func__, wincert->dwLength, sizeof(*wincert));
-			goto err;
+		if ((void *)wincert + sizeof(*wincert) >= wincerts_end)
+			break;
+
+		if (wincert->dwLength <= sizeof(*wincert)) {
+			debug("dwLength too small: %u < %zu\n",
+			      wincert->dwLength, sizeof(*wincert));
+			continue;
 		}
-		msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert),
-					  wincert->dwLength - sizeof(*wincert));
+
+		debug("WIN_CERTIFICATE_TYPE: 0x%x\n",
+		      wincert->wCertificateType);
+
+		auth = (void *)wincert + sizeof(*wincert);
+		auth_size = wincert->dwLength - sizeof(*wincert);
+		if (wincert->wCertificateType == WIN_CERT_TYPE_EFI_GUID) {
+			if (auth + sizeof(efi_guid_t) >= wincerts_end)
+				break;
+
+			if (auth_size <= sizeof(efi_guid_t)) {
+				debug("dwLength too small: %u < %zu\n",
+				      wincert->dwLength, sizeof(*wincert));
+				continue;
+			}
+			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
+				debug("Certificate type not supported: %pUl\n",
+				      auth);
+				continue;
+			}
+
+			auth += sizeof(efi_guid_t);
+			auth_size -= sizeof(efi_guid_t);
+		} else if (wincert->wCertificateType
+				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
+			debug("Certificate type not supported\n");
+			continue;
+		}
+
+		msg = pkcs7_parse_message(auth, auth_size);
 		if (IS_ERR(msg)) {
 			debug("Parsing image's signature failed\n");
 			msg = NULL;
-			goto err;
+			continue;
 		}
 
 		/* try black-list first */
-- 
2.25.2

[PATCH 02/13] efi_loader: image_loader: add a check against certificate type of authenticode

[Heinrich Schuchardt]

On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> UEFI specification requires that we shall support three type of
> certificates of authenticode in PE image:
>   WIN_CERT_TYPE_EFI_GUID with the guid, EFI_CERT_TYPE_PCKS7_GUID
>   WIN_CERT_TYPE_PKCS_SIGNED_DATA
>   WIN_CERT_TYPE_EFI_PKCS1_15
>
> As EDK2 does, we will support the first two that are pkcs7 SignedData.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_image_loader.c | 55 ++++++++++++++++++++++++-------
>  1 file changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index 7f35b1e58fe5..a13ba3692ec2 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -375,7 +375,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>  	}
>
>  	/* Return Certificates Table */
> -	if (authsz) {
> +	if (authsz > 0) {

authsz is unsigned. We should avoid superfluous comparisons with 0.

>  		if (len < authoff + authsz) {
>  			debug("%s: Size for auth too large: %u >= %zu\n",
>  			      __func__, authsz, len - authoff);
> @@ -480,8 +480,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  	struct pkcs7_message *msg = NULL;
>  	struct efi_signature_store *db = NULL, *dbx = NULL;
>  	struct x509_certificate *cert = NULL;
> -	void *new_efi = NULL;
> -	size_t new_efi_size;
> +	void *new_efi = NULL, *auth, *wincerts_end;
> +	size_t new_efi_size, auth_size;
>  	bool ret = false;
>
>  	if (!efi_secure_boot_enabled())
> @@ -530,20 +530,51 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  	}
>
>  	/* go through WIN_CERTIFICATE list */
> -	for (wincert = wincerts;
> -	     (void *)wincert < (void *)wincerts + wincerts_len;
> +	for (wincert = wincerts, wincerts_end = (void *)wincerts + wincerts_len;
> +	     (void *)wincert < wincerts_end;

Adding to (void *) is not defined in the C spec (though gcc treats it
according to your intention). Please, use (u8 *) if you want to add byte
lengths.

>  	     wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
> -		if (wincert->dwLength < sizeof(*wincert)) {
> -			debug("%s: dwLength too small: %u < %zu\n",
> -			      __func__, wincert->dwLength, sizeof(*wincert));
> -			goto err;
> +		if ((void *)wincert + sizeof(*wincert) >= wincerts_end)
> +			break;
> +
> +		if (wincert->dwLength <= sizeof(*wincert)) {
> +			debug("dwLength too small: %u < %zu\n",
> +			      wincert->dwLength, sizeof(*wincert));
> +			continue;
>  		}
> -		msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert),
> -					  wincert->dwLength - sizeof(*wincert));
> +
> +		debug("WIN_CERTIFICATE_TYPE: 0x%x\n",
> +		      wincert->wCertificateType);

Should this EFI_PRINT()?

Best regards

Heinrich

> +
> +		auth = (void *)wincert + sizeof(*wincert);
> +		auth_size = wincert->dwLength - sizeof(*wincert);
> +		if (wincert->wCertificateType == WIN_CERT_TYPE_EFI_GUID) {
> +			if (auth + sizeof(efi_guid_t) >= wincerts_end)
> +				break;
> +
> +			if (auth_size <= sizeof(efi_guid_t)) {
> +				debug("dwLength too small: %u < %zu\n",
> +				      wincert->dwLength, sizeof(*wincert));
> +				continue;
> +			}
> +			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
> +				debug("Certificate type not supported: %pUl\n",
> +				      auth);
> +				continue;
> +			}
> +
> +			auth += sizeof(efi_guid_t);
> +			auth_size -= sizeof(efi_guid_t);
> +		} else if (wincert->wCertificateType
> +				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> +			debug("Certificate type not supported\n");
> +			continue;
> +		}
> +
> +		msg = pkcs7_parse_message(auth, auth_size);
>  		if (IS_ERR(msg)) {
>  			debug("Parsing image's signature failed\n");
>  			msg = NULL;
> -			goto err;
> +			continue;
>  		}
>
>  		/* try black-list first */
>

[PATCH 02/13] efi_loader: image_loader: add a check against certificate type of authenticode

[AKASHI Takahiro]

Heinrich,

On Fri, May 29, 2020 at 12:37:26PM +0200, Heinrich Schuchardt wrote:
> On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> > UEFI specification requires that we shall support three type of
> > certificates of authenticode in PE image:
> >   WIN_CERT_TYPE_EFI_GUID with the guid, EFI_CERT_TYPE_PCKS7_GUID
> >   WIN_CERT_TYPE_PKCS_SIGNED_DATA
> >   WIN_CERT_TYPE_EFI_PKCS1_15
> >
> > As EDK2 does, we will support the first two that are pkcs7 SignedData.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  lib/efi_loader/efi_image_loader.c | 55 ++++++++++++++++++++++++-------
> >  1 file changed, 43 insertions(+), 12 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> > index 7f35b1e58fe5..a13ba3692ec2 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -375,7 +375,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> >  	}
> >
> >  	/* Return Certificates Table */
> > -	if (authsz) {
> > +	if (authsz > 0) {
> 
> authsz is unsigned. We should avoid superfluous comparisons with 0.

Okay. I was a bit confused here.

> >  		if (len < authoff + authsz) {
> >  			debug("%s: Size for auth too large: %u >= %zu\n",
> >  			      __func__, authsz, len - authoff);
> > @@ -480,8 +480,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  	struct pkcs7_message *msg = NULL;
> >  	struct efi_signature_store *db = NULL, *dbx = NULL;
> >  	struct x509_certificate *cert = NULL;
> > -	void *new_efi = NULL;
> > -	size_t new_efi_size;
> > +	void *new_efi = NULL, *auth, *wincerts_end;
> > +	size_t new_efi_size, auth_size;
> >  	bool ret = false;
> >
> >  	if (!efi_secure_boot_enabled())
> > @@ -530,20 +530,51 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  	}
> >
> >  	/* go through WIN_CERTIFICATE list */
> > -	for (wincert = wincerts;
> > -	     (void *)wincert < (void *)wincerts + wincerts_len;
> > +	for (wincert = wincerts, wincerts_end = (void *)wincerts + wincerts_len;
> > +	     (void *)wincert < wincerts_end;
> 
> Adding to (void *) is not defined in the C spec (though gcc treats it
> according to your intention). Please, use (u8 *) if you want to add byte
> lengths.

Okay.

> >  	     wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
> > -		if (wincert->dwLength < sizeof(*wincert)) {
> > -			debug("%s: dwLength too small: %u < %zu\n",
> > -			      __func__, wincert->dwLength, sizeof(*wincert));
> > -			goto err;
> > +		if ((void *)wincert + sizeof(*wincert) >= wincerts_end)
> > +			break;
> > +
> > +		if (wincert->dwLength <= sizeof(*wincert)) {
> > +			debug("dwLength too small: %u < %zu\n",
> > +			      wincert->dwLength, sizeof(*wincert));
> > +			continue;
> >  		}
> > -		msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert),
> > -					  wincert->dwLength - sizeof(*wincert));
> > +
> > +		debug("WIN_CERTIFICATE_TYPE: 0x%x\n",
> > +		      wincert->wCertificateType);
> 
> Should this EFI_PRINT()?

Okay, I will replace all.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +
> > +		auth = (void *)wincert + sizeof(*wincert);
> > +		auth_size = wincert->dwLength - sizeof(*wincert);
> > +		if (wincert->wCertificateType == WIN_CERT_TYPE_EFI_GUID) {
> > +			if (auth + sizeof(efi_guid_t) >= wincerts_end)
> > +				break;
> > +
> > +			if (auth_size <= sizeof(efi_guid_t)) {
> > +				debug("dwLength too small: %u < %zu\n",
> > +				      wincert->dwLength, sizeof(*wincert));
> > +				continue;
> > +			}
> > +			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
> > +				debug("Certificate type not supported: %pUl\n",
> > +				      auth);
> > +				continue;
> > +			}
> > +
> > +			auth += sizeof(efi_guid_t);
> > +			auth_size -= sizeof(efi_guid_t);
> > +		} else if (wincert->wCertificateType
> > +				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> > +			debug("Certificate type not supported\n");
> > +			continue;
> > +		}
> > +
> > +		msg = pkcs7_parse_message(auth, auth_size);
> >  		if (IS_ERR(msg)) {
> >  			debug("Parsing image's signature failed\n");
> >  			msg = NULL;
> > -			goto err;
> > +			continue;
> >  		}
> >
> >  		/* try black-list first */
> >
> 

[PATCH 03/13] efi_loader: image_loader: retrieve authenticode only if it exists

[AKASHI Takahiro]

Since certificate table, which is indexed by IMAGE_DIRECTORY_ENTRY_SECURITY
and contains authenticode in PE image, doesn't always exist, we should make
sure that we will retrieve its pointer only if it exists.

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

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index a13ba3692ec2..5cdaa93519e7 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -266,6 +266,8 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 
 	dos = (void *)efi;
 	nt = (void *)(efi + dos->e_lfanew);
+	authoff = 0;
+	authsz = 0;
 
 	/*
 	 * Count maximum number of regions to be digested.
@@ -304,25 +306,36 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 			efi_image_region_add(regs,
 					     &opt->DataDirectory[ctidx] + 1,
 					     efi + opt->SizeOfHeaders, 0);
+
+			authoff = opt->DataDirectory[ctidx].VirtualAddress;
+			authsz = opt->DataDirectory[ctidx].Size;
 		}
 
 		bytes_hashed = opt->SizeOfHeaders;
 		align = opt->FileAlignment;
-		authoff = opt->DataDirectory[ctidx].VirtualAddress;
-		authsz = opt->DataDirectory[ctidx].Size;
 	} else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
 		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
 
+		/* Skip CheckSum */
 		efi_image_region_add(regs, efi, &opt->CheckSum, 0);
-		efi_image_region_add(regs, &opt->Subsystem,
-				     &opt->DataDirectory[ctidx], 0);
-		efi_image_region_add(regs, &opt->DataDirectory[ctidx] + 1,
-				     efi + opt->SizeOfHeaders, 0);
+		if (nt->OptionalHeader.NumberOfRvaAndSizes <= ctidx) {
+			efi_image_region_add(regs,
+					     &opt->Subsystem,
+					     efi + opt->SizeOfHeaders, 0);
+		} else {
+			/* Skip Certificates Table */
+			efi_image_region_add(regs, &opt->Subsystem,
+					     &opt->DataDirectory[ctidx], 0);
+			efi_image_region_add(regs,
+					     &opt->DataDirectory[ctidx] + 1,
+					     efi + opt->SizeOfHeaders, 0);
+
+			authoff = opt->DataDirectory[ctidx].VirtualAddress;
+			authsz = opt->DataDirectory[ctidx].Size;
+		}
 
 		bytes_hashed = opt->SizeOfHeaders;
 		align = opt->FileAlignment;
-		authoff = opt->DataDirectory[ctidx].VirtualAddress;
-		authsz = opt->DataDirectory[ctidx].Size;
 	} else {
 		debug("%s: Invalid optional header magic %x\n", __func__,
 		      nt->OptionalHeader.Magic);
-- 
2.25.2

[PATCH 03/13] efi_loader: image_loader: retrieve authenticode only if it exists

[Heinrich Schuchardt]

On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> Since certificate table, which is indexed by IMAGE_DIRECTORY_ENTRY_SECURITY

%s/Since certificate/Since the certificate/

> and contains authenticode in PE image, doesn't always exist, we should make
> sure that we will retrieve its pointer only if it exists.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[PATCH 04/13] efi_loader: signature: fix a size check against revocation list

[AKASHI Takahiro]

Since the size check against an entry in efi_search_siglist() is
incorrect, this function will never find out a to-be-matched certificate
and its associated revocation time in signature list.

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

diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index be6491c6e255..35f678de057e 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -432,10 +432,11 @@ static bool efi_search_siglist(struct x509_certificate *cert,
 		 *	time64_t revocation_time;
 		 * };
 		 */
-		if ((sig_data->size == SHA256_SUM_LEN) &&
-		    !memcmp(sig_data->data, hash, SHA256_SUM_LEN)) {
+		if ((sig_data->size >= SHA256_SUM_LEN + sizeof(time64_t)) &&
+		    !memcmp(sig_data->data, msg, SHA256_SUM_LEN)) {
 			memcpy(revoc_time, sig_data->data + SHA256_SUM_LEN,
 			       sizeof(*revoc_time));
+			debug("revocation time: 0x%llx\n", *revoc_time);
 			found = true;
 			goto out;
 		}
-- 
2.25.2

[PATCH 04/13] efi_loader: signature: fix a size check against revocation list

[Heinrich Schuchardt]

On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> Since the size check against an entry in efi_search_siglist() is
> incorrect, this function will never find out a to-be-matched certificate
> and its associated revocation time in signature list.

%s/in signature/in the signature/

>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_signature.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index be6491c6e255..35f678de057e 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -432,10 +432,11 @@ static bool efi_search_siglist(struct x509_certificate *cert,
>  		 *	time64_t revocation_time;
>  		 * };
>  		 */
> -		if ((sig_data->size == SHA256_SUM_LEN) &&
> -		    !memcmp(sig_data->data, hash, SHA256_SUM_LEN)) {
> +		if ((sig_data->size >= SHA256_SUM_LEN + sizeof(time64_t)) &&
> +		    !memcmp(sig_data->data, msg, SHA256_SUM_LEN)) {
>  			memcpy(revoc_time, sig_data->data + SHA256_SUM_LEN,
>  			       sizeof(*revoc_time));
> +			debug("revocation time: 0x%llx\n", *revoc_time);

Since this is seconds since 1970 wouldn't it be reasonable to use
decimal output (%llu)?

Otherwise:
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

>  			found = true;
>  			goto out;
>  		}
>

[PATCH 05/13] efi_loader: signature: make efi_hash_regions more generic

[AKASHI Takahiro]

There are a couple of occurrences of hash calculations in which a new
efi_hash_regions will be commonly used.

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

diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index 35f678de057e..00e442783059 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -30,6 +30,7 @@ const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
 /**
  * efi_hash_regions - calculate a hash value
  * @regs:	List of regions
+ * @count:	Number of regions
  * @hash:	Pointer to a pointer to buffer holding a hash value
  * @size:	Size of buffer to be returned
  *
@@ -37,18 +38,20 @@ const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
  *
  * Return:	true on success, false on error
  */
-static bool efi_hash_regions(struct efi_image_regions *regs, void **hash,
-			     size_t *size)
+static bool efi_hash_regions(struct image_region *regs, int count,
+			     void **hash, size_t *size)
 {
-	*size = 0;
-	*hash = calloc(1, SHA256_SUM_LEN);
 	if (!*hash) {
-		debug("Out of memory\n");
-		return false;
+		*hash = calloc(1, SHA256_SUM_LEN);
+		if (!*hash) {
+			debug("Out of memory\n");
+			return false;
+		}
 	}
-	*size = SHA256_SUM_LEN;
+	if (size)
+		*size = SHA256_SUM_LEN;
 
-	hash_calculate("sha256", regs->reg, regs->num, *hash);
+	hash_calculate("sha256", regs, count, *hash);
 #ifdef DEBUG
 	debug("hash calculated:\n");
 	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
@@ -73,26 +76,10 @@ static bool efi_hash_msg_content(struct pkcs7_message *msg, void **hash,
 {
 	struct image_region regtmp;
 
-	*size = 0;
-	*hash = calloc(1, SHA256_SUM_LEN);
-	if (!*hash) {
-		debug("Out of memory\n");
-		free(msg);
-		return false;
-	}
-	*size = SHA256_SUM_LEN;
-
 	regtmp.data = msg->data;
 	regtmp.size = msg->data_len;
 
-	hash_calculate("sha256", &regtmp, 1, *hash);
-#ifdef DEBUG
-	debug("hash calculated based on contentInfo:\n");
-	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
-		       *hash, SHA256_SUM_LEN, false);
-#endif
-
-	return true;
+	return efi_hash_regions(&regtmp, 1, hash, size);
 }
 
 /**
@@ -168,9 +155,10 @@ static bool efi_signature_verify(struct efi_image_regions *regs,
 			       false);
 #endif
 		/* against contentInfo first */
+		hash = NULL;
 		if ((msg->data && efi_hash_msg_content(msg, &hash, &size)) ||
 				/* for signed image */
-		    efi_hash_regions(regs, &hash, &size)) {
+		    efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
 				/* for authenticated variable */
 			if (ps_info->msgdigest_len != size ||
 			    memcmp(hash, ps_info->msgdigest, size)) {
@@ -238,7 +226,7 @@ bool efi_signature_verify_with_list(struct efi_image_regions *regs,
 	      regs, signed_info, siglist, valid_cert);
 
 	if (!signed_info) {
-		void *hash;
+		void *hash = NULL;
 		size_t size;
 
 		debug("%s: unsigned image\n", __func__);
@@ -252,7 +240,7 @@ bool efi_signature_verify_with_list(struct efi_image_regions *regs,
 			goto out;
 		}
 
-		if (!efi_hash_regions(regs, &hash, &size)) {
+		if (!efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
 			debug("Digesting unsigned image failed\n");
 			goto out;
 		}
-- 
2.25.2

[PATCH 05/13] efi_loader: signature: make efi_hash_regions more generic

[Heinrich Schuchardt]

On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> There are a couple of occurrences of hash calculations in which a new
> efi_hash_regions will be commonly used.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_signature.c | 44 +++++++++++++---------------------
>  1 file changed, 16 insertions(+), 28 deletions(-)
>
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index 35f678de057e..00e442783059 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -30,6 +30,7 @@ const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
>  /**
>   * efi_hash_regions - calculate a hash value
>   * @regs:	List of regions
> + * @count:	Number of regions
>   * @hash:	Pointer to a pointer to buffer holding a hash value
>   * @size:	Size of buffer to be returned
>   *
> @@ -37,18 +38,20 @@ const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
>   *
>   * Return:	true on success, false on error
>   */
> -static bool efi_hash_regions(struct efi_image_regions *regs, void **hash,
> -			     size_t *size)
> +static bool efi_hash_regions(struct image_region *regs, int count,
> +			     void **hash, size_t *size)

What is this size parameter good for if we know all signatures are
SHA256? Should it be eliminiated?

Otherwise
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

>  {
> -	*size = 0;
> -	*hash = calloc(1, SHA256_SUM_LEN);
>  	if (!*hash) {
> -		debug("Out of memory\n");
> -		return false;
> +		*hash = calloc(1, SHA256_SUM_LEN);
> +		if (!*hash) {
> +			debug("Out of memory\n");
> +			return false;
> +		}
>  	}
> -	*size = SHA256_SUM_LEN;
> +	if (size)
> +		*size = SHA256_SUM_LEN;
>
> -	hash_calculate("sha256", regs->reg, regs->num, *hash);
> +	hash_calculate("sha256", regs, count, *hash);
>  #ifdef DEBUG
>  	debug("hash calculated:\n");
>  	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> @@ -73,26 +76,10 @@ static bool efi_hash_msg_content(struct pkcs7_message *msg, void **hash,
>  {
>  	struct image_region regtmp;
>
> -	*size = 0;
> -	*hash = calloc(1, SHA256_SUM_LEN);
> -	if (!*hash) {
> -		debug("Out of memory\n");
> -		free(msg);
> -		return false;
> -	}
> -	*size = SHA256_SUM_LEN;
> -
>  	regtmp.data = msg->data;
>  	regtmp.size = msg->data_len;
>
> -	hash_calculate("sha256", &regtmp, 1, *hash);
> -#ifdef DEBUG
> -	debug("hash calculated based on contentInfo:\n");
> -	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> -		       *hash, SHA256_SUM_LEN, false);
> -#endif
> -
> -	return true;
> +	return efi_hash_regions(&regtmp, 1, hash, size);
>  }
>
>  /**
> @@ -168,9 +155,10 @@ static bool efi_signature_verify(struct efi_image_regions *regs,
>  			       false);
>  #endif
>  		/* against contentInfo first */
> +		hash = NULL;
>  		if ((msg->data && efi_hash_msg_content(msg, &hash, &size)) ||
>  				/* for signed image */
> -		    efi_hash_regions(regs, &hash, &size)) {
> +		    efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
>  				/* for authenticated variable */
>  			if (ps_info->msgdigest_len != size ||
>  			    memcmp(hash, ps_info->msgdigest, size)) {
> @@ -238,7 +226,7 @@ bool efi_signature_verify_with_list(struct efi_image_regions *regs,
>  	      regs, signed_info, siglist, valid_cert);
>
>  	if (!signed_info) {
> -		void *hash;
> +		void *hash = NULL;
>  		size_t size;
>
>  		debug("%s: unsigned image\n", __func__);
> @@ -252,7 +240,7 @@ bool efi_signature_verify_with_list(struct efi_image_regions *regs,
>  			goto out;
>  		}
>
> -		if (!efi_hash_regions(regs, &hash, &size)) {
> +		if (!efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
>  			debug("Digesting unsigned image failed\n");
>  			goto out;
>  		}
>

[PATCH 05/13] efi_loader: signature: make efi_hash_regions more generic

[AKASHI Takahiro]

Heinrich,

On Sat, May 30, 2020 at 08:58:02AM +0200, Heinrich Schuchardt wrote:
> On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> > There are a couple of occurrences of hash calculations in which a new
> > efi_hash_regions will be commonly used.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  lib/efi_loader/efi_signature.c | 44 +++++++++++++---------------------
> >  1 file changed, 16 insertions(+), 28 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> > index 35f678de057e..00e442783059 100644
> > --- a/lib/efi_loader/efi_signature.c
> > +++ b/lib/efi_loader/efi_signature.c
> > @@ -30,6 +30,7 @@ const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
> >  /**
> >   * efi_hash_regions - calculate a hash value
> >   * @regs:	List of regions
> > + * @count:	Number of regions
> >   * @hash:	Pointer to a pointer to buffer holding a hash value
> >   * @size:	Size of buffer to be returned
> >   *
> > @@ -37,18 +38,20 @@ const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
> >   *
> >   * Return:	true on success, false on error
> >   */
> > -static bool efi_hash_regions(struct efi_image_regions *regs, void **hash,
> > -			     size_t *size)
> > +static bool efi_hash_regions(struct image_region *regs, int count,
> > +			     void **hash, size_t *size)
> 
> What is this size parameter good for if we know all signatures are
> SHA256? Should it be eliminiated?

It would make more sense when we support algorithms other than sha256.
The caller, then, could look like:

  efi_hash_regions("sha256", regs, count, hash, size);
  if (!memcmp(data, hash, size))
    ...

That way, callers in different use cases won't have to use an immediate
value for actual hash size.
For example, an certificate entry in the revocation list can be
digested with, according to UEFI specification,
  EFI_CERT_X509_SHA256_GUID
  EFI_CERT_X509_SHA384_GUID
  EFI_CERT_X509_SHA512_GUID

Meanwhile, signatures can be different type of message digests, as well.
  EFI_CERT_SHA1_GUID
  EFI_CERT_SHA224_GUID
  EFI_CERT_SHA256_GUID
  EFI_CERT_SHA384_GUID
  EFI_CERT_SHA512_GUID

> Otherwise
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Thanks,
-Takahiro Akashi


> >  {
> > -	*size = 0;
> > -	*hash = calloc(1, SHA256_SUM_LEN);
> >  	if (!*hash) {
> > -		debug("Out of memory\n");
> > -		return false;
> > +		*hash = calloc(1, SHA256_SUM_LEN);
> > +		if (!*hash) {
> > +			debug("Out of memory\n");
> > +			return false;
> > +		}
> >  	}
> > -	*size = SHA256_SUM_LEN;
> > +	if (size)
> > +		*size = SHA256_SUM_LEN;
> >
> > -	hash_calculate("sha256", regs->reg, regs->num, *hash);
> > +	hash_calculate("sha256", regs, count, *hash);
> >  #ifdef DEBUG
> >  	debug("hash calculated:\n");
> >  	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> > @@ -73,26 +76,10 @@ static bool efi_hash_msg_content(struct pkcs7_message *msg, void **hash,
> >  {
> >  	struct image_region regtmp;
> >
> > -	*size = 0;
> > -	*hash = calloc(1, SHA256_SUM_LEN);
> > -	if (!*hash) {
> > -		debug("Out of memory\n");
> > -		free(msg);
> > -		return false;
> > -	}
> > -	*size = SHA256_SUM_LEN;
> > -
> >  	regtmp.data = msg->data;
> >  	regtmp.size = msg->data_len;
> >
> > -	hash_calculate("sha256", &regtmp, 1, *hash);
> > -#ifdef DEBUG
> > -	debug("hash calculated based on contentInfo:\n");
> > -	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> > -		       *hash, SHA256_SUM_LEN, false);
> > -#endif
> > -
> > -	return true;
> > +	return efi_hash_regions(&regtmp, 1, hash, size);
> >  }
> >
> >  /**
> > @@ -168,9 +155,10 @@ static bool efi_signature_verify(struct efi_image_regions *regs,
> >  			       false);
> >  #endif
> >  		/* against contentInfo first */
> > +		hash = NULL;
> >  		if ((msg->data && efi_hash_msg_content(msg, &hash, &size)) ||
> >  				/* for signed image */
> > -		    efi_hash_regions(regs, &hash, &size)) {
> > +		    efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
> >  				/* for authenticated variable */
> >  			if (ps_info->msgdigest_len != size ||
> >  			    memcmp(hash, ps_info->msgdigest, size)) {
> > @@ -238,7 +226,7 @@ bool efi_signature_verify_with_list(struct efi_image_regions *regs,
> >  	      regs, signed_info, siglist, valid_cert);
> >
> >  	if (!signed_info) {
> > -		void *hash;
> > +		void *hash = NULL;
> >  		size_t size;
> >
> >  		debug("%s: unsigned image\n", __func__);
> > @@ -252,7 +240,7 @@ bool efi_signature_verify_with_list(struct efi_image_regions *regs,
> >  			goto out;
> >  		}
> >
> > -		if (!efi_hash_regions(regs, &hash, &size)) {
> > +		if (!efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
> >  			debug("Digesting unsigned image failed\n");
> >  			goto out;
> >  		}
> >
> 

[PATCH 06/13] efi_loader: image_loader: verification for all signatures should pass

[AKASHI Takahiro]

A signed image may have multiple signatures in
  - each WIN_CERTIFICATE in authenticode, and/or
  - each SignerInfo in pkcs7 SignedData (of WIN_CERTIFICATE)

In the initial implementation of efi_image_authenticate(), the criteria
of verification check for multiple signatures case is a bit ambiguous
and it may cause inconsistent result.

With this patch, we will make sure that verification check in
efi_image_authenticate() should pass against all the signatures.
The only exception would be
  - the case where a digest algorithm used in signature is not supported by
    U-Boot, or
  - the case where parsing some portion of authenticode has failed
In those cases, we don't know how the signature be handled and should
just ignore them.

Please note that, due to this change, efi_signature_verify_with_sigdb()'s
function prototype will be modified, taking "dbx" as well as "db"
instead of outputing a "certificate." If "dbx" is null, the behavior would
be the exact same as before.
The function's name will be changed to efi_signature_verify() once
current efi_signature_verify() has gone due to further improvement
in intermedaite certificates support.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_loader.h              |  10 +-
 lib/efi_loader/efi_image_loader.c |  37 +++--
 lib/efi_loader/efi_signature.c    | 266 ++++++++++++++----------------
 3 files changed, 146 insertions(+), 167 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 9533df26dc9e..2cbd52e273d4 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -761,14 +761,12 @@ struct efi_signature_store {
 struct x509_certificate;
 struct pkcs7_message;
 
-bool efi_signature_verify_cert(struct x509_certificate *cert,
-			       struct efi_signature_store *dbx);
-bool efi_signature_verify_signers(struct pkcs7_message *msg,
-				  struct efi_signature_store *dbx);
 bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
 				     struct pkcs7_message *msg,
-				  struct efi_signature_store *db,
-				  struct x509_certificate **cert);
+				     struct efi_signature_store *db,
+				     struct efi_signature_store *dbx);
+bool efi_signature_check_signers(struct pkcs7_message *msg,
+				 struct efi_signature_store *dbx);
 
 efi_status_t efi_image_region_add(struct efi_image_regions *regs,
 				  const void *start, const void *end,
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index 5cdaa93519e7..33ffb43f3886 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -492,11 +492,12 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 	size_t wincerts_len;
 	struct pkcs7_message *msg = NULL;
 	struct efi_signature_store *db = NULL, *dbx = NULL;
-	struct x509_certificate *cert = NULL;
 	void *new_efi = NULL, *auth, *wincerts_end;
 	size_t new_efi_size, auth_size;
 	bool ret = false;
 
+	debug("%s: Enter, %d\n", __func__, ret);
+
 	if (!efi_secure_boot_enabled())
 		return true;
 
@@ -542,7 +543,17 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 		goto err;
 	}
 
-	/* go through WIN_CERTIFICATE list */
+	/*
+	 * go through WIN_CERTIFICATE list
+	 * NOTE:
+	 * We may have multiple signatures either as WIN_CERTIFICATE's
+	 * in PE header, or as pkcs7 SignerInfo's in SignedData.
+	 * So the verification policy here is:
+	 *   - Success if, at least, one of signatures is verified
+	 *   - unless
+	 *       any of signatures is rejected explicitly, or
+	 *       none of digest algorithms are supported
+	 */
 	for (wincert = wincerts, wincerts_end = (void *)wincerts + wincerts_len;
 	     (void *)wincert < wincerts_end;
 	     wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
@@ -596,37 +607,27 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 			goto err;
 		}
 
-		if (!efi_signature_verify_signers(msg, dbx)) {
-			debug("Signer was rejected by \"dbx\"\n");
+		if (!efi_signature_check_signers(msg, dbx)) {
+			debug("Signer(s) in \"dbx\"\n");
 			goto err;
-		} else {
-			ret = true;
 		}
 
 		/* try white-list */
-		if (!efi_signature_verify_with_sigdb(regs, msg, db, &cert)) {
-			debug("Verifying signature with \"db\" failed\n");
+		if (!efi_signature_verify_with_sigdb(regs, msg, db, dbx)) {
+			debug("Signature was not verified by \"db\"\n");
 			goto err;
-		} else {
-			ret = true;
-		}
-
-		if (!efi_signature_verify_cert(cert, dbx)) {
-			debug("Certificate was rejected by \"dbx\"\n");
-			goto err;
-		} else {
-			ret = true;
 		}
 	}
+	ret = true;
 
 err:
-	x509_free_certificate(cert);
 	efi_sigstore_free(db);
 	efi_sigstore_free(dbx);
 	pkcs7_free_message(msg);
 	free(regs);
 	free(new_efi);
 
+	debug("%s: Exit, %d\n", __func__, ret);
 	return ret;
 }
 #else
diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index 00e442783059..ab5687040a38 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -300,27 +300,111 @@ out:
 }
 
 /**
- * efi_signature_verify_with_sigdb - verify a signature with db
+ * efi_signature_check_revocation - check revocation with dbx
+ * @sinfo:	Signer's info
+ * @cert:	x509 certificate
+ * @dbx:	Revocation signature database
+ *
+ * Search revocation signature database pointed to by @dbx and find
+ * an entry matching to certificate pointed to by @cert.
+ *
+ * While this entry contains revocation time, we don't support timestamp
+ * protocol at this time and any image will be unconditionally revoked
+ * when this match occurs.
+ *
+ * Return:	true if check passed, false otherwise.
+ */
+static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
+					   struct x509_certificate *cert,
+					   struct efi_signature_store *dbx)
+{
+	struct efi_signature_store *siglist;
+	struct efi_sig_data *sig_data;
+	struct image_region reg[1];
+	void *hash = NULL;
+	size_t size = 0;
+	time64_t revoc_time;
+	bool revoked = false;
+
+	debug("%s: Enter, %p, %p, %p\n", __func__, sinfo, cert, dbx);
+
+	if (!sinfo || !cert || !dbx || !dbx->sig_data_list)
+		goto out;
+
+	debug("Checking revocation against %s\n", cert->subject);
+	for (siglist = dbx; siglist; siglist = siglist->next) {
+		if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256))
+			continue;
+
+		/* calculate hash of TBSCertificate */
+		reg[0].data = cert->tbs;
+		reg[0].size = cert->tbs_size;
+		if (!efi_hash_regions(reg, 1, &hash, &size))
+			goto out;
+
+		for (sig_data = siglist->sig_data_list; sig_data;
+		     sig_data = sig_data->next) {
+			/*
+			 * struct efi_cert_x509_sha256 {
+			 *	u8 tbs_hash[256/8];
+			 *	time64_t revocation_time;
+			 * };
+			 */
+#ifdef DEBUG
+			if (sig_data->size >= size) {
+				debug("hash in db:\n");
+				print_hex_dump("    ", DUMP_PREFIX_OFFSET,
+					       16, 1,
+					       sig_data->data, size, false);
+			}
+#endif
+			if ((sig_data->size < size + sizeof(time64_t)) ||
+			    memcmp(sig_data->data, hash, size))
+				continue;
+
+			memcpy(&revoc_time, sig_data->data + size,
+			       sizeof(revoc_time));
+			debug("revocation time: 0x%llx\n", revoc_time);
+			/*
+			 * TODO: compare signing timestamp in sinfo
+			 * with revocation time
+			 */
+
+			revoked = true;
+			free(hash);
+			goto out;
+		}
+		free(hash);
+		hash = NULL;
+	}
+out:
+	debug("%s: Exit, revoked: %d\n", __func__, revoked);
+	return !revoked;
+}
+
+/**
+ * efi_signature_verify_with_sigdb - verify signatures with db and dbx
  * @regs:	List of regions to be authenticated
  * @msg:	Signature
  * @db:		Signature database for trusted certificates
- * @cert:	x509 certificate that verifies this signature
+ * @dbx:	Revocation signature database
  *
- * Signature pointed to by @msg against image pointed to by @regs
- * is verified by signature database pointed to by @db.
+ * All the signature pointed to by @msg against image pointed to by @regs
+ * will be verified by signature database pointed to by @db and @dbx.
  *
- * Return:	true if signature is verified, false if not
+ * Return:	true if verification for all signatures passed, false otherwise
  */
 bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
 				     struct pkcs7_message *msg,
 				     struct efi_signature_store *db,
-				     struct x509_certificate **cert)
+				     struct efi_signature_store *dbx)
 {
 	struct pkcs7_signed_info *info;
 	struct efi_signature_store *siglist;
+	struct x509_certificate *cert;
 	bool verified = false;
 
-	debug("%s: Enter, %p, %p, %p, %p\n", __func__, regs, msg, db, cert);
+	debug("%s: Enter, %p, %p, %p, %p\n", __func__, regs, msg, db, dbx);
 
 	if (!db)
 		goto out;
@@ -333,7 +417,7 @@ bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
 		debug("%s: Verify unsigned image with db\n", __func__);
 		for (siglist = db; siglist; siglist = siglist->next)
 			if (efi_signature_verify_with_list(regs, NULL, NULL,
-							   siglist, cert)) {
+							   siglist, &cert)) {
 				verified = true;
 				goto out;
 			}
@@ -349,12 +433,21 @@ bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
 
 		for (siglist = db; siglist; siglist = siglist->next) {
 			if (efi_signature_verify_with_list(regs, msg, info,
-							   siglist, cert)) {
-				verified = true;
-				goto out;
-			}
+							   siglist, &cert))
+				break;
 		}
+		if (!siglist) {
+			debug("No valid certificate in \"db\"\n");
+			goto out;
+		}
+
+		if (!dbx || efi_signature_check_revocation(info, cert, dbx))
+			continue;
+
+		debug("Certificate in \"dbx\"\n");
+		goto out;
 	}
+	verified = true;
 
 out:
 	debug("%s: Exit, verified: %d\n", __func__, verified);
@@ -362,150 +455,37 @@ out:
 }
 
 /**
- * efi_search_siglist - search signature list for a certificate
- * @cert:	x509 certificate
- * @siglist:	Signature list
- * @revoc_time:	Pointer to buffer for revocation time
- *
- * Search signature list pointed to by @siglist and find a certificate
- * pointed to by @cert.
- * If found, revocation time that is specified in signature database is
- * returned in @revoc_time.
- *
- * Return:	true if certificate is found, false if not
- */
-static bool efi_search_siglist(struct x509_certificate *cert,
-			       struct efi_signature_store *siglist,
-			       time64_t *revoc_time)
-{
-	struct image_region reg[1];
-	void *hash = NULL, *msg = NULL;
-	struct efi_sig_data *sig_data;
-	bool found = false;
-
-	/* can be null */
-	if (!siglist->sig_data_list)
-		return false;
-
-	if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256)) {
-		/* TODO: other hash algos */
-		debug("Certificate's digest type is not supported: %pUl\n",
-		      &siglist->sig_type);
-		goto out;
-	}
-
-	/* calculate hash of TBSCertificate */
-	msg = calloc(1, SHA256_SUM_LEN);
-	if (!msg) {
-		debug("Out of memory\n");
-		goto out;
-	}
-
-	hash = calloc(1, SHA256_SUM_LEN);
-	if (!hash) {
-		debug("Out of memory\n");
-		goto out;
-	}
-
-	reg[0].data = cert->tbs;
-	reg[0].size = cert->tbs_size;
-	hash_calculate("sha256", reg, 1, msg);
-
-	/* go through signature list */
-	for (sig_data = siglist->sig_data_list; sig_data;
-	     sig_data = sig_data->next) {
-		/*
-		 * struct efi_cert_x509_sha256 {
-		 *	u8 tbs_hash[256/8];
-		 *	time64_t revocation_time;
-		 * };
-		 */
-		if ((sig_data->size >= SHA256_SUM_LEN + sizeof(time64_t)) &&
-		    !memcmp(sig_data->data, msg, SHA256_SUM_LEN)) {
-			memcpy(revoc_time, sig_data->data + SHA256_SUM_LEN,
-			       sizeof(*revoc_time));
-			debug("revocation time: 0x%llx\n", *revoc_time);
-			found = true;
-			goto out;
-		}
-	}
-
-out:
-	free(hash);
-	free(msg);
-
-	return found;
-}
-
-/**
- * efi_signature_verify_cert - verify a certificate with dbx
- * @cert:	x509 certificate
- * @dbx:	Signature database
- *
- * Search signature database pointed to by @dbx and find a certificate
- * pointed to by @cert.
- * This function is expected to be used against "dbx".
- *
- * Return:	true if a certificate is not rejected, false otherwise.
- */
-bool efi_signature_verify_cert(struct x509_certificate *cert,
-			       struct efi_signature_store *dbx)
-{
-	struct efi_signature_store *siglist;
-	time64_t revoc_time;
-	bool found = false;
-
-	debug("%s: Enter, %p, %p\n", __func__, dbx, cert);
-
-	if (!cert)
-		return false;
-
-	for (siglist = dbx; siglist; siglist = siglist->next) {
-		if (efi_search_siglist(cert, siglist, &revoc_time)) {
-			/* TODO */
-			/* compare signing time with revocation time */
-
-			found = true;
-			break;
-		}
-	}
-
-	debug("%s: Exit, verified: %d\n", __func__, !found);
-	return !found;
-}
-
-/**
- * efi_signature_verify_signers - verify signers' certificates with dbx
+ * efi_signature_check_signers - check revocation against all signers with dbx
  * @msg:	Signature
- * @dbx:	Signature database
+ * @dbx:	Revocation signature database
  *
- * Determine if any of signers' certificates in @msg may be verified
- * by any of certificates in signature database pointed to by @dbx.
- * This function is expected to be used against "dbx".
+ * Determine if none of signers' certificates in @msg are revoked
+ * by signature database pointed to by @dbx.
  *
- * Return:	true if none of certificates is rejected, false otherwise.
+ * Return:	true if all signers passed, false otherwise.
  */
-bool efi_signature_verify_signers(struct pkcs7_message *msg,
-				  struct efi_signature_store *dbx)
+bool efi_signature_check_signers(struct pkcs7_message *msg,
+				 struct efi_signature_store *dbx)
 {
-	struct pkcs7_signed_info *info;
-	bool found = false;
+	struct pkcs7_signed_info *sinfo;
+	bool revoked = false;
 
 	debug("%s: Enter, %p, %p\n", __func__, msg, dbx);
 
-	if (!msg)
+	if (!msg || !dbx)
 		goto out;
 
-	for (info = msg->signed_infos; info; info = info->next) {
-		if (info->signer &&
-		    !efi_signature_verify_cert(info->signer, dbx)) {
-			found = true;
-			goto out;
+	for (sinfo = msg->signed_infos; sinfo; sinfo = sinfo->next) {
+		if (sinfo->signer &&
+		    !efi_signature_check_revocation(sinfo, sinfo->signer,
+						    dbx)) {
+			revoked = true;
+			break;
 		}
 	}
 out:
-	debug("%s: Exit, verified: %d\n", __func__, !found);
-	return !found;
+	debug("%s: Exit, revoked: %d\n", __func__, revoked);
+	return !revoked;
 }
 
 /**
-- 
2.25.2

[PATCH 06/13] efi_loader: image_loader: verification for all signatures should pass

[Heinrich Schuchardt]

On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> A signed image may have multiple signatures in
>   - each WIN_CERTIFICATE in authenticode, and/or
>   - each SignerInfo in pkcs7 SignedData (of WIN_CERTIFICATE)
>
> In the initial implementation of efi_image_authenticate(), the criteria
> of verification check for multiple signatures case is a bit ambiguous
> and it may cause inconsistent result.
>
> With this patch, we will make sure that verification check in
> efi_image_authenticate() should pass against all the signatures.
> The only exception would be
>   - the case where a digest algorithm used in signature is not supported by
>     U-Boot, or
>   - the case where parsing some portion of authenticode has failed
> In those cases, we don't know how the signature be handled and should
> just ignore them.
>
> Please note that, due to this change, efi_signature_verify_with_sigdb()'s
> function prototype will be modified, taking "dbx" as well as "db"
> instead of outputing a "certificate." If "dbx" is null, the behavior would
> be the exact same as before.
> The function's name will be changed to efi_signature_verify() once
> current efi_signature_verify() has gone due to further improvement
> in intermedaite certificates support.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/efi_loader.h              |  10 +-
>  lib/efi_loader/efi_image_loader.c |  37 +++--
>  lib/efi_loader/efi_signature.c    | 266 ++++++++++++++----------------
>  3 files changed, 146 insertions(+), 167 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 9533df26dc9e..2cbd52e273d4 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -761,14 +761,12 @@ struct efi_signature_store {
>  struct x509_certificate;
>  struct pkcs7_message;
>
> -bool efi_signature_verify_cert(struct x509_certificate *cert,
> -			       struct efi_signature_store *dbx);
> -bool efi_signature_verify_signers(struct pkcs7_message *msg,
> -				  struct efi_signature_store *dbx);
>  bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
>  				     struct pkcs7_message *msg,
> -				  struct efi_signature_store *db,
> -				  struct x509_certificate **cert);
> +				     struct efi_signature_store *db,
> +				     struct efi_signature_store *dbx);
> +bool efi_signature_check_signers(struct pkcs7_message *msg,
> +				 struct efi_signature_store *dbx);
>
>  efi_status_t efi_image_region_add(struct efi_image_regions *regs,
>  				  const void *start, const void *end,
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index 5cdaa93519e7..33ffb43f3886 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -492,11 +492,12 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  	size_t wincerts_len;
>  	struct pkcs7_message *msg = NULL;
>  	struct efi_signature_store *db = NULL, *dbx = NULL;
> -	struct x509_certificate *cert = NULL;
>  	void *new_efi = NULL, *auth, *wincerts_end;
>  	size_t new_efi_size, auth_size;
>  	bool ret = false;
>
> +	debug("%s: Enter, %d\n", __func__, ret);
> +
>  	if (!efi_secure_boot_enabled())
>  		return true;
>
> @@ -542,7 +543,17 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  		goto err;
>  	}
>
> -	/* go through WIN_CERTIFICATE list */
> +	/*
> +	 * go through WIN_CERTIFICATE list
> +	 * NOTE:
> +	 * We may have multiple signatures either as WIN_CERTIFICATE's
> +	 * in PE header, or as pkcs7 SignerInfo's in SignedData.
> +	 * So the verification policy here is:
> +	 *   - Success if, at least, one of signatures is verified
> +	 *   - unless
> +	 *       any of signatures is rejected explicitly, or

If one signature is good and the others are rejected (e.g. due to the
revocation list), why won't you be happy with the one good signature? Is
this a requirement in the UEFI spec?

Best regards

Heinrich

> +	 *       none of digest algorithms are supported
> +	 */
>  	for (wincert = wincerts, wincerts_end = (void *)wincerts + wincerts_len;
>  	     (void *)wincert < wincerts_end;
>  	     wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
> @@ -596,37 +607,27 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  			goto err;
>  		}
>
> -		if (!efi_signature_verify_signers(msg, dbx)) {
> -			debug("Signer was rejected by \"dbx\"\n");
> +		if (!efi_signature_check_signers(msg, dbx)) {
> +			debug("Signer(s) in \"dbx\"\n");
>  			goto err;
> -		} else {
> -			ret = true;
>  		}
>
>  		/* try white-list */
> -		if (!efi_signature_verify_with_sigdb(regs, msg, db, &cert)) {
> -			debug("Verifying signature with \"db\" failed\n");
> +		if (!efi_signature_verify_with_sigdb(regs, msg, db, dbx)) {
> +			debug("Signature was not verified by \"db\"\n");
>  			goto err;
> -		} else {
> -			ret = true;
> -		}
> -
> -		if (!efi_signature_verify_cert(cert, dbx)) {
> -			debug("Certificate was rejected by \"dbx\"\n");
> -			goto err;
> -		} else {
> -			ret = true;
>  		}
>  	}
> +	ret = true;
>
>  err:
> -	x509_free_certificate(cert);
>  	efi_sigstore_free(db);
>  	efi_sigstore_free(dbx);
>  	pkcs7_free_message(msg);
>  	free(regs);
>  	free(new_efi);
>
> +	debug("%s: Exit, %d\n", __func__, ret);
>  	return ret;
>  }
>  #else
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index 00e442783059..ab5687040a38 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -300,27 +300,111 @@ out:
>  }
>
>  /**
> - * efi_signature_verify_with_sigdb - verify a signature with db
> + * efi_signature_check_revocation - check revocation with dbx
> + * @sinfo:	Signer's info
> + * @cert:	x509 certificate
> + * @dbx:	Revocation signature database
> + *
> + * Search revocation signature database pointed to by @dbx and find
> + * an entry matching to certificate pointed to by @cert.
> + *
> + * While this entry contains revocation time, we don't support timestamp
> + * protocol at this time and any image will be unconditionally revoked
> + * when this match occurs.
> + *
> + * Return:	true if check passed, false otherwise.
> + */
> +static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
> +					   struct x509_certificate *cert,
> +					   struct efi_signature_store *dbx)
> +{
> +	struct efi_signature_store *siglist;
> +	struct efi_sig_data *sig_data;
> +	struct image_region reg[1];
> +	void *hash = NULL;
> +	size_t size = 0;
> +	time64_t revoc_time;
> +	bool revoked = false;
> +
> +	debug("%s: Enter, %p, %p, %p\n", __func__, sinfo, cert, dbx);
> +
> +	if (!sinfo || !cert || !dbx || !dbx->sig_data_list)
> +		goto out;
> +
> +	debug("Checking revocation against %s\n", cert->subject);
> +	for (siglist = dbx; siglist; siglist = siglist->next) {
> +		if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256))
> +			continue;
> +
> +		/* calculate hash of TBSCertificate */
> +		reg[0].data = cert->tbs;
> +		reg[0].size = cert->tbs_size;
> +		if (!efi_hash_regions(reg, 1, &hash, &size))
> +			goto out;
> +
> +		for (sig_data = siglist->sig_data_list; sig_data;
> +		     sig_data = sig_data->next) {
> +			/*
> +			 * struct efi_cert_x509_sha256 {
> +			 *	u8 tbs_hash[256/8];
> +			 *	time64_t revocation_time;
> +			 * };
> +			 */
> +#ifdef DEBUG
> +			if (sig_data->size >= size) {
> +				debug("hash in db:\n");
> +				print_hex_dump("    ", DUMP_PREFIX_OFFSET,
> +					       16, 1,
> +					       sig_data->data, size, false);
> +			}
> +#endif
> +			if ((sig_data->size < size + sizeof(time64_t)) ||
> +			    memcmp(sig_data->data, hash, size))
> +				continue;
> +
> +			memcpy(&revoc_time, sig_data->data + size,
> +			       sizeof(revoc_time));
> +			debug("revocation time: 0x%llx\n", revoc_time);
> +			/*
> +			 * TODO: compare signing timestamp in sinfo
> +			 * with revocation time
> +			 */
> +
> +			revoked = true;
> +			free(hash);
> +			goto out;
> +		}
> +		free(hash);
> +		hash = NULL;
> +	}
> +out:
> +	debug("%s: Exit, revoked: %d\n", __func__, revoked);
> +	return !revoked;
> +}
> +
> +/**
> + * efi_signature_verify_with_sigdb - verify signatures with db and dbx
>   * @regs:	List of regions to be authenticated
>   * @msg:	Signature
>   * @db:		Signature database for trusted certificates
> - * @cert:	x509 certificate that verifies this signature
> + * @dbx:	Revocation signature database
>   *
> - * Signature pointed to by @msg against image pointed to by @regs
> - * is verified by signature database pointed to by @db.
> + * All the signature pointed to by @msg against image pointed to by @regs
> + * will be verified by signature database pointed to by @db and @dbx.
>   *
> - * Return:	true if signature is verified, false if not
> + * Return:	true if verification for all signatures passed, false otherwise
>   */
>  bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
>  				     struct pkcs7_message *msg,
>  				     struct efi_signature_store *db,
> -				     struct x509_certificate **cert)
> +				     struct efi_signature_store *dbx)
>  {
>  	struct pkcs7_signed_info *info;
>  	struct efi_signature_store *siglist;
> +	struct x509_certificate *cert;
>  	bool verified = false;
>
> -	debug("%s: Enter, %p, %p, %p, %p\n", __func__, regs, msg, db, cert);
> +	debug("%s: Enter, %p, %p, %p, %p\n", __func__, regs, msg, db, dbx);
>
>  	if (!db)
>  		goto out;
> @@ -333,7 +417,7 @@ bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
>  		debug("%s: Verify unsigned image with db\n", __func__);
>  		for (siglist = db; siglist; siglist = siglist->next)
>  			if (efi_signature_verify_with_list(regs, NULL, NULL,
> -							   siglist, cert)) {
> +							   siglist, &cert)) {
>  				verified = true;
>  				goto out;
>  			}
> @@ -349,12 +433,21 @@ bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
>
>  		for (siglist = db; siglist; siglist = siglist->next) {
>  			if (efi_signature_verify_with_list(regs, msg, info,
> -							   siglist, cert)) {
> -				verified = true;
> -				goto out;
> -			}
> +							   siglist, &cert))
> +				break;
>  		}
> +		if (!siglist) {
> +			debug("No valid certificate in \"db\"\n");
> +			goto out;
> +		}
> +
> +		if (!dbx || efi_signature_check_revocation(info, cert, dbx))
> +			continue;
> +
> +		debug("Certificate in \"dbx\"\n");
> +		goto out;
>  	}
> +	verified = true;
>
>  out:
>  	debug("%s: Exit, verified: %d\n", __func__, verified);
> @@ -362,150 +455,37 @@ out:
>  }
>
>  /**
> - * efi_search_siglist - search signature list for a certificate
> - * @cert:	x509 certificate
> - * @siglist:	Signature list
> - * @revoc_time:	Pointer to buffer for revocation time
> - *
> - * Search signature list pointed to by @siglist and find a certificate
> - * pointed to by @cert.
> - * If found, revocation time that is specified in signature database is
> - * returned in @revoc_time.
> - *
> - * Return:	true if certificate is found, false if not
> - */
> -static bool efi_search_siglist(struct x509_certificate *cert,
> -			       struct efi_signature_store *siglist,
> -			       time64_t *revoc_time)
> -{
> -	struct image_region reg[1];
> -	void *hash = NULL, *msg = NULL;
> -	struct efi_sig_data *sig_data;
> -	bool found = false;
> -
> -	/* can be null */
> -	if (!siglist->sig_data_list)
> -		return false;
> -
> -	if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256)) {
> -		/* TODO: other hash algos */
> -		debug("Certificate's digest type is not supported: %pUl\n",
> -		      &siglist->sig_type);
> -		goto out;
> -	}
> -
> -	/* calculate hash of TBSCertificate */
> -	msg = calloc(1, SHA256_SUM_LEN);
> -	if (!msg) {
> -		debug("Out of memory\n");
> -		goto out;
> -	}
> -
> -	hash = calloc(1, SHA256_SUM_LEN);
> -	if (!hash) {
> -		debug("Out of memory\n");
> -		goto out;
> -	}
> -
> -	reg[0].data = cert->tbs;
> -	reg[0].size = cert->tbs_size;
> -	hash_calculate("sha256", reg, 1, msg);
> -
> -	/* go through signature list */
> -	for (sig_data = siglist->sig_data_list; sig_data;
> -	     sig_data = sig_data->next) {
> -		/*
> -		 * struct efi_cert_x509_sha256 {
> -		 *	u8 tbs_hash[256/8];
> -		 *	time64_t revocation_time;
> -		 * };
> -		 */
> -		if ((sig_data->size >= SHA256_SUM_LEN + sizeof(time64_t)) &&
> -		    !memcmp(sig_data->data, msg, SHA256_SUM_LEN)) {
> -			memcpy(revoc_time, sig_data->data + SHA256_SUM_LEN,
> -			       sizeof(*revoc_time));
> -			debug("revocation time: 0x%llx\n", *revoc_time);
> -			found = true;
> -			goto out;
> -		}
> -	}
> -
> -out:
> -	free(hash);
> -	free(msg);
> -
> -	return found;
> -}
> -
> -/**
> - * efi_signature_verify_cert - verify a certificate with dbx
> - * @cert:	x509 certificate
> - * @dbx:	Signature database
> - *
> - * Search signature database pointed to by @dbx and find a certificate
> - * pointed to by @cert.
> - * This function is expected to be used against "dbx".
> - *
> - * Return:	true if a certificate is not rejected, false otherwise.
> - */
> -bool efi_signature_verify_cert(struct x509_certificate *cert,
> -			       struct efi_signature_store *dbx)
> -{
> -	struct efi_signature_store *siglist;
> -	time64_t revoc_time;
> -	bool found = false;
> -
> -	debug("%s: Enter, %p, %p\n", __func__, dbx, cert);
> -
> -	if (!cert)
> -		return false;
> -
> -	for (siglist = dbx; siglist; siglist = siglist->next) {
> -		if (efi_search_siglist(cert, siglist, &revoc_time)) {
> -			/* TODO */
> -			/* compare signing time with revocation time */
> -
> -			found = true;
> -			break;
> -		}
> -	}
> -
> -	debug("%s: Exit, verified: %d\n", __func__, !found);
> -	return !found;
> -}
> -
> -/**
> - * efi_signature_verify_signers - verify signers' certificates with dbx
> + * efi_signature_check_signers - check revocation against all signers with dbx
>   * @msg:	Signature
> - * @dbx:	Signature database
> + * @dbx:	Revocation signature database
>   *
> - * Determine if any of signers' certificates in @msg may be verified
> - * by any of certificates in signature database pointed to by @dbx.
> - * This function is expected to be used against "dbx".
> + * Determine if none of signers' certificates in @msg are revoked
> + * by signature database pointed to by @dbx.
>   *
> - * Return:	true if none of certificates is rejected, false otherwise.
> + * Return:	true if all signers passed, false otherwise.
>   */
> -bool efi_signature_verify_signers(struct pkcs7_message *msg,
> -				  struct efi_signature_store *dbx)
> +bool efi_signature_check_signers(struct pkcs7_message *msg,
> +				 struct efi_signature_store *dbx)
>  {
> -	struct pkcs7_signed_info *info;
> -	bool found = false;
> +	struct pkcs7_signed_info *sinfo;
> +	bool revoked = false;
>
>  	debug("%s: Enter, %p, %p\n", __func__, msg, dbx);
>
> -	if (!msg)
> +	if (!msg || !dbx)
>  		goto out;
>
> -	for (info = msg->signed_infos; info; info = info->next) {
> -		if (info->signer &&
> -		    !efi_signature_verify_cert(info->signer, dbx)) {
> -			found = true;
> -			goto out;
> +	for (sinfo = msg->signed_infos; sinfo; sinfo = sinfo->next) {
> +		if (sinfo->signer &&
> +		    !efi_signature_check_revocation(sinfo, sinfo->signer,
> +						    dbx)) {
> +			revoked = true;
> +			break;
>  		}
>  	}
>  out:
> -	debug("%s: Exit, verified: %d\n", __func__, !found);
> -	return !found;
> +	debug("%s: Exit, revoked: %d\n", __func__, revoked);
> +	return !revoked;
>  }
>
>  /**
>

[PATCH 06/13] efi_loader: image_loader: verification for all signatures should pass

[AKASHI Takahiro]

Heinrich,

On Sat, May 30, 2020 at 09:01:53AM +0200, Heinrich Schuchardt wrote:
> On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> > A signed image may have multiple signatures in
> >   - each WIN_CERTIFICATE in authenticode, and/or
> >   - each SignerInfo in pkcs7 SignedData (of WIN_CERTIFICATE)
> >
> > In the initial implementation of efi_image_authenticate(), the criteria
> > of verification check for multiple signatures case is a bit ambiguous
> > and it may cause inconsistent result.
> >
> > With this patch, we will make sure that verification check in
> > efi_image_authenticate() should pass against all the signatures.
> > The only exception would be
> >   - the case where a digest algorithm used in signature is not supported by
> >     U-Boot, or
> >   - the case where parsing some portion of authenticode has failed
> > In those cases, we don't know how the signature be handled and should
> > just ignore them.
> >
> > Please note that, due to this change, efi_signature_verify_with_sigdb()'s
> > function prototype will be modified, taking "dbx" as well as "db"
> > instead of outputing a "certificate." If "dbx" is null, the behavior would
> > be the exact same as before.
> > The function's name will be changed to efi_signature_verify() once
> > current efi_signature_verify() has gone due to further improvement
> > in intermedaite certificates support.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  include/efi_loader.h              |  10 +-
> >  lib/efi_loader/efi_image_loader.c |  37 +++--
> >  lib/efi_loader/efi_signature.c    | 266 ++++++++++++++----------------
> >  3 files changed, 146 insertions(+), 167 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 9533df26dc9e..2cbd52e273d4 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -761,14 +761,12 @@ struct efi_signature_store {
> >  struct x509_certificate;
> >  struct pkcs7_message;
> >
> > -bool efi_signature_verify_cert(struct x509_certificate *cert,
> > -			       struct efi_signature_store *dbx);
> > -bool efi_signature_verify_signers(struct pkcs7_message *msg,
> > -				  struct efi_signature_store *dbx);
> >  bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
> >  				     struct pkcs7_message *msg,
> > -				  struct efi_signature_store *db,
> > -				  struct x509_certificate **cert);
> > +				     struct efi_signature_store *db,
> > +				     struct efi_signature_store *dbx);
> > +bool efi_signature_check_signers(struct pkcs7_message *msg,
> > +				 struct efi_signature_store *dbx);
> >
> >  efi_status_t efi_image_region_add(struct efi_image_regions *regs,
> >  				  const void *start, const void *end,
> > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> > index 5cdaa93519e7..33ffb43f3886 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -492,11 +492,12 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  	size_t wincerts_len;
> >  	struct pkcs7_message *msg = NULL;
> >  	struct efi_signature_store *db = NULL, *dbx = NULL;
> > -	struct x509_certificate *cert = NULL;
> >  	void *new_efi = NULL, *auth, *wincerts_end;
> >  	size_t new_efi_size, auth_size;
> >  	bool ret = false;
> >
> > +	debug("%s: Enter, %d\n", __func__, ret);
> > +
> >  	if (!efi_secure_boot_enabled())
> >  		return true;
> >
> > @@ -542,7 +543,17 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  		goto err;
> >  	}
> >
> > -	/* go through WIN_CERTIFICATE list */
> > +	/*
> > +	 * go through WIN_CERTIFICATE list
> > +	 * NOTE:
> > +	 * We may have multiple signatures either as WIN_CERTIFICATE's
> > +	 * in PE header, or as pkcs7 SignerInfo's in SignedData.
> > +	 * So the verification policy here is:
> > +	 *   - Success if, at least, one of signatures is verified
> > +	 *   - unless
> > +	 *       any of signatures is rejected explicitly, or
> 
> If one signature is good and the others are rejected (e.g. due to the
> revocation list), why won't you be happy with the one good signature? Is
> this a requirement in the UEFI spec?

Actually, I don't know the right answer.
UEFI specification doesn't say anything here and as far as I correctly
understand EDK2 code, it behaves in the same way.
Since there is no conformance test available (in UEFI SCT), it is
difficult to confirm that.
# FYI, I have been requesting such test cases in SCT since last October:
  https://bugzilla.tianocore.org/show_bug.cgi?id=2230

I also think that applying more strict rules and then easing them
later would be much easier and safer than the reverse(?).

Thanks,
-Takahiro Akashi



> Best regards
> 
> Heinrich
> 
> > +	 *       none of digest algorithms are supported
> > +	 */
> >  	for (wincert = wincerts, wincerts_end = (void *)wincerts + wincerts_len;
> >  	     (void *)wincert < wincerts_end;
> >  	     wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
> > @@ -596,37 +607,27 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  			goto err;
> >  		}
> >
> > -		if (!efi_signature_verify_signers(msg, dbx)) {
> > -			debug("Signer was rejected by \"dbx\"\n");
> > +		if (!efi_signature_check_signers(msg, dbx)) {
> > +			debug("Signer(s) in \"dbx\"\n");
> >  			goto err;
> > -		} else {
> > -			ret = true;
> >  		}
> >
> >  		/* try white-list */
> > -		if (!efi_signature_verify_with_sigdb(regs, msg, db, &cert)) {
> > -			debug("Verifying signature with \"db\" failed\n");
> > +		if (!efi_signature_verify_with_sigdb(regs, msg, db, dbx)) {
> > +			debug("Signature was not verified by \"db\"\n");
> >  			goto err;
> > -		} else {
> > -			ret = true;
> > -		}
> > -
> > -		if (!efi_signature_verify_cert(cert, dbx)) {
> > -			debug("Certificate was rejected by \"dbx\"\n");
> > -			goto err;
> > -		} else {
> > -			ret = true;
> >  		}
> >  	}
> > +	ret = true;
> >
> >  err:
> > -	x509_free_certificate(cert);
> >  	efi_sigstore_free(db);
> >  	efi_sigstore_free(dbx);
> >  	pkcs7_free_message(msg);
> >  	free(regs);
> >  	free(new_efi);
> >
> > +	debug("%s: Exit, %d\n", __func__, ret);
> >  	return ret;
> >  }
> >  #else
> > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> > index 00e442783059..ab5687040a38 100644
> > --- a/lib/efi_loader/efi_signature.c
> > +++ b/lib/efi_loader/efi_signature.c
> > @@ -300,27 +300,111 @@ out:
> >  }
> >
> >  /**
> > - * efi_signature_verify_with_sigdb - verify a signature with db
> > + * efi_signature_check_revocation - check revocation with dbx
> > + * @sinfo:	Signer's info
> > + * @cert:	x509 certificate
> > + * @dbx:	Revocation signature database
> > + *
> > + * Search revocation signature database pointed to by @dbx and find
> > + * an entry matching to certificate pointed to by @cert.
> > + *
> > + * While this entry contains revocation time, we don't support timestamp
> > + * protocol at this time and any image will be unconditionally revoked
> > + * when this match occurs.
> > + *
> > + * Return:	true if check passed, false otherwise.
> > + */
> > +static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
> > +					   struct x509_certificate *cert,
> > +					   struct efi_signature_store *dbx)
> > +{
> > +	struct efi_signature_store *siglist;
> > +	struct efi_sig_data *sig_data;
> > +	struct image_region reg[1];
> > +	void *hash = NULL;
> > +	size_t size = 0;
> > +	time64_t revoc_time;
> > +	bool revoked = false;
> > +
> > +	debug("%s: Enter, %p, %p, %p\n", __func__, sinfo, cert, dbx);
> > +
> > +	if (!sinfo || !cert || !dbx || !dbx->sig_data_list)
> > +		goto out;
> > +
> > +	debug("Checking revocation against %s\n", cert->subject);
> > +	for (siglist = dbx; siglist; siglist = siglist->next) {
> > +		if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256))
> > +			continue;
> > +
> > +		/* calculate hash of TBSCertificate */
> > +		reg[0].data = cert->tbs;
> > +		reg[0].size = cert->tbs_size;
> > +		if (!efi_hash_regions(reg, 1, &hash, &size))
> > +			goto out;
> > +
> > +		for (sig_data = siglist->sig_data_list; sig_data;
> > +		     sig_data = sig_data->next) {
> > +			/*
> > +			 * struct efi_cert_x509_sha256 {
> > +			 *	u8 tbs_hash[256/8];
> > +			 *	time64_t revocation_time;
> > +			 * };
> > +			 */
> > +#ifdef DEBUG
> > +			if (sig_data->size >= size) {
> > +				debug("hash in db:\n");
> > +				print_hex_dump("    ", DUMP_PREFIX_OFFSET,
> > +					       16, 1,
> > +					       sig_data->data, size, false);
> > +			}
> > +#endif
> > +			if ((sig_data->size < size + sizeof(time64_t)) ||
> > +			    memcmp(sig_data->data, hash, size))
> > +				continue;
> > +
> > +			memcpy(&revoc_time, sig_data->data + size,
> > +			       sizeof(revoc_time));
> > +			debug("revocation time: 0x%llx\n", revoc_time);
> > +			/*
> > +			 * TODO: compare signing timestamp in sinfo
> > +			 * with revocation time
> > +			 */
> > +
> > +			revoked = true;
> > +			free(hash);
> > +			goto out;
> > +		}
> > +		free(hash);
> > +		hash = NULL;
> > +	}
> > +out:
> > +	debug("%s: Exit, revoked: %d\n", __func__, revoked);
> > +	return !revoked;
> > +}
> > +
> > +/**
> > + * efi_signature_verify_with_sigdb - verify signatures with db and dbx
> >   * @regs:	List of regions to be authenticated
> >   * @msg:	Signature
> >   * @db:		Signature database for trusted certificates
> > - * @cert:	x509 certificate that verifies this signature
> > + * @dbx:	Revocation signature database
> >   *
> > - * Signature pointed to by @msg against image pointed to by @regs
> > - * is verified by signature database pointed to by @db.
> > + * All the signature pointed to by @msg against image pointed to by @regs
> > + * will be verified by signature database pointed to by @db and @dbx.
> >   *
> > - * Return:	true if signature is verified, false if not
> > + * Return:	true if verification for all signatures passed, false otherwise
> >   */
> >  bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
> >  				     struct pkcs7_message *msg,
> >  				     struct efi_signature_store *db,
> > -				     struct x509_certificate **cert)
> > +				     struct efi_signature_store *dbx)
> >  {
> >  	struct pkcs7_signed_info *info;
> >  	struct efi_signature_store *siglist;
> > +	struct x509_certificate *cert;
> >  	bool verified = false;
> >
> > -	debug("%s: Enter, %p, %p, %p, %p\n", __func__, regs, msg, db, cert);
> > +	debug("%s: Enter, %p, %p, %p, %p\n", __func__, regs, msg, db, dbx);
> >
> >  	if (!db)
> >  		goto out;
> > @@ -333,7 +417,7 @@ bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
> >  		debug("%s: Verify unsigned image with db\n", __func__);
> >  		for (siglist = db; siglist; siglist = siglist->next)
> >  			if (efi_signature_verify_with_list(regs, NULL, NULL,
> > -							   siglist, cert)) {
> > +							   siglist, &cert)) {
> >  				verified = true;
> >  				goto out;
> >  			}
> > @@ -349,12 +433,21 @@ bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
> >
> >  		for (siglist = db; siglist; siglist = siglist->next) {
> >  			if (efi_signature_verify_with_list(regs, msg, info,
> > -							   siglist, cert)) {
> > -				verified = true;
> > -				goto out;
> > -			}
> > +							   siglist, &cert))
> > +				break;
> >  		}
> > +		if (!siglist) {
> > +			debug("No valid certificate in \"db\"\n");
> > +			goto out;
> > +		}
> > +
> > +		if (!dbx || efi_signature_check_revocation(info, cert, dbx))
> > +			continue;
> > +
> > +		debug("Certificate in \"dbx\"\n");
> > +		goto out;
> >  	}
> > +	verified = true;
> >
> >  out:
> >  	debug("%s: Exit, verified: %d\n", __func__, verified);
> > @@ -362,150 +455,37 @@ out:
> >  }
> >
> >  /**
> > - * efi_search_siglist - search signature list for a certificate
> > - * @cert:	x509 certificate
> > - * @siglist:	Signature list
> > - * @revoc_time:	Pointer to buffer for revocation time
> > - *
> > - * Search signature list pointed to by @siglist and find a certificate
> > - * pointed to by @cert.
> > - * If found, revocation time that is specified in signature database is
> > - * returned in @revoc_time.
> > - *
> > - * Return:	true if certificate is found, false if not
> > - */
> > -static bool efi_search_siglist(struct x509_certificate *cert,
> > -			       struct efi_signature_store *siglist,
> > -			       time64_t *revoc_time)
> > -{
> > -	struct image_region reg[1];
> > -	void *hash = NULL, *msg = NULL;
> > -	struct efi_sig_data *sig_data;
> > -	bool found = false;
> > -
> > -	/* can be null */
> > -	if (!siglist->sig_data_list)
> > -		return false;
> > -
> > -	if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256)) {
> > -		/* TODO: other hash algos */
> > -		debug("Certificate's digest type is not supported: %pUl\n",
> > -		      &siglist->sig_type);
> > -		goto out;
> > -	}
> > -
> > -	/* calculate hash of TBSCertificate */
> > -	msg = calloc(1, SHA256_SUM_LEN);
> > -	if (!msg) {
> > -		debug("Out of memory\n");
> > -		goto out;
> > -	}
> > -
> > -	hash = calloc(1, SHA256_SUM_LEN);
> > -	if (!hash) {
> > -		debug("Out of memory\n");
> > -		goto out;
> > -	}
> > -
> > -	reg[0].data = cert->tbs;
> > -	reg[0].size = cert->tbs_size;
> > -	hash_calculate("sha256", reg, 1, msg);
> > -
> > -	/* go through signature list */
> > -	for (sig_data = siglist->sig_data_list; sig_data;
> > -	     sig_data = sig_data->next) {
> > -		/*
> > -		 * struct efi_cert_x509_sha256 {
> > -		 *	u8 tbs_hash[256/8];
> > -		 *	time64_t revocation_time;
> > -		 * };
> > -		 */
> > -		if ((sig_data->size >= SHA256_SUM_LEN + sizeof(time64_t)) &&
> > -		    !memcmp(sig_data->data, msg, SHA256_SUM_LEN)) {
> > -			memcpy(revoc_time, sig_data->data + SHA256_SUM_LEN,
> > -			       sizeof(*revoc_time));
> > -			debug("revocation time: 0x%llx\n", *revoc_time);
> > -			found = true;
> > -			goto out;
> > -		}
> > -	}
> > -
> > -out:
> > -	free(hash);
> > -	free(msg);
> > -
> > -	return found;
> > -}
> > -
> > -/**
> > - * efi_signature_verify_cert - verify a certificate with dbx
> > - * @cert:	x509 certificate
> > - * @dbx:	Signature database
> > - *
> > - * Search signature database pointed to by @dbx and find a certificate
> > - * pointed to by @cert.
> > - * This function is expected to be used against "dbx".
> > - *
> > - * Return:	true if a certificate is not rejected, false otherwise.
> > - */
> > -bool efi_signature_verify_cert(struct x509_certificate *cert,
> > -			       struct efi_signature_store *dbx)
> > -{
> > -	struct efi_signature_store *siglist;
> > -	time64_t revoc_time;
> > -	bool found = false;
> > -
> > -	debug("%s: Enter, %p, %p\n", __func__, dbx, cert);
> > -
> > -	if (!cert)
> > -		return false;
> > -
> > -	for (siglist = dbx; siglist; siglist = siglist->next) {
> > -		if (efi_search_siglist(cert, siglist, &revoc_time)) {
> > -			/* TODO */
> > -			/* compare signing time with revocation time */
> > -
> > -			found = true;
> > -			break;
> > -		}
> > -	}
> > -
> > -	debug("%s: Exit, verified: %d\n", __func__, !found);
> > -	return !found;
> > -}
> > -
> > -/**
> > - * efi_signature_verify_signers - verify signers' certificates with dbx
> > + * efi_signature_check_signers - check revocation against all signers with dbx
> >   * @msg:	Signature
> > - * @dbx:	Signature database
> > + * @dbx:	Revocation signature database
> >   *
> > - * Determine if any of signers' certificates in @msg may be verified
> > - * by any of certificates in signature database pointed to by @dbx.
> > - * This function is expected to be used against "dbx".
> > + * Determine if none of signers' certificates in @msg are revoked
> > + * by signature database pointed to by @dbx.
> >   *
> > - * Return:	true if none of certificates is rejected, false otherwise.
> > + * Return:	true if all signers passed, false otherwise.
> >   */
> > -bool efi_signature_verify_signers(struct pkcs7_message *msg,
> > -				  struct efi_signature_store *dbx)
> > +bool efi_signature_check_signers(struct pkcs7_message *msg,
> > +				 struct efi_signature_store *dbx)
> >  {
> > -	struct pkcs7_signed_info *info;
> > -	bool found = false;
> > +	struct pkcs7_signed_info *sinfo;
> > +	bool revoked = false;
> >
> >  	debug("%s: Enter, %p, %p\n", __func__, msg, dbx);
> >
> > -	if (!msg)
> > +	if (!msg || !dbx)
> >  		goto out;
> >
> > -	for (info = msg->signed_infos; info; info = info->next) {
> > -		if (info->signer &&
> > -		    !efi_signature_verify_cert(info->signer, dbx)) {
> > -			found = true;
> > -			goto out;
> > +	for (sinfo = msg->signed_infos; sinfo; sinfo = sinfo->next) {
> > +		if (sinfo->signer &&
> > +		    !efi_signature_check_revocation(sinfo, sinfo->signer,
> > +						    dbx)) {
> > +			revoked = true;
> > +			break;
> >  		}
> >  	}
> >  out:
> > -	debug("%s: Exit, verified: %d\n", __func__, !found);
> > -	return !found;
> > +	debug("%s: Exit, revoked: %d\n", __func__, revoked);
> > +	return !revoked;
> >  }
> >
> >  /**
> >
> 

[PATCH 07/13] efi_loader: image_loader: add digest-based verification for signed image

[AKASHI Takahiro]

In case that a type of certificate in "db" or "dbx" is
EFI_CERT_X509_SHA256_GUID, it is actually not a certificate which contains
a public key for RSA decryption, but a digest of image to be loaded.
If the value matches to a value calculated from a given binary image, it is
granted for loading.

With this patch, common digest check code, which used to be used for
unsigned image verification, will be extracted from
efi_signature_verify_with_sigdb() into efi_signature_lookup_digest(), and
extra step for digest check will be added to efi_image_authenticate().

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

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 2cbd52e273d4..30470bc35ce2 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -761,6 +761,8 @@ struct efi_signature_store {
 struct x509_certificate;
 struct pkcs7_message;
 
+bool efi_signature_lookup_digest(struct efi_image_regions *regs,
+				 struct efi_signature_store *db);
 bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
 				     struct pkcs7_message *msg,
 				     struct efi_signature_store *db,
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index 33ffb43f3886..c5849b4afebd 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -446,16 +446,16 @@ static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs)
 	}
 
 	/* try black-list first */
-	if (efi_signature_verify_with_sigdb(regs, NULL, dbx, NULL)) {
-		debug("Image is not signed and rejected by \"dbx\"\n");
+	if (efi_signature_lookup_digest(regs, dbx)) {
+		debug("Image is not signed and its digest found in \"dbx\"\n");
 		goto out;
 	}
 
 	/* try white-list */
-	if (efi_signature_verify_with_sigdb(regs, NULL, db, NULL))
+	if (efi_signature_lookup_digest(regs, db))
 		ret = true;
 	else
-		debug("Image is not signed and not found in \"db\" or \"dbx\"\n");
+		debug("Image is not signed and its digest not found in \"db\" or \"dbx\"\n");
 
 out:
 	efi_sigstore_free(db);
@@ -612,11 +612,22 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 			goto err;
 		}
 
+		if (efi_signature_lookup_digest(regs, dbx)) {
+			debug("Image's digest was found in \"dbx\"\n");
+			goto err;
+		}
+
 		/* try white-list */
-		if (!efi_signature_verify_with_sigdb(regs, msg, db, dbx)) {
-			debug("Signature was not verified by \"db\"\n");
-			goto err;
-		}
+		if (efi_signature_verify_with_sigdb(regs, msg, db, dbx))
+			continue;
+
+		debug("Signature was not verified by \"db\"\n");
+
+		if (efi_signature_lookup_digest(regs, db))
+			continue;
+
+		debug("Image's digest was not found in \"db\" or \"dbx\"\n");
+		goto err;
 	}
 	ret = true;
 
diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index ab5687040a38..d2ded5cfac78 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -196,6 +196,68 @@ out:
 	return verified;
 }
 
+/**
+ * efi_signature_lookup_digest - search for an image's digest in sigdb
+ * @regs:	List of regions to be authenticated
+ * @db:		Signature database for trusted certificates
+ *
+ * A message digest of image pointed to by @regs is calculated and
+ * its hash value is compared to entries in signature database pointed
+ * to by @db.
+ *
+ * Return:	true if found, false if not
+ */
+bool efi_signature_lookup_digest(struct efi_image_regions *regs,
+				 struct efi_signature_store *db)
+{
+	struct efi_signature_store *siglist;
+	struct efi_sig_data *sig_data;
+	void *hash = NULL;
+	size_t size = 0;
+	bool found = false;
+
+	debug("%s: Enter, %p, %p\n", __func__, regs, db);
+
+	if (!regs || !db || !db->sig_data_list)
+		goto out;
+
+	for (siglist = db; siglist; siglist = siglist->next) {
+		/* TODO: support other hash algorithms */
+		if (guidcmp(&siglist->sig_type, &efi_guid_sha256)) {
+			debug("Digest algorithm is not supported: %pUl\n",
+			      &siglist->sig_type);
+			break;
+		}
+
+		if (!efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
+			debug("Digesting an image failed\n");
+			break;
+		}
+
+		for (sig_data = siglist->sig_data_list; sig_data;
+		     sig_data = sig_data->next) {
+#ifdef DEBUG
+			debug("Msg digest in database:\n");
+			print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
+				       sig_data->data, sig_data->size, false);
+#endif
+			if (sig_data->size == size &&
+			    !memcmp(sig_data->data, hash, size)) {
+				found = true;
+				free(hash);
+				goto out;
+			}
+		}
+
+		free(hash);
+		hash = NULL;
+	}
+
+out:
+	debug("%s: Exit, found: %d\n", __func__, found);
+	return found;
+}
+
 /**
  * efi_signature_verify_with_list - verify a signature with signature list
  * @regs:		List of regions to be authenticated
@@ -225,46 +287,6 @@ bool efi_signature_verify_with_list(struct efi_image_regions *regs,
 	debug("%s: Enter, %p, %p, %p, %p\n", __func__,
 	      regs, signed_info, siglist, valid_cert);
 
-	if (!signed_info) {
-		void *hash = NULL;
-		size_t size;
-
-		debug("%s: unsigned image\n", __func__);
-		/*
-		 * verify based on calculated hash value
-		 * TODO: support other hash algorithms
-		 */
-		if (guidcmp(&siglist->sig_type, &efi_guid_sha256)) {
-			debug("Digest algorithm is not supported: %pUl\n",
-			      &siglist->sig_type);
-			goto out;
-		}
-
-		if (!efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
-			debug("Digesting unsigned image failed\n");
-			goto out;
-		}
-
-		/* go through the list */
-		for (sig_data = siglist->sig_data_list; sig_data;
-		     sig_data = sig_data->next) {
-#ifdef DEBUG
-			debug("Msg digest in database:\n");
-			print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
-				       sig_data->data, sig_data->size, false);
-#endif
-			if ((sig_data->size == size) &&
-			    !memcmp(sig_data->data, hash, size)) {
-				verified = true;
-				free(hash);
-				goto out;
-			}
-		}
-		free(hash);
-		goto out;
-	}
-
-	debug("%s: signed image\n", __func__);
 	if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509)) {
 		debug("Signature type is not supported: %pUl\n",
 		      &siglist->sig_type);
@@ -406,27 +428,9 @@ bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
 
 	debug("%s: Enter, %p, %p, %p, %p\n", __func__, regs, msg, db, dbx);
 
-	if (!db)
+	if (!regs || !msg || !db || !db->sig_data_list)
 		goto out;
 
-	if (!db->sig_data_list)
-		goto out;
-
-	/* for unsigned image */
-	if (!msg) {
-		debug("%s: Verify unsigned image with db\n", __func__);
-		for (siglist = db; siglist; siglist = siglist->next)
-			if (efi_signature_verify_with_list(regs, NULL, NULL,
-							   siglist, &cert)) {
-				verified = true;
-				goto out;
-			}
-
-		goto out;
-	}
-
-	/* for signed image or variable */
-	debug("%s: Verify signed image with db\n", __func__);
 	for (info = msg->signed_infos; info; info = info->next) {
 		debug("Signed Info: digest algo: %s, pkey algo: %s\n",
 		      info->sig->hash_algo, info->sig->pkey_algo);
-- 
2.25.2

[PATCH 07/13] efi_loader: image_loader: add digest-based verification for signed image

[Heinrich Schuchardt]

On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> In case that a type of certificate in "db" or "dbx" is
> EFI_CERT_X509_SHA256_GUID, it is actually not a certificate which contains
> a public key for RSA decryption, but a digest of image to be loaded.
> If the value matches to a value calculated from a given binary image, it is
> granted for loading.
>
> With this patch, common digest check code, which used to be used for
> unsigned image verification, will be extracted from
> efi_signature_verify_with_sigdb() into efi_signature_lookup_digest(), and
> extra step for digest check will be added to efi_image_authenticate().

Could you, please, add comments in the code describing this process flow.

Best regards

Heinrich

[PATCH 07/13] efi_loader: image_loader: add digest-based verification for signed image

[AKASHI Takahiro]

Heinrich,

On Sat, May 30, 2020 at 09:09:30AM +0200, Heinrich Schuchardt wrote:
> On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> > In case that a type of certificate in "db" or "dbx" is
> > EFI_CERT_X509_SHA256_GUID, it is actually not a certificate which contains
> > a public key for RSA decryption, but a digest of image to be loaded.
> > If the value matches to a value calculated from a given binary image, it is
> > granted for loading.
> >
> > With this patch, common digest check code, which used to be used for
> > unsigned image verification, will be extracted from
> > efi_signature_verify_with_sigdb() into efi_signature_lookup_digest(), and
> > extra step for digest check will be added to efi_image_authenticate().
> 
> Could you, please, add comments in the code describing this process flow.

All the necessary code is contained in efi_signature_lookup_digest(),
but I'll add some comments in efi_image_authenticate().

Thanks,
-Takahiro Akashi


> Best regards
> 
> Heinrich

[PATCH 08/13] test/py: efi_secboot: remove all "re.search"

[AKASHI Takahiro]

Currently, we don't use any regular expression in matching outputs from
U-Boot. Since its use is just redundant, we can remove all.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 .../py/tests/test_efi_secboot/test_authvar.py | 73 +++++++++----------
 test/py/tests/test_efi_secboot/test_signed.py | 34 ++++-----
 .../tests/test_efi_secboot/test_unsigned.py   | 32 ++++----
 3 files changed, 65 insertions(+), 74 deletions(-)

diff --git a/test/py/tests/test_efi_secboot/test_authvar.py b/test/py/tests/test_efi_secboot/test_authvar.py
index 55dcaa95f1ea..766ed9283605 100644
--- a/test/py/tests/test_efi_secboot/test_authvar.py
+++ b/test/py/tests/test_efi_secboot/test_authvar.py
@@ -9,7 +9,6 @@ This test verifies variable authentication
 """
 
 import pytest
-import re
 from defs import *
 
 @pytest.mark.boardspec('sandbox')
@@ -40,7 +39,7 @@ class TestEfiAuthVar(object):
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 PK.auth',
                 'setenv -e -nv -bs -rt -i 4000000,$filesize PK'])
-            assert(re.search('Failed to set EFI variable', ''.join(output)))
+            assert('Failed to set EFI variable' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 1c'):
             # Test Case 1c, install PK
@@ -48,7 +47,7 @@ class TestEfiAuthVar(object):
                 'fatload host 0:1 4000000 PK.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize PK',
                 'printenv -e -n PK'])
-            assert(re.search('PK:', ''.join(output)))
+            assert('PK:' in ''.join(output))
 
             output = u_boot_console.run_command(
                 'printenv -e SecureBoot')
@@ -62,25 +61,25 @@ class TestEfiAuthVar(object):
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 db.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize db'])
-            assert(re.search('Failed to set EFI variable', ''.join(output)))
+            assert('Failed to set EFI variable' in ''.join(output))
 
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 db.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize dbx'])
-            assert(re.search('Failed to set EFI variable', ''.join(output)))
+            assert('Failed to set EFI variable' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 1e'):
             # Test Case 1e, install KEK
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 KEK.auth',
                 'setenv -e -nv -bs -rt -i 4000000,$filesize KEK'])
-            assert(re.search('Failed to set EFI variable', ''.join(output)))
+            assert('Failed to set EFI variable' in ''.join(output))
 
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 KEK.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize KEK',
                 'printenv -e -n KEK'])
-            assert(re.search('KEK:', ''.join(output)))
+            assert('KEK:' in ''.join(output))
 
             output = u_boot_console.run_command(
                 'printenv -e SecureBoot')
@@ -91,14 +90,14 @@ class TestEfiAuthVar(object):
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 db.auth',
                 'setenv -e -nv -bs -rt -i 4000000,$filesize db'])
-            assert(re.search('Failed to set EFI variable', ''.join(output)))
+            assert('Failed to set EFI variable' in ''.join(output))
 
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 db.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize db',
                 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db'])
-            assert(not re.search('Failed to set EFI variable', ''.join(output)))
-            assert(re.search('db:', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            assert('db:' in ''.join(output))
 
             output = u_boot_console.run_command(
                 'printenv -e SecureBoot')
@@ -109,14 +108,14 @@ class TestEfiAuthVar(object):
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 db.auth',
                 'setenv -e -nv -bs -rt -i 4000000,$filesize db'])
-            assert(re.search('Failed to set EFI variable', ''.join(output)))
+            assert('Failed to set EFI variable' in ''.join(output))
 
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 db.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize db',
                 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db'])
-            assert(not re.search('Failed to set EFI variable', ''.join(output)))
-            assert(re.search('db:', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            assert('db:' in ''.join(output))
 
             output = u_boot_console.run_command(
                 'printenv -e SecureBoot')
@@ -139,20 +138,20 @@ class TestEfiAuthVar(object):
                 'fatload host 0:1 4000000 db.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize db',
                 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db'])
-            assert(not re.search('Failed to set EFI variable', ''.join(output)))
-            assert(re.search('db:', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            assert('db:' in ''.join(output))
 
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 db1.auth',
                 'setenv -e -nv -bs -rt -i 4000000,$filesize db'])
-            assert(re.search('Failed to set EFI variable', ''.join(output)))
+            assert('Failed to set EFI variable' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 2b'):
             # Test Case 2b, update without correct signature
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 db.esl',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize db'])
-            assert(re.search('Failed to set EFI variable', ''.join(output)))
+            assert('Failed to set EFI variable' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 2c'):
             # Test Case 2c, update with correct signature
@@ -160,8 +159,8 @@ class TestEfiAuthVar(object):
                 'fatload host 0:1 4000000 db1.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize db',
                 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db'])
-            assert(not re.search('Failed to set EFI variable', ''.join(output)))
-            assert(re.search('db:', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            assert('db:' in ''.join(output))
 
     def test_efi_var_auth3(self, u_boot_console, efi_boot_env):
         """
@@ -180,20 +179,20 @@ class TestEfiAuthVar(object):
                 'fatload host 0:1 4000000 db.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize db',
                 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db'])
-            assert(not re.search('Failed to set EFI variable', ''.join(output)))
-            assert(re.search('db:', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            assert('db:' in ''.join(output))
 
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 db1.auth',
                 'setenv -e -nv -bs -rt -a -i 4000000,$filesize db'])
-            assert(re.search('Failed to set EFI variable', ''.join(output)))
+            assert('Failed to set EFI variable' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 3b'):
             # Test Case 3b, update without correct signature
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 db.esl',
                 'setenv -e -nv -bs -rt -at -a -i 4000000,$filesize db'])
-            assert(re.search('Failed to set EFI variable', ''.join(output)))
+            assert('Failed to set EFI variable' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 3c'):
             # Test Case 3c, update with correct signature
@@ -201,8 +200,8 @@ class TestEfiAuthVar(object):
                 'fatload host 0:1 4000000 db1.auth',
                 'setenv -e -nv -bs -rt -at -a -i 4000000,$filesize db',
                 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db'])
-            assert(not re.search('Failed to set EFI variable', ''.join(output)))
-            assert(re.search('db:', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            assert('db:' in ''.join(output))
 
     def test_efi_var_auth4(self, u_boot_console, efi_boot_env):
         """
@@ -221,22 +220,22 @@ class TestEfiAuthVar(object):
                 'fatload host 0:1 4000000 db.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize db',
                 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db'])
-            assert(not re.search('Failed to set EFI variable', ''.join(output)))
-            assert(re.search('db:', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            assert('db:' in ''.join(output))
 
             output = u_boot_console.run_command_list([
                 'setenv -e -nv -bs -rt db',
                 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db'])
-            assert(re.search('Failed to set EFI variable', ''.join(output)))
-            assert(re.search('db:', ''.join(output)))
+            assert('Failed to set EFI variable' in ''.join(output))
+            assert('db:' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 4b'):
             # Test Case 4b, update without correct signature/data
             output = u_boot_console.run_command_list([
                 'setenv -e -nv -bs -rt -at db',
                 'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db'])
-            assert(re.search('Failed to set EFI variable', ''.join(output)))
-            assert(re.search('db:', ''.join(output)))
+            assert('Failed to set EFI variable' in ''.join(output))
+            assert('db:' in ''.join(output))
 
     def test_efi_var_auth5(self, u_boot_console, efi_boot_env):
         """
@@ -255,15 +254,15 @@ class TestEfiAuthVar(object):
                 'fatload host 0:1 4000000 db.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize db',
                 'printenv -e -n PK'])
-            assert(not re.search('Failed to set EFI variable', ''.join(output)))
-            assert(re.search('PK:', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            assert('PK:' in ''.join(output))
 
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 PK_null.esl',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize PK',
                 'printenv -e -n PK'])
-            assert(re.search('Failed to set EFI variable', ''.join(output)))
-            assert(re.search('PK:', ''.join(output)))
+            assert('Failed to set EFI variable' in ''.join(output))
+            assert('PK:' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 5b'):
             # Test Case 5b, Uninstall PK with correct signature
@@ -271,8 +270,8 @@ class TestEfiAuthVar(object):
                 'fatload host 0:1 4000000 PK_null.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize PK',
                 'printenv -e -n PK'])
-            assert(not re.search('Failed to set EFI variable', ''.join(output)))
-            assert(re.search('\"PK\" not defined', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            assert('\"PK\" not defined' in ''.join(output))
 
             output = u_boot_console.run_command(
                 'printenv -e SecureBoot')
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
index 584282b338bc..19d78b1b64e0 100644
--- a/test/py/tests/test_efi_secboot/test_signed.py
+++ b/test/py/tests/test_efi_secboot/test_signed.py
@@ -9,7 +9,6 @@ This test verifies image authentication for signed images.
 """
 
 import pytest
-import re
 from defs import *
 
 @pytest.mark.boardspec('sandbox')
@@ -32,7 +31,7 @@ class TestEfiSignedImage(object):
                 'efidebug boot add 1 HELLO1 host 0:1 /helloworld.efi.signed ""',
                 'efidebug boot next 1',
                 'bootefi bootmgr'])
-            assert(re.search('Hello, world!', ''.join(output)))
+            assert('Hello, world!' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 1b'):
             # Test Case 1b, run unsigned image if no db/dbx
@@ -40,7 +39,7 @@ class TestEfiSignedImage(object):
                 'efidebug boot add 2 HELLO2 host 0:1 /helloworld.efi ""',
                 'efidebug boot next 2',
                 'bootefi bootmgr'])
-            assert(re.search('Hello, world!', ''.join(output)))
+            assert('Hello, world!' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 1c'):
             # Test Case 1c, not authenticated by db
@@ -51,24 +50,23 @@ class TestEfiSignedImage(object):
                 '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(not re.search('Failed to set EFI variable', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
             output = u_boot_console.run_command_list([
                 'efidebug boot next 2',
                 'bootefi bootmgr'])
-            assert(re.search('\'HELLO2\' failed', ''.join(output)))
+            assert('\'HELLO2\' failed' in ''.join(output))
             output = u_boot_console.run_command_list([
                 'efidebug boot next 2',
                 'efidebug test bootmgr'])
-            assert(re.search('efi_start_image[(][)] returned: 26',
-                ''.join(output)))
-            assert(not re.search('Hello, world!', ''.join(output)))
+            assert('efi_start_image() returned: 26' in ''.join(output))
+            assert(not 'Hello, world!' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 1d'):
             # Test Case 1d, authenticated by db
             output = u_boot_console.run_command_list([
                 'efidebug boot next 1',
                 'bootefi bootmgr'])
-            assert(re.search('Hello, world!', ''.join(output)))
+            assert('Hello, world!' in ''.join(output))
 
     def test_efi_signed_image_auth2(self, u_boot_console, efi_boot_env):
         """
@@ -86,32 +84,30 @@ class TestEfiSignedImage(object):
                 '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(not re.search('Failed to set EFI variable', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
             output = u_boot_console.run_command_list([
                 'efidebug boot add 1 HELLO host 0:1 /helloworld.efi.signed ""',
                 'efidebug boot next 1',
                 'bootefi bootmgr'])
-            assert(re.search('\'HELLO\' failed', ''.join(output)))
+            assert('\'HELLO\' failed' in ''.join(output))
             output = u_boot_console.run_command_list([
                 'efidebug boot next 1',
                 'efidebug test bootmgr'])
-            assert(re.search('efi_start_image[(][)] returned: 26',
-                ''.join(output)))
-            assert(not re.search('Hello, world!', ''.join(output)))
+            assert('efi_start_image() returned: 26' in ''.join(output))
+            assert(not 'Hello, world!' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 2b'):
             # Test Case 2b, rejected by dbx even if db allows
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 db.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize db'])
-            assert(not re.search('Failed to set EFI variable', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
             output = u_boot_console.run_command_list([
                 'efidebug boot next 1',
                 'bootefi bootmgr'])
-            assert(re.search('\'HELLO\' failed', ''.join(output)))
+            assert('\'HELLO\' failed' in ''.join(output))
             output = u_boot_console.run_command_list([
                 'efidebug boot next 1',
                 'efidebug test bootmgr'])
-            assert(re.search('efi_start_image[(][)] returned: 26',
-                ''.join(output)))
-            assert(not re.search('Hello, world!', ''.join(output)))
+            assert('efi_start_image() returned: 26' in ''.join(output))
+            assert(not 'Hello, world!' in ''.join(output))
diff --git a/test/py/tests/test_efi_secboot/test_unsigned.py b/test/py/tests/test_efi_secboot/test_unsigned.py
index 22d849afb89b..c42c5ddc4774 100644
--- a/test/py/tests/test_efi_secboot/test_unsigned.py
+++ b/test/py/tests/test_efi_secboot/test_unsigned.py
@@ -9,7 +9,6 @@ This test verifies image authentication for unsigned images.
 """
 
 import pytest
-import re
 from defs import *
 
 @pytest.mark.boardspec('sandbox')
@@ -33,19 +32,18 @@ class TestEfiUnsignedImage(object):
 		'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(not re.search('Failed to set EFI variable', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
 
             output = u_boot_console.run_command_list([
                 'efidebug boot add 1 HELLO host 0:1 /helloworld.efi ""',
                 'efidebug boot next 1',
                 'bootefi bootmgr'])
-            assert(re.search('\'HELLO\' failed', ''.join(output)))
+            assert('\'HELLO\' failed' in ''.join(output))
             output = u_boot_console.run_command_list([
                 'efidebug boot next 1',
                 'efidebug test bootmgr'])
-            assert(re.search('efi_start_image[(][)] returned: 26',
-                ''.join(output)))
-            assert(not re.search('Hello, world!', ''.join(output)))
+            assert('efi_start_image() returned: 26' in ''.join(output))
+            assert(not 'Hello, world!' in ''.join(output))
 
     def test_efi_unsigned_image_auth2(self, u_boot_console, efi_boot_env):
         """
@@ -63,13 +61,13 @@ class TestEfiUnsignedImage(object):
 		'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(not re.search('Failed to set EFI variable', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
 
             output = u_boot_console.run_command_list([
                 'efidebug boot add 1 HELLO host 0:1 /helloworld.efi ""',
                 'efidebug boot next 1',
                 'bootefi bootmgr'])
-            assert(re.search('Hello, world!', ''.join(output)))
+            assert('Hello, world!' in ''.join(output))
 
     def test_efi_unsigned_image_auth3(self, u_boot_console, efi_boot_env):
         """
@@ -87,35 +85,33 @@ class TestEfiUnsignedImage(object):
 		'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(not re.search('Failed to set EFI variable', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
 
             output = u_boot_console.run_command_list([
                 'efidebug boot add 1 HELLO host 0:1 /helloworld.efi ""',
                 'efidebug boot next 1',
                 'bootefi bootmgr'])
-            assert(re.search('\'HELLO\' failed', ''.join(output)))
+            assert('\'HELLO\' failed' in ''.join(output))
             output = u_boot_console.run_command_list([
                 'efidebug boot next 1',
                 'efidebug test bootmgr'])
-            assert(re.search('efi_start_image[(][)] returned: 26',
-                ''.join(output)))
-            assert(not re.search('Hello, world!', ''.join(output)))
+            assert('efi_start_image() returned: 26' in ''.join(output))
+            assert(not 'Hello, world!' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 3b'):
             # Test Case 3b, rejected by dbx even if db allows
             output = u_boot_console.run_command_list([
 		'fatload host 0:1 4000000 db_hello.auth',
 		'setenv -e -nv -bs -rt -at -i 4000000,$filesize db'])
-            assert(not re.search('Failed to set EFI variable', ''.join(output)))
+            assert(not 'Failed to set EFI variable' in ''.join(output))
 
             output = u_boot_console.run_command_list([
                 'efidebug boot add 1 HELLO host 0:1 /helloworld.efi ""',
                 'efidebug boot next 1',
                 'bootefi bootmgr'])
-            assert(re.search('\'HELLO\' failed', ''.join(output)))
+            assert('\'HELLO\' failed' in ''.join(output))
             output = u_boot_console.run_command_list([
                 'efidebug boot next 1',
                 'efidebug test bootmgr'])
-            assert(re.search('efi_start_image[(][)] returned: 26',
-                ''.join(output)))
-            assert(not re.search('Hello, world!', ''.join(output)))
+            assert('efi_start_image() returned: 26' in ''.join(output))
+            assert(not 'Hello, world!' in ''.join(output))
-- 
2.25.2

[PATCH 08/13] test/py: efi_secboot: remove all "re.search"

[Heinrich Schuchardt]

On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> Currently, we don't use any regular expression in matching outputs from
> U-Boot. Since its use is just redundant, we can remove all.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Applying: test/py: efi_secboot: remove all "re.search"
error: patch failed: test/py/tests/test_efi_secboot/test_signed.py:32
error: test/py/tests/test_efi_secboot/test_signed.py: patch does not apply
error: patch failed: test/py/tests/test_efi_secboot/test_unsigned.py:33
error: test/py/tests/test_efi_secboot/test_unsigned.py: patch does not apply
Patch failed at 0001 test/py: efi_secboot: remove all "re.search"

Could you, please, rebase your patches.

Best regards

Heinrich

[PATCH 08/13] test/py: efi_secboot: remove all "re.search"

[AKASHI Takahiro]

On Sat, May 30, 2020 at 09:04:11AM +0200, Heinrich Schuchardt wrote:
> On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> > Currently, we don't use any regular expression in matching outputs from
> > U-Boot. Since its use is just redundant, we can remove all.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> Applying: test/py: efi_secboot: remove all "re.search"
> error: patch failed: test/py/tests/test_efi_secboot/test_signed.py:32
> error: test/py/tests/test_efi_secboot/test_signed.py: patch does not apply
> error: patch failed: test/py/tests/test_efi_secboot/test_unsigned.py:33
> error: test/py/tests/test_efi_secboot/test_unsigned.py: patch does not apply
> Patch failed at 0001 test/py: efi_secboot: remove all "re.search"
> 
> Could you, please, rebase your patches.

Probably due to the commit,
===
commit 5827c2545849
Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
Date:   Mon May 4 12:21:51 2020 +0200

    test: stabilize test_efi_secboot
===

I have already proposed to revert this patch in
https://lists.denx.de/pipermail/u-boot/2020-May/412859.html

Anyway,
the essential issue is not my test, but efi_console which
tries to unconditionally send control code which a terminal may not
understand.

-Takahiro Akashi


> Best regards
> 
> Heinrich

[PATCH 08/13] test/py: efi_secboot: remove all "re.search"

[Heinrich Schuchardt]

On 02.06.20 07:58, AKASHI Takahiro wrote:
> On Sat, May 30, 2020 at 09:04:11AM +0200, Heinrich Schuchardt wrote:
>> On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
>>> Currently, we don't use any regular expression in matching outputs from
>>> U-Boot. Since its use is just redundant, we can remove all.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>
>> Applying: test/py: efi_secboot: remove all "re.search"
>> error: patch failed: test/py/tests/test_efi_secboot/test_signed.py:32
>> error: test/py/tests/test_efi_secboot/test_signed.py: patch does not apply
>> error: patch failed: test/py/tests/test_efi_secboot/test_unsigned.py:33
>> error: test/py/tests/test_efi_secboot/test_unsigned.py: patch does not apply
>> Patch failed at 0001 test/py: efi_secboot: remove all "re.search"
>>
>> Could you, please, rebase your patches.
>
> Probably due to the commit,
> ===
> commit 5827c2545849
> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Date:   Mon May 4 12:21:51 2020 +0200
>
>     test: stabilize test_efi_secboot
> ===
>
> I have already proposed to revert this patch in
> https://lists.denx.de/pipermail/u-boot/2020-May/412859.html
>
> Anyway,
> the essential issue is not my test, but efi_console which
> tries to unconditionally send control code which a terminal may not
> understand.

When working via the serial console I am not aware of any other
possibility to discover the screen size which is needed for correct
scrolling.

The relevant specification is ECMA-48 with the following chapters:

8.3.14 CPR - ACTIVE POSITION REPORT
8.3.21 CUP - CURSOR POSITION
8.3.35 DSR - DEVICE STATUS REPORT

The other escape sequences used are described in the Linux man page
console_codes.4
(https://www.man7.org/linux/man-pages/man4/console_codes.4.html).

As long as you use a Linux console or xterm or a VT100 compatible
terminal you can expect the control codes to work.

If the environment variable stdout has the value "vidconsole", the
escape sequences are not issued.

We need a solution for the Python tests that will not break Gitlab CI
and Travis CI.

Best regards

Heinrich

[PATCH 08/13] test/py: efi_secboot: remove all "re.search"

[Heinrich Schuchardt]

On 29.05.20 08:41, AKASHI Takahiro wrote:
> Currently, we don't use any regular expression in matching outputs from
> U-Boot. Since its use is just redundant, we can remove all.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.gpk@gmx.de>

[PATCH 09/13] test/py: efi_secboot: fix test case 1g of test_authvar

[AKASHI Takahiro]

In the test case (1g) of test_authvar, "db" is mistakenly used,
and it ends up being the exact same as (1f).
So correct it as "dbx" test case.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 test/py/tests/test_efi_secboot/test_authvar.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/test/py/tests/test_efi_secboot/test_authvar.py b/test/py/tests/test_efi_secboot/test_authvar.py
index 766ed9283605..148aa3123e4f 100644
--- a/test/py/tests/test_efi_secboot/test_authvar.py
+++ b/test/py/tests/test_efi_secboot/test_authvar.py
@@ -106,16 +106,16 @@ class TestEfiAuthVar(object):
         with u_boot_console.log.section('Test Case 1g'):
             # Test Case 1g, install dbx
             output = u_boot_console.run_command_list([
-                'fatload host 0:1 4000000 db.auth',
-                'setenv -e -nv -bs -rt -i 4000000,$filesize db'])
+                'fatload host 0:1 4000000 dbx.auth',
+                'setenv -e -nv -bs -rt -i 4000000,$filesize dbx'])
             assert('Failed to set EFI variable' in ''.join(output))
 
             output = u_boot_console.run_command_list([
-                'fatload host 0:1 4000000 db.auth',
-                'setenv -e -nv -bs -rt -at -i 4000000,$filesize db',
-                'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db'])
+                'fatload host 0:1 4000000 dbx.auth',
+                'setenv -e -nv -bs -rt -at -i 4000000,$filesize dbx',
+                'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f dbx'])
             assert(not 'Failed to set EFI variable' in ''.join(output))
-            assert('db:' in ''.join(output))
+            assert('dbx:' in ''.join(output))
 
             output = u_boot_console.run_command(
                 'printenv -e SecureBoot')
-- 
2.25.2

[PATCH 09/13] test/py: efi_secboot: fix test case 1g of test_authvar

[Heinrich Schuchardt]

On 29.05.20 08:41, AKASHI Takahiro wrote:
> In the test case (1g) of test_authvar, "db" is mistakenly used,
> and it ends up being the exact same as (1f).
> So correct it as "dbx" test case.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[PATCH 10/13] test/py: efi_secboot: split "signed image" test case-1 into two cases

[AKASHI Takahiro]

Split the existing test case-1 into case1 and a new case-2:
case-1 for non-SecureBoot mode; case-2 for SecureBoot mode.

In addition, one corner case is added to case-2; a image is signed
but a corresponding certificate is not yet installed in "db."

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 test/py/tests/test_efi_secboot/test_signed.py | 98 ++++++++++---------
 1 file changed, 54 insertions(+), 44 deletions(-)

diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
index 19d78b1b64e0..5267b7ab4e86 100644
--- a/test/py/tests/test_efi_secboot/test_signed.py
+++ b/test/py/tests/test_efi_secboot/test_signed.py
@@ -20,12 +20,12 @@ from defs import *
 class TestEfiSignedImage(object):
     def test_efi_signed_image_auth1(self, u_boot_console, efi_boot_env):
         """
-        Test Case 1 - authenticated by db
+        Test Case 1 - Secure boot is not in force
         """
         u_boot_console.restart_uboot()
         disk_img = efi_boot_env
         with u_boot_console.log.section('Test Case 1a'):
-            # Test Case 1a, run signed image if no db/dbx
+            # Test Case 1a, run signed image if no PK
             output = u_boot_console.run_command_list([
                 'host bind 0 %s' % disk_img,
                 'efidebug boot add 1 HELLO1 host 0:1 /helloworld.efi.signed ""',
@@ -34,48 +34,66 @@ class TestEfiSignedImage(object):
             assert('Hello, world!' in ''.join(output))
 
         with u_boot_console.log.section('Test Case 1b'):
-            # Test Case 1b, run unsigned image if no db/dbx
+            # Test Case 1b, run unsigned image if no PK
             output = u_boot_console.run_command_list([
                 'efidebug boot add 2 HELLO2 host 0:1 /helloworld.efi ""',
                 'efidebug boot next 2',
                 'bootefi bootmgr'])
             assert('Hello, world!' in ''.join(output))
 
-        with u_boot_console.log.section('Test Case 1c'):
-            # Test Case 1c, not authenticated by db
-            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(not 'Failed to set EFI variable' in ''.join(output))
-            output = u_boot_console.run_command_list([
-                'efidebug boot next 2',
-                'bootefi bootmgr'])
-            assert('\'HELLO2\' failed' in ''.join(output))
-            output = u_boot_console.run_command_list([
-                'efidebug boot next 2',
-                'efidebug test bootmgr'])
-            assert('efi_start_image() returned: 26' in ''.join(output))
-            assert(not 'Hello, world!' in ''.join(output))
-
-        with u_boot_console.log.section('Test Case 1d'):
-            # Test Case 1d, authenticated by db
-            output = u_boot_console.run_command_list([
-                'efidebug boot next 1',
-                'bootefi bootmgr'])
-            assert('Hello, world!' in ''.join(output))
-
     def test_efi_signed_image_auth2(self, u_boot_console, efi_boot_env):
         """
-        Test Case 2 - rejected by dbx
+        Test Case 2 - Secure boot is in force,
+                      authenticated by db (TEST_db certificate in db)
         """
         u_boot_console.restart_uboot()
         disk_img = efi_boot_env
         with u_boot_console.log.section('Test Case 2a'):
-            # Test Case 2a, rejected by dbx
+            # Test Case 2a, db is not yet installed
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % disk_img,
+                '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(not 'Failed to set EFI variable' in ''.join(output))
+            output = u_boot_console.run_command_list([
+                'efidebug boot add 1 HELLO1 host 0:1 /helloworld.efi.signed ""',
+                'efidebug boot next 1',
+                'efidebug test bootmgr'])
+            assert('\'HELLO1\' failed' in ''.join(output))
+            assert('efi_start_image() returned: 26' in ''.join(output))
+            output = u_boot_console.run_command_list([
+                'efidebug boot add 2 HELLO2 host 0:1 /helloworld.efi ""',
+                'efidebug boot next 2',
+                'efidebug test bootmgr'])
+            assert('\'HELLO2\' failed' in ''.join(output))
+            assert('efi_start_image() returned: 26' in ''.join(output))
+
+        with u_boot_console.log.section('Test Case 2b'):
+            # Test Case 2b, authenticated by db
+            output = u_boot_console.run_command_list([
+                'fatload host 0:1 4000000 db.auth',
+                'setenv -e -nv -bs -rt -at -i 4000000,$filesize db'])
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            output = u_boot_console.run_command_list([
+                'efidebug boot next 2',
+                'efidebug test bootmgr'])
+            assert('\'HELLO2\' failed' in ''.join(output))
+            assert('efi_start_image() returned: 26' in ''.join(output))
+            output = u_boot_console.run_command_list([
+                'efidebug boot next 1',
+                'bootefi bootmgr'])
+            assert('Hello, world!' in ''.join(output))
+
+    def test_efi_signed_image_auth3(self, u_boot_console, efi_boot_env):
+        """
+        Test Case 3 - rejected by dbx (TEST_db certificate in dbx)
+        """
+        u_boot_console.restart_uboot()
+        disk_img = efi_boot_env
+        with u_boot_console.log.section('Test Case 3a'):
+            # Test Case 3a, rejected by dbx
             output = u_boot_console.run_command_list([
                 'host bind 0 %s' % disk_img,
                 'fatload host 0:1 4000000 db.auth',
@@ -87,27 +105,19 @@ class TestEfiSignedImage(object):
             assert(not 'Failed to set EFI variable' in ''.join(output))
             output = u_boot_console.run_command_list([
                 'efidebug boot add 1 HELLO host 0:1 /helloworld.efi.signed ""',
-                'efidebug boot next 1',
-                'bootefi bootmgr'])
-            assert('\'HELLO\' failed' in ''.join(output))
-            output = u_boot_console.run_command_list([
                 'efidebug boot next 1',
                 'efidebug test bootmgr'])
+            assert('\'HELLO\' failed' in ''.join(output))
             assert('efi_start_image() returned: 26' in ''.join(output))
-            assert(not 'Hello, world!' in ''.join(output))
 
-        with u_boot_console.log.section('Test Case 2b'):
-            # Test Case 2b, rejected by dbx even if db allows
+        with u_boot_console.log.section('Test Case 3b'):
+            # Test Case 3b, rejected by dbx even if db allows
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 db.auth',
                 'setenv -e -nv -bs -rt -at -i 4000000,$filesize db'])
             assert(not 'Failed to set EFI variable' in ''.join(output))
-            output = u_boot_console.run_command_list([
-                'efidebug boot next 1',
-                'bootefi bootmgr'])
-            assert('\'HELLO\' failed' in ''.join(output))
             output = u_boot_console.run_command_list([
                 'efidebug boot next 1',
                 'efidebug test bootmgr'])
+            assert('\'HELLO\' failed' in ''.join(output))
             assert('efi_start_image() returned: 26' in ''.join(output))
-            assert(not 'Hello, world!' in ''.join(output))
-- 
2.25.2

[PATCH 11/13] test/py: efi_secboot: add a test against certificate revocation

[AKASHI Takahiro]

Revocation database (dbx) may have not only certificates, but also
message digests of certificates with revocation time
(EFI_CERT_X509_SHA256_GUILD).

In this test case, if the database has such a digest and if the value
matches to a certificate that created a given image's signature,
authentication should fail.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 test/py/tests/test_efi_secboot/conftest.py    |  6 ++++-
 test/py/tests/test_efi_secboot/test_signed.py | 26 +++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
index 5d99b8b7189e..13687a2da1a6 100644
--- a/test/py/tests/test_efi_secboot/conftest.py
+++ b/test/py/tests/test_efi_secboot/conftest.py
@@ -103,12 +103,16 @@ def efi_boot_env(request, u_boot_config):
         ## db1-update
         check_call('cd %s; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key db db1.esl db1-update.auth'
                             % (mnt_point, EFITOOLS_PATH), shell=True)
-        ## dbx
+        ## dbx (TEST_dbx certificate)
         check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_dbx/ -keyout dbx.key -out dbx.crt -nodes -days 365'
                             % mnt_point, shell=True)
         check_call('cd %s; %scert-to-efi-sig-list -g %s dbx.crt dbx.esl; %ssign-efi-sig-list -c KEK.crt -k KEK.key dbx dbx.esl dbx.auth'
                             % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
                             shell=True)
+        ## dbx_hash (digest of TEST_db certificate)
+        check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -s 256 db.crt dbx_hash.crl; %ssign-efi-sig-list -c KEK.crt -k KEK.key dbx dbx_hash.crl dbx_hash.auth'
+                            % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
+                            shell=True)
 
         # Copy image
         check_call('cp %s %s' % (HELLO_PATH, mnt_point), shell=True)
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
index 5267b7ab4e86..21ae2bc5ed48 100644
--- a/test/py/tests/test_efi_secboot/test_signed.py
+++ b/test/py/tests/test_efi_secboot/test_signed.py
@@ -121,3 +121,29 @@ 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_auth4(self, u_boot_console, efi_boot_env):
+        """
+        Test Case 4 - revoked by dbx (digest of TEST_db certificate in dbx)
+        """
+        u_boot_console.restart_uboot()
+        disk_img = efi_boot_env
+        with u_boot_console.log.section('Test Case 4'):
+            # Test Case 4, rejected by dbx
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % disk_img,
+                'fatload host 0:1 4000000 dbx_hash.auth',
+                'setenv -e -nv -bs -rt -at -i 4000000,$filesize dbx',
+                '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(not 'Failed to set EFI variable' in ''.join(output))
+            output = u_boot_console.run_command_list([
+                'efidebug boot add 1 HELLO host 0:1 /helloworld.efi.signed ""',
+                'efidebug boot next 1',
+                'efidebug test bootmgr'])
+            assert('\'HELLO\' failed' in ''.join(output))
+            assert('efi_start_image() returned: 26' in ''.join(output))
-- 
2.25.2

[PATCH 12/13] test/py: efi_secboot: add a test for multiple signatures

[AKASHI Takahiro]

In this test case, an image is signed multiple times with different
keys. If any of signatures contained is not verified, the whole
authentication check should fail.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 test/py/tests/test_efi_secboot/conftest.py    |  3 ++
 test/py/tests/test_efi_secboot/test_signed.py | 51 +++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
index 13687a2da1a6..8d656d5cafba 100644
--- a/test/py/tests/test_efi_secboot/conftest.py
+++ b/test/py/tests/test_efi_secboot/conftest.py
@@ -120,6 +120,9 @@ def efi_boot_env(request, u_boot_config):
         ## Sign image
         check_call('cd %s; sbsign --key db.key --cert db.crt helloworld.efi'
                             % mnt_point, shell=True)
+        ## 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)
         ## Digest image
         check_call('cd %s; %shash-to-efi-sig-list helloworld.efi db_hello.hash; %ssign-efi-sig-list -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/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
index 21ae2bc5ed48..2ef95c6e1a14 100644
--- a/test/py/tests/test_efi_secboot/test_signed.py
+++ b/test/py/tests/test_efi_secboot/test_signed.py
@@ -147,3 +147,54 @@ 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_auth5(self, u_boot_console, efi_boot_env):
+        """
+        Test Case 5 - multiple signatures
+                        one signed with TEST_db, and
+                        one signed with TEST_db1
+        """
+        u_boot_console.restart_uboot()
+        disk_img = efi_boot_env
+        with u_boot_console.log.section('Test Case 5a'):
+            # Test Case 5a, rejected if any of signatures is not verified
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % disk_img,
+                '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(not 'Failed to set EFI variable' in ''.join(output))
+            output = u_boot_console.run_command_list([
+                'efidebug boot add 1 HELLO host 0:1 /helloworld.efi.signed_2sigs ""',
+                'efidebug boot next 1',
+                'efidebug test bootmgr'])
+            assert('\'HELLO\' failed' in ''.join(output))
+            assert('efi_start_image() returned: 26' in ''.join(output))
+
+        with u_boot_console.log.section('Test Case 5b'):
+            # Test Case 5b, authenticated if both signatures are verified
+            output = u_boot_console.run_command_list([
+                'fatload host 0:1 4000000 db1.auth',
+                'setenv -e -nv -bs -rt -at -a -i 4000000,$filesize db'])
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            output = u_boot_console.run_command_list([
+                'efidebug boot add 1 HELLO host 0:1 /helloworld.efi.signed_2sigs ""',
+                'efidebug boot next 1',
+                'bootefi bootmgr'])
+            assert('Hello, world!' in ''.join(output))
+
+        with u_boot_console.log.section('Test Case 5c'):
+            # Test Case 5c, rejected if any of signatures is revoked
+            output = u_boot_console.run_command_list([
+                'fatload host 0:1 4000000 dbx_hash.auth',
+                'setenv -e -nv -bs -rt -at -i 4000000,$filesize dbx'])
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            output = u_boot_console.run_command_list([
+                'efidebug boot add 1 HELLO host 0:1 /helloworld.efi.signed_2sigs ""',
+                'efidebug boot next 1',
+                'efidebug test bootmgr'])
+            assert('\'HELLO\' failed' in ''.join(output))
+            assert('efi_start_image() returned: 26' in ''.join(output))
-- 
2.25.2

[PATCH 13/13] test/py: efi_secboot: add a test for verifying with digest of signed image

[AKASHI Takahiro]

Signature database (db or dbx) may have not only certificates that contain
a public key for RSA decryption, but also digests of signed images.

In this test case, if database has an image's digest (EFI_CERT_SHA256_GUID)
and if the value matches to a hash value calculated from image's binary,
authentication should pass in case of db, and fail in case of dbx.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 test/py/tests/test_efi_secboot/conftest.py    | 11 +++++
 test/py/tests/test_efi_secboot/test_signed.py | 49 +++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
index 8d656d5cafba..d28f903c6897 100644
--- a/test/py/tests/test_efi_secboot/conftest.py
+++ b/test/py/tests/test_efi_secboot/conftest.py
@@ -113,6 +113,10 @@ def efi_boot_env(request, u_boot_config):
         check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -s 256 db.crt dbx_hash.crl; %ssign-efi-sig-list -c KEK.crt -k KEK.key dbx dbx_hash.crl dbx_hash.auth'
                             % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
                             shell=True)
+        ## dbx_db (with TEST_db certificate)
+        check_call('cd %s; %ssign-efi-sig-list -c KEK.crt -k KEK.key dbx db.esl dbx_db.auth'
+                            % (mnt_point, EFITOOLS_PATH),
+                            shell=True)
 
         # Copy image
         check_call('cp %s %s' % (HELLO_PATH, mnt_point), shell=True)
@@ -127,6 +131,13 @@ def efi_boot_env(request, u_boot_config):
         check_call('cd %s; %shash-to-efi-sig-list helloworld.efi db_hello.hash; %ssign-efi-sig-list -c KEK.crt -k KEK.key db db_hello.hash db_hello.auth'
                             % (mnt_point, EFITOOLS_PATH, EFITOOLS_PATH),
                             shell=True)
+        check_call('cd %s; %shash-to-efi-sig-list helloworld.efi.signed db_hello_signed.hash; %ssign-efi-sig-list -c KEK.crt -k KEK.key db db_hello_signed.hash db_hello_signed.auth'
+                            % (mnt_point, EFITOOLS_PATH, EFITOOLS_PATH),
+                            shell=True)
+        check_call('cd %s; %ssign-efi-sig-list -c KEK.crt -k KEK.key dbx db_hello_signed.hash dbx_hello_signed.auth'
+                            % (mnt_point, EFITOOLS_PATH),
+                            shell=True)
+
 
         check_call('sudo umount %s' % loop_dev, shell=True)
         check_call('sudo losetup -d %s' % loop_dev, shell=True)
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
index 2ef95c6e1a14..5cae1e3f5eef 100644
--- a/test/py/tests/test_efi_secboot/test_signed.py
+++ b/test/py/tests/test_efi_secboot/test_signed.py
@@ -198,3 +198,52 @@ 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_auth6(self, u_boot_console, efi_boot_env):
+        """
+        Test Case 6 - using digest of signed image in database
+        """
+        u_boot_console.restart_uboot()
+        disk_img = efi_boot_env
+        with u_boot_console.log.section('Test Case 6a'):
+            # Test Case 6a, verified by image's digest in db
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % disk_img,
+                'fatload host 0:1 4000000 db_hello_signed.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(not 'Failed to set EFI variable' in ''.join(output))
+            output = u_boot_console.run_command_list([
+                'efidebug boot add 1 HELLO host 0:1 /helloworld.efi.signed ""',
+                'efidebug boot next 1',
+                'bootefi bootmgr'])
+            assert('Hello, world!' in ''.join(output))
+
+        with u_boot_console.log.section('Test Case 6b'):
+            # Test Case 6b, rejected by TEST_db certificate in dbx
+            output = u_boot_console.run_command_list([
+                'fatload host 0:1 4000000 dbx_db.auth',
+                'setenv -e -nv -bs -rt -at -i 4000000,$filesize dbx'])
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            output = u_boot_console.run_command_list([
+                'efidebug boot next 1',
+                'efidebug test bootmgr'])
+            assert('\'HELLO\' failed' in ''.join(output))
+            assert('efi_start_image() returned: 26' in ''.join(output))
+
+        with u_boot_console.log.section('Test Case 6c'):
+            # Test Case 6c, rejected by image's digest in dbx
+            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 dbx_hello_signed.auth',
+                'setenv -e -nv -bs -rt -at -i 4000000,$filesize dbx'])
+            assert(not 'Failed to set EFI variable' in ''.join(output))
+            output = u_boot_console.run_command_list([
+                'efidebug boot next 1',
+                'efidebug test bootmgr'])
+            assert('\'HELLO\' failed' in ''.join(output))
+            assert('efi_start_image() returned: 26' in ''.join(output))
-- 
2.25.2