Hi Rasmus, On Thu, 27 Jan 2022 at 08:41, Rasmus Villemoes <rasmus.villem...@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.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?
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