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

[Sean Anderson]

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).
- 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.

That said, if we do determine the image based on a config, we should
definitely verify it.

--Sean