Skip to content

Conversation

@de-nordic
Copy link
Contributor

@de-nordic de-nordic commented Sep 20, 2024

KMU support, where image signature is verified using KMU provisioned keys.

To test:

In order to make building possible: Need to include #352 :

--- a/boot/zephyr/prj.conf
+++ b/boot/zephyr/prj.conf
@@ -1,7 +1,6 @@
 CONFIG_PM=n
 
 CONFIG_MAIN_STACK_SIZE=10240
-CONFIG_MBEDTLS_CFG_FILE="mcuboot-mbedtls-cfg.h"
 
 CONFIG_BOOT_SWAP_SAVE_ENCTLV=n
 CONFIG_BOOT_ENCRYPT_IMAGE=n
west build  -p -d builds/hello_54_sb_ed_psa_kmu2 -b nrf54l15dk/nrf54l15/cpuapp zephyr/samples/hello_world/ -DSB_CONFIG_BOOTLOADER_MCUBOOT=y -DSB_CONFIG_BOOT_SIGNATURE_TYPE_ED25519=y -Dmcuboot_CONFIG_NRF_SECURITY=y -Dmcuboot_CONFIG_MBEDTLS=n -Dmcuboot_CONFIG_BOOT_ED25519_PSA=y -Dmcuboot_CONFIG_PM_PARTITION_SIZE_MCUBOOT=0x10000 -Dmcuboot_CONFIG_BOOT_SIGNATURE_USING_KMU=y

Console output should looks like:

I: Starting bootloader
I: Primary image: magic=unset, swap_type=0x1, copy_done=0x3, image_ok=0x3
I: Secondary image: magic=unset, swap_type=0x1, copy_done=0x3, image_ok=0x3
I: Boot source: none
I: Image index: 0, Swap type: none
E: ED25519 signature verification failed -136
E: ED25519 signature verification failed -133
I: Bootloader chainload address offset: 0x10000
I: Jumping to the first image slot
*** Booting nRF Connect SDK v2.7.99-3d0a30462cc2 ***
*** Using Zephyr OS v3.7.99-6ccfcdc1051d ***
Hello World! nrf54l15dk/nrf54l15/cpuapp

@de-nordic de-nordic force-pushed the x25519_kmu branch 2 times, most recently from ef5a064 to 4107577 Compare October 2, 2024 15:58
@de-nordic de-nordic marked this pull request as ready for review October 2, 2024 15:58
@de-nordic de-nordic changed the title X25519 kmu ED25519 KMU Oct 2, 2024
@de-nordic de-nordic force-pushed the x25519_kmu branch 2 times, most recently from e3c7a13 to e73e418 Compare October 8, 2024 13:01
Comment on lines 89 to 90
const uint8_t signature[64],
const uint8_t public_key[32])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use defines

Comment on lines +109 to +111
status = psa_verify_message(kid, PSA_ALG_PURE_EDDSA, message,
message_len, signature,
EDDSA_SIGNAGURE_LENGTH);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know about PSA but isn't the point in things like PSA that you need things like FIH enabled to prevent voltage/timing glitches from causing single variable assignments to get the wrong value and falsely pass when they really failed? And that being the whole reason MCUboot has FIH?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that would be a common problem for all the signature verification.
That includes all the ED25519_verify variants, even pre-existing, that do not use FIH

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would but e.g. nrf52/nrf53 MCUboot is not PSA certified, this is meant to be for PSA level 3 on nrf54l15?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change this but what it would really do would be to use status to set some FIH variable, of course the call to ED25519_verify needs to change too.
So we will have something like:

FIH_DECLARE(fih_rc, FIH_FAILURE)
...
for (i = 0; i < KMU_KEY_COUNT; ++i)
{
....
}

if (status == PSA_SUCCESS)
{
FIH_SET(fih_rc, FIH_SUCCESS);
}
FIH_RET(fih_rc);

OK, so if I have status returned from PSA in non-FIH manner, all I have to do is to make the if (status == PSA_SUCCESS) to get true.

I do not know whether timing attack can do that, voltage probably can, em glitching too.

I guess that the comparison will be resolved as status bit check after loading some cpu register with status, or there will be arithmetic operation that will result in status register bits set and then branch instruction depending on them.

At this point, setting of FIH_SET(fih_rc, FIH_SUCCESS); depends on one bit. If I can em-beam that bit, voltage glitch setting that bit, or whatever, the gap between the PSA returned status and when it is used to set the FIH status, is place of exploitation that is not mitigated by FIH.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: not requesting changes here for this, was just surprised to see it, I think the security/PSA people should be responsible for making any decisions on this since they know what is needed for PSA certification, I do not

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually thinking on making the change across all the paths of verification of signatures. That is why I have that quick 'one-bit' response, I just got completely lost enthusiasm into using the FIH, as we can not enforce it on PSA api.

https://www.youtube.com/watch?v=q9o9sKY2hk8

