[RFC PATCH 1/2] efi_loader: fix dual signed image certification

[RFC PATCH 1/2] efi_loader: fix dual signed image certification

[Ilias Apalodimas]

The EFI spec allows for images to carry multiple signatures. Currently
we don't adhere to the verification process for such images.

The spec says:
"Multiple signatures are allowed to exist in the binary's certificate
table (as per PE/COFF Section "Attribute Certificate Table"). Only one
hash or signature is required to be present in db in order to pass
validation, so long as neither the SHA-256 hash of the binary nor any
present signature is reflected in dbx."

With our current implementation signing the image with two certificates
and inserting both of them in db and one of them dbx doesn't always reject
the image.  The rejection depends on the order that the image was signed
and the order the certificates are read (and checked) in db.

While at it move the sha256 hash verification outside the signature
checking loop, since it only needs to run once per image and get simplify
the logic for authenticating an unsigned imahe using sha256 hashes.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/efi_image_loader.c | 88 +++++++------------------------
 1 file changed, 18 insertions(+), 70 deletions(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index f41cfa4fccd5..5df35939f702 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -516,53 +516,6 @@ err:
 }
 
 #ifdef CONFIG_EFI_SECURE_BOOT
-/**
- * efi_image_unsigned_authenticate() - authenticate unsigned image with
- * SHA256 hash
- * @regs:	List of regions to be verified
- *
- * If an image is not signed, it doesn't have a signature. In this case,
- * its message digest is calculated and it will be compared with one of
- * hash values stored in signature databases.
- *
- * Return:	true if authenticated, false if not
- */
-static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs)
-{
-	struct efi_signature_store *db = NULL, *dbx = NULL;
-	bool ret = false;
-
-	dbx = efi_sigstore_parse_sigdb(u"dbx");
-	if (!dbx) {
-		EFI_PRINT("Getting signature database(dbx) failed\n");
-		goto out;
-	}
-
-	db = efi_sigstore_parse_sigdb(u"db");
-	if (!db) {
-		EFI_PRINT("Getting signature database(db) failed\n");
-		goto out;
-	}
-
-	/* try black-list first */
-	if (efi_signature_lookup_digest(regs, dbx, true)) {
-		EFI_PRINT("Image is not signed and its digest found in \"dbx\"\n");
-		goto out;
-	}
-
-	/* try white-list */
-	if (efi_signature_lookup_digest(regs, db, false))
-		ret = true;
-	else
-		EFI_PRINT("Image is not signed and its digest not found in \"db\" or \"dbx\"\n");
-
-out:
-	efi_sigstore_free(db);
-	efi_sigstore_free(dbx);
-
-	return ret;
-}
-
 /**
  * efi_image_authenticate() - verify a signature of signed image
  * @efi:	Pointer to image
@@ -608,14 +561,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 	if (!efi_image_parse(new_efi, efi_size, &regs, &wincerts,
 			     &wincerts_len)) {
 		EFI_PRINT("Parsing PE executable image failed\n");
-		goto err;
-	}
-
-	if (!wincerts) {
-		/* The image is not signed */
-		ret = efi_image_unsigned_authenticate(regs);
-
-		goto err;
+		goto out;
 	}
 
 	/*
@@ -624,18 +570,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 	db = efi_sigstore_parse_sigdb(u"db");
 	if (!db) {
 		EFI_PRINT("Getting signature database(db) failed\n");
-		goto err;
+		goto out;
 	}
 
 	dbx = efi_sigstore_parse_sigdb(u"dbx");
 	if (!dbx) {
 		EFI_PRINT("Getting signature database(dbx) failed\n");
-		goto err;
+		goto out;
 	}
 
 	if (efi_signature_lookup_digest(regs, dbx, true)) {
 		EFI_PRINT("Image's digest was found in \"dbx\"\n");
-		goto err;
+		goto out;
 	}
 
 	/*
@@ -678,7 +624,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
 				EFI_PRINT("Certificate type not supported: %pUs\n",
 					  auth);
-				continue;
+				ret = false;
+				goto out;
 			}
 
 			auth += sizeof(efi_guid_t);
@@ -686,7 +633,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 		} else if (wincert->wCertificateType
 				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
 			EFI_PRINT("Certificate type not supported\n");
-			continue;
+			ret = false;
+			goto out;
 		}
 
 		msg = pkcs7_parse_message(auth, auth_size);
@@ -717,32 +665,32 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 		 */
 		/* try black-list first */
 		if (efi_signature_verify_one(regs, msg, dbx)) {
+			ret = false;
 			EFI_PRINT("Signature was rejected by \"dbx\"\n");
-			continue;
+			goto out;
 		}
 
 		if (!efi_signature_check_signers(msg, dbx)) {
+			ret = false;
 			EFI_PRINT("Signer(s) in \"dbx\"\n");
-			continue;
+			goto out;
 		}
 
 		/* try white-list */
 		if (efi_signature_verify(regs, msg, db, dbx)) {
 			ret = true;
-			break;
+			continue;
 		}
 
 		EFI_PRINT("Signature was not verified by \"db\"\n");
+	}
 
-		if (efi_signature_lookup_digest(regs, db, false)) {
-			ret = true;
-			break;
-		}
 
-		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
-	}
+	/* last resort try the image sha256 hash in db */
+	if (!ret && efi_signature_lookup_digest(regs, db, false))
+		ret = true;
 
-err:
+out:
 	efi_sigstore_free(db);
 	efi_sigstore_free(dbx);
 	pkcs7_free_message(msg);
-- 
2.32.0

[RFC PATCH 2/2] test/py: efi_secboot: adjust secure boot tests to code changes

[Ilias Apalodimas]

The previous patch is changing U-Boot's behavior wrt certificate based
binary authentication.  Specifically an image who's digest of a
certificate is found in dbx is now rejected.  Fix the test accordingly

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 test/py/tests/test_efi_secboot/test_signed.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
index 0aee34479f55..7f5ec78261da 100644
--- a/test/py/tests/test_efi_secboot/test_signed.py
+++ b/test/py/tests/test_efi_secboot/test_signed.py
@@ -186,7 +186,7 @@ class TestEfiSignedImage(object):
             assert 'Hello, world!' in ''.join(output)
 
         with u_boot_console.log.section('Test Case 5c'):
