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

[Simon Glass]

Hi Pegorer,

On Tue, 27 Dec 2022 at 10:33, Pegorer Massimo <Massimo.Pegorer at vimar.com> wrote:
>
> 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.

Sorry if I am a bit late with this comment, but I agree with this.

Regards,
Simon
[..]