Hi, I've also noticed this bug and sent patch [1] to fix it.
[1] https://lists.openembedded.org/g/openembedded-core/message/211761 Leonard Am Donnerstag, dem 20.02.2025 um 10:46 +0000 schrieb Jose Quaresma via lists.openembedded.org: > Hi, > > Any news about this regression? > If we don't get a fix for this bug asap maybe we should revert and > drop this patch since it broke our scarthgap stable LTS > > Jose > > > Rogerio Guerra Borin via lists.openembedded.org > <rogerio.borin=toradex....@lists.openembedded.org> escreveu (terça, > 18/02/2025 à(s) 03:49): > > > This message originated from outside your organization > > > > > > On 2/17/25 11:34 AM, Quentin Schulz wrote: > > > > +Cc Marek > > > > > > Hi, > > > > Thanks for the quick answer! > > > > > > > > > On 2/16/25 7:36 PM, Rogerio Guerra Borin via > > > lists.openembedded.org > > > wrote: > > > >> You don't often get email from > > > >> rogerio.borin=toradex....@lists.openembedded.org. Learn why > > > this is > > > >> important<https://aka.ms/LearnAboutSenderIdentification > > > <https://aka.ms/LearnAboutSenderIdentification>> > > > >> Hello, > > > >> > > > >> At Toradex we're using class "kernel-fitimage" to produce > > > our signed > > > >> kernel FIT images and we've found that the recent commit > > > d7bd9c627661 > > > >> ("u-boot: kernel-fitimage: Fix dependency loop if > > > UBOOT_SIGN_ENABLE > > > >> and UBOOT_ENV enabled") broke our builds. The logs of a > > > failed build > > > >> show the following message that I found noteworthy: > > > > > > Can you mail me the entire log file for completeness ? > > > > Sure. > > > > I've added it as attachment > > (log.do_uboot_assemble_fitimage.841485). > > > > > >> log.do_uboot_assemble_fitimage.841485: > > > >> ... > > > >> Couldn't open RSA private key: '/secboot-data/fit- > > > keys/dev2.key': No > > > >> such file or directory > > > >> > > > >> The odd thing was that the build was setting: > > > >> > > > >> UBOOT_SIGN_KEYNAME = "dev" > > > >> UBOOT_SIGN_IMG_KEYNAME = "dev2" > > > >> FIT_SIGN_INDIVIDUAL = "0" > > > >> FIT_GENERATE_KEYS = "0" > > > > > > FIT_SIGN_INDIVIDUAL = "0" is correct, you do want to sign > > > configurations, not individual images in the fitImage, because > > > signing > > > individual images is susceptible to mix-and-match attack. > > > > > > >> From the above we see the individual images signing is > > > disabled but > > > >> the system is trying to access the key file for the > > > individual signing > > > >> (dev2) regardless of that. Checking the code in uboot- > > > sign.bbclass I > > > >> found this line: > > > >> > > > >> > > > https://git.openembedded.org/openembedded-core/tree/meta/classes- > > > < > > > https://git.openembedded.org/openembedded-core/tree/meta/classes- > > > > > > > >> recipe/uboot-sign.bbclass?h=scarthgap#n116 > > > >> > > > >> that is responsible for that access (inside function > > > concat_dtb()). > > > >> The code is always referencing variable > > > UBOOT_SIGN_IMG_KEYNAME > > > >> regardless of FIT_SIGN_INDIVIDUAL being set to "1" or "0" > > > even though > > > >> there's a condition a couple lines above L116 where > > > >> FIT_SIGN_INDIVIDUAL is tested to decide in which mode to run > > > the > > > >> signing tool. > > > > > > Uh, good find, does the following patch fix your problem ? > > > > > > """ > > > diff --git a/meta/classes-recipe/uboot-sign.bbclass > > > b/meta/classes-recipe/uboot-sign.bbclass > > > index 96c47ab016..74ef5697ca 100644 > > > --- a/meta/classes-recipe/uboot-sign.bbclass > > > +++ b/meta/classes-recipe/uboot-sign.bbclass > > > @@ -107,13 +107,16 @@ concat_dtb() { > > > # makes fitImage susceptible to mix-and-match attack. > > > if [ "${FIT_SIGN_INDIVIDUAL}" = "1" ] ; then > > > UBOOT_MKIMAGE_MODE="auto" > > > + UBOOT_MKIMAGE_KEYNAME=${UBOOT_SIGN_IMG_KEYNAME} > > > + else > > > + UBOOT_MKIMAGE_KEYNAME=${UBOOT_SIGN_KEYNAME} > > > fi > > > ${UBOOT_MKIMAGE_SIGN} \ > > > ${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if > > > len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \ > > > -f $UBOOT_MKIMAGE_MODE \ > > > -k "${UBOOT_SIGN_KEYDIR}" \ > > > -o "${FIT_HASH_ALG},${FIT_SIGN_ALG}" \ > > > - -g "${UBOOT_SIGN_IMG_KEYNAME}" \ > > > + -g "$UBOOT_MKIMAGE_KEYNAME" \ > > > -K "${UBOOT_DTB_BINARY}" \ > > > -d /dev/null \ > > > -r ${B}/unused.itb \ > > > """ > > > > Looking at the patch I'm pretty sure it would solve my problem, > > particularly because we set FIT_SIGN_INDIVIDUAL="0" in our builds. > > However, I see it keeps the concept of the signing being either on > > the > > config or on the individual images, which I wanted to further > > comment > > down below. > > > > > >> To better understand the behavior I created dev2. > > > >> {key,crt} files and retried a build which worked. However, > > > checking > > > >> the contents of the final u-boot.dtb file I found that the > > > "dev2" key > > > >> node was present but the "dev" key wasn't. Then, checking > > > the contents > > > >> of the final fitImage I noticed the configuration nodes were > > > signed by > > > >> the dev key while the individual images were not signed (as > > > expected). > > > >> But that means such a system wouldn't boot since the > > > required "dev" > > > >> key is not present in the U-Boot DTB, from what I conclude > > > the FIT > > > >> signing is broken at the moment. > > > >> > > > >> So, first of all I wanted to let you guys know about this > > > issue > > > >> (hoping we're not doing anything wrong with our builds). > > > > > > No, your builds are fine, this is a real bug. > > > > Thanks for the confirmation. > > > > > >> As a second point, I wanted to confirm the expected behavior > > > regarding > > > >> variable FIT_SIGN_INDIVIDUAL. According to the > > > documentation: > > > >> > > > >> > > > https://docs.yoctoproject.org/5.0.6/ref-manual/variables.html#term > > > - > > > < > > > https://docs.yoctoproject.org/5.0.6/ref-manual/variables.html#term > > > -> > > > >> FIT_SIGN_INDIVIDUAL > > > >> > > > >> when its value is "1" the individual images will be signed > > > *in > > > >> addition* to the FIT image being signed. > > > > > > That is a bit weird and no, I don't think that is how it is > > > supposed to > > > work. > > > > > > FIT_SIGN_INDIVIDUAL=1 -> only image subnodes in fitImage /images > > > nodes > > > are signed AND fitImage is susceptible to mix-and-match attack > > > (basically attacker can replace individual signed images in > > > fitImage > > > with different individual signed images, e.g. with older > > > versions, to > > > achieve a combination of images which allows them to somehow > > > break the > > > system). > > > > > > FIT_SIGN_INDIVIDUAL=0 (recommended) -> images are hashed, > > > configurations > > > are hashed, hashes are combined and signed. Because images and > > > configurations (which images are to be used together) are signed, > > > attacker cannot mix-and-match images into an un-allowed > > > configuration. > > > > > > >> From that, I understand that > > > >> the configurations will be always signed and the individual > > > images may > > > >> or may not be signed depending on FIT_SIGN_INDIVIDUAL. > > > > > > The other way around. The configurations are signed by default, > > > which is > > > good, but this can be disabled if needed (unlikely). > > > > > > >> I kind of > > > >> confirmed this behavior by checking our build artifacts on > > > kirkstone: > > > >> with FIT_SIGN_INDIVIDUAL="0" the U-Boot DTB has only the > > > "dev" key and > > > >> the fit image is signed by that same key; with > > > FIT_SIGN_INDIVIDUAL="1" > > > >> both keys are present in the DTB, the configurations are > > > signed by the > > > >> "dev" key and the images by the "dev2" key. I understand > > > this is the > > > >> correct behavior, right? > > > > > > Images shouldn't be signed by different keys than configurations, > > > that > > > is a bit odd. > > > > > > >> I'm asking because looking at the comment at: > > > >> > > > >> > > > https://git.openembedded.org/openembedded-core/tree/meta/classes- > > > < > > > https://git.openembedded.org/openembedded-core/tree/meta/classes- > > > > > > > >> recipe/uboot-sign.bbclass?h=scarthgap#n106 > > > >> > > > >> it gives me the impression that FIT_SIGN_INDIVIDUAL would > > > select > > > >> between either signing individual images > > > > > > Yes > > > > > > >> or signing the > > > >> configurations > > > > > > Signed configuration also includes hash of the images that the > > > configuration references in it, that's what prevents the mix-and- > > > match > > > attack. > > > > > > >> , which doesn't match the documented behavior. I believe > > > >> the code in concat_dtb() also seems to assume only one of > > > the keys > > > >> (dev/dev2) would need be present in the DTB. So the fix > > > would involve > > > >> a review on this as well I imagine. > > > IIRC Adrian was looking into improving those docs. > > > > Regarding this second topic, let me show you some other information > > I > > gathered from a couple of builds that may help with your > > assessment: > > > > FSI := FIT_SIGN_INDIVIDUAL > > CFG := Whether or not the configurations inside the FIT image were > > signed > > IMG := Whether or not the subimages inside the FIT image were > > signed > > > > | Branch | Version | FSI | Keys inside DTB | CFG | IMG | > > |-----------+---------------+-----+------------------+-----+-----| > > | kirkstone | latest | "0" | {dev} | Y | N | > > | kirkstone | latest | "1" | {dev,dev2} | Y | Y | > > |-----------+---------------+-----+------------------+-----+-----| > > | scarthgap | latest | "0" | Build failed (!) | n/a | n/a | > > | | + dummy dev2 | | {dev2} | Y | N | > > | scarthgap | latest | "1" | {dev2} (!) | Y | Y | > > |-----------+---------------+-----+------------------+-----+-----| > > | scarthgap | w/o @d7bd9c62 | "0" | {dev} | Y | N | > > | scarthgap | w/o @d7bd9c62 | "1" | {dev,dev2} | Y | Y | > > > > The first two lines show that on kirkstone when > > FIT_SIGN_INDIVIDUAL="0" > > only the configurations are signed and the DTB contains only the > > "dev" > > key. When FIT_SIGN_INDIVIDUAL="1" both "dev" and "dev2" are present > > in > > the DTB and both the configurations and the images are signed. And > > that's the exact same behavior I got on scarthgap after reverting > > the > > commit d7bd9c62. And, as I said, this seems to match the > > documentation. > > I've attached snippets of the fdtdump for the FIT image and U-Boot > > DTB > > from my build on kirkstone to illustrate what I said. > > > > So, if the expected behavior is according to what you described > > then the > > behavior so far was wrong (for a long time) and the new behavior is > > introducing a breaking change for anyone relying on what's > > documented > > for the FIT_SIGN_INDIVIDUAL="1" case, right? Btw, for us at Toradex > > this > > is not a big deal because we don't fall into that case but > > considering > > the general use of the layer the change in behavior could be an > > issue. > > > > Another piece of information that might be useful: I made a change > > to > > concat_dtb() to make it add both keys (dev/dev2) to the DTB (by > > invoking > > mkimage twice) and I found the invocation to "uboot-fit_check_sign" > > failed afterwards. I confirmed that the DTB had the two keys, but > > because the FIT image only had the configurations signed, the tool > > threw > > an error saying the signature of "require key dev2" could not be > > verified but the showed that the check of the "dev" key succeeded. > > Unfortunately I don't have the logs available for that case, but I > > could > > repeat the tests if needed. Anyway, my point is that the > > "uboot-fit_check_sign" tool seems capable of checking both keys as > > well. > > > > Interestingly, if I removed the verification step (performed on the > > "unused.itb" file), then the image generation succeeded and > > everything > > seemed to work as before both when FIT_SIGN_INDIVIDUAL is "0" or > > "1". > > So, it's not hard to get the old behavior, though we'd need to > > figure > > out how to properly do the verification when mkimage is invoked > > with -f > > "auto" or -f "auto-conf". I could post my unfinished patch if > > helpful. > > > > > > > > > -- > Best regards, > > José Quaresma > > >
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#211763): https://lists.openembedded.org/g/openembedded-core/message/211763 Mute This Topic: https://lists.openembedded.org/mt/111218371/21656 Group Owner: openembedded-core+ow...@lists.openembedded.org Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