help
The MCUboot will use keys provisioned to board for signature verification
instead of compiling in a key data.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

help text goes last

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come I always do this in Kconfigs?


config BOOT_SIGNATURE_USING_KMU
bool "Use KMU stored keys for signature verification"
default n
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
default n

Never needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, and this is another thing I can not learn not do do.

@nvlsianpu
Copy link
Contributor

Approved in advance - apply Jamie's comments

@nvlsianpu nvlsianpu added this to the ncs-2.8.0 milestone Oct 10, 2024
@de-nordic de-nordic requested a review from nordicjm October 10, 2024 15:46
Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also:

/tmp/bb/bootloader/mcuboot/boot/bootutil/src/image_ed25519.c: In function 'bootutil_verify_sig':
/tmp/bb/bootloader/mcuboot/boot/bootutil/src/image_ed25519.c:105:53: error: type of formal parameter 4 is incomplete
  105 |     rc = ED25519_verify(hash, IMAGE_HASH_SIZE, sig, pubkey);
      |                                                     ^~~~~~
/tmp/bb/bootloader/mcuboot/boot/bootutil/src/image_ed25519.c: In function 'bootutil_verify_img':
/tmp/bb/bootloader/mcuboot/boot/bootutil/src/image_ed25519.c:146:41: error: type of formal parameter 4 is incomplete
  146 |     rc = ED25519_verify(img, size, sig, pubkey);
      |                                         ^~~~~~

Comment on lines 31 to 36
const uint8_t public_key[NUM_ED25519_BYTES]);

#if !defined(CONFIG_BOOT_SIGNATURE_USING_KMU)

static const uint8_t ed25519_pubkey_oid[] = MBEDTLS_OID_ISO_IDENTIFIED_ORG "\x65\x70";
#define NUM_ED25519_BYTES 32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how can this work when this is defined after it is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have messed up rebases while keeping parallel PRs that touch the same file.

Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is a mess and does not work, MCUboot either needs a key or it needs BOOT_HW_KEY selected which then needs a way of loading the key from an external source i.e. hardware, I have selected that Kconfig but the code will not compile because that function is not implemented. If that option is not selected, it won't compile because the key variable is not defined. And the Kconfig added here should just be an addition to BOOT_HW_KEY, not some extra frankenstein option.

@nvlsianpu
Copy link
Contributor

@de-nordic Can you help on what @nordicjm encountered?

Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I check out the nrf PR and do west update, I then configure MCUboot for nrf54l15dk and change the following using menuconfig:

CONFIG_BOOT_USE_PSA_CRYPTO=y
CONFIG_BOOT_IMG_HASH_DIRECTLY_ON_STORAGE=y
CONFIG_BOOT_IMG_HASH_ALG_SHA512=y
CONFIG_BOOT_SIGNATURE_TYPE_ED25519=y
CONFIG_BOOT_SIGNATURE_TYPE_PURE=y
CONFIG_BOOT_ED25519_PSA=y
CONFIG_BOOT_SIGNATURE_USING_KMU=y

I am then stuck in an infinite Kconfig error loop that I can't get out of (bad):

