* two questions on verified boot @ 2021-11-21 14:55 Rasmus Villemoes 2021-11-21 19:32 ` Dhananjay Phadke 2022-01-27 15:06 ` Simon Glass 0 siblings, 2 replies; 5+ messages in thread From: Rasmus Villemoes @ 2021-11-21 14:55 UTC (permalink / raw) To: u-boot; +Cc: Simon Glass (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. (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 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>".) 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. 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. Rasmus ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: two questions on verified boot 2021-11-21 14:55 two questions on verified boot Rasmus Villemoes @ 2021-11-21 19:32 ` Dhananjay Phadke 2022-01-27 15:06 ` Simon Glass 1 sibling, 0 replies; 5+ messages in thread From: Dhananjay Phadke @ 2021-11-21 19:32 UTC (permalink / raw) To: rasmus.villemoes; +Cc: sjg, u-boot On 11/21/2021 6:55 AM, Rasmus Villemoes wrote: > (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 > > and I know that the boot process uses $loadaddr = 0x40000000. What is to > stop me from modifying that FIT image to read > > 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>".) > > 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. 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. Couldn't agree more, I've been wondering on similar lines. It would be great to actually run digest over entire image (data + attributes) or config node (minus signature and hash subnodes if re-signing). It would have avoided CVE-2020-10648? Regards, Dhananjay ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: two questions on verified boot 2021-11-21 14:55 two questions on verified boot Rasmus Villemoes 2021-11-21 19:32 ` Dhananjay Phadke @ 2022-01-27 15:06 ` Simon Glass 2022-01-27 15:41 ` Rasmus Villemoes 1 sibling, 1 reply; 5+ messages in thread From: Simon Glass @ 2022-01-27 15:06 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: u-boot Hi Rasmus, On Sun, 21 Nov 2021 at 07:55, Rasmus Villemoes <rasmus.villemoes@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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: two questions on verified boot 2022-01-27 15:06 ` Simon Glass @ 2022-01-27 15:41 ` Rasmus Villemoes 2022-03-12 2:24 ` Simon Glass 0 siblings, 1 reply; 5+ messages in thread From: Rasmus Villemoes @ 2022-01-27 15:41 UTC (permalink / raw) To: Simon Glass; +Cc: u-boot On 27/01/2022 16.06, Simon Glass wrote: > Hi Rasmus, > > On Sun, 21 Nov 2021 at 07:55, Rasmus Villemoes > <rasmus.villemoes@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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: two questions on verified boot 2022-01-27 15:41 ` Rasmus Villemoes @ 2022-03-12 2:24 ` Simon Glass 0 siblings, 0 replies; 5+ messages in thread From: Simon Glass @ 2022-03-12 2:24 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: u-boot Hi Rasmus, On Thu, 27 Jan 2022 at 08:41, Rasmus Villemoes <rasmus.villemoes@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@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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-12 2:28 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-21 14:55 two questions on verified boot Rasmus Villemoes 2021-11-21 19:32 ` Dhananjay Phadke 2022-01-27 15:06 ` Simon Glass 2022-01-27 15:41 ` Rasmus Villemoes 2022-03-12 2:24 ` Simon Glass
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.