[RFC PATCH] efi_loader: add sha384/512 on certificate revocation

[RFC PATCH] efi_loader: add sha384/512 on certificate revocation

[Ilias Apalodimas]

Currently we don't support sha384/512 for the X.509
certificate To-Be-Signed contents.  Moreover if we come across such a
hash we skip the check and approve the image,  although the image
might needs to be rejected.

It's worth noting here that efi_hash_regions() can now be reused from
efi_signature_lookup_digest() and add sha348/512 support there as well

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 include/efi_api.h              |  6 ++++
 lib/efi_loader/efi_signature.c | 62 ++++++++++++++++++++++++++--------
 2 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index 982c2001728d..b9a04958f9ba 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -1873,6 +1873,12 @@ struct efi_system_resource_table {
 #define EFI_CERT_X509_SHA256_GUID \
 	EFI_GUID(0x3bd2a492, 0x96c0, 0x4079, 0xb4, 0x20, \
 		 0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed)
+#define EFI_CERT_X509_SHA384_GUID \
+	EFI_GUID(0x7076876e, 0x80c2, 0x4ee6,		\
+		 0xaa, 0xd2, 0x28, 0xb3, 0x49, 0xa6, 0x86, 0x5b)
+#define EFI_CERT_X509_SHA512_GUID \
+	EFI_GUID(0x446dbf63, 0x2502, 0x4cda,		\
+		 0xbc, 0xfa, 0x24, 0x65, 0xd2, 0xb0, 0xfe, 0x9d)
 #define EFI_CERT_TYPE_PKCS7_GUID \
 	EFI_GUID(0x4aafd29d, 0x68df, 0x49ee, 0x8a, 0xa9, \
 		 0x34, 0x7d, 0x37, 0x56, 0x65, 0xa7)
diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index 79ed077ae7dd..392eae6c0d64 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -24,6 +24,8 @@ 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_x509_sha384 = EFI_CERT_X509_SHA384_GUID;
+const efi_guid_t efi_guid_cert_x509_sha512 = EFI_CERT_X509_SHA512_GUID;
 const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
 
 static u8 pkcs7_hdr[] = {
@@ -124,23 +126,32 @@ struct pkcs7_message *efi_parse_pkcs7_header(const void *buf,
  * Return:	true on success, false on error
  */
 static bool efi_hash_regions(struct image_region *regs, int count,
-			     void **hash, size_t *size)
+			     void **hash, size_t size)
 {
+	char hash_algo[16];
+	int ret;
+
+	/* basic sanity checking */
+	if (!size)
+		return false;
+
+	ret = snprintf(hash_algo, sizeof(hash_algo), "sha%ld", size * 8);
+	if (ret >= sizeof(hash_algo))
+		return false;
+
 	if (!*hash) {
-		*hash = calloc(1, SHA256_SUM_LEN);
+		*hash = calloc(1, size);
 		if (!*hash) {
 			EFI_PRINT("Out of memory\n");
 			return false;
 		}
 	}
-	if (size)
-		*size = SHA256_SUM_LEN;
 
-	hash_calculate("sha256", regs, count, *hash);
+	hash_calculate(hash_algo, regs, count, *hash);
 #ifdef DEBUG
 	EFI_PRINT("hash calculated:\n");
 	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
-		       *hash, SHA256_SUM_LEN, false);
+		       *hash, size, false);
 #endif
 
 	return true;
@@ -190,7 +201,7 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
 	struct efi_signature_store *siglist;
 	struct efi_sig_data *sig_data;
 	void *hash = NULL;
-	size_t size = 0;
+	size_t size = SHA256_SUM_LEN;
 	bool found = false;
 	bool hash_done = false;
 
@@ -216,7 +227,7 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
 			continue;
 
 		if (!hash_done &&
-		    !efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
+		    !efi_hash_regions(regs->reg, regs->num, &hash, size)) {
 			EFI_PRINT("Digesting an image failed\n");
 			break;
 		}
@@ -263,7 +274,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
 	struct efi_sig_data *sig_data;
 	struct image_region reg[1];
 	void *hash = NULL, *hash_tmp = NULL;
-	size_t size = 0;
+	size_t size = SHA256_SUM_LEN;
 	bool found = false;
 
 	EFI_PRINT("%s: Enter, %p, %p\n", __func__, cert, db);
@@ -278,7 +289,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
 	/* calculate hash of TBSCertificate */
 	reg[0].data = cert->tbs;
 	reg[0].size = cert->tbs_size;
-	if (!efi_hash_regions(reg, 1, &hash, &size))
+	if (!efi_hash_regions(reg, 1, &hash, size))
 		goto out;
 
 	EFI_PRINT("%s: searching for %s\n", __func__, cert->subject);
@@ -300,7 +311,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
 				  cert_tmp->subject);
 			reg[0].data = cert_tmp->tbs;
 			reg[0].size = cert_tmp->tbs_size;
