[PATCH v2] spl: Add callback for preprocessing loaded FIT header before parsing

[Philippe REYNES]

Hi Simon and Alex,

Le 23/03/2021 à 01:56, Simon Glass a écrit :
> Hi Alex,
>
> On Tue, 23 Mar 2021 at 04:12, Alex G. <mr.nuke.me at gmail.com> wrote:
>> On 3/22/21 9:27 AM, Philippe REYNES wrote:
>>> Hi all,
>>>
>>>
>>> Le 11/03/2021 à 00:10, Alex G a écrit :
>> [snip]
>>> I reach the same issue, my customers are also worried with the actual
>>> signature check scheme on u-boot.
>>> The fit data/node are parsed before being checked : data should be used
>>> only after being checked, not before.
>>> The code become quite complex for a signature, and the more complex the
>>> code is more risk to have/introduce a bug or security issue.
>> [snip]
>>
>>>>> The reason I used a weak function was to mirror the already
>>>>> upstreamed board_spl_fit_post_load(),
>>>> I see why you'd think it was a good idea. board_spl_fit_pre_load()
>>>> sneaks in a dependency on arch-specific code (CONFIG_IMX_HAB). I don't
>>>> really like the way it's implemented, and I don't know if it would
>>>> work with SPL_LOAD_FIT_FULL or bootm.
>>>>
>>> As I reach the same issue, I was also thinking strongly about adding a
>>> "hook" before the fit image is launched/analyzed. In my mind this "pre
>>> load" function should be able to do some check/update to the fit image,
>>> but also modify the beginning of the fit image (to remove a header for
>>> example). Such function/feature may allow to:
>>> - check a signature for the full fit (without parsing the node)
>>> - cipher the full fit (even the node)
>>> - compress the full fit
>>> - probably that users will find a lot of others ideas .....
>>>
>>> I think that this feature pre load should be implemented in spl and
>>> bootm command.
>>>
>>> I have understood the feedback about a useful implementation/usage of
>>> pre_load.
>>> I propose to sent an example soon (probably based on signature check).
>> So "what" you want to do is verify untrusted metadata before using it.
>> That's a very logical and reasonable thing to do.
>>
>> "How" you are trying to do this is by
>>    (1) adding a weak function
>>    (2) allowing each board to have a completely different implementation
>>
>> Those are two terrible ideas.
>>
>> I agree that there is a deficiency in the way FIT images are signed. Can
>> we stick the signature between the fdt_header and before dt_struct?
> That seems like a reasonable idea to me. Even better might be to have
> it completely separate, e.g. before the FIT starts, so no parsing at
> all is needed?


That's my idea, a header with only the minimum information (like fit 
size and signature).
The information about the signature (hash, algo, padding, public key, 
...) may be stored
in the u-boot device tree. So u-boot won't parse the fit image, only 
compute the hash
to check the signature.

>
> Also, which signature? FIT supports multiple signatures which can be
> added at different times. Perhaps this could be for a base signature,
> enough to get through to verifying the 'real' signature.


I was thinking that the signature information could be stored in the 
u-boot device tree
(or hardcoded in the u-boot configuration). The idea is to have a very 
simple header.
I also think that this signature may be used with the signature in the 
fit.  It is two
options, so users may eanble both options.

As we agree on the principle, I will sent a RFC asap.

> Regards,
> Simon


Regards,
Philippe