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

[Sean Anderson]

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