On 2/17/25 11:34 AM, Quentin Schulz wrote:
+Cc Marek

Hi,

On 2/16/25 7:36 PM, Rogerio Guerra Borin via lists.openembedded.org wrote:
You don't often get email from [email protected]. Learn why this is important<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 ?

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- 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 \
"""

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.

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- 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- 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.
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#211556): 
https://lists.openembedded.org/g/openembedded-core/message/211556
Mute This Topic: https://lists.openembedded.org/mt/111218371/21656
Group Owner: [email protected]
Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub 
[[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-

  • [OE-core] Broken FIT image... Rogerio Guerra Borin via lists.openembedded.org
    • Re: [OE-core] Broken ... Rogerio Guerra Borin via lists.openembedded.org
    • Re: [OE-core] Broken ... Quentin Schulz via lists.openembedded.org
      • Re: [OE-core] Bro... Marek Vasut via lists.openembedded.org
        • Re: [OE-core]... Rogerio Guerra Borin via lists.openembedded.org
          • Re: [OE-c... Jose Quaresma via lists.openembedded.org
            • Re: ... Leonard Anderweit via lists.openembedded.org
              • ... Jose Quaresma via lists.openembedded.org
              • ... Marek Vasut via lists.openembedded.org
          • Re: [OE-c... Marek Vasut via lists.openembedded.org
            • Re: ... Sean Anderson via lists.openembedded.org
              • ... Rogerio Guerra Borin via lists.openembedded.org
                • ... Adrian Freihofer via lists.openembedded.org
                • ... Marek Vasut via lists.openembedded.org

Reply via email to