-            # Test Case 5c, not rejected if one of signatures (digest of
+            # Test Case 5c, rejected if one of signatures (digest of
             # certificate) is revoked
             output = u_boot_console.run_command_list([
                 'fatload host 0:1 4000000 dbx_hash.auth',
@@ -195,7 +195,8 @@ class TestEfiSignedImage(object):
             output = u_boot_console.run_command_list([
                 'efidebug boot next 1',
                 'efidebug test bootmgr'])
-            assert 'Hello, world!' in ''.join(output)
+            assert '\'HELLO\' failed' in ''.join(output)
+            assert 'efi_start_image() returned: 26' in ''.join(output)
 
         with u_boot_console.log.section('Test Case 5d'):
             # Test Case 5d, rejected if both of signatures are revoked
-- 
2.32.0

Re: [RFC PATCH 1/2] efi_loader: fix dual signed image certification

[AKASHI Takahiro]

Hi Ilias,

Thank you for reviewing the logic.

On Fri, Feb 04, 2022 at 09:32:01AM +0200, Ilias Apalodimas wrote:
> The EFI spec allows for images to carry multiple signatures. Currently
> we don't adhere to the verification process for such images.

In this patch, you're trying to do three things:
* remove efi_image_unsigned_authenticate()
* pull efi_signature_lookup_digest() out of a while loop
* change the logic of authentication

I'd prefer to see those changes in separate patches for better reviewing.

> The spec says:
> "Multiple signatures are allowed to exist in the binary's certificate
> table (as per PE/COFF Section "Attribute Certificate Table"). Only one
> hash or signature is required to be present in db in order to pass
> validation, so long as neither the SHA-256 hash of the binary nor any
> present signature is reflected in dbx."

I have some concern about what the last phrase, "neither the SHA-256 hash
of the binary nor any present signature is reflected in dbx" means.
See the comment below.

> With our current implementation signing the image with two certificates
> and inserting both of them in db and one of them dbx doesn't always reject
> the image.  The rejection depends on the order that the image was signed
> and the order the certificates are read (and checked) in db.
> 
> While at it move the sha256 hash verification outside the signature
> checking loop, since it only needs to run once per image and get simplify
> the logic for authenticating an unsigned imahe using sha256 hashes.
> 
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  lib/efi_loader/efi_image_loader.c | 88 +++++++------------------------
>  1 file changed, 18 insertions(+), 70 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index f41cfa4fccd5..5df35939f702 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -516,53 +516,6 @@ err:
>  }
>  
>  #ifdef CONFIG_EFI_SECURE_BOOT
> -/**
> - * efi_image_unsigned_authenticate() - authenticate unsigned image with
> - * SHA256 hash
> - * @regs:	List of regions to be verified
> - *
> - * If an image is not signed, it doesn't have a signature. In this case,
> - * its message digest is calculated and it will be compared with one of
> - * hash values stored in signature databases.
> - *
> - * Return:	true if authenticated, false if not
> - */
> -static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs)
> -{
> -	struct efi_signature_store *db = NULL, *dbx = NULL;
> -	bool ret = false;
> -
> -	dbx = efi_sigstore_parse_sigdb(u"dbx");
> -	if (!dbx) {
> -		EFI_PRINT("Getting signature database(dbx) failed\n");
> -		goto out;
> -	}
> -
> -	db = efi_sigstore_parse_sigdb(u"db");
> -	if (!db) {
> -		EFI_PRINT("Getting signature database(db) failed\n");
> -		goto out;
> -	}
> -
> -	/* try black-list first */
> -	if (efi_signature_lookup_digest(regs, dbx, true)) {
> -		EFI_PRINT("Image is not signed and its digest found in \"dbx\"\n");
> -		goto out;
> -	}
> -
> -	/* try white-list */
> -	if (efi_signature_lookup_digest(regs, db, false))
> -		ret = true;
> -	else
> -		EFI_PRINT("Image is not signed and its digest not found in \"db\" or \"dbx\"\n");
> -
> -out:
> -	efi_sigstore_free(db);
> -	efi_sigstore_free(dbx);
> -
> -	return ret;
> -}
> -
>  /**
>   * efi_image_authenticate() - verify a signature of signed image
>   * @efi:	Pointer to image
> @@ -608,14 +561,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  	if (!efi_image_parse(new_efi, efi_size, &regs, &wincerts,
>  			     &wincerts_len)) {
>  		EFI_PRINT("Parsing PE executable image failed\n");
> -		goto err;
> -	}
> -
> -	if (!wincerts) {
> -		/* The image is not signed */
> -		ret = efi_image_unsigned_authenticate(regs);
> -
> -		goto err;
> +		goto out;
>  	}
>  
>  	/*
> @@ -624,18 +570,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  	db = efi_sigstore_parse_sigdb(u"db");
>  	if (!db) {
>  		EFI_PRINT("Getting signature database(db) failed\n");
> -		goto err;
> +		goto out;
>  	}
>  
>  	dbx = efi_sigstore_parse_sigdb(u"dbx");
>  	if (!dbx) {
>  		EFI_PRINT("Getting signature database(dbx) failed\n");
> -		goto err;
> +		goto out;
>  	}
>  
>  	if (efi_signature_lookup_digest(regs, dbx, true)) {
>  		EFI_PRINT("Image's digest was found in \"dbx\"\n");
> -		goto err;
> +		goto out;
>  	}
>  
>  	/*
> @@ -678,7 +624,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
>  				EFI_PRINT("Certificate type not supported: %pUs\n",
>  					  auth);
> -				continue;
> +				ret = false;
> +				goto out;

Why should we break the loop here?

>  			}
>  
>  			auth += sizeof(efi_guid_t);
> @@ -686,7 +633,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  		} else if (wincert->wCertificateType
>  				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
>  			EFI_PRINT("Certificate type not supported\n");
> -			continue;
> +			ret = false;
> +			goto out;
>  		}
>  
>  		msg = pkcs7_parse_message(auth, auth_size);
> @@ -717,32 +665,32 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  		 */
>  		/* try black-list first */
>  		if (efi_signature_verify_one(regs, msg, dbx)) {
> +			ret = false;
>  			EFI_PRINT("Signature was rejected by \"dbx\"\n");
> -			continue;
> +			goto out;

If we go to "out" here, we have no chance to verify some cases:
1) An image has two signatures, for instance, one signed by SHA1 cert
   and the other signed by SHA256 cert. A user wants to reject SHA1 cert
   and put the cert in dbx.
   But this image can and should yet be verified by SHA256 cert.
2) A user knows that a given image is safe for some reason even though
   he or she doesn't trust the certficate which is used for signing
   the image.

-Takahiro Akashi

>  		}
>  
>  		if (!efi_signature_check_signers(msg, dbx)) {
> +			ret = false;
>  			EFI_PRINT("Signer(s) in \"dbx\"\n");
> -			continue;
> +			goto out;
>  		}
>  
>  		/* try white-list */
>  		if (efi_signature_verify(regs, msg, db, dbx)) {
>  			ret = true;
> -			break;
> +			continue;
>  		}
>  
>  		EFI_PRINT("Signature was not verified by \"db\"\n");
> +	}
>  
> -		if (efi_signature_lookup_digest(regs, db, false)) {
> -			ret = true;
> -			break;
> -		}
>  
> -		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
> -	}
> +	/* last resort try the image sha256 hash in db */
> +	if (!ret && efi_signature_lookup_digest(regs, db, false))
> +		ret = true;
>  
> -err:
> +out:
>  	efi_sigstore_free(db);
>  	efi_sigstore_free(dbx);
>  	pkcs7_free_message(msg);
> -- 
> 2.32.0
> 

Re: [RFC PATCH 2/2] test/py: efi_secboot: adjust secure boot tests to code changes

[AKASHI Takahiro]

On Fri, Feb 04, 2022 at 09:32:02AM +0200, Ilias Apalodimas wrote:
> The previous patch is changing U-Boot's behavior wrt certificate based
> binary authentication.  Specifically an image who's digest of a
> certificate is found in dbx is now rejected.  Fix the test accordingly

Please not only fix the given test case, but also add more cases
if needed or appropriate for wider coverage of corner cases.
You mentioned in the previous commit that the order of certificates
should not affect the verification result.
If so, we need, at least, one more test case where the order of certificates
in db is different.

I think that trying to maintain the test scenario that way will help improve
the robustness of verification logic.

-Takahiro Akashi


> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  test/py/tests/test_efi_secboot/test_signed.py | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
> index 0aee34479f55..7f5ec78261da 100644
> --- a/test/py/tests/test_efi_secboot/test_signed.py
> +++ b/test/py/tests/test_efi_secboot/test_signed.py
> @@ -186,7 +186,7 @@ class TestEfiSignedImage(object):
>              assert 'Hello, world!' in ''.join(output)
>  
>          with u_boot_console.log.section('Test Case 5c'):
> -            # Test Case 5c, not rejected if one of signatures (digest of
> +            # Test Case 5c, rejected if one of signatures (digest of
>              # certificate) is revoked
>              output = u_boot_console.run_command_list([
>                  'fatload host 0:1 4000000 dbx_hash.auth',
> @@ -195,7 +195,8 @@ class TestEfiSignedImage(object):
>              output = u_boot_console.run_command_list([
>                  'efidebug boot next 1',
>                  'efidebug test bootmgr'])
> -            assert 'Hello, world!' in ''.join(output)
> +            assert '\'HELLO\' failed' in ''.join(output)
> +            assert 'efi_start_image() returned: 26' in ''.join(output)
>  
>          with u_boot_console.log.section('Test Case 5d'):
>              # Test Case 5d, rejected if both of signatures are revoked
> -- 
> 2.32.0
> 

Re: [RFC PATCH 1/2] efi_loader: fix dual signed image certification

[Ilias Apalodimas]

On Thu, Feb 10, 2022 at 02:13:48PM +0900, AKASHI Takahiro wrote:
> Hi Ilias,
> 
> Thank you for reviewing the logic.
> 
> On Fri, Feb 04, 2022 at 09:32:01AM +0200, Ilias Apalodimas wrote:
> > The EFI spec allows for images to carry multiple signatures. Currently
> > we don't adhere to the verification process for such images.
> 
> In this patch, you're trying to do three things:
> * remove efi_image_unsigned_authenticate()
> * pull efi_signature_lookup_digest() out of a while loop
> * change the logic of authentication
> 
> I'd prefer to see those changes in separate patches for better reviewing.

I tried both and the current one seemed easier to review.  Heinrich any
preference?

> 
> > The spec says:
> > "Multiple signatures are allowed to exist in the binary's certificate
> > table (as per PE/COFF Section "Attribute Certificate Table"). Only one
> > hash or signature is required to be present in db in order to pass
> > validation, so long as neither the SHA-256 hash of the binary nor any
> > present signature is reflected in dbx."
> 
> I have some concern about what the last phrase, "neither the SHA-256 hash
> of the binary nor any present signature is reflected in dbx" means.
> See the comment below.
> 
> > With our current implementation signing the image with two certificates
> > and inserting both of them in db and one of them dbx doesn't always reject
> > the image.  The rejection depends on the order that the image was signed
> > and the order the certificates are read (and checked) in db.
> > 
> > While at it move the sha256 hash verification outside the signature
> > checking loop, since it only needs to run once per image and get simplify
> > the logic for authenticating an unsigned imahe using sha256 hashes.
> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  lib/efi_loader/efi_image_loader.c | 88 +++++++------------------------
> >  1 file changed, 18 insertions(+), 70 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> > index f41cfa4fccd5..5df35939f702 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -516,53 +516,6 @@ err:
> >  }
> >  
> >  #ifdef CONFIG_EFI_SECURE_BOOT
> > -/**
> > - * efi_image_unsigned_authenticate() - authenticate unsigned image with
> > - * SHA256 hash
> > - * @regs:	List of regions to be verified
> > - *
> > - * If an image is not signed, it doesn't have a signature. In this case,
> > - * its message digest is calculated and it will be compared with one of
> > - * hash values stored in signature databases.
> > - *
> > - * Return:	true if authenticated, false if not
> > - */
> > -static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs)
> > -{
> > -	struct efi_signature_store *db = NULL, *dbx = NULL;
> > -	bool ret = false;
> > -
> > -	dbx = efi_sigstore_parse_sigdb(u"dbx");
> > -	if (!dbx) {
> > -		EFI_PRINT("Getting signature database(dbx) failed\n");
> > -		goto out;
> > -	}
> > -
> > -	db = efi_sigstore_parse_sigdb(u"db");
> > -	if (!db) {
> > -		EFI_PRINT("Getting signature database(db) failed\n");
> > -		goto out;
> > -	}
> > -
> > -	/* try black-list first */
> > -	if (efi_signature_lookup_digest(regs, dbx, true)) {
> > -		EFI_PRINT("Image is not signed and its digest found in \"dbx\"\n");
> > -		goto out;
> > -	}
> > -
> > -	/* try white-list */
> > -	if (efi_signature_lookup_digest(regs, db, false))
> > -		ret = true;
> > -	else
> > -		EFI_PRINT("Image is not signed and its digest not found in \"db\" or \"dbx\"\n");
> > -
> > -out:
> > -	efi_sigstore_free(db);
> > -	efi_sigstore_free(dbx);
> > -
> > -	return ret;
> > -}
> > -
> >  /**
> >   * efi_image_authenticate() - verify a signature of signed image
> >   * @efi:	Pointer to image
> > @@ -608,14 +561,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  	if (!efi_image_parse(new_efi, efi_size, &regs, &wincerts,
> >  			     &wincerts_len)) {
> >  		EFI_PRINT("Parsing PE executable image failed\n");
> > -		goto err;
> > -	}
> > -
> > -	if (!wincerts) {
> > -		/* The image is not signed */
> > -		ret = efi_image_unsigned_authenticate(regs);
> > -
> > -		goto err;
> > +		goto out;
> >  	}
> >  
> >  	/*
> > @@ -624,18 +570,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  	db = efi_sigstore_parse_sigdb(u"db");
> >  	if (!db) {
> >  		EFI_PRINT("Getting signature database(db) failed\n");
> > -		goto err;
> > +		goto out;
> >  	}
> >  
> >  	dbx = efi_sigstore_parse_sigdb(u"dbx");
> >  	if (!dbx) {
> >  		EFI_PRINT("Getting signature database(dbx) failed\n");
> > -		goto err;
> > +		goto out;
> >  	}
> >  
> >  	if (efi_signature_lookup_digest(regs, dbx, true)) {
> >  		EFI_PRINT("Image's digest was found in \"dbx\"\n");
> > -		goto err;
> > +		goto out;
> >  	}
> >  
> >  	/*
> > @@ -678,7 +624,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
> >  				EFI_PRINT("Certificate type not supported: %pUs\n",
> >  					  auth);
> > -				continue;
> > +				ret = false;
> > +				goto out;
> 
> Why should we break the loop here?

We were trying to reject signature verification that we don't support,
since the equivalent cert might be in dbx.  But I am not 100% sure taht's
what we want here.

> 
> >  			}
> >  
> >  			auth += sizeof(efi_guid_t);
> > @@ -686,7 +633,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  		} else if (wincert->wCertificateType
> >  				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> >  			EFI_PRINT("Certificate type not supported\n");
> > -			continue;
> > +			ret = false;
> > +			goto out;
> >  		}
> >  
> >  		msg = pkcs7_parse_message(auth, auth_size);
> > @@ -717,32 +665,32 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  		 */
> >  		/* try black-list first */
> >  		if (efi_signature_verify_one(regs, msg, dbx)) {
> > +			ret = false;
> >  			EFI_PRINT("Signature was rejected by \"dbx\"\n");
> > -			continue;
> > +			goto out;
> 
> If we go to "out" here, we have no chance to verify some cases:
> 1) An image has two signatures, for instance, one signed by SHA1 cert
>    and the other signed by SHA256 cert. A user wants to reject SHA1 cert
>    and put the cert in dbx.

I am not sure I am following,  what does he gain be rejecting the SHA1
portion only?  Avoid potential collisions?

>    But this image can and should yet be verified by SHA256 cert.

Why should it be verified?  My understanding of the EFI spec is that any match
in dbx of any certificate in the signing chain of the signature being verified means 
reject the image. 

> 2) A user knows that a given image is safe for some reason even though
>    he or she doesn't trust the certficate which is used for signing
>    the image.
> 
> -Takahiro Akashi
> 
> >  		}
> >  
> >  		if (!efi_signature_check_signers(msg, dbx)) {
> > +			ret = false;
> >  			EFI_PRINT("Signer(s) in \"dbx\"\n");
> > -			continue;
> > +			goto out;
> >  		}
> >  
> >  		/* try white-list */
> >  		if (efi_signature_verify(regs, msg, db, dbx)) {
> >  			ret = true;
> > -			break;
> > +			continue;
> >  		}
> >  
> >  		EFI_PRINT("Signature was not verified by \"db\"\n");
> > +	}
> >  
> > -		if (efi_signature_lookup_digest(regs, db, false)) {
> > -			ret = true;
> > -			break;
> > -		}
> >  
> > -		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
> > -	}
> > +	/* last resort try the image sha256 hash in db */
> > +	if (!ret && efi_signature_lookup_digest(regs, db, false))
> > +		ret = true;
> >  
> > -err:
> > +out:
> >  	efi_sigstore_free(db);
> >  	efi_sigstore_free(dbx);
> >  	pkcs7_free_message(msg);
> > -- 
> > 2.32.0
> > 

Thanks
/Ilias

Re: [RFC PATCH 2/2] test/py: efi_secboot: adjust secure boot tests to code changes

[Ilias Apalodimas]

Akashi-san

On Thu, 10 Feb 2022 at 07:22, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> On Fri, Feb 04, 2022 at 09:32:02AM +0200, Ilias Apalodimas wrote:
> > The previous patch is changing U-Boot's behavior wrt certificate based
> > binary authentication.  Specifically an image who's digest of a
> > certificate is found in dbx is now rejected.  Fix the test accordingly
>
> Please not only fix the given test case, but also add more cases
> if needed or appropriate for wider coverage of corner cases.
> You mentioned in the previous commit that the order of certificates
> should not affect the verification result.
> If so, we need, at least, one more test case where the order of certificates
> in db is different.
>
> I think that trying to maintain the test scenario that way will help improve
> the robustness of verification logic.

And we agree, but my concern right now is fix the existing use cases.
There are some SCT tests wrt certification of binaries,  so I intend
to port more cases for those in the future.

