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

Ilias,

On Mon, Apr 18, 2022 at 09:07:22PM +0300, Ilias Apalodimas wrote:

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

Rework the code a bit and fix it by adding an array of structs with the supported GUIDs, len and literal used in the U-Boot crypto APIs instead of hardcoding the GUID types.

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

changes since v2:

• updated changelog (there was no v1)

changes since RFC:

• add an array of structs with the algo info info of a function
• checking hash_calculate result in efi_hash_regions()

include/efi_api.h | 6 +++ include/efi_loader.h | 7 +++ lib/efi_loader/efi_helper.c | 85 ++++++++++++++++++++++++++++++++++ lib/efi_loader/efi_signature.c | 76 +++++++++++++++++++++--------- 4 files changed, 151 insertions(+), 23 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/include/efi_loader.h b/include/efi_loader.h index af36639ec6a7..ce221ee9317b 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -300,6 +300,8 @@ extern const efi_guid_t efi_guid_image_security_database; extern const efi_guid_t efi_guid_sha256; extern const efi_guid_t efi_guid_cert_x509; extern const efi_guid_t efi_guid_cert_x509_sha256; +extern const efi_guid_t efi_guid_cert_x509_sha384; +extern const efi_guid_t efi_guid_cert_x509_sha512; extern const efi_guid_t efi_guid_cert_type_pkcs7;

/* GUID of RNG protocol */ @@ -671,6 +673,11 @@ efi_status_t efi_file_size(struct efi_file_handle *fh, efi_uintn_t *size); /* get a device path from a Boot#### option */ struct efi_device_path *efi_get_dp_from_boot(const efi_guid_t guid);

+/* get len, string (used in u-boot crypto from a guid */ +const char *guid_to_sha_str(const efi_guid_t *guid); +int guid_to_sha_len(const efi_guid_t *guid); +int algo_to_len(const char *algo);

/**

• efi_size_in_pages() - convert size in bytes to size in pages

diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c index 802d39ed97b6..c186ba4a3c01 100644 --- a/lib/efi_loader/efi_helper.c +++ b/lib/efi_loader/efi_helper.c @@ -92,3 +92,88 @@ err: free(var_value); return NULL; }

+const struct guid_to_hash_map {

• efi_guid_t guid;
• const char algo[32];
• u32 bits;

+} guid_to_hash[] = {

• {

• EFI_CERT_X509_SHA256_GUID,
  

• "sha256",
  

• 256,
  

=> SHA256_SUM_LEN * 8

• },
• {

• EFI_CERT_SHA256_GUID,
  

• "sha256",
  

• 256,
  

• },
• {

• EFI_CERT_X509_SHA384_GUID,
  

• "sha384",
  

• 384,
  

• },
• {

• EFI_CERT_X509_SHA512_GUID,
  

• "sha512",
  

• 512,
  

• },

+};

+#define MAX_GUID_TO_HASH_COUNT ARRAY_SIZE(guid_to_hash)

+/** guid_to_sha_str - return the sha string e.g "sha256" for a given guid

• •                used on EFI security databases
    

• • @guid: guid to check
• • Return: len or 0 if no match is found
• */

+const char *guid_to_sha_str(const efi_guid_t *guid) +{

• size_t i;
• for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {

• if (!guidcmp(guid, &guid_to_hash[i].guid))
  

• 	return guid_to_hash[i].algo;
  

• }
• return NULL;

+}

+/** guid_to_sha_len - return the sha size in bytes for a given guid

• •                used on EFI security databases
    

• • @guid: guid to check
• • Return: len or 0 if no match is found
• */

+int guid_to_sha_len(const efi_guid_t *guid) +{

• size_t i;
• for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {

• if (!guidcmp(guid, &guid_to_hash[i].guid))
  

• 	return guid_to_hash[i].bits / 8;
  

• }
• return 0;

+}

If we have algo_to_len(), we don't need guid_to_sha_len().

+/** algo_to_len - return the sha size in bytes for a given string

• • @guid: string to check
• • Return: len or 0 if no match is found
• */

+int algo_to_len(const char *algo)

This function seems to be quite generic and useful for others. Why not implement it along with hash_calculate() in hash-checksum.c (without using guid_to_hash[]).

+{

• size_t i;
• for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {

• if (!strcmp(algo, guid_to_hash[i].algo))
  

• 	return guid_to_hash[i].bits / 8;
  

• }
• return 0;

+} diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c index 79ed077ae7dd..cf01f21b4d04 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, const char *hash_algo, int *len)
  

{

• int ret;
• if (!hash_algo || !len)

• return false;
  

• *len = algo_to_len(hash_algo);

Please modify it this way; int hash_len;

hash_len = algo_to_len(hash_algo); if (len) *len = hash_len;

Then use hash_len instead of *len hereafterr.

This way, we don't have to define a unused local variable below code.

• if (!*len)

• return false;
  

• if (!*hash) {

• *hash = calloc(1, SHA256_SUM_LEN);
  

• *hash = calloc(1, *len);
  

  if (!*hash) { EFI_PRINT("Out of memory\n"); return false; } }

• if (size)

• *size = SHA256_SUM_LEN;
  
  

• hash_calculate("sha256", regs, count, *hash);

• ret = hash_calculate(hash_algo, regs, count, *hash);
• if (ret)

• return false;
  

#ifdef DEBUG EFI_PRINT("hash calculated:\n"); print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1,

•        *hash, SHA256_SUM_LEN, false);
  

•        *hash, *len, false);
  

#endif

return true; @@ -190,7 +201,6 @@ 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; bool found = false; bool hash_done = false;

@@ -200,6 +210,8 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs, goto out;

for (siglist = db; siglist; siglist = siglist->next) {

• int len = 0;
  

• const char *hash_algo = NULL;
  

  /*
  • if the hash algorithm is unsupported and we get an entry in
  • dbx reject the image

@@ -215,8 +227,14 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs, if (guidcmp(&siglist->sig_type, &efi_guid_sha256)) continue;

• hash_algo = guid_to_sha_str(&efi_guid_sha256);
  

• /*
  

•  * We could check size and hash_algo but efi_hash_regions()
  

•  * will do that for us
  

•  */
  

  if (!hash_done &&

•     !efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
  

•     !efi_hash_regions(regs->reg, regs->num, &hash, hash_algo,
  

• 		      &len)) {
  EFI_PRINT("Digesting an image failed\n");
  break;
  

  }

@@ -229,8 +247,8 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs, print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, sig_data->data, sig_data->size, false); #endif

• 	if (sig_data->size == size &&
  

• 	    !memcmp(sig_data->data, hash, size)) {
  

• 	if (sig_data->size == len &&
  

• 	    !memcmp(sig_data->data, hash, len)) {
  	found = true;
  	free(hash);
  	goto out;
  

@@ -263,8 +281,9 @@ 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;

• int len = 0; bool found = false;

• const char *hash_algo = NULL;

  EFI_PRINT("%s: Enter, %p, %p\n", __func__, cert, db);

@@ -278,7 +297,11 @@ 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))

• /* We just need any sha256 algo to start the matching */
• hash_algo = guid_to_sha_str(&efi_guid_sha256);

Why not check if hash_algo is NULL?

• len = guid_to_sha_len(&efi_guid_sha256);

=> len = algo_to_len(hash_algo);

Or even we don't need this line as efi_has_regions() will set it for us.

• if (!efi_hash_regions(reg, 1, &hash, hash_algo, &len)) goto out;

  EFI_PRINT("%s: searching for %s\n", __func__, cert->subject);

@@ -290,6 +313,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert, for (sig_data = siglist->sig_data_list; sig_data; sig_data = sig_data->next) { struct x509_certificate *cert_tmp;

• 	int len_tmp;
  

We don't need len_tmp if the change I mentioned above is made. # we don't want to define a unused variable.

	cert_tmp = x509_cert_parse(sig_data->data,
				   sig_data->size);

@@ -300,12 +324,13 @@ 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, hash_algo,
  

• 			      &len_tmp))
  

=> NULL

		goto out;

	x509_free_certificate(cert_tmp);

• 	if (!memcmp(hash, hash_tmp, size)) {
  

• 	if (!memcmp(hash, hash_tmp, len)) {
  	found = true;
  	goto out;
  }
  

@@ -400,9 +425,10 @@ 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;

• int len = 0; time64_t revoc_time; bool revoked = false;

• const char *hash_algo = NULL;

  EFI_PRINT("%s: Enter, %p, %p, %p\n", __func__, sinfo, cert, dbx);

@@ -411,13 +437,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))
  

• hash_algo = guid_to_sha_str(&siglist->sig_type);
  

• if (!hash_algo)
  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, hash_algo, &len))
  goto out;
  
  

  for (sig_data = siglist->sig_data_list; sig_data;

@@ -429,18 +456,18 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo, * }; */ #ifdef DEBUG

• 	if (sig_data->size >= size) {
  

• 	if (sig_data->size >= len) {
  	EFI_PRINT("hash in db:\n");
  	print_hex_dump("    ", DUMP_PREFIX_OFFSET,
  		       16, 1,
  

• 			       sig_data->data, size, false);
  

• 			       sig_data->data, len, false);
  }
  

#endif

• 	if ((sig_data->size < size + sizeof(time64_t)) ||
  

• 	    memcmp(sig_data->data, hash, size))
  

• 	if ((sig_data->size < len + sizeof(time64_t)) ||
  

• 	    memcmp(sig_data->data, hash, len))
  	continue;
  
  

• 	memcpy(&revoc_time, sig_data->data + size,
  

• 	memcpy(&revoc_time, sig_data->data + len,
         sizeof(revoc_time));
  EFI_PRINT("revocation time: 0x%llx\n", revoc_time);
  /*
  

@@ -488,6 +515,7 @@ bool efi_signature_verify(struct efi_image_regions *regs, goto out;

for (sinfo = msg->signed_infos; sinfo; sinfo = sinfo->next) {

• int len;
  

ditto

EFI_PRINT("Signed Info: digest algo: %s, pkey algo: %s\n",
	  sinfo->sig->hash_algo, sinfo->sig->pkey_algo);

@@ -500,7 +528,9 @@ 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,
  

• 		      guid_to_sha_str(&efi_guid_sha256),
  

• 		      &len)) {
  

=> NULL

-Takahiro Akashi

	EFI_PRINT("Digesting an image failed\n");
	goto out;
}

-- 2.32.0