[RFC] uboot-sign: Fix u-boot dtb signatures

[RFC] uboot-sign: Fix u-boot dtb signatures

[Leonard Anderweit]

With UBOOT_SIGN_ENABLE enabled commit 3fb215a3af24 (u-boot:
kernel-fitimage: Fix dependency loop if UBOOT_SIGN_ENABLE and UBOOT_ENV
enabled) always adds the signature of UBOOT_SIGN_IMG_KEYNAME to the
u-boot dtb, independent of FIT_SIGN_INDIVIDUAL. The kernel fitimage
configuration node is signed with UBOOT_SIGN_KEYNAME but the u-boot dtb
contains the signature of UBOOT_SIGN_IMG_KEYNAME. U-boot is therefore
unable to verify the signed kernel fitimage.
Before that commit the signature of all keys used in the kernel fitimage
would be added to the u-boot dtb.

To fix this, always add the signature of UBOOT_SIGN_KEYNAME for
configuration nodes to the u-boot dtb. If FIT_SIGN_INDIVIDUAL is 1 also
add the signature of UBOOT_SIGN_IMG_KEYNAME for individual images.

This has one drawback at the moment: The signing of individual images is
not tested with fit_check_sign during concat_dtb.

Fixes: 3fb215a3af24 (u-boot: kernel-fitimage: Fix dependency loop if
UBOOT_SIGN_ENABLE and UBOOT_ENV enabled)
Reported-by: Rogerio Guerra Borin <rogerio.borin@toradex.com>
Signed-off-by: Leonard Anderweit <l.anderweit@phytec.de>
---
Link to bug report:
https://lists.openembedded.org/g/openembedded-core/topic/111218371#msg211507
---
 meta/classes-recipe/uboot-sign.bbclass | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-recipe/uboot-sign.bbclass