-			if (!efi_hash_regions(reg, 1, &hash_tmp, NULL))
+			if (!efi_hash_regions(reg, 1, &hash_tmp, size))
 				goto out;
 
 			x509_free_certificate(cert_tmp);
@@ -377,6 +388,26 @@ out:
 	return verified;
 }
 
+/** guid_to_sha_len - return the sha size in bytes for a given guid
+ *                    used of EFI security databases
+ *
+ * @guid: guid to check
+ *
+ * Return: len or 0 if no match is found
+ */
+static int guid_to_sha_len(efi_guid_t *guid)
+{
+	int size = 0;
+
+	if (!guidcmp(guid, &efi_guid_cert_x509_sha256))
+		size = SHA256_SUM_LEN;
+	else if (!guidcmp(guid, &efi_guid_cert_x509_sha384))
+		size = SHA384_SUM_LEN;
+	else if (!guidcmp(guid, &efi_guid_cert_x509_sha512))
+		size = SHA512_SUM_LEN;
+
+	return size;
+}
 /**
  * efi_signature_check_revocation - check revocation with dbx
  * @sinfo:	Signer's info
@@ -400,7 +431,7 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
 	struct efi_sig_data *sig_data;
 	struct image_region reg[1];
 	void *hash = NULL;
-	size_t size = 0;
+	size_t size = SHA256_SUM_LEN;
 	time64_t revoc_time;
 	bool revoked = false;
 
@@ -411,13 +442,14 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
 
 	EFI_PRINT("Checking revocation against %s\n", cert->subject);
 	for (siglist = dbx; siglist; siglist = siglist->next) {
-		if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256))
+		size = guid_to_sha_len(&siglist->sig_type);
+		if (!size)
 			continue;
 
 		/* calculate hash of TBSCertificate */
 		reg[0].data = cert->tbs;
 		reg[0].size = cert->tbs_size;
