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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 1/18/22 19:12, Ilias Apalodimas wrote:
> Hi Heinrich,
>
> On Tue, 18 Jan 2022 at 18:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 1/18/22 15:03, Ilias Apalodimas wrote:
>>> Hi Heinrich,
>>>
>>>>>>> -         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?
>>>
>>> We don't,  but the current code allows it as well.  Should we enforce this
>>> from U-Boot  though?  The spec doesn't forbid it as far as I remember
>>
>> Collisions for SHA1 have been first created successfully in 2017.
>>
>> It is feasible to create two different EFI binaries with the same SHA1.
>> One will be reviewed and signed. After copying the signature to the
>> other one it will happily boot on U-Boot. Ouch. This is exactly what
>> signatures are meant to avoid.
>>
>> We must not accept SHA1 for signatures.
>
> Right, but is this the right place to do it? This is function to
> verify signatures.  Isn't it better to keep this as is and then
> explicitly deny adding sha1 hashed keys into db?

You must assume that PK, KEK, db are preexisting or are seeded from a
file. So the check should be done when loading an image.

To be more precise:

An image should not be validated based on an SHA1 signature or SHA1 hash
but it must be possible to reject an image based on an SHA1 hash.

Best regards

Heinrich

>
> Cheers
> /Ilias
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Regards
>>> /Ilias
>>>>
>>>> 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
>>>>>>>
>>>>
>>