R: [PATCH] image: fit: Fix not verifying data configuration

[Pegorer Massimo]

Hi,

> Da: U-Boot <u-boot-bounces at 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.anderson at 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.anderson at 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.anderson at 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-a9dbaa9eeded@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.

Regards,
Massimo

> 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