Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Hi Ilias,

On Tue, Jan 18, 2022 at 01:12:37PM +0200, Ilias Apalodimas wrote:
> Right now the code explicitly limits us to sha1,256 hashes with RSA2048
> encryption.  But the limitation is artificial since U-Boot supports
> a wider range of algorithms.
> 
> The internal image_get_[checksum|crypto]_algo() functions expect an
> argument in the format of <checksum>,<crypto>.  So let's remove the size
> checking and create the needed string on the fly in order to support
> more hash/signing combinations.
> 
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  lib/crypto/public_key.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/crypto/public_key.c b/lib/crypto/public_key.c
> index df6033cdb499..b783c63f5a51 100644
> --- a/lib/crypto/public_key.c
> +++ b/lib/crypto/public_key.c
> @@ -97,6 +97,7 @@ int public_key_verify_signature(const struct public_key *pkey,
>  				const struct public_key_signature *sig)
>  {
>  	struct image_sign_info info;
> +	char algo[256];
>  	int ret;
>  
>  	pr_devel("==>%s()\n", __func__);
> @@ -108,29 +109,27 @@ int public_key_verify_signature(const struct public_key *pkey,
>  		return -EINVAL;
>  
>  	memset(&info, '\0', sizeof(info));
> +	memset(algo, 0, sizeof(algo));
>  	info.padding = image_get_padding_algo("pkcs-1.5");
>  	/*
>  	 * Note: image_get_[checksum|crypto]_algo takes a string
>  	 * argument like "<checksum>,<crypto>"
>  	 * TODO: support other hash algorithms
>  	 */

If this patch is applied, the TODO comment above will make no sense :)

> -	if (strcmp(sig->pkey_algo, "rsa") || (sig->s_size * 8) != 2048) {
> -		pr_warn("Encryption is not RSA2048: %s%d\n",
> -			sig->pkey_algo, sig->s_size * 8);
> -		return -ENOPKG;
> -	}
> -	if (!strcmp(sig->hash_algo, "sha1")) {
> -		info.checksum = image_get_checksum_algo("sha1,rsa2048");
> -		info.name = "sha1,rsa2048";
> -	} else if (!strcmp(sig->hash_algo, "sha256")) {
> -		info.checksum = image_get_checksum_algo("sha256,rsa2048");
> -		info.name = "sha256,rsa2048";
> -	} else {
> -		pr_warn("unknown msg digest algo: %s\n", sig->hash_algo);
> +	if (strcmp(sig->pkey_algo, "rsa")) {
> +		pr_err("Encryption is not RSA: %s\n", sig->pkey_algo);
>  		return -ENOPKG;
>  	}
> +	ret = snprintf(algo, sizeof(algo), "%s,%s%d", sig->hash_algo,
> +		       sig->pkey_algo, sig->s_size * 8);

I'm not sure that this naming rule, in particular the latter part, will
always hold in the future while all the existing algo's observe it.
(Maybe we need some note somewhere?)

-Takahiro Akashi

> +
> +	if (ret >= sizeof(algo))
> +		return -EINVAL;
> +
> +	info.checksum = image_get_checksum_algo((const char *)algo);
> +	info.name = (const char *)algo;
>  	info.crypto = image_get_crypto_algo(info.name);
> -	if (IS_ERR(info.checksum) || IS_ERR(info.crypto))
> +	if (!info.checksum || !info.crypto)
>  		return -ENOPKG;
>  
>  	info.key = pkey->key;
> -- 
> 2.30.2
>