two questions on verified boot

[Simon Glass]

Hi Rasmus,

On Thu, 27 Jan 2022 at 08:41, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> On 27/01/2022 16.06, Simon Glass wrote:
> > 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?
>
> By what?

See fit_config_verify() which is called to verify the config. It is
called from fit_image_load().

>
>  At
> > least, that is how bootm works.
> >
> > So it seems that the scripts are being done in a different way.
>
> The thing is, for a script there's really no such thing as a
> "configuration", there are not multiple subimages that need to be
> stitched together.
>
> >>
> >> (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
>
> I don't care about mix'n'match, they are completely unlikely to be
> relevant. But...
>
> >>
> >>   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.
>
> ... this then means that 'required = "image"' is not just a little less
> safe than the "signed configurations" model, it is completely and
> utterly broken.
>
> >>
> >> 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.
>
> That's kind of what I guessed, but to rephrase my question: How do I
> sign a boot script and have that verified, when the source.c code only
> calls fit_image_verify(), which eventually calls
> fit_image_verify_required_sigs() which only cares about the keys that
> have 'required = "image"'.

Well you cannot just verify the image, since you are then open to the
mix-and-match attach.

Perhaps you can add a way to add the script to the config and verify
the config first?

>
> I'd prefer to just have one public key for verifying both the boot
> script and the kernel FIT image. I don't think it would work very well
> to have two keys (possibly the same one just added under different key
> names), one with required=image and one with required=conf, because
> AFAICT then the kernel FIT image would have to have signatures on _both_
> image and configuation nodes. If I add just one key with required=conf,
> the boot script isn't verified at all, and if it says required=image,
> well, then I'm vulnerable to the trivial modification of entry=
> mentioned above.

Yes I agree, two keys doesn't sound useful.

Regards,
Simon