Hi Pegorer, On Tue, 27 Dec 2022 at 10:33, Pegorer Massimo <massimo.pego...@vimar.com> wrote: > > Hi, > > > Da: U-Boot <u-boot-boun...@lists.denx.de> Per conto di Sean Anderson > > Inviato: venerdì 23 dicembre 2022 00:06 > > > > On 11/18/22 15:50, Simon Glass wrote: > > > Hi Sean, > > > > > > On Thu, 13 Oct 2022 at 09:41, Sean Anderson <sean.ander...@seco.com> > > > wrote: > > >> > > >> > > >> > > >> On 10/13/22 3:14 AM, Rasmus Villemoes wrote: > > >> > On 12/10/2022 18.28, Sean Anderson wrote: > > >> >> On 10/12/22 08:59, Simon Glass wrote: > > >> >>> Hi Sean, > > >> >>> > > >> >>> On Tue, 11 Oct 2022 at 17:25, Sean Anderson > > >> >>> <sean.ander...@seco.com> > > >> >>> wrote: > > >> >>>> > > >> >>>> When reading data from a FIT image, we must verify the > > >> >>>> configuration we get it from. This is because when we have a key > > >> >>>> with required = "conf", the image does not need any particular > > >> >>>> signature or hash. The configuration is the only required > > >> >>>> verification, so > > >> >>>> we must verify it. > > >> >>>> > > >> >>>> Users of fit_get_data_node are liable to load unsigned data > > >> >>>> unless the user has set required = "image". Even then, they are > > >> >>>> vulnerable to mix-and-match attacks. This also affects other > > >> >>>> callers of fit_image_verify which don't first call > > >> >>>> fit_config_verify, such as source and imxtract. I don't think > > >> >>>> there is a backwards-compatible way to fix these interfaces. > > >> >>>> Fundamentally, selecting data by image when images are not required > > >> >>>> to be verified is unsafe. > > >> >>>> > > >> >>>> Fixes: 37feaf2f727 ("image: fit: Add some helpers for getting > > >> >>>> data") > > >> >>>> Signed-off-by: Sean Anderson <sean.ander...@seco.com> > > >> >>>> --- > > >> >>>> > > >> >>>> boot/image-fit.c | 9 ++++++++- > > >> >>>> 1 file changed, 8 insertions(+), 1 deletion(-) > > >> >>>> > > >> >>>> diff --git a/boot/image-fit.c b/boot/image-fit.c index > > >> >>>> 9c04ff78a15..632fd405e29 100644 > > >> >>>> --- a/boot/image-fit.c > > >> >>>> +++ b/boot/image-fit.c > > >> >>>> @@ -1948,7 +1948,14 @@ int fit_get_data_node(const void *fit, > > >> >>>> const char *image_uname, > > >> >>>> int fit_get_data_conf_prop(const void *fit, const char *prop_name, > > >> >>>> const void **data, size_t *size) > > >> >>>> { > > >> >>>> - int noffset = fit_conf_get_node(fit, NULL); > > >> >>>> + int ret, noffset = fit_conf_get_node(fit, NULL); > > >> >>>> + > > >> >>>> + if (noffset < 0) > > >> >>>> + return noffset; > > >> >>>> + > > >> >>>> + ret = fit_config_verify(fit, noffset); > > >> >>>> + if (ret) > > >> >>>> + return ret; > > >> >>>> > > >> >>>> noffset = fit_conf_get_prop_node(fit, noffset, prop_name); > > >> >>>> return fit_get_data_tail(fit, noffset, data, size); > > >> >>>> -- > > >> >>>> 2.35.1.1320.gc452695387.dirty > > >> >>>> > > >> >>> > > >> >>> This is supposed to work by first verifying the configuration > > >> >>> with fit_config_verify(). After that, images in that > > >> >>> configuration can be freely loaded, verified by the hash that each > > >> >>> image > > >> >>> has. > > >> >> > > >> >> Well, this function was made to replaces several cases where code > > >> >> loaded a FIT image from somewhere, and then wanted to get data > > >> >> from an image based on the configuration. Typically they only want > > >> >> to extract one image, which is the common case for e.g. loading > > >> >> firmware. This idea of this function is to make the common case of > > >> >> "find me the image data from the default config and verify it" > > >> >> easier. If you look at the existing code which uses this function, > > >> >> they do not verify the configuration first. This is mainly because > > >> >> the original versions of this code which I replaced with this > > >> >> function did > > >> >> not verify the configuration either. > > >> >> > > >> >> So while the above process works for an integrated verification > > >> >> process, like what is done by bootm, it doesn't really work for > > >> >> one-off loading of image data. In particular, the requirements to > > >> >> make this secure (using required = "image" for your key), are not > > >> >> default. When I was trying to determine whether the source command > > >> >> would be OK to use to load some configuration, I looked at it and > > >> >> saw that it did fit_image_verify. I thought that was fine, but if > > >> >> you use required = "config", then all that does is verify the hash. > > >> > > > >> > Yeah, so I've raised this problem with the "source" shell command > > >> > previously, but never got a satisfactory answer: > > >> > > > >> > https://lore.kernel.org/u-boot/7d711133-d513-5bcb-52f2-a9dbaa9ee...@prevas.dk/ > > >> > > > >> > So does your patch now mean that it's possible to get a > > >> > bootscript-wrapped-in-a-FIT-image verified, possibly by adding some > > >> > dummy (or not so dummy?) "configurations" node? Can you give a > > >> > complete .its showing how I can build a verifiable boot script? > > >> > > >> No. I didn't convert source because it also checks to ensure that the > > >> image type is correct, which fit_get_data_node doesn't check. > > >> However, it still uses the image name to determine the data to > > >> source, which has all the problems as discussed above. > > >> > > >> I think to do this right we would need either > > >> > > >> - A version of fit_image_verify which treats required = "config" as > > >> required = "image". This could be used for cases where the caller > > >> doesn't verify a config (such as in cases when the user specifies an > > >> image directly). > > > > > > Without config verification we are subject to mix-and-match attacks. > > > > We already have several functions which just load data from one image (e.g. > > firmware). There is nothing to mix or match. The load address/entry point > > are > > not used, so they do not need to be protected. > > Would such modified fit_image_verify require signatures for all the images > in the FIT? Or just for those not in a configuration node? As you stated, > this would not be backwards-compatible, so why not to define a new required > mode, different from "image" and "config", to select this new behaviour? > > IMO, a cleaner design should - when required = "config" is set - reject > to load data from any image which is not part of a signed and verified > configuration. Of course, this behaviour would not be backwards-compatible, > too. Therefore, it should be probably be addressed with a "config+" or > "config-strict" or whatever else new required mode. > > I feel it cleaner as the definitions of the configs in the FIT will state, > themselves, what will be accessible/loadable and what will not, once a > configuration has been selected. It requires just to hash all images and > sign configurations, and it is safe against mix-and-match attacks.
Sorry if I am a bit late with this comment, but I agree with this. Regards, Simon [..]