From mboxrd@z Thu Jan 1 00:00:00 1970 From: Philippe REYNES Date: Tue, 23 Mar 2021 18:16:39 +0100 Subject: [PATCH v2] spl: Add callback for preprocessing loaded FIT header before parsing In-Reply-To: References: <20210224232531.22899-1-farhan.ali@broadcom.com> <20210309235530.184179-1-farhan.ali@broadcom.com> <389855fe-9401-83b6-350f-eb453ab214ae@softathome.com> <96f54554-b201-12c4-287b-794efaff19e9@gmail.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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. 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