Cheers
/Ilias
>
> -Takahiro Akashi
>
>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  test/py/tests/test_efi_secboot/test_signed.py | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
> > index 0aee34479f55..7f5ec78261da 100644
> > --- a/test/py/tests/test_efi_secboot/test_signed.py
> > +++ b/test/py/tests/test_efi_secboot/test_signed.py
> > @@ -186,7 +186,7 @@ class TestEfiSignedImage(object):
> >              assert 'Hello, world!' in ''.join(output)
> >
> >          with u_boot_console.log.section('Test Case 5c'):
> > -            # Test Case 5c, not rejected if one of signatures (digest of
> > +            # Test Case 5c, rejected if one of signatures (digest of
> >              # certificate) is revoked
> >              output = u_boot_console.run_command_list([
> >                  'fatload host 0:1 4000000 dbx_hash.auth',
> > @@ -195,7 +195,8 @@ class TestEfiSignedImage(object):
> >              output = u_boot_console.run_command_list([
> >                  'efidebug boot next 1',
> >                  'efidebug test bootmgr'])
> > -            assert 'Hello, world!' in ''.join(output)
> > +            assert '\'HELLO\' failed' in ''.join(output)
> > +            assert 'efi_start_image() returned: 26' in ''.join(output)
> >
> >          with u_boot_console.log.section('Test Case 5d'):
> >              # Test Case 5d, rejected if both of signatures are revoked
> > --
> > 2.32.0
> >

Re: [RFC PATCH 2/2] test/py: efi_secboot: adjust secure boot tests to code changes

[AKASHI Takahiro]

On Thu, Feb 10, 2022 at 09:14:25AM +0200, Ilias Apalodimas wrote:
> Akashi-san
> 
> On Thu, 10 Feb 2022 at 07:22, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > On Fri, Feb 04, 2022 at 09:32:02AM +0200, Ilias Apalodimas wrote:
> > > The previous patch is changing U-Boot's behavior wrt certificate based
> > > binary authentication.  Specifically an image who's digest of a
> > > certificate is found in dbx is now rejected.  Fix the test accordingly
> >
> > Please not only fix the given test case, but also add more cases
> > if needed or appropriate for wider coverage of corner cases.
> > You mentioned in the previous commit that the order of certificates
> > should not affect the verification result.
> > If so, we need, at least, one more test case where the order of certificates
> > in db is different.
> >
> > I think that trying to maintain the test scenario that way will help improve
> > the robustness of verification logic.
> 
> And we agree, but my concern right now is fix the existing use cases.

But you have to verify the logic works in the same way whatever the order of
certificates is. I think that is your intent in this patch.

-Takahiro Akashi


> There are some SCT tests wrt certification of binaries,  so I intend
> to port more cases for those in the future.
> 
> Cheers
> /Ilias
> >
> > -Takahiro Akashi
> >
> >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > >  test/py/tests/test_efi_secboot/test_signed.py | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
> > > index 0aee34479f55..7f5ec78261da 100644
> > > --- a/test/py/tests/test_efi_secboot/test_signed.py
> > > +++ b/test/py/tests/test_efi_secboot/test_signed.py
> > > @@ -186,7 +186,7 @@ class TestEfiSignedImage(object):
> > >              assert 'Hello, world!' in ''.join(output)
> > >
> > >          with u_boot_console.log.section('Test Case 5c'):
> > > -            # Test Case 5c, not rejected if one of signatures (digest of
> > > +            # Test Case 5c, rejected if one of signatures (digest of
> > >              # certificate) is revoked
> > >              output = u_boot_console.run_command_list([
> > >                  'fatload host 0:1 4000000 dbx_hash.auth',
> > > @@ -195,7 +195,8 @@ class TestEfiSignedImage(object):
> > >              output = u_boot_console.run_command_list([
> > >                  'efidebug boot next 1',
> > >                  'efidebug test bootmgr'])
> > > -            assert 'Hello, world!' in ''.join(output)
> > > +            assert '\'HELLO\' failed' in ''.join(output)
> > > +            assert 'efi_start_image() returned: 26' in ''.join(output)
> > >
> > >          with u_boot_console.log.section('Test Case 5d'):
> > >              # Test Case 5d, rejected if both of signatures are revoked
> > > --
> > > 2.32.0
> > >

Re: [RFC PATCH 1/2] efi_loader: fix dual signed image certification

[Heinrich Schuchardt]

On 2/10/22 08:13, Ilias Apalodimas wrote:
> On Thu, Feb 10, 2022 at 02:13:48PM +0900, AKASHI Takahiro wrote:
>> Hi Ilias,
>>
>> Thank you for reviewing the logic.
>>
>> On Fri, Feb 04, 2022 at 09:32:01AM +0200, Ilias Apalodimas wrote:
>>> The EFI spec allows for images to carry multiple signatures. Currently
>>> we don't adhere to the verification process for such images.
>>
>> In this patch, you're trying to do three things:
>> * remove efi_image_unsigned_authenticate()
>> * pull efi_signature_lookup_digest() out of a while loop
>> * change the logic of authentication
>>
>> I'd prefer to see those changes in separate patches for better reviewing.
>
> I tried both and the current one seemed easier to review.  Heinrich any
> preference?

The amount of new code is ok.

>
>>
>>> The spec says:
>>> "Multiple signatures are allowed to exist in the binary's certificate
>>> table (as per PE/COFF Section "Attribute Certificate Table"). Only one
>>> hash or signature is required to be present in db in order to pass
>>> validation, so long as neither the SHA-256 hash of the binary nor any
>>> present signature is reflected in dbx."
>>
>> I have some concern about what the last phrase, "neither the SHA-256 hash
>> of the binary nor any present signature is reflected in dbx" means.
>> See the comment below.
>>
>>> With our current implementation signing the image with two certificates
>>> and inserting both of them in db and one of them dbx doesn't always reject
>>> the image.  The rejection depends on the order that the image was signed
>>> and the order the certificates are read (and checked) in db.
>>>
>>> While at it move the sha256 hash verification outside the signature
>>> checking loop, since it only needs to run once per image and get simplify
>>> the logic for authenticating an unsigned imahe using sha256 hashes.
>>>
>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>> ---
>>>   lib/efi_loader/efi_image_loader.c | 88 +++++++------------------------
>>>   1 file changed, 18 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
>>> index f41cfa4fccd5..5df35939f702 100644
>>> --- a/lib/efi_loader/efi_image_loader.c
>>> +++ b/lib/efi_loader/efi_image_loader.c
>>> @@ -516,53 +516,6 @@ err:
>>>   }
>>>
>>>   #ifdef CONFIG_EFI_SECURE_BOOT
>>> -/**
>>> - * efi_image_unsigned_authenticate() - authenticate unsigned image with
>>> - * SHA256 hash
>>> - * @regs:	List of regions to be verified
>>> - *
>>> - * If an image is not signed, it doesn't have a signature. In this case,
>>> - * its message digest is calculated and it will be compared with one of
>>> - * hash values stored in signature databases.
>>> - *
>>> - * Return:	true if authenticated, false if not
>>> - */
>>> -static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs)
>>> -{
>>> -	struct efi_signature_store *db = NULL, *dbx = NULL;
>>> -	bool ret = false;
>>> -
>>> -	dbx = efi_sigstore_parse_sigdb(u"dbx");
>>> -	if (!dbx) {
>>> -		EFI_PRINT("Getting signature database(dbx) failed\n");
>>> -		goto out;
>>> -	}
>>> -
>>> -	db = efi_sigstore_parse_sigdb(u"db");
>>> -	if (!db) {
>>> -		EFI_PRINT("Getting signature database(db) failed\n");
>>> -		goto out;
>>> -	}
>>> -
>>> -	/* try black-list first */
>>> -	if (efi_signature_lookup_digest(regs, dbx, true)) {
>>> -		EFI_PRINT("Image is not signed and its digest found in \"dbx\"\n");
>>> -		goto out;
>>> -	}
>>> -
>>> -	/* try white-list */
>>> -	if (efi_signature_lookup_digest(regs, db, false))
>>> -		ret = true;
>>> -	else
>>> -		EFI_PRINT("Image is not signed and its digest not found in \"db\" or \"dbx\"\n");
>>> -
>>> -out:
>>> -	efi_sigstore_free(db);
>>> -	efi_sigstore_free(dbx);
>>> -
>>> -	return ret;
>>> -}
>>> -
>>>   /**
>>>    * efi_image_authenticate() - verify a signature of signed image
>>>    * @efi:	Pointer to image
>>> @@ -608,14 +561,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>>>   	if (!efi_image_parse(new_efi, efi_size, &regs, &wincerts,
>>>   			     &wincerts_len)) {
>>>   		EFI_PRINT("Parsing PE executable image failed\n");
>>> -		goto err;
>>> -	}
>>> -
>>> -	if (!wincerts) {
>>> -		/* The image is not signed */
>>> -		ret = efi_image_unsigned_authenticate(regs);
>>> -
>>> -		goto err;
>>> +		goto out;
>>>   	}
>>>
>>>   	/*
>>> @@ -624,18 +570,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>>>   	db = efi_sigstore_parse_sigdb(u"db");
>>>   	if (!db) {
>>>   		EFI_PRINT("Getting signature database(db) failed\n");
>>> -		goto err;
>>> +		goto out;
>>>   	}
>>>
>>>   	dbx = efi_sigstore_parse_sigdb(u"dbx");
>>>   	if (!dbx) {
>>>   		EFI_PRINT("Getting signature database(dbx) failed\n");
>>> -		goto err;
>>> +		goto out;
>>>   	}
>>>
>>>   	if (efi_signature_lookup_digest(regs, dbx, true)) {
>>>   		EFI_PRINT("Image's digest was found in \"dbx\"\n");
>>> -		goto err;
>>> +		goto out;
>>>   	}
>>>
>>>   	/*
>>> @@ -678,7 +624,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>>>   			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
>>>   				EFI_PRINT("Certificate type not supported: %pUs\n",
>>>   					  auth);
>>> -				continue;
>>> +				ret = false;
>>> +				goto out;
>>
>> Why should we break the loop here?
>
> We were trying to reject signature verification that we don't support,
> since the equivalent cert might be in dbx.  But I am not 100% sure taht's
> what we want here.
>
>>
>>>   			}
>>>
>>>   			auth += sizeof(efi_guid_t);
>>> @@ -686,7 +633,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>>>   		} else if (wincert->wCertificateType
>>>   				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
>>>   			EFI_PRINT("Certificate type not supported\n");
>>> -			continue;
>>> +			ret = false;
>>> +			goto out;
>>>   		}
>>>
>>>   		msg = pkcs7_parse_message(auth, auth_size);
>>> @@ -717,32 +665,32 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>>>   		 */
>>>   		/* try black-list first */
>>>   		if (efi_signature_verify_one(regs, msg, dbx)) {
>>> +			ret = false;
>>>   			EFI_PRINT("Signature was rejected by \"dbx\"\n");
>>> -			continue;
>>> +			goto out;
>>
>> If we go to "out" here, we have no chance to verify some cases:
>> 1) An image has two signatures, for instance, one signed by SHA1 cert
>>     and the other signed by SHA256 cert. A user wants to reject SHA1 cert
>>     and put the cert in dbx.
>
> I am not sure I am following,  what does he gain be rejecting the SHA1
> portion only?  Avoid potential collisions?

If an image has a SHA1 and a SHA256 signature attached and SHA1 *or*
SHA256 is in dbx, we must reject the image. Don't expect a dbx entry for
each of the hashes. - But isn't this what your are doing here: for all
signatures of the image look for one hit in dbx?

Best regards

Heinrich

>
>>     But this image can and should yet be verified by SHA256 cert.
>
> Why should it be verified?  My understanding of the EFI spec is that any match
> in dbx of any certificate in the signing chain of the signature being verified means
> reject the image.
>
>> 2) A user knows that a given image is safe for some reason even though
>>     he or she doesn't trust the certficate which is used for signing
>>     the image.
>>
>> -Takahiro Akashi
>>
>>>   		}
>>>
>>>   		if (!efi_signature_check_signers(msg, dbx)) {
>>> +			ret = false;
>>>   			EFI_PRINT("Signer(s) in \"dbx\"\n");
>>> -			continue;
>>> +			goto out;
>>>   		}
>>>
>>>   		/* try white-list */
>>>   		if (efi_signature_verify(regs, msg, db, dbx)) {
>>>   			ret = true;
>>> -			break;
>>> +			continue;
>>>   		}
>>>
>>>   		EFI_PRINT("Signature was not verified by \"db\"\n");
>>> +	}
>>>
>>> -		if (efi_signature_lookup_digest(regs, db, false)) {
>>> -			ret = true;
>>> -			break;
>>> -		}
>>>
>>> -		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
>>> -	}
>>> +	/* last resort try the image sha256 hash in db */
>>> +	if (!ret && efi_signature_lookup_digest(regs, db, false))
>>> +		ret = true;
>>>
>>> -err:
>>> +out:
>>>   	efi_sigstore_free(db);
>>>   	efi_sigstore_free(dbx);
>>>   	pkcs7_free_message(msg);
>>> --
>>> 2.32.0
>>>
>
> Thanks
> /Ilias

