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

[Philippe REYNES]

Hi Farhan,

Le 30/03/2021 à 01:10, Farhan Ali a écrit :
> Phillipe,
>
> In our implementation we store our binaries outside the FIT header, 
> and introduce a gap between the header and the start of binary data 
> (-p and -E option in mkimage). After the FIT has been generated, we 
> sign the FIT header and insert the signature into this gap. The weak 
> function then checks the signature after 'only' the header has been 
> loaded, but before any of the FIT fields have been parsed.
>
> Whatever common implementation we decide on, it is imperative that the 
> signature can be inserted 'AFTER' the complete FIT has been generated. 
> The reason this is so critical is to allow for off-line signing via 
> customer HSMs.
>
I think we are in line. I just sent a patch that add a stage pre-load to 
the command bootm.
The idea is to add a header to the fit image with:
- a magic : to check that it is a header with a signature
- the size of the image : to know what size should be checked
- the size of the signature
- the signature
All others information are in the u-boot device (header size, public 
key, ...), so the header is minimal.
I think that his "header" is compatible with the patch you have sent.

I  also plan to support multiple cascaded headers. So we could also 
cipher the full fit, or compress the full fit, or any other idea ...


> Regards,
> Farhan
>

Best regards,
Philippe


> On Wed, Mar 24, 2021 at 12:09 AM Simon Glass <sjg at chromium.org 
> <mailto:sjg at chromium.org>> wrote:
>
>     Hi Philippe,
>
>     On Wed, 24 Mar 2021 at 06:16, Philippe REYNES
>     <philippe.reynes at softathome.com
>     <mailto:philippe.reynes at softathome.com>> wrote:
>     >
>     > 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
>     <mailto: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.
>
>     You can store the public key (or whatever is used) in the U-Boot
>     devicetree, but the signature presumably has to be attached to the
>     FIT, right?
>
>     Regards,
>     Simon
>