[PATCH v4 09/10] rockchip: binman: Support use of crc32 for SPL_FIT_SIGNATURE

[Quentin Schulz]

Hi Simon,

On 4/9/25 6:35 PM, Simon Glass wrote:
> Hi Quentin,
> 
> On Wed, 9 Apr 2025 at 10:11, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>>
>> Hi Jonas,
>>
>> On 4/9/25 5:38 PM, Jonas Karlman wrote:
>>> Hi Quentin,
>>>
>>> On 2025-04-09 13:06, Quentin Schulz wrote:
>>>> Hi Jonas,
>>>>
>>>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>>>>> Use of SHA256 checksum validation on ARMv7 SoCs can be very time
>>>>> consuming compared to ARMv8 SoCs with Crypto Extensions.
>>>>>
>>>>> Add support for use of the crc32 hash algo when SHA256 is not supported.
>>>>> Also use a HAS_HASH to simplify the ifdefs when no known hash algo is
>>>>> compiled.
>>>>>
>>>>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>>>>
>>>> I don't know enough about general security but this very much looks like
>>>> a bad idea to me.
>>>>
>>>> https://web.archive.org/web/20170210173741/http://www.derkeiler.com/Newsgroups/sci.crypt/2003-07/1451.html
>>>>
>>>> """
>>>> While properly designed CRC's are good at detecting random errors in
>>>> the data (due to e.g. line noise), the CRC is useless as a secure
>>>> indicator of intentional manipulation of the data. And this is
>>>> because it's not hard at all to modify the data to produce any CRC
>>>> you desire (e.g. the same CRC as the original data, to try to
>>>> disguise your data manipulation).
>>>> """
>>>>
>>>> (yes I took the "first" link my web search engine returned me, thanks
>>>> confirmation bias!).
>>>
>>> I am fully aware, and the fallback to use crc32, and current primarily
>>> use of sha256, should not be considered a security feature. It is
>>> intended purely for a checksum validation of the binary blob after it
>>> has been loaded into memory and before U-Boot otherwise unconditionally
>>> jumps to and execute the loaded blob of binary code.
>>>
>>>>
>>>> I don't want to give people a false sense of security. If it really
>>>> comes down to it, I'd rather have an explicit Kconfig symbol to set this
>>>> value (maybe have a `choice` even) and be very clear about security
>>>> implications.
>>>
>>> Prior to the addition of the hash { algo=sha256 }, there was no checksum
>>> test and U-Boot SPL would unconditionally jump to the loaded data, even
>>> if it had been corrupted, was only halfway written or otherwise
>>> overwritten.
>>>
>>> The addition of crc32 instead of sha256 is just to allow older board
>>> variants with weaker SoCs to at least have some sort of checksum
>>> validation in use without affecting the boot time too much.
>>>
>>> On my rk3228 board, validation using sha256 could take a few seconds,
>>> and with crc32 it could be measured in ms.
>>>
>>> To me, having at least some default checksum validation is preferred
>>> over none at all.
>>>
>>
>> Except if this confuses people into thinking they have a secure system
>> except they use CRC32 instead of something more appropriate like SHA256.
>>
>> It seems like the "hash" node presence doesn't mean a FIT conf or image
>> node is signed. I also don't see many device trees with a signature
>> node... which is kinda odd. Looking a bit into the signature node in the
>> spec and binman tests, it's not clear to me if the hash node is reused
>> by the signature node if if exists and if their algo should match and
>> what exactly is checked by the signature node (like signing the hash
>> string contained in the hash node(s) listed in sign-images property, or
>> (re-)generating a hash of those and signing it at build time?
>>
>> If
>> a) it is not possible to have a hash node's algo differ from a signature
>> node's algo, then I'm fine with this as crc32 won't be allowed
>> b) the signature node regenerates a hash regardless of the hash in the
>> hash node, meaning the signature will be "appropriately secure"
>> regardless of the algorithm listed in the hash node, then I'm fine with
>> it too,
>> c) it is possible to sign a crc32 hash, don't want it :)
>>
>> @Simon, do you have some hints about this?
> 
> In fit.py :
>                  # The hash subnodes here are for mkimage, not binman.
>                  entry.SetUpdateHash(False)
> 
> so yes they are independent, so far as Binman is concerned.
> 
> But of course, configuration signing relies on hashes in the image
> nodes, so at least one strong hash is needed.
> 

Would/Could/Should those image node hash subnodes be autogenerated 
during signing based on the selected algo? In which case, only the algo 
in the signature subnode would actually matter from security PoV.

Cheers,
Quentin