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

[Sean Anderson]

On 10/12/22 12: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. Same thing for
> imxtract. Almost every instance of FIT loading outside of bootm has this
> issue, which you can easily see when grepping for fit_config_verify. The
> only other users are the SPL boot process, and fdt checksign. The latter
> isn't even that useful, since you then need to re-parse the fit in hush
> to determine the default configuration and determine the image names to
> use.
> 
> Unfortunately, it's not trivial to determine whether any existing
> systems are vulnerable to this issue. If they set required = "image",
> then they can use source and imxtract (and any of the firmware loading
> methods) however they want. But if they don't (and there is no option to
> mkimage to do this, you have to use fdtset or something), then there is
> a problem.
> 
>> So we need to make sure that first step is taken in all code paths,
>> rather than adding it willy nilly.
>>
>> If people don't add hashes (or perhaps signature if they want to use
>> more CPU time) for the images, then there is no protection. We could
>> add warnings or errors to mkimage for this case?
> 
> Yes, there probably should be a warning if you skip cryptographic hashes
> on images with signed configs. But this is not really the root of the
> issue.
> 
> --Sean


ping?

--Sean