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

[Philippe REYNES <philippe.reynes@softathome.com>]

Hi all,


Le 11/03/2021 ? 00:10, Alex G a ?crit?:
> On 3/10/21 2:49 PM, Farhan Ali wrote:
>> On Wed, Mar 10, 2021 at 11:38 AM Alex G. <mr.nuke.me@gmail.com ??? 
>> This patch describes "how" you're trying to achieve it, but "what" you
>> ??? want to achieve. I'll get later into why I think the "how" is
>> ??? fundamentally flawed.
>>
>> The 'what' is basically this: I want to be able to parse the fit 
>> header for correctness before
>> any image loading takes place. This 'correctness' will be user defined
>
> I'd expect such code to be part of this series. Having a function that 
> a "user" might define sounds a lot like a vendor-specific hook with no 
> upstream code, hence the skepticism. This series should include a 
> useful implementation of board_spl_fit_pre_load().
>
>
>> ??The main use case for us is two folds:
>> (1) Customers are worried about our reliance on libfdt for FIT 
>> parsing and want to prescan the FIT header to
>> check for any future exploits
>> (2) We implement a signature on the entire FIT header ( instead of 
>> individual nodes ).
>
> Do you believe the current FIT signing scheme is inappropriate for 
> your needs? Have you looked at signed configs? Is there a reason why 
> they are not appropriate?
>
> There was a potential issue where a bad FIT could place itself 
> anywhere in memory. This was fixed in commit 03f1f78a9b ("spl: fit: 
> Prefer a malloc()'d buffer for loading images"). Keep in mind that, in 
> this case, checking the FIT header would not have guarded against the 
> exploit.
>

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.



>
>> ??? Second issue is that spl_simple_fit_read() is intended to bring a 
>> FIT
>> ??? image to memory. If you need to make decisions on the content of 
>> that
>> ??? image, then spl_simple_fit_read() is the wrong place to do it. A 
>> better
>> ??? place might be spl_simple_fit_parse().
>>
>> spl_simple_fit_parse()? parses the 'contents' of the fit using 
>> standard APIs. We need to check
>> the FIT header for correctness BEFORE its contents are parsed, using 
>> a user defined 'safe'
>> parsing function. The standard FIT loading flow checks for only a few 
>> things ( hashes/configuration etc),
>> there can be a lot of other USER defined checks which may need to be 
>> checked. This callback will achieve this
>
> This patch is calling board_spl_fit_pre_load() after the FIT is read. 
> On a FIT with embedded data, you've also loaded all the binaries. It 
> seems that checking a header now is a moot point.
>
> If you need to make sure that the FIT wasn't tampered, the signed 
> configs were designed exactly for that. You mentioned earlier that you 
> want to sign the FIT header. What is the FIT header in this case? Is 
> it the FDT of a FIT with external data? Is it struct fdt_header?
>

As mentioned above, we (my company and my customer) thought that the fit 
node should only be used after the fit signature is verified (and OK).

>
>> 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).


Regards,

Philippe



> Alex