-- Including generated dts.cmake file: /tmp/bb/bootloader/mcuboot/boot/zephyr/_AA/zephyr/dts.cmake
warning: MBEDTLS_PSA_CRYPTO_C (defined at /tmp/bb/nrf/subsys/nrf_security/Kconfig.psa:7, /tmp/bb/nrf/subsys/suit/platform/Kconfig:58, modules/mbedtls/Kconfig.tls-generic:563, modules/mbedtls/Kconfig.tls-generic:563) has direct dependencies NRF_SECURITY || (MBEDTLS && SOC_POSIX && SUIT) || (!BUILD_WITH_TFM && MBEDTLS_BUILTIN && MBEDTLS_CFG_FILE = "config-tls-generic.h" && MBEDTLS) || (!BUILD_WITH_TFM && MBEDTLS_BUILTIN && MBEDTLS_CFG_FILE = "config-tls-generic.h" && MBEDTLS && 0) with value n, but is currently being y-selected by the following symbols:
- BOOT_USE_PSA_CRYPTO (defined at /tmp/bb/bootloader/mcuboot/boot/zephyr/Kconfig:30), with value y, direct dependencies y (value: y)
warning: PSA_WANT_ALG_GCM (defined at modules/mbedtls/Kconfig.psa:67, modules/mbedtls/Kconfig.psa:67, modules/mbedtls/Kconfig.psa:67) has direct dependencies (PSA_CRYPTO_CLIENT && MBEDTLS_PSA_CRYPTO_C && NRF_SECURITY) || PSA_CRYPTO_CLIENT || (PSA_CRYPTO_CLIENT && 0) with value n, but is currently being y-selected by the following symbols:
- BOOT_SIGNATURE_USING_KMU (defined at /tmp/bb/bootloader/mcuboot/boot/zephyr/Kconfig:305), with value y, direct dependencies y (value: y)
warning: PSA_WANT_ALG_CMAC (defined at modules/mbedtls/Kconfig.psa:35, modules/mbedtls/Kconfig.psa:35, modules/mbedtls/Kconfig.psa:35) has direct dependencies (PSA_CRYPTO_CLIENT && MBEDTLS_PSA_CRYPTO_C && NRF_SECURITY) || PSA_CRYPTO_CLIENT || (PSA_CRYPTO_CLIENT && 0) with value n, but is currently being y-selected by the following symbols:
- BOOT_SIGNATURE_USING_KMU (defined at /tmp/bb/bootloader/mcuboot/boot/zephyr/Kconfig:305), with value y, direct dependencies y (value: y)
warning: PSA_WANT_ALG_ECB_NO_PADDING (defined at modules/mbedtls/Kconfig.psa:51, modules/mbedtls/Kconfig.psa:51, modules/mbedtls/Kconfig.psa:51) has direct dependencies (PSA_CRYPTO_CLIENT && MBEDTLS_PSA_CRYPTO_C && NRF_SECURITY) || PSA_CRYPTO_CLIENT || (PSA_CRYPTO_CLIENT && 0) with value n, but is currently being y-selected by the following symbols:
- BOOT_SIGNATURE_USING_KMU (defined at /tmp/bb/bootloader/mcuboot/boot/zephyr/Kconfig:305), with value y, direct dependencies y (value: y)
warning: PSA_WANT_ALG_SHA_512 (defined at modules/mbedtls/Kconfig.psa:140, modules/mbedtls/Kconfig.psa:140, modules/mbedtls/Kconfig.psa:140) has direct dependencies (PSA_CRYPTO_CLIENT && MBEDTLS_PSA_CRYPTO_C && NRF_SECURITY) || PSA_CRYPTO_CLIENT || (PSA_CRYPTO_CLIENT && 0) with value n, but is currently being y-selected by the following symbols:
- BOOT_ED25519_PSA_DEPENDENCIES (defined at /tmp/bb/bootloader/mcuboot/boot/zephyr/Kconfig:81), with value y, direct dependencies BOOT_USE_PSA_CRYPTO (value: y), and select condition BOOT_USE_PSA_CRYPTO (value: y)
warning: PSA_WANT_KEY_TYPE_AES (defined at modules/mbedtls/Kconfig.psa:244, modules/mbedtls/Kconfig.psa:244, modules/mbedtls/Kconfig.psa:244) has direct dependencies (PSA_CRYPTO_CLIENT && MBEDTLS_PSA_CRYPTO_C && NRF_SECURITY) || PSA_CRYPTO_CLIENT || (PSA_CRYPTO_CLIENT && 0) with value n, but is currently being y-selected by the following symbols:
- BOOT_SIGNATURE_USING_KMU (defined at /tmp/bb/bootloader/mcuboot/boot/zephyr/Kconfig:305), with value y, direct dependencies y (value: y)
warning: PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_IMPORT (defined at modules/mbedtls/Kconfig.psa:274, modules/mbedtls/Kconfig.psa:274, modules/mbedtls/Kconfig.psa:274) has direct dependencies (PSA_CRYPTO_CLIENT && MBEDTLS_PSA_CRYPTO_C && NRF_SECURITY) || PSA_CRYPTO_CLIENT || (PSA_CRYPTO_CLIENT && 0) with value n, but is currently being y-selected by the following symbols:
- BOOT_ED25519_PSA_DEPENDENCIES (defined at /tmp/bb/bootloader/mcuboot/boot/zephyr/Kconfig:81), with value y, direct dependencies BOOT_USE_PSA_CRYPTO (value: y), and select condition BOOT_USE_PSA_CRYPTO (value: y)
warning: PSA_WANT_AES_KEY_SIZE_256 (defined at /tmp/bb/nrf/subsys/nrf_security/src/drivers/Kconfig:233) has direct dependencies MBEDTLS_PSA_CRYPTO_C && NRF_SECURITY with value n, but is currently being y-selected by the following symbols:
- BOOT_SIGNATURE_USING_KMU (defined at /tmp/bb/bootloader/mcuboot/boot/zephyr/Kconfig:305), with value y, direct dependencies y (value: y)
error: Aborting due to Kconfig warnings

Whatever these dependencies are, they need to be properly set up in Kconfig so that this error loop is not possible to enter

@de-nordic de-nordic force-pushed the x25519_kmu branch 3 times, most recently from a80ec69 to b53c185 Compare October 23, 2024 13:53
The commit adds verification of image using keys stored in KMU.

Signed-off-by: Dominik Ermel <[email protected]>
@nordicjm nordicjm merged commit 1dbca8f into nrfconnect:main Oct 24, 2024
@de-nordic de-nordic deleted the x25519_kmu branch May 13, 2025 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants