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. These functions are great candidates for forcing image verification, no config needed. --Sean >> - Add support for specifying a config node. This would be something like >> the addr#config syntax used by bootm. Of course, this doesn't address >> existing users of fit_get_data_node. > > Yes, let's do this one. > >> >> That said, if we do determine the image based on a config, we should >> definitely verify it. > > Yes > > Regards, > Simon