On 27/01/2022 16.06, Simon Glass wrote:
> Hi Rasmus,
>
> On Sun, 21 Nov 2021 at 07:55, Rasmus Villemoes
> <rasmus.villem...@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
