two questions on verified boot

[Simon Glass]

Hi Rasmus,

On Sun, 21 Nov 2021 at 07:55, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> (1) When one wants to get rid of CONFIG_LEGACY_IMAGE_FORMAT, one also
> has to wrap any boot script in a FIT rather than a uImage. While it's
> not directly documented anywhere how to do that, it seems that a minimal
> .its for achieving it is
>
> /dts-v1/;
>
> / {
>         description = "U-Boot script(s)";
>
>         images {
>                 default = "boot";
>                 boot {
>                         description = "Bootscript";
>                         data = /incbin/("boot.txt");
>                         type = "script";
>                         compression = "none";
>                         hash-1 {
>                                 algo = "sha256";
>                         };
>                 };
>         };
> };
>
> [The UX when one doesn't guess that the description string is mandatory
> is rather sad, but that's a separate story.]
>
> Now, I can get that signed if I include a signature-foo node inside the
> "boot" node, and also add a dummy empty /configurations node (otherwise
> mkimage refuses to process it). But that will only work if I have added
> a "required = image" property with the public key; if I want to use the
> safer/saner "required = conf", how do I make sure any boot script is
> properly signed?
>
> The code in source.c only cares about the images node and calls
> fit_image_verify(), and there's no concept of "configuration" and
> combining multiple images when talking about a simple boot script.

By then the configuration should have been checked, though, right? At
least, that is how bootm works.

So it seems that the scripts are being done in a different way.

>
> (2) Assuming for the moment that I would be happy with just using
> required=image, am I right in that not only does that mean that the
> combination of kernel/fdt/initramfs is not verified, merely the
> individual parts, but more importantly (a mix'n'match attack isn't
> really very likely), _only_ the data property in each node is part of
> what gets signed, not the other important properties such as load= and
> entry=? IOW, suppose I have a FIT image with

Yes the 'image' check does not protect against a mix & match attack -
see doc/uImage.FIT/signature.txt

>
>   images {
>      kernel {
>         data = blabla;
>         load = 0x1000000;
>         entry = 0x10000000;
>         signature {
>            ... // correct signature of blabla
>         };
>      };
>    };
>
> and I know that the boot process uses $loadaddr = 0x40000000. What is to
> stop me from modifying that FIT image to read
>
>   images {
>      kernel {
>         data = blabla;
>         load = 0x1000000;
>         entry = 0x400abcde;
>         signature {
>            ... // correct signature of blabla
>         };
>      };
>   };
>   some-other-node {
>       pwned = /incbin/("pwned");
>   };
>
> where 0xabcde is chosen to coincide with where the data part of the
> pwned property lies in the modified FIT? (That pwned property can be put
> anywhere; I could even just replace the signer-name property inside the
> signature node with a value of "mkimage\0<padding><my payload>".)

Yes I believe that is right.

>
> In fit_config_process_sig(), there's this elaborate dance with
> fit_config_get_data()/fdt_find_regions() which, AFAICT, ends up
> including all the property values (and the FDT_PROP tags and string
> offsets etc.), and then we call info.crypto->sign() with some
> appropriate region_count.

Yes

> But in fit_image_process_sig(), we call
> info.crypto->sign() with nregions==1, and AFAICT, the data being signed
> is just the value of the "data" property, nothing else.

Yes, this matches the behaviour of the existing hashing properties.
They only hash on the data itself.

We could expand this to include the properties of the image node, I
suppose. But do note that you really need 'config' signing to make
things secure.

Regards,
Simon