index 96c47ab01651..b2fcb5a31546 100644
--- a/meta/classes-recipe/uboot-sign.bbclass
+++ b/meta/classes-recipe/uboot-sign.bbclass
@@ -102,26 +102,36 @@ concat_dtb() {
 
 	if [ -e "${UBOOT_DTB_BINARY}" ]; then
 		# Re-sign the kernel in order to add the keys to our dtb
-		UBOOT_MKIMAGE_MODE="auto-conf"
-		# Signing individual images is not recommended as that
-		# makes fitImage susceptible to mix-and-match attack.
-		if [ "${FIT_SIGN_INDIVIDUAL}" = "1" ] ; then
-			UBOOT_MKIMAGE_MODE="auto"
-		fi
 		${UBOOT_MKIMAGE_SIGN} \
 			${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \
-			-f $UBOOT_MKIMAGE_MODE \
+			-f auto-conf \
 			-k "${UBOOT_SIGN_KEYDIR}" \
 			-o "${FIT_HASH_ALG},${FIT_SIGN_ALG}" \
-			-g "${UBOOT_SIGN_IMG_KEYNAME}" \
+			-g "${UBOOT_SIGN_KEYNAME}" \
 			-K "${UBOOT_DTB_BINARY}" \
 			-d /dev/null \
 			-r ${B}/unused.itb \
 			${UBOOT_MKIMAGE_SIGN_ARGS}
+
 		# Verify the kernel image and u-boot dtb
 		${UBOOT_FIT_CHECK_SIGN} \
 			-k "${UBOOT_DTB_BINARY}" \
 			-f ${B}/unused.itb
+
+		# Signing individual images is not recommended as that
+		# makes fitImage susceptible to mix-and-match attack.
+		if [ "${FIT_SIGN_INDIVIDUAL}" = "1" ] ; then
+			${UBOOT_MKIMAGE_SIGN} \
+				${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \
+				-f auto \
+				-k "${UBOOT_SIGN_KEYDIR}" \
+				-o "${FIT_HASH_ALG},${FIT_SIGN_ALG}" \
+				-g "${UBOOT_SIGN_IMG_KEYNAME}" \
+				-K "${UBOOT_DTB_BINARY}" \
+				-d /dev/null \
+				-r ${B}/unused.itb \
+				${UBOOT_MKIMAGE_SIGN_ARGS}
+		fi
 		cp ${UBOOT_DTB_BINARY} ${UBOOT_DTB_SIGNED}
 	fi
 
-- 
2.25.1

Re: [RFC] uboot-sign: Fix u-boot dtb signatures

[Rogerio Guerra Borin]

[-- Attachment #1: Type: text/plain, Size: 1319 bytes --]

Hi Leonard,

I've tested your patch and I wanted to let you know it worked fine for me both when FIT_SIGN_INDIVIDUAL="1" or "0". I've checked the contents of the u-boot dtb (for the presence of the required pubkeys) and the fitImage (for the signatures) and the results match what we had before commit d7bd9c62766 ("u-boot: kernel-fitimage: Fix dependency loop if UBOOT_SIGN_ENABLE and UBOOT_ENV enabled").

As for the patch, since the understanding is that when FIT_SIGN_INDIVIDUAL="1" the individual images will be signed besides the signing of the configurations then I'd say that sentence in the comment "Signing individual images is not recommended as that makes fitImage susceptible to mix-and-match attack" seems unnecessary/misleading to me since it gives the impression that one would get either images or configurations signed.

As for the check performed at build time by the "fit_check_sign" tool, the fact that now the check is done only on the configuration doesn't seem like a big loss to me. Though I imagine the ideal solution would be to have that check on the final fitImage rather than on a temporary one (unused.itb) in order to provide stronger guarantees that the image is correctly signed. However, this would likely complicate things which may make it not worth the effort...

Regards

[-- Attachment #2: Type: text/html, Size: 1442 bytes --]

Re: [OE-core] [RFC] uboot-sign: Fix u-boot dtb signatures

[Jose Quaresma]

[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]

Hi Leonard,

I tested it on a couple of machines on my side and it fixes the regression.

Tested-by: Jose Quaresma <jose.quaresma@foundries.io>

Thanks for working on the solution.

Jose

Rogerio Guerra Borin via lists.openembedded.org <rogerio.borin=
toradex.com@lists.openembedded.org> escreveu (quinta, 20/02/2025 à(s)
19:22):

> Hi Leonard,
>
> I've tested your patch and I wanted to let you know it worked fine for me
> both when FIT_SIGN_INDIVIDUAL="1" or "0". I've checked the contents of the
> u-boot dtb (for the presence of the required pubkeys) and the fitImage (for
> the signatures) and the results match what we had before commit d7bd9c62766
> ("u-boot: kernel-fitimage: Fix dependency loop if UBOOT_SIGN_ENABLE and
> UBOOT_ENV enabled").
>
> As for the patch, since the understanding is that when
> FIT_SIGN_INDIVIDUAL="1" the individual images will be signed besides the
> signing of the configurations then I'd say that sentence in the comment
> "Signing individual images is not recommended as that makes fitImage
> susceptible to mix-and-match attack" seems unnecessary/misleading to me
> since it gives the impression that one would get either images or
> configurations signed.
>
> As for the check performed at build time by the "fit_check_sign" tool, the
> fact that now the check is done only on the configuration doesn't seem like
> a big loss to me. Though I imagine the ideal solution would be to have that
> check on the final fitImage rather than on a temporary one (unused.itb) in
> order to provide stronger guarantees that the image is correctly signed.
> However, this would likely complicate things which may make it not worth
> the effort...
>
> Regards
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#211775):
> https://lists.openembedded.org/g/openembedded-core/message/211775
> Mute This Topic: https://lists.openembedded.org/mt/111289801/5052612
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [
> quaresma.jose@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>

-- 
Best regards,

José Quaresma

[-- Attachment #2: Type: text/html, Size: 3414 bytes --]