-		if (!efi_hash_regions(reg, 1, &hash, &size))
+		if (!efi_hash_regions(reg, 1, &hash, size))
 			goto out;
 
 		for (sig_data = siglist->sig_data_list; sig_data;
@@ -500,7 +532,7 @@ bool efi_signature_verify(struct efi_image_regions *regs,
 		 */
 		if (!msg->data &&
 		    !efi_hash_regions(regs->reg, regs->num,
-				      (void **)&sinfo->sig->digest, NULL)) {
+				      (void **)&sinfo->sig->digest, SHA256_SUM_LEN)) {
 			EFI_PRINT("Digesting an image failed\n");
 			goto out;
 		}
-- 
2.32.0

Re: [RFC PATCH] efi_loader: add sha384/512 on certificate revocation

[AKASHI Takahiro]

On Mon, Apr 11, 2022 at 10:56:22AM +0300, Ilias Apalodimas wrote:
> Currently we don't support sha384/512 for the X.509
> certificate To-Be-Signed contents.  Moreover if we come across such a
> hash we skip the check and approve the image,  although the image
> might needs to be rejected.

Are you sure? You seem to be talking about efi_signature_check_revocation() here.
Please be more specific.

> It's worth noting here that efi_hash_regions() can now be reused from
> efi_signature_lookup_digest() and add sha348/512 support there as well
> 
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  include/efi_api.h              |  6 ++++
>  lib/efi_loader/efi_signature.c | 62 ++++++++++++++++++++++++++--------
>  2 files changed, 53 insertions(+), 15 deletions(-)
> 
> diff --git a/include/efi_api.h b/include/efi_api.h
> index 982c2001728d..b9a04958f9ba 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -1873,6 +1873,12 @@ struct efi_system_resource_table {
>  #define EFI_CERT_X509_SHA256_GUID \
>  	EFI_GUID(0x3bd2a492, 0x96c0, 0x4079, 0xb4, 0x20, \
>  		 0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed)
> +#define EFI_CERT_X509_SHA384_GUID \
> +	EFI_GUID(0x7076876e, 0x80c2, 0x4ee6,		\
> +		 0xaa, 0xd2, 0x28, 0xb3, 0x49, 0xa6, 0x86, 0x5b)
> +#define EFI_CERT_X509_SHA512_GUID \
> +	EFI_GUID(0x446dbf63, 0x2502, 0x4cda,		\
> +		 0xbc, 0xfa, 0x24, 0x65, 0xd2, 0xb0, 0xfe, 0x9d)
>  #define EFI_CERT_TYPE_PKCS7_GUID \
>  	EFI_GUID(0x4aafd29d, 0x68df, 0x49ee, 0x8a, 0xa9, \
>  		 0x34, 0x7d, 0x37, 0x56, 0x65, 0xa7)
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index 79ed077ae7dd..392eae6c0d64 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -24,6 +24,8 @@ 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_x509_sha384 = EFI_CERT_X509_SHA384_GUID;
> +const efi_guid_t efi_guid_cert_x509_sha512 = EFI_CERT_X509_SHA512_GUID;

Please add, at least, one test case along with this patch
if you want to expand the coverage of UEFI specification.

>  const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
>  
>  static u8 pkcs7_hdr[] = {
> @@ -124,23 +126,32 @@ struct pkcs7_message *efi_parse_pkcs7_header(const void *buf,
>   * Return:	true on success, false on error
>   */
>  static bool efi_hash_regions(struct image_region *regs, int count,
> -			     void **hash, size_t *size)
> +			     void **hash, size_t size)

It would be better to hand off either an algorithm name or a corresponding guid
rather than "size" as an identification of hash algo for extending this function
in the future.

>  {
> +	char hash_algo[16];
> +	int ret;
> +
> +	/* basic sanity checking */
> +	if (!size)
> +		return false;
> +
> +	ret = snprintf(hash_algo, sizeof(hash_algo), "sha%ld", size * 8);
> +	if (ret >= sizeof(hash_algo))
> +		return false;
> +
>  	if (!*hash) {
> -		*hash = calloc(1, SHA256_SUM_LEN);
> +		*hash = calloc(1, size);
>  		if (!*hash) {
>  			EFI_PRINT("Out of memory\n");
>  			return false;
>  		}
>  	}
> -	if (size)
> -		*size = SHA256_SUM_LEN;
>  
> -	hash_calculate("sha256", regs, count, *hash);
> +	hash_calculate(hash_algo, regs, count, *hash);
>  #ifdef DEBUG
>  	EFI_PRINT("hash calculated:\n");
>  	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> -		       *hash, SHA256_SUM_LEN, false);
> +		       *hash, size, false);
>  #endif
>  
>  	return true;
> @@ -190,7 +201,7 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
>  	struct efi_signature_store *siglist;
>  	struct efi_sig_data *sig_data;
>  	void *hash = NULL;
> -	size_t size = 0;
> +	size_t size = SHA256_SUM_LEN;
>  	bool found = false;
>  	bool hash_done = false;
>  
> @@ -216,7 +227,7 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
>  			continue;
>  
>  		if (!hash_done &&
> -		    !efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
> +		    !efi_hash_regions(regs->reg, regs->num, &hash, size)) {
>  			EFI_PRINT("Digesting an image failed\n");
>  			break;
>  		}
> @@ -263,7 +274,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
>  	struct efi_sig_data *sig_data;
>  	struct image_region reg[1];
>  	void *hash = NULL, *hash_tmp = NULL;
> -	size_t size = 0;
> +	size_t size = SHA256_SUM_LEN;
>  	bool found = false;
>  
>  	EFI_PRINT("%s: Enter, %p, %p\n", __func__, cert, db);
> @@ -278,7 +289,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
>  	/* calculate hash of TBSCertificate */
>  	reg[0].data = cert->tbs;
>  	reg[0].size = cert->tbs_size;
> -	if (!efi_hash_regions(reg, 1, &hash, &size))
> +	if (!efi_hash_regions(reg, 1, &hash, size))
>  		goto out;
>  
>  	EFI_PRINT("%s: searching for %s\n", __func__, cert->subject);
> @@ -300,7 +311,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
>  				  cert_tmp->subject);
>  			reg[0].data = cert_tmp->tbs;
>  			reg[0].size = cert_tmp->tbs_size;
> -			if (!efi_hash_regions(reg, 1, &hash_tmp, NULL))
> +			if (!efi_hash_regions(reg, 1, &hash_tmp, size))
>  				goto out;
>  
>  			x509_free_certificate(cert_tmp);
> @@ -377,6 +388,26 @@ out:
>  	return verified;
>  }
>  
> +/** guid_to_sha_len - return the sha size in bytes for a given guid
> + *                    used of EFI security databases
> + *
> + * @guid: guid to check
> + *
> + * Return: len or 0 if no match is found
> + */
> +static int guid_to_sha_len(efi_guid_t *guid)
> +{
> +	int size = 0;
> +
> +	if (!guidcmp(guid, &efi_guid_cert_x509_sha256))
> +		size = SHA256_SUM_LEN;
> +	else if (!guidcmp(guid, &efi_guid_cert_x509_sha384))
> +		size = SHA384_SUM_LEN;
> +	else if (!guidcmp(guid, &efi_guid_cert_x509_sha512))
> +		size = SHA512_SUM_LEN;
> +
> +	return size;
> +}
>  /**
>   * efi_signature_check_revocation - check revocation with dbx
>   * @sinfo:	Signer's info
> @@ -400,7 +431,7 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
>  	struct efi_sig_data *sig_data;
>  	struct image_region reg[1];
>  	void *hash = NULL;
> -	size_t size = 0;
> +	size_t size = SHA256_SUM_LEN;
>  	time64_t revoc_time;
>  	bool revoked = false;
>  
> @@ -411,13 +442,14 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
>  
>  	EFI_PRINT("Checking revocation against %s\n", cert->subject);
>  	for (siglist = dbx; siglist; siglist = siglist->next) {
> -		if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256))
> +		size = guid_to_sha_len(&siglist->sig_type);
> +		if (!size)

Even with this change,
> Moreover if we come across such a
> hash we skip the check and approve the image,  although the image
> might needs to be rejected.

the behavior won't be fixed.

-Takahiro Akashi

>  			continue;
>  
>  		/* calculate hash of TBSCertificate */
>  		reg[0].data = cert->tbs;
>  		reg[0].size = cert->tbs_size;
> -		if (!efi_hash_regions(reg, 1, &hash, &size))
> +		if (!efi_hash_regions(reg, 1, &hash, size))
>  			goto out;
>  
>  		for (sig_data = siglist->sig_data_list; sig_data;
> @@ -500,7 +532,7 @@ bool efi_signature_verify(struct efi_image_regions *regs,
>  		 */
>  		if (!msg->data &&
>  		    !efi_hash_regions(regs->reg, regs->num,
> -				      (void **)&sinfo->sig->digest, NULL)) {
> +				      (void **)&sinfo->sig->digest, SHA256_SUM_LEN)) {
>  			EFI_PRINT("Digesting an image failed\n");
>  			goto out;
>  		}
> -- 
> 2.32.0
> 

Re: [RFC PATCH] efi_loader: add sha384/512 on certificate revocation

[Ilias Apalodimas]

Hi Akashi-san,

> On Mon, Apr 11, 2022 at 05:31:08PM +0900, AKASHI Takahiro wrote:
> On Mon, Apr 11, 2022 at 10:56:22AM +0300, Ilias Apalodimas wrote:
> > Currently we don't support sha384/512 for the X.509
> > certificate To-Be-Signed contents.  Moreover if we come across such a
> > hash we skip the check and approve the image,  although the image
> > might needs to be rejected.
> 
> Are you sure? You seem to be talking about efi_signature_check_revocation() here.
> Please be more specific.

Arm has a security ACS testsuite [1].  The whole checking fails exactly on
this bug. 

> 
> > It's worth noting here that efi_hash_regions() can now be reused from
> > efi_signature_lookup_digest() and add sha348/512 support there as well
> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  include/efi_api.h              |  6 ++++
> >  lib/efi_loader/efi_signature.c | 62 ++++++++++++++++++++++++++--------
> >  2 files changed, 53 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/efi_api.h b/include/efi_api.h
> > index 982c2001728d..b9a04958f9ba 100644
> > --- a/include/efi_api.h
> > +++ b/include/efi_api.h
> > @@ -1873,6 +1873,12 @@ struct efi_system_resource_table {
> >  #define EFI_CERT_X509_SHA256_GUID \
> >  	EFI_GUID(0x3bd2a492, 0x96c0, 0x4079, 0xb4, 0x20, \
> >  		 0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed)
> > +#define EFI_CERT_X509_SHA384_GUID \
> > +	EFI_GUID(0x7076876e, 0x80c2, 0x4ee6,		\
> > +		 0xaa, 0xd2, 0x28, 0xb3, 0x49, 0xa6, 0x86, 0x5b)
> > +#define EFI_CERT_X509_SHA512_GUID \
> > +	EFI_GUID(0x446dbf63, 0x2502, 0x4cda,		\
> > +		 0xbc, 0xfa, 0x24, 0x65, 0xd2, 0xb0, 0xfe, 0x9d)
> >  #define EFI_CERT_TYPE_PKCS7_GUID \
> >  	EFI_GUID(0x4aafd29d, 0x68df, 0x49ee, 0x8a, 0xa9, \
> >  		 0x34, 0x7d, 0x37, 0x56, 0x65, 0xa7)
> > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> > index 79ed077ae7dd..392eae6c0d64 100644
> > --- a/lib/efi_loader/efi_signature.c
> > +++ b/lib/efi_loader/efi_signature.c
> > @@ -24,6 +24,8 @@ 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_x509_sha384 = EFI_CERT_X509_SHA384_GUID;
> > +const efi_guid_t efi_guid_cert_x509_sha512 = EFI_CERT_X509_SHA512_GUID;
> 
> Please add, at least, one test case along with this patch
> if you want to expand the coverage of UEFI specification.

Sure, once the RFC is discussed 

> 
> >  const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
> >  
> >  static u8 pkcs7_hdr[] = {
> > @@ -124,23 +126,32 @@ struct pkcs7_message *efi_parse_pkcs7_header(const void *buf,
> >   * Return:	true on success, false on error
> >   */
> >  static bool efi_hash_regions(struct image_region *regs, int count,
> > -			     void **hash, size_t *size)
> > +			     void **hash, size_t size)
> 
> It would be better to hand off either an algorithm name or a corresponding guid
> rather than "size" as an identification of hash algo for extending this function
> in the future.
> 

That's exactly what happens here.  There's a  guid_to_sha_len() that
derives the len for us.  I'd rather keep this as is, since the impact on
the rest of the code is minimal. 

> >  {
> > +	char hash_algo[16];
> > +	int ret;
> > +
> > +	/* basic sanity checking */
> > +	if (!size)
> > +		return false;
> > +
> > +	ret = snprintf(hash_algo, sizeof(hash_algo), "sha%ld", size * 8);
> > +	if (ret >= sizeof(hash_algo))
> > +		return false;
> > +
> >  	if (!*hash) {
> > -		*hash = calloc(1, SHA256_SUM_LEN);
> > +		*hash = calloc(1, size);
> >  		if (!*hash) {
> >  			EFI_PRINT("Out of memory\n");
> >  			return false;
> >  		}
> >  	}
> > -	if (size)
> > -		*size = SHA256_SUM_LEN;
> >  
> > -	hash_calculate("sha256", regs, count, *hash);
> > +	hash_calculate(hash_algo, regs, count, *hash);
> >  #ifdef DEBUG
> >  	EFI_PRINT("hash calculated:\n");
> >  	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> > -		       *hash, SHA256_SUM_LEN, false);
> > +		       *hash, size, false);
> >  #endif
> >  
> >  	return true;
> > @@ -190,7 +201,7 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
> >  	struct efi_signature_store *siglist;
> >  	struct efi_sig_data *sig_data;
> >  	void *hash = NULL;
> > -	size_t size = 0;
> > +	size_t size = SHA256_SUM_LEN;
> >  	bool found = false;
> >  	bool hash_done = false;
> >  
> > @@ -216,7 +227,7 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
> >  			continue;
> >  
> >  		if (!hash_done &&
> > -		    !efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
> > +		    !efi_hash_regions(regs->reg, regs->num, &hash, size)) {
> >  			EFI_PRINT("Digesting an image failed\n");
> >  			break;
> >  		}
> > @@ -263,7 +274,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
> >  	struct efi_sig_data *sig_data;
> >  	struct image_region reg[1];
> >  	void *hash = NULL, *hash_tmp = NULL;
> > -	size_t size = 0;
> > +	size_t size = SHA256_SUM_LEN;
> >  	bool found = false;
> >  
> >  	EFI_PRINT("%s: Enter, %p, %p\n", __func__, cert, db);
> > @@ -278,7 +289,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
> >  	/* calculate hash of TBSCertificate */
> >  	reg[0].data = cert->tbs;
> >  	reg[0].size = cert->tbs_size;
> > -	if (!efi_hash_regions(reg, 1, &hash, &size))
> > +	if (!efi_hash_regions(reg, 1, &hash, size))
> >  		goto out;
> >  
> >  	EFI_PRINT("%s: searching for %s\n", __func__, cert->subject);
> > @@ -300,7 +311,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
> >  				  cert_tmp->subject);
> >  			reg[0].data = cert_tmp->tbs;
> >  			reg[0].size = cert_tmp->tbs_size;
> > -			if (!efi_hash_regions(reg, 1, &hash_tmp, NULL))
> > +			if (!efi_hash_regions(reg, 1, &hash_tmp, size))
> >  				goto out;
> >  
> >  			x509_free_certificate(cert_tmp);
> > @@ -377,6 +388,26 @@ out:
> >  	return verified;
> >  }
> >  
> > +/** guid_to_sha_len - return the sha size in bytes for a given guid
> > + *                    used of EFI security databases
> > + *
> > + * @guid: guid to check
> > + *
> > + * Return: len or 0 if no match is found
> > + */
> > +static int guid_to_sha_len(efi_guid_t *guid)
> > +{
> > +	int size = 0;
> > +
> > +	if (!guidcmp(guid, &efi_guid_cert_x509_sha256))
> > +		size = SHA256_SUM_LEN;
> > +	else if (!guidcmp(guid, &efi_guid_cert_x509_sha384))
> > +		size = SHA384_SUM_LEN;
> > +	else if (!guidcmp(guid, &efi_guid_cert_x509_sha512))
> > +		size = SHA512_SUM_LEN;
> > +
> > +	return size;
> > +}
> >  /**
> >   * efi_signature_check_revocation - check revocation with dbx
> >   * @sinfo:	Signer's info
> > @@ -400,7 +431,7 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
> >  	struct efi_sig_data *sig_data;
> >  	struct image_region reg[1];
> >  	void *hash = NULL;
> > -	size_t size = 0;
> > +	size_t size = SHA256_SUM_LEN;
> >  	time64_t revoc_time;
> >  	bool revoked = false;
> >  
> > @@ -411,13 +442,14 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
> >  
> >  	EFI_PRINT("Checking revocation against %s\n", cert->subject);
> >  	for (siglist = dbx; siglist; siglist = siglist->next) {
> > -		if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256))
> > +		size = guid_to_sha_len(&siglist->sig_type);
> > +		if (!size)
> 
> Even with this change,
> > Moreover if we come across such a
> > hash we skip the check and approve the image,  although the image
> > might needs to be rejected.
> 
> the behavior won't be fixed.
> 

Are there other x509_shaxxx for dbx that the spec describes?

> -Takahiro Akashi
> 
> >  			continue;
> >  
> >  		/* calculate hash of TBSCertificate */
> >  		reg[0].data = cert->tbs;
> >  		reg[0].size = cert->tbs_size;
> > -		if (!efi_hash_regions(reg, 1, &hash, &size))
> > +		if (!efi_hash_regions(reg, 1, &hash, size))
> >  			goto out;
> >  
> >  		for (sig_data = siglist->sig_data_list; sig_data;
> > @@ -500,7 +532,7 @@ bool efi_signature_verify(struct efi_image_regions *regs,
> >  		 */
> >  		if (!msg->data &&
> >  		    !efi_hash_regions(regs->reg, regs->num,
> > -				      (void **)&sinfo->sig->digest, NULL)) {
> > +				      (void **)&sinfo->sig->digest, SHA256_SUM_LEN)) {
> >  			EFI_PRINT("Digesting an image failed\n");
> >  			goto out;
> >  		}
> > -- 
> > 2.32.0
> > 

[1] https://github.com/ARM-software/arm-systemready/tree/security-extension-acs

Thanks
/Ilias

Re: [RFC PATCH] efi_loader: add sha384/512 on certificate revocation

[Stuart Yoder]

On 4/11/22 3:40 AM, Ilias Apalodimas wrote:
> Hi Akashi-san,
> 
>> On Mon, Apr 11, 2022 at 05:31:08PM +0900, AKASHI Takahiro wrote:
>> On Mon, Apr 11, 2022 at 10:56:22AM +0300, Ilias Apalodimas wrote:
>>> Currently we don't support sha384/512 for the X.509
>>> certificate To-Be-Signed contents.  Moreover if we come across such a
>>> hash we skip the check and approve the image,  although the image
>>> might needs to be rejected.
>>
>> Are you sure? You seem to be talking about efi_signature_check_revocation() here.
>> Please be more specific.
> 
> Arm has a security ACS testsuite [1].  The whole checking fails exactly on
> this bug.

[cut]

> 
> [1] https://github.com/ARM-software/arm-systemready/tree/security-extension-acs
> 
> Thanks
> /Ilias

Note, the above link is from the alpha release.  Please use the EAC
release branch:

https://github.com/ARM-software/arm-systemready/tree/security-interface-extension-acs

Thanks,
Stuart