Hello Tom, On 3/2/26 23:09, Tom Rini wrote: > There is a flaw in how U-Boot verifies and generates signatures for FIT > images. To prevent mix and match style attacks, it is recommended to > use signed configurations. How this is supposed to work is documented in > doc/usage/fit/signature.rst. > > Crucially, the `hashed-nodes` property of the `signature` node contains > which nodes of the FIT device tree were hashed as part of the signature > and should be verified. However, this property itself is not part of the > hash and can therefore be modified by an attacker. Furthermore, the > signature only contains the name of each node and not the path in the > device tree to the node. > > This patch reworks the code to address this specific oversight.
Do I understand correctly that this is a breaking change for FIT with signed configurations? - New U-Boot hashes more than intended for old FIT - Old U-Boot hashes less than intended for new FIT Thanks, Ahmad > > Thanks to Apple Security Engineering and Architecture (SEAR) for > reporting this issue and then coming up with a fix. > > Reported-by: Apple Security Engineering and Architecture (SEAR) > Signed-off-by: Tom Rini <[email protected]> > --- > I assume one question will be about adding a test case. I have been > asked to not share the testcase for a few releases yet, so plan to add > that later in the year. > --- > boot/image-fit-sig.c | 31 +++++++++++++++++++++++-------- > include/image.h | 12 ++++++++++++ > tools/image-host.c | 9 +++++++-- > 3 files changed, 42 insertions(+), 10 deletions(-) > > diff --git a/boot/image-fit-sig.c b/boot/image-fit-sig.c > index f23e9d5d0b0f..dce8bbe30e93 100644 > --- a/boot/image-fit-sig.c > +++ b/boot/image-fit-sig.c > @@ -44,12 +44,6 @@ struct image_region *fit_region_make_list(const void *fit, > debug("Hash regions:\n"); > debug("%10s %10s\n", "Offset", "Size"); > > - /* > - * Use malloc() except in SPL (to save code size). In SPL the caller > - * must allocate the array. > - */ > - if (!IS_ENABLED(CONFIG_XPL_BUILD) && !region) > - region = calloc(sizeof(*region), count); > if (!region) > return NULL; > for (i = 0; i < count; i++) { > @@ -62,6 +56,23 @@ struct image_region *fit_region_make_list(const void *fit, > return region; > } > > +int fit_region_add_hashed_nodes(struct image_region *region, int count, > + const char* hashed_nodes, int > hashed_nodes_len) > +{ > + /* Add the spacer to ensure the hashed strings and hashed-nodes cannot > "overlap". */ > + const char* HASHED_NODES_SPACER = "hashed-nodes"; > + region[count].data = HASHED_NODES_SPACER; > + region[count].size = strlen(HASHED_NODES_SPACER); > + count++; > + > + region[count].data = hashed_nodes; > + region[count].size = hashed_nodes_len; > + count++; > + > + /* Now add the actual hashed-nodes value. */ > + return count; > +} > + > static int fit_image_setup_verify(struct image_sign_info *info, > const void *fit, int noffset, > const void *key_blob, int required_keynode, > @@ -376,10 +387,14 @@ static int fit_config_check_sig(const void *fit, int > noffset, int conf_noffset, > count++; > } > > - /* Allocate the region list on the stack */ > - struct image_region region[count]; > + /* Allocate the region list on the stack, +2 for the hashed-nodes */ > + struct image_region region[count+2]; > > fit_region_make_list(fit, fdt_regions, count, region); > + > + /* Add the hashed-nodes */ > + count = fit_region_add_hashed_nodes(region, count, prop, prop_len); > + > if (info.crypto->verify(&info, region, count, fit_value, > fit_value_len)) { > *err_msgp = "Verification failed"; > diff --git a/include/image.h b/include/image.h > index 34efac6056dd..5b847527b586 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -1771,6 +1771,18 @@ struct image_region *fit_region_make_list(const void > *fit, > struct fdt_region *fdt_regions, int count, > struct image_region *region); > > +/** > + * fit_region_add_hashed_nodes() - Add a list of hashed nodes to the regions > + * > + * @region: List of regions to add to > + * @count: Number of existing regions (Note: region needs to have > space for an additional two regions) > + * @hashed_nodes: List of hashed nodes > + * @hashed_nodes_len: Length of list > + * Return: The updated count value (i.e. count+2). > + */ > +int fit_region_add_hashed_nodes(struct image_region *region, int count, > + const char* hashed_nodes, int hashed_nodes_len); > + > static inline int fit_image_check_target_arch(const void *fdt, int node) > { > #ifndef USE_HOSTCC > diff --git a/tools/image-host.c b/tools/image-host.c > index 48d69191c921..1e7c38a9a211 100644 > --- a/tools/image-host.c > +++ b/tools/image-host.c > @@ -1018,13 +1018,15 @@ static int fit_config_get_regions(const void *fit, > int conf_noffset, > return -EINVAL; > } > > - /* Build our list of data blocks */ > - region = fit_region_make_list(fit, fdt_regions, count, NULL); > + /* Allocate the region array, +2 for the hashed-nodes region. */ > + region = calloc(count+2, sizeof(*region)); > if (!region) { > fprintf(stderr, "Out of memory hashing configuration '%s/%s'\n", > conf_name, sig_name); > return -ENOMEM; > } > + /* Build our list of data blocks */ > + fit_region_make_list(fit, fdt_regions, count, region); > > /* Create a list of all hashed properties */ > debug("Hash nodes:\n"); > @@ -1043,6 +1045,9 @@ static int fit_config_get_regions(const void *fit, int > conf_noffset, > strcpy(region_prop + len, node_inc.strings[i]); > strlist_free(&node_inc); > > + /* Add the hashed-nodes. */ > + count = fit_region_add_hashed_nodes(region, count, region_prop, len); > + > *region_countp = count; > *regionp = region; > *region_propp = region_prop; -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