Re: [RFC PATCH 1/2] efi_loader: fix dual signed image certification

[Ilias Apalodimas]

> > > >   		msg = pkcs7_parse_message(auth, auth_size);

[...]

> > > > @@ -717,32 +665,32 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > > >   		 */
> > > >   		/* try black-list first */
> > > >   		if (efi_signature_verify_one(regs, msg, dbx)) {
> > > > +			ret = false;
> > > >   			EFI_PRINT("Signature was rejected by \"dbx\"\n");
> > > > -			continue;
> > > > +			goto out;
> > > 
> > > If we go to "out" here, we have no chance to verify some cases:
> > > 1) An image has two signatures, for instance, one signed by SHA1 cert
> > >     and the other signed by SHA256 cert. A user wants to reject SHA1 cert
> > >     and put the cert in dbx.
> > 
> > I am not sure I am following,  what does he gain be rejecting the SHA1
> > portion only?  Avoid potential collisions?
> 
> If an image has a SHA1 and a SHA256 signature attached and SHA1 *or*
> SHA256 is in dbx, we must reject the image. Don't expect a dbx entry for
> each of the hashes. - But isn't this what your are doing here: for all
> signatures of the image look for one hit in dbx?
> 

Yes exactly. Any match on dbx of any certificate or sha256 of a certificate
or a sha256 of the executable will reject the image.

Regards
/Ilias
> Best regards
> 
> Heinrich
> 
> > 
> > >     But this image can and should yet be verified by SHA256 cert.
> > 
> > Why should it be verified?  My understanding of the EFI spec is that any match
> > in dbx of any certificate in the signing chain of the signature being verified means
> > reject the image.
> > 
> > > 2) A user knows that a given image is safe for some reason even though
> > >     he or she doesn't trust the certficate which is used for signing
> > >     the image.
> > > 
> > > -Takahiro Akashi
> > > 
> > > >   		}
> > > > 
> > > >   		if (!efi_signature_check_signers(msg, dbx)) {
> > > > +			ret = false;
> > > >   			EFI_PRINT("Signer(s) in \"dbx\"\n");
> > > > -			continue;
> > > > +			goto out;
> > > >   		}
> > > > 
> > > >   		/* try white-list */
> > > >   		if (efi_signature_verify(regs, msg, db, dbx)) {
> > > >   			ret = true;
> > > > -			break;
> > > > +			continue;
> > > >   		}
> > > > 
> > > >   		EFI_PRINT("Signature was not verified by \"db\"\n");
> > > > +	}
> > > > 
> > > > -		if (efi_signature_lookup_digest(regs, db, false)) {
> > > > -			ret = true;
> > > > -			break;
> > > > -		}
> > > > 
> > > > -		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
> > > > -	}
> > > > +	/* last resort try the image sha256 hash in db */
> > > > +	if (!ret && efi_signature_lookup_digest(regs, db, false))
> > > > +		ret = true;
> > > > 
> > > > -err:
> > > > +out:
> > > >   	efi_sigstore_free(db);
> > > >   	efi_sigstore_free(dbx);
> > > >   	pkcs7_free_message(msg);
> > > > --
> > > > 2.32.0
> > > > 
> > 
> > Thanks
> > /Ilias
> 

Re: [RFC PATCH 1/2] efi_loader: fix dual signed image certification

[AKASHI Takahiro]

On Thu, Feb 10, 2022 at 09:13:34AM +0200, Ilias Apalodimas wrote:
> On Thu, Feb 10, 2022 at 02:13:48PM +0900, AKASHI Takahiro wrote:
> > Hi Ilias,
> > 
> > Thank you for reviewing the logic.
> > 
> > On Fri, Feb 04, 2022 at 09:32:01AM +0200, Ilias Apalodimas wrote:
> > > The EFI spec allows for images to carry multiple signatures. Currently
> > > we don't adhere to the verification process for such images.
> > 
> > In this patch, you're trying to do three things:
> > * remove efi_image_unsigned_authenticate()
> > * pull efi_signature_lookup_digest() out of a while loop
> > * change the logic of authentication
> > 
> > I'd prefer to see those changes in separate patches for better reviewing.
> 
> I tried both and the current one seemed easier to review.  Heinrich any
> preference?

Those three changes are basically independent from each other.
Such changes should be in speparate patchs.
I believe it is what Heinrich always requires me to do.

