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

[Sean Anderson]

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.

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