two questions on verified boot

[Rasmus Villemoes]

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?

 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"'.

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.

Rasmus