> > 
> > > The spec says:
> > > "Multiple signatures are allowed to exist in the binary's certificate
> > > table (as per PE/COFF Section "Attribute Certificate Table"). Only one
> > > hash or signature is required to be present in db in order to pass
> > > validation, so long as neither the SHA-256 hash of the binary nor any
> > > present signature is reflected in dbx."
> > 
> > I have some concern about what the last phrase, "neither the SHA-256 hash
> > of the binary nor any present signature is reflected in dbx" means.
> > See the comment below.
> > 
> > > With our current implementation signing the image with two certificates
> > > and inserting both of them in db and one of them dbx doesn't always reject
> > > the image.  The rejection depends on the order that the image was signed
> > > and the order the certificates are read (and checked) in db.
> > > 
> > > While at it move the sha256 hash verification outside the signature
> > > checking loop, since it only needs to run once per image and get simplify
> > > the logic for authenticating an unsigned imahe using sha256 hashes.
> > > 
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > >  lib/efi_loader/efi_image_loader.c | 88 +++++++------------------------
> > >  1 file changed, 18 insertions(+), 70 deletions(-)
> > > 
> > > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> > > index f41cfa4fccd5..5df35939f702 100644
> > > --- a/lib/efi_loader/efi_image_loader.c
> > > +++ b/lib/efi_loader/efi_image_loader.c
> > > @@ -516,53 +516,6 @@ err:
> > >  }
> > >  
> > >  #ifdef CONFIG_EFI_SECURE_BOOT
> > > -/**
> > > - * efi_image_unsigned_authenticate() - authenticate unsigned image with
> > > - * SHA256 hash
> > > - * @regs:	List of regions to be verified
> > > - *
> > > - * If an image is not signed, it doesn't have a signature. In this case,
> > > - * its message digest is calculated and it will be compared with one of
> > > - * hash values stored in signature databases.
> > > - *
> > > - * Return:	true if authenticated, false if not
> > > - */
> > > -static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs)
> > > -{
> > > -	struct efi_signature_store *db = NULL, *dbx = NULL;
> > > -	bool ret = false;
> > > -
> > > -	dbx = efi_sigstore_parse_sigdb(u"dbx");
> > > -	if (!dbx) {
> > > -		EFI_PRINT("Getting signature database(dbx) failed\n");
> > > -		goto out;
> > > -	}
> > > -
> > > -	db = efi_sigstore_parse_sigdb(u"db");
> > > -	if (!db) {
> > > -		EFI_PRINT("Getting signature database(db) failed\n");
> > > -		goto out;
> > > -	}
> > > -
> > > -	/* try black-list first */
> > > -	if (efi_signature_lookup_digest(regs, dbx, true)) {
> > > -		EFI_PRINT("Image is not signed and its digest found in \"dbx\"\n");
> > > -		goto out;
> > > -	}
> > > -
> > > -	/* try white-list */
> > > -	if (efi_signature_lookup_digest(regs, db, false))
> > > -		ret = true;
> > > -	else
> > > -		EFI_PRINT("Image is not signed and its digest not found in \"db\" or \"dbx\"\n");
> > > -
> > > -out:
> > > -	efi_sigstore_free(db);
> > > -	efi_sigstore_free(dbx);
> > > -
> > > -	return ret;
> > > -}
> > > -
> > >  /**
> > >   * efi_image_authenticate() - verify a signature of signed image
> > >   * @efi:	Pointer to image
> > > @@ -608,14 +561,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > >  	if (!efi_image_parse(new_efi, efi_size, &regs, &wincerts,
> > >  			     &wincerts_len)) {
> > >  		EFI_PRINT("Parsing PE executable image failed\n");
> > > -		goto err;
> > > -	}
> > > -
> > > -	if (!wincerts) {
> > > -		/* The image is not signed */
> > > -		ret = efi_image_unsigned_authenticate(regs);
> > > -
> > > -		goto err;
> > > +		goto out;
> > >  	}
> > >  
> > >  	/*
> > > @@ -624,18 +570,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > >  	db = efi_sigstore_parse_sigdb(u"db");
> > >  	if (!db) {
> > >  		EFI_PRINT("Getting signature database(db) failed\n");
> > > -		goto err;
> > > +		goto out;
> > >  	}
> > >  
> > >  	dbx = efi_sigstore_parse_sigdb(u"dbx");
> > >  	if (!dbx) {
> > >  		EFI_PRINT("Getting signature database(dbx) failed\n");
> > > -		goto err;
> > > +		goto out;
> > >  	}
> > >  
> > >  	if (efi_signature_lookup_digest(regs, dbx, true)) {
> > >  		EFI_PRINT("Image's digest was found in \"dbx\"\n");
> > > -		goto err;
> > > +		goto out;
> > >  	}
> > >  
> > >  	/*
> > > @@ -678,7 +624,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > >  			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
> > >  				EFI_PRINT("Certificate type not supported: %pUs\n",
> > >  					  auth);
> > > -				continue;
> > > +				ret = false;
> > > +				goto out;
> > 
> > Why should we break the loop here?
> 
> We were trying to reject signature verification that we don't support,
> since the equivalent cert might be in dbx.  But I am not 100% sure taht's
> what we want here.
> 
> > 
> > >  			}
> > >  
> > >  			auth += sizeof(efi_guid_t);
> > > @@ -686,7 +633,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > >  		} else if (wincert->wCertificateType
> > >  				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> > >  			EFI_PRINT("Certificate type not supported\n");
> > > -			continue;
> > > +			ret = false;
> > > +			goto out;
> > >  		}
> > >  
> > >  		msg = pkcs7_parse_message(auth, auth_size);
> > > @@ -717,32 +665,32 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > >  		 */
> > >  		/* try black-list first */
> > >  		if (efi_signature_verify_one(regs, msg, dbx)) {
> > > +			ret = false;
> > >  			EFI_PRINT("Signature was rejected by \"dbx\"\n");
> > > -			continue;
> > > +			goto out;
> > 
> > If we go to "out" here, we have no chance to verify some cases:
> > 1) An image has two signatures, for instance, one signed by SHA1 cert
> >    and the other signed by SHA256 cert. A user wants to reject SHA1 cert
> >    and put the cert in dbx.
> 
> I am not sure I am following,  what does he gain be rejecting the SHA1
> portion only?  Avoid potential collisions?

I will reply to Heinrich's comment later.

-Takahiro Akashi

> >    But this image can and should yet be verified by SHA256 cert.
> 
> Why should it be verified?  My understanding of the EFI spec is that any match
> in dbx of any certificate in the signing chain of the signature being verified means 
> reject the image. 
> 
> > 2) A user knows that a given image is safe for some reason even though
> >    he or she doesn't trust the certficate which is used for signing
> >    the image.
> > 
> > -Takahiro Akashi
> > 
> > >  		}
> > >  
> > >  		if (!efi_signature_check_signers(msg, dbx)) {
> > > +			ret = false;
> > >  			EFI_PRINT("Signer(s) in \"dbx\"\n");
> > > -			continue;
> > > +			goto out;
> > >  		}
> > >  
> > >  		/* try white-list */
> > >  		if (efi_signature_verify(regs, msg, db, dbx)) {
> > >  			ret = true;
> > > -			break;
> > > +			continue;
> > >  		}
> > >  
> > >  		EFI_PRINT("Signature was not verified by \"db\"\n");
> > > +	}
> > >  
> > > -		if (efi_signature_lookup_digest(regs, db, false)) {
> > > -			ret = true;
> > > -			break;
> > > -		}
> > >  
> > > -		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
> > > -	}
> > > +	/* last resort try the image sha256 hash in db */
> > > +	if (!ret && efi_signature_lookup_digest(regs, db, false))
> > > +		ret = true;
> > >  
> > > -err:
> > > +out:
> > >  	efi_sigstore_free(db);
> > >  	efi_sigstore_free(dbx);
> > >  	pkcs7_free_message(msg);
> > > -- 
> > > 2.32.0
> > > 
> 
> Thanks
> /Ilias

Re: [RFC PATCH 1/2] efi_loader: fix dual signed image certification

[AKASHI Takahiro]

On Thu, Feb 10, 2022 at 09:33:46AM +0200, Ilias Apalodimas wrote:
> > > > >   		msg = pkcs7_parse_message(auth, auth_size);
> 
> [...]
> 
> > > > > @@ -717,32 +665,32 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > > > >   		 */
> > > > >   		/* try black-list first */
> > > > >   		if (efi_signature_verify_one(regs, msg, dbx)) {
> > > > > +			ret = false;
> > > > >   			EFI_PRINT("Signature was rejected by \"dbx\"\n");
> > > > > -			continue;
> > > > > +			goto out;
> > > > 
> > > > If we go to "out" here, we have no chance to verify some cases:
> > > > 1) An image has two signatures, for instance, one signed by SHA1 cert
> > > >     and the other signed by SHA256 cert. A user wants to reject SHA1 cert
> > > >     and put the cert in dbx.
> > > 
> > > I am not sure I am following,  what does he gain be rejecting the SHA1
> > > portion only?  Avoid potential collisions?
> > 
> > If an image has a SHA1 and a SHA256 signature attached and SHA1 *or*
> > SHA256 is in dbx, we must reject the image. Don't expect a dbx entry for
> > each of the hashes. - But isn't this what your are doing here: for all
> > signatures of the image look for one hit in dbx?
> > 
> 
> Yes exactly. Any match on dbx of any certificate or sha256 of a certificate
> or a sha256 of the executable will reject the image.

But we believe that SHA256-based signature is still valid
even if we don't trust SHA1.

> Regards
> /Ilias
> > Best regards
> > 
> > Heinrich
> > 
> > > 
> > > >     But this image can and should yet be verified by SHA256 cert.
> > > 
> > > Why should it be verified?  My understanding of the EFI spec is that any match
> > > in dbx of any certificate in the signing chain of the signature being verified means
> > > reject the image.
> > > 
> > > > 2) A user knows that a given image is safe for some reason even though
> > > >     he or she doesn't trust the certficate which is used for signing
> > > >     the image.

What do you think of this case?

-Takahiro Akashi

> > > > -Takahiro Akashi
> > > > 
> > > > >   		}
> > > > > 
> > > > >   		if (!efi_signature_check_signers(msg, dbx)) {
> > > > > +			ret = false;
> > > > >   			EFI_PRINT("Signer(s) in \"dbx\"\n");
> > > > > -			continue;
> > > > > +			goto out;
> > > > >   		}
> > > > > 
> > > > >   		/* try white-list */
> > > > >   		if (efi_signature_verify(regs, msg, db, dbx)) {
> > > > >   			ret = true;
> > > > > -			break;
> > > > > +			continue;
> > > > >   		}
> > > > > 
> > > > >   		EFI_PRINT("Signature was not verified by \"db\"\n");
> > > > > +	}
> > > > > 
> > > > > -		if (efi_signature_lookup_digest(regs, db, false)) {
> > > > > -			ret = true;
> > > > > -			break;
> > > > > -		}
> > > > > 
> > > > > -		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
> > > > > -	}
> > > > > +	/* last resort try the image sha256 hash in db */
> > > > > +	if (!ret && efi_signature_lookup_digest(regs, db, false))
> > > > > +		ret = true;
> > > > > 
> > > > > -err:
> > > > > +out:
> > > > >   	efi_sigstore_free(db);
> > > > >   	efi_sigstore_free(dbx);
> > > > >   	pkcs7_free_message(msg);
> > > > > --
> > > > > 2.32.0
> > > > > 
> > > 
> > > Thanks
> > > /Ilias
> > 

