| Message ID | 20221011232525.186166-1-sean.anderson@seco.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | image: fit: Fix not verifying data configuration | expand |
Hi Sean, On Tue, 11 Oct 2022 at 17:25, Sean Anderson <sean.anderson@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@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. 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? Regards, Simon
On 10/12/22 08:59, Simon Glass wrote: > Hi Sean, > > On Tue, 11 Oct 2022 at 17:25, Sean Anderson <sean.anderson@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@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
On 10/12/22 08:59, Simon Glass wrote: > Hi Sean, > > On Tue, 11 Oct 2022 at 17:25, Sean Anderson <sean.anderson@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@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
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@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@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? Rasmus
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@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@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
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@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@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
Hi Sean, On Thu, 13 Oct 2022 at 09:41, Sean Anderson <sean.anderson@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@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@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. > - 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
On 11/18/22 15:50, Simon Glass wrote: > Hi Sean, > > On Thu, 13 Oct 2022 at 09:41, Sean Anderson <sean.anderson@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@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@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
Hi, > Da: U-Boot <u-boot-bounces@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@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@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@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. Regards, Massimo > 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
Hi Pegorer, On Tue, 27 Dec 2022 at 10:33, Pegorer Massimo <Massimo.Pegorer@vimar.com> wrote: > > Hi, > > > Da: U-Boot <u-boot-bounces@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@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@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@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 [..]
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);
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@seco.com> --- boot/image-fit.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)