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

[Heinrich Schuchardt]

On 1/18/22 13:50, Ilias Apalodimas wrote:
> Akashi-san,
>
> On Tue, Jan 18, 2022 at 09:38:22PM +0900, AKASHI Takahiro wrote:
>> 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 at 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 :)
>
> We are still only handle SHA,  but there's a printable error now, so i'll
> get rid of the comment.
>
>>
>>> -	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);

How do we ensure that the unsafe SHA1 algorithm is not used?

Best regards

Heinrich

>>
>> 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?)
>
> The if a few lines below will shield us and return -EINVAL.  How about
> adding an error message there?
>
> Cheers
> /Ilias
>>
>> -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
>>>