Re: [RFC PATCH 1/2] efi_loader: fix dual signed image certification

[Ilias Apalodimas]

On Thu, Feb 10, 2022 at 04:41:15PM +0900, AKASHI Takahiro wrote:
> On Thu, Feb 10, 2022 at 09:33:46AM +0200, Ilias Apalodimas wrote:
> > > > > >   		msg = pkcs7_parse_message(auth, auth_size);
> > 
> > [...]
> > 
> > > > > > @@ -717,32 +665,32 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > > > > >   		 */
> > > > > >   		/* try black-list first */
> > > > > >   		if (efi_signature_verify_one(regs, msg, dbx)) {
> > > > > > +			ret = false;
> > > > > >   			EFI_PRINT("Signature was rejected by \"dbx\"\n");
> > > > > > -			continue;
> > > > > > +			goto out;
> > > > > 
> > > > > If we go to "out" here, we have no chance to verify some cases:
> > > > > 1) An image has two signatures, for instance, one signed by SHA1 cert
> > > > >     and the other signed by SHA256 cert. A user wants to reject SHA1 cert
> > > > >     and put the cert in dbx.
> > > > 
> > > > I am not sure I am following,  what does he gain be rejecting the SHA1
> > > > portion only?  Avoid potential collisions?
> > > 
> > > If an image has a SHA1 and a SHA256 signature attached and SHA1 *or*
> > > SHA256 is in dbx, we must reject the image. Don't expect a dbx entry for
> > > each of the hashes. - But isn't this what your are doing here: for all
> > > signatures of the image look for one hit in dbx?
> > > 
> > 
> > Yes exactly. Any match on dbx of any certificate or sha256 of a certificate
> > or a sha256 of the executable will reject the image.
> 
> But we believe that SHA256-based signature is still valid
> even if we don't trust SHA1.

UEFI spec 2.9 page 1715 describes exaclty what we propose here as a
change.  The SHAxxx choise is irrelevant, any potential match should reject
the image. 

> 
> > Regards
> > /Ilias
> > > Best regards
> > > 
> > > Heinrich
> > > 
> > > > 
> > > > >     But this image can and should yet be verified by SHA256 cert.
> > > > 
> > > > Why should it be verified?  My understanding of the EFI spec is that any match
> > > > in dbx of any certificate in the signing chain of the signature being verified means
> > > > reject the image.
> > > > 
> > > > > 2) A user knows that a given image is safe for some reason even though
> > > > >     he or she doesn't trust the certficate which is used for signing
> > > > >     the image.

Then he should resign his image with a proper certificate.

Regards
/Ilias
> 
> What do you think of this case?
> 
> -Takahiro Akashi
> 
> > > > > -Takahiro Akashi
> > > > > 
> > > > > >   		}
> > > > > > 
> > > > > >   		if (!efi_signature_check_signers(msg, dbx)) {
> > > > > > +			ret = false;
> > > > > >   			EFI_PRINT("Signer(s) in \"dbx\"\n");
> > > > > > -			continue;
> > > > > > +			goto out;
> > > > > >   		}
> > > > > > 
> > > > > >   		/* try white-list */
> > > > > >   		if (efi_signature_verify(regs, msg, db, dbx)) {
> > > > > >   			ret = true;
> > > > > > -			break;
> > > > > > +			continue;
> > > > > >   		}
> > > > > > 
> > > > > >   		EFI_PRINT("Signature was not verified by \"db\"\n");
> > > > > > +	}
> > > > > > 
> > > > > > -		if (efi_signature_lookup_digest(regs, db, false)) {
> > > > > > -			ret = true;
> > > > > > -			break;
> > > > > > -		}
> > > > > > 
> > > > > > -		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
> > > > > > -	}
> > > > > > +	/* last resort try the image sha256 hash in db */
> > > > > > +	if (!ret && efi_signature_lookup_digest(regs, db, false))
> > > > > > +		ret = true;
> > > > > > 
> > > > > > -err:
> > > > > > +out:
> > > > > >   	efi_sigstore_free(db);
> > > > > >   	efi_sigstore_free(dbx);
> > > > > >   	pkcs7_free_message(msg);
> > > > > > --
> > > > > > 2.32.0
> > > > > > 
> > > > 
> > > > Thanks
> > > > /Ilias
> > > 

Re: [RFC PATCH 2/2] test/py: efi_secboot: adjust secure boot tests to code changes

[Ilias Apalodimas]

Akashi-san


On Thu, 10 Feb 2022 at 09:31, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> On Thu, Feb 10, 2022 at 09:14:25AM +0200, Ilias Apalodimas wrote:
> > Akashi-san
> >
> > On Thu, 10 Feb 2022 at 07:22, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Fri, Feb 04, 2022 at 09:32:02AM +0200, Ilias Apalodimas wrote:
> > > > The previous patch is changing U-Boot's behavior wrt certificate based
> > > > binary authentication.  Specifically an image who's digest of a
> > > > certificate is found in dbx is now rejected.  Fix the test accordingly
> > >
> > > Please not only fix the given test case, but also add more cases
> > > if needed or appropriate for wider coverage of corner cases.
> > > You mentioned in the previous commit that the order of certificates
> > > should not affect the verification result.
> > > If so, we need, at least, one more test case where the order of certificates
> > > in db is different.
> > >
> > > I think that trying to maintain the test scenario that way will help improve
> > > the robustness of verification logic.
> >
> > And we agree, but my concern right now is fix the existing use cases.
>
> But you have to verify the logic works in the same way whatever the order of
> certificates is. I think that is your intent in this patch.

Fair enough, I'll add a test case for that.  FWIW I think this patch
needs rework as is, because the last 2 cases reject the image.  But we
don't really know if the rejection comes from an x509 cert or it's
sha256.

Cheers
/Ilias
>
> -Takahiro Akashi
>
>
> > There are some SCT tests wrt certification of binaries,  so I intend
> > to port more cases for those in the future.
> >
> > Cheers
> > /Ilias
> > >
> > > -Takahiro Akashi
> > >
> > >
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > ---
> > > >  test/py/tests/test_efi_secboot/test_signed.py | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
> > > > index 0aee34479f55..7f5ec78261da 100644
> > > > --- a/test/py/tests/test_efi_secboot/test_signed.py
> > > > +++ b/test/py/tests/test_efi_secboot/test_signed.py
> > > > @@ -186,7 +186,7 @@ class TestEfiSignedImage(object):
> > > >              assert 'Hello, world!' in ''.join(output)
> > > >
> > > >          with u_boot_console.log.section('Test Case 5c'):
> > > > -            # Test Case 5c, not rejected if one of signatures (digest of
> > > > +            # Test Case 5c, rejected if one of signatures (digest of
> > > >              # certificate) is revoked
> > > >              output = u_boot_console.run_command_list([
> > > >                  'fatload host 0:1 4000000 dbx_hash.auth',
> > > > @@ -195,7 +195,8 @@ class TestEfiSignedImage(object):
> > > >              output = u_boot_console.run_command_list([
> > > >                  'efidebug boot next 1',
> > > >                  'efidebug test bootmgr'])
> > > > -            assert 'Hello, world!' in ''.join(output)
> > > > +            assert '\'HELLO\' failed' in ''.join(output)
> > > > +            assert 'efi_start_image() returned: 26' in ''.join(output)
> > > >
> > > >          with u_boot_console.log.section('Test Case 5d'):
> > > >              # Test Case 5d, rejected if both of signatures are revoked
> > > > --
> > > > 2.32.0
> > > >

Re: [RFC PATCH 1/2] efi_loader: fix dual signed image certification

[AKASHI Takahiro]

On Thu, Feb 10, 2022 at 09:55:20AM +0200, Ilias Apalodimas wrote:
> On Thu, Feb 10, 2022 at 04:41:15PM +0900, AKASHI Takahiro wrote:
> > On Thu, Feb 10, 2022 at 09:33:46AM +0200, Ilias Apalodimas wrote:
> > > > > > >   		msg = pkcs7_parse_message(auth, auth_size);
> > > 
> > > [...]
> > > 
> > > > > > > @@ -717,32 +665,32 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > > > > > >   		 */
> > > > > > >   		/* try black-list first */
> > > > > > >   		if (efi_signature_verify_one(regs, msg, dbx)) {
> > > > > > > +			ret = false;
> > > > > > >   			EFI_PRINT("Signature was rejected by \"dbx\"\n");
> > > > > > > -			continue;
> > > > > > > +			goto out;
> > > > > > 
> > > > > > If we go to "out" here, we have no chance to verify some cases:
> > > > > > 1) An image has two signatures, for instance, one signed by SHA1 cert
> > > > > >     and the other signed by SHA256 cert. A user wants to reject SHA1 cert
> > > > > >     and put the cert in dbx.
> > > > > 
> > > > > I am not sure I am following,  what does he gain be rejecting the SHA1
> > > > > portion only?  Avoid potential collisions?
> > > > 
> > > > If an image has a SHA1 and a SHA256 signature attached and SHA1 *or*
> > > > SHA256 is in dbx, we must reject the image. Don't expect a dbx entry for
> > > > each of the hashes. - But isn't this what your are doing here: for all
> > > > signatures of the image look for one hit in dbx?
> > > > 
> > > 
> > > Yes exactly. Any match on dbx of any certificate or sha256 of a certificate
> > > or a sha256 of the executable will reject the image.
> > 
> > But we believe that SHA256-based signature is still valid
> > even if we don't trust SHA1.
> 
> UEFI spec 2.9 page 1715 describes exaclty what we propose here as a
> change.  The SHAxxx choise is irrelevant, any potential match should reject
> the image. 
> 
> > 
> > > Regards
> > > /Ilias
> > > > Best regards
> > > > 
> > > > Heinrich
> > > > 
> > > > > 
> > > > > >     But this image can and should yet be verified by SHA256 cert.
> > > > > 
> > > > > Why should it be verified?  My understanding of the EFI spec is that any match
> > > > > in dbx of any certificate in the signing chain of the signature being verified means
> > > > > reject the image.
> > > > > 
> > > > > > 2) A user knows that a given image is safe for some reason even though
> > > > > >     he or she doesn't trust the certficate which is used for signing
> > > > > >     the image.
> 
> Then he should resign his image with a proper certificate.

No, I don't think so. The hash-based verification is for that.

-Takahiro Akashi

> Regards
> /Ilias
> > 
> > What do you think of this case?
> > 
> > -Takahiro Akashi
> > 
> > > > > > -Takahiro Akashi
> > > > > > 
> > > > > > >   		}
> > > > > > > 
> > > > > > >   		if (!efi_signature_check_signers(msg, dbx)) {
> > > > > > > +			ret = false;
> > > > > > >   			EFI_PRINT("Signer(s) in \"dbx\"\n");
> > > > > > > -			continue;
> > > > > > > +			goto out;
> > > > > > >   		}
> > > > > > > 
> > > > > > >   		/* try white-list */
> > > > > > >   		if (efi_signature_verify(regs, msg, db, dbx)) {
> > > > > > >   			ret = true;
> > > > > > > -			break;
> > > > > > > +			continue;
> > > > > > >   		}
> > > > > > > 
> > > > > > >   		EFI_PRINT("Signature was not verified by \"db\"\n");
> > > > > > > +	}
> > > > > > > 
> > > > > > > -		if (efi_signature_lookup_digest(regs, db, false)) {
> > > > > > > -			ret = true;
> > > > > > > -			break;
> > > > > > > -		}
> > > > > > > 
> > > > > > > -		EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
> > > > > > > -	}
> > > > > > > +	/* last resort try the image sha256 hash in db */
> > > > > > > +	if (!ret && efi_signature_lookup_digest(regs, db, false))
> > > > > > > +		ret = true;
> > > > > > > 
> > > > > > > -err:
> > > > > > > +out:
> > > > > > >   	efi_sigstore_free(db);
> > > > > > >   	efi_sigstore_free(dbx);
> > > > > > >   	pkcs7_free_message(msg);
> > > > > > > --
> > > > > > > 2.32.0
> > > > > > > 
> > > > > 
> > > > > Thanks
> > > > > /Ilias
> > > > 

Re: [RFC PATCH 1/2] efi_loader: fix dual signed image certification

[Ilias Apalodimas]

On Thu, 10 Feb 2022 at 10:01, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> On Thu, Feb 10, 2022 at 09:55:20AM +0200, Ilias Apalodimas wrote:
> > On Thu, Feb 10, 2022 at 04:41:15PM +0900, AKASHI Takahiro wrote:
> > > On Thu, Feb 10, 2022 at 09:33:46AM +0200, Ilias Apalodimas wrote:
> > > > > > > >                   msg = pkcs7_parse_message(auth, auth_size);
> > > >
> > > > [...]
> > > >
> > > > > > > > @@ -717,32 +665,32 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > > > > > > >                    */
> > > > > > > >                   /* try black-list first */
> > > > > > > >                   if (efi_signature_verify_one(regs, msg, dbx)) {
> > > > > > > > +                 ret = false;
> > > > > > > >                           EFI_PRINT("Signature was rejected by \"dbx\"\n");
> > > > > > > > -                 continue;
> > > > > > > > +                 goto out;
> > > > > > >
> > > > > > > If we go to "out" here, we have no chance to verify some cases:
> > > > > > > 1) An image has two signatures, for instance, one signed by SHA1 cert
> > > > > > >     and the other signed by SHA256 cert. A user wants to reject SHA1 cert
> > > > > > >     and put the cert in dbx.
> > > > > >
> > > > > > I am not sure I am following,  what does he gain be rejecting the SHA1
> > > > > > portion only?  Avoid potential collisions?
> > > > >
> > > > > If an image has a SHA1 and a SHA256 signature attached and SHA1 *or*
> > > > > SHA256 is in dbx, we must reject the image. Don't expect a dbx entry for
> > > > > each of the hashes. - But isn't this what your are doing here: for all
> > > > > signatures of the image look for one hit in dbx?
> > > > >
> > > >
> > > > Yes exactly. Any match on dbx of any certificate or sha256 of a certificate
> > > > or a sha256 of the executable will reject the image.
> > >
> > > But we believe that SHA256-based signature is still valid
> > > even if we don't trust SHA1.
> >
> > UEFI spec 2.9 page 1715 describes exaclty what we propose here as a
> > change.  The SHAxxx choise is irrelevant, any potential match should reject
> > the image.
> >
> > >
> > > > Regards
> > > > /Ilias
> > > > > Best regards
> > > > >
> > > > > Heinrich
> > > > >
> > > > > >
> > > > > > >     But this image can and should yet be verified by SHA256 cert.
> > > > > >
> > > > > > Why should it be verified?  My understanding of the EFI spec is that any match
> > > > > > in dbx of any certificate in the signing chain of the signature being verified means
> > > > > > reject the image.
> > > > > >
> > > > > > > 2) A user knows that a given image is safe for some reason even though
> > > > > > >     he or she doesn't trust the certficate which is used for signing
> > > > > > >     the image.
> >
> > Then he should resign his image with a proper certificate.
>
> No, I don't think so. The hash-based verification is for that.

If an image is rejected by a corresponding x509 in dbx or a shaxxx of
the certificate, execution should be denied. I am not really sure what
you are trying to describe here.

Regards
/Ilias

>
> -Takahiro Akashi
>
> > Regards
> > /Ilias
> > >
> > > What do you think of this case?
> > >
> > > -Takahiro Akashi
> > >
> > > > > > > -Takahiro Akashi
> > > > > > >
> > > > > > > >                   }
> > > > > > > >
> > > > > > > >                   if (!efi_signature_check_signers(msg, dbx)) {
> > > > > > > > +                 ret = false;
> > > > > > > >                           EFI_PRINT("Signer(s) in \"dbx\"\n");
> > > > > > > > -                 continue;
> > > > > > > > +                 goto out;
> > > > > > > >                   }
> > > > > > > >
> > > > > > > >                   /* try white-list */
> > > > > > > >                   if (efi_signature_verify(regs, msg, db, dbx)) {
> > > > > > > >                           ret = true;
> > > > > > > > -                 break;
> > > > > > > > +                 continue;
> > > > > > > >                   }
> > > > > > > >
> > > > > > > >                   EFI_PRINT("Signature was not verified by \"db\"\n");
> > > > > > > > + }
> > > > > > > >
> > > > > > > > -         if (efi_signature_lookup_digest(regs, db, false)) {
> > > > > > > > -                 ret = true;
> > > > > > > > -                 break;
> > > > > > > > -         }
> > > > > > > >
> > > > > > > > -         EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n");
> > > > > > > > - }
> > > > > > > > + /* last resort try the image sha256 hash in db */
> > > > > > > > + if (!ret && efi_signature_lookup_digest(regs, db, false))
> > > > > > > > +         ret = true;
> > > > > > > >
> > > > > > > > -err:
> > > > > > > > +out:
> > > > > > > >           efi_sigstore_free(db);
> > > > > > > >           efi_sigstore_free(dbx);
> > > > > > > >           pkcs7_free_message(msg);
> > > > > > > > --
> > > > > > > > 2.32.0
> > > > > > > >
> > > > > >
> > > > > > Thanks
> > > > > > /Ilias
> > > > >