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

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

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
>

Akashi-san,

> > +	{
> > +		EFI_CERT_X509_SHA256_GUID,
> > +		"sha256",
> > +		256,
> 
>                 => SHA256_SUM_LEN * 8
> 

Sure

> > +	},
> > +	{
> > +		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().

True, I'll get rid of this

> 
> > +
> > +/** 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[]).
> 

Tbh I have similar stuff in efi_tcg2.c, but they are all bound to EFI atm.
We could refactor all of the structures a bit and have a more generic
version.  Can we leave it for now and I can send a refactoring series
after this gets merged?

> > +{
> > +	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.

Sure

[...]

> > +
> > +	/* 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.
> 

Yea good catch. I was playing with a version where len wasn't a ptr in
efi_hash_regions() and that's a leftover (as well as guid_to_sha_len()).


[...]

Thanks for having a look
/Ilias