PSA implementation of ed25519 image verification and x25519 random key encryption/decryption #323

de-nordic · 2024-07-24T17:06:54Z

The PSA encryption is quite complete.
The mbedTLS configuration is rather dodgy.
There are few requirements from upstream and mcuboot upmerge would be nice.

The TODO for reviewers:

check licenses on [noups]

nvlsianpu · 2024-08-06T17:24:47Z

boot/zephyr/Kconfig

+config BOOT_ED25519_PSA_DEPENDENCIES
+	bool
+	default n
+	select PSA_WANT_ALG_SHA_256


PSA_WANT_ALG_SHA_256 is stranger there - won't be there once we will introduce corrected signature right?

Yes, that would be with #325 and mcu-tools/mcuboot#2029

nvlsianpu · 2024-08-09T15:48:13Z

boot/bootutil/src/ed25519_psa.c

+        return 0;
+    }
+
+    status = psa_verify_message(kid, PSA_ALG_PURE_EDDSA, message, message_len,


Is this the place where psa_verify_hash() will be used instead for full sha512 based signature check?

Yes. Exactly. In the following PR 33a7ec4 there is already that part done.

The difference is that instead of the sha256, which is the messge here, we will get sha512, and therefore we do not rely on the internal sha512 calculation of psa_verify_message, but rather just verify the hash.

valeriosetti

I just had a quick look to the last 2 commits of this PR and I added some minor comments. I'll take a deeper look ASAP

boot/zephyr/Kconfig

valeriosetti · 2024-08-30T05:24:07Z

boot/zephyr/Kconfig

 	default BOOT_ED25519_TINYCRYPT
 config BOOT_ED25519_TINYCRYPT
 	bool "Use tinycrypt"
 	select BOOT_USE_TINYCRYPT
+	depends on !NRF_SECURITY


Question: isn't this a bit redundant with line 184? I mean we already have default BOOT_ED25519_PSA if NRF_SECURITY there, so I don't know if it's worth to add depends on !NRF_SECURITY here and on line 194 for Mbed TLS. Wdyt?

It just makes these options inaccessible.

valeriosetti · 2024-08-30T05:28:58Z

boot/bootutil/include/bootutil/crypto/aes_ctr.h

@@ -15,7 +15,7 @@
 #include "mcuboot_config/mcuboot_config.h"

 #if (defined(MCUBOOT_USE_MBED_TLS) + \
-     defined(MCUBOOT_USE_TINYCRYPT)) != 1
+     defined(MCUBOOT_USE_TINYCRYPT) + defined(MCUBOOT_USE_PSA_CRYPTO)) != 1
    #error "One crypto backend must be defined: either MBED_TLS or TINYCRYPT"


Suggested change

#error "One crypto backend must be defined: either MBED_TLS or TINYCRYPT"

#error "One crypto backend must be defined: either MBED_TLS or TINYCRYPT or PSA"

valeriosetti · 2024-08-30T06:50:10Z

boot/bootutil/src/image_ed25519.c

@@ -9,6 +9,11 @@

 #include "mcuboot_config/mcuboot_config.h"

+#if defined(CONFIG_NRF_SECURITY)
+/* We are not really using the MBEDTLS but need the ASN.1 parsing funcitons */


Minor detail, but I don't agree much with this comment. You are not using Mbed TLS for crypto, but you are using its ASN1 parse capabilities. So I would rephrase it as:

/* Enable ASN1 parse functions from Mbed TLS */

Wdyt?

Yeah, I can replace that. btw:#326

valeriosetti

I completed the 2nd pass I mentioned this morning and I left some new comments.

valeriosetti · 2024-08-30T15:16:41Z

boot/bootutil/src/encrypted_psa.c

+    psa_ret = psa_crypto_init();
+    if (psa_ret != PSA_SUCCESS) {
+        BOOT_LOG_ERR("AES crypto init failed %d", psa_ret);
+        return -1;


I think that you might add memset(private_key, 0, sizeof(private_key)); here as well just in case psa_crypto_init fails.

I will re-order operations so that the private key is not loaded before crypto init happens.

valeriosetti · 2024-08-30T15:21:15Z

boot/bootutil/src/encrypted_psa.c

+    psa_ret = psa_key_derivation_setup(&key_do, key_do_alg);
+    if (psa_ret != PSA_SUCCESS) {
+        BOOT_LOG_ERR("Key derivation setup failed %d", psa_ret);
+        return -1;


Here you might add psa_destroy_key(kid); just in case psa_key_derivation_setup fails.
From a practical point of view this might not matter much because the device won't boot in this case and the key is volatile (so it will be lost on power cycle), but it might be nice for cleanliness.

valeriosetti · 2024-08-30T15:30:13Z

boot/bootutil/src/encrypted.c

@@ -415,7 +387,7 @@ static int fake_rng(void *p_rng, unsigned char *output, size_t len)
 * @param enckey An AES-128 or AES-256 key sized buffer to store to plain key.
 */
 int
-boot_enc_decrypt(const uint8_t *buf, uint8_t *enckey)
+boot_decrypt_key(const uint8_t *buf, uint8_t *enckey)


Why not moving these functions out of encrypted.c for better code organization? In this case you would have 2 source files implementing these functions, i.e. encrypted_psa.c and the new one, without a huge #if !defined(CONFIG_BOOT_ED25519_PSA) in encrypted.c...

I do not want to do that within this PR, that is something I would like to do, but that has to be done upstream.
Because we are trying to do things with [nrf noups] till we are able to push the changes upstream, I do not want to divert from the upstream code too much.

nordicjm · 2024-09-16T08:31:22Z

boot/bootutil/zephyr/CMakeLists.txt

+target_include_directories(MCUBOOT_BOOTUTIL INTERFACE
+   ${ZEPHYR_MBEDTLS_MODULE_DIR}/include
+)


2 space indent for cmake. Is this going to be submitted upstream?

Having a look below, wouldn't be keeping the existing one and adding a new one for psa crypto work better since you're not changing a piece of the upstream code?

The general idea is to, at some point, bring this upstream.
I am keeping the PSA code separate because the change is significant and the code is already badly ifdefed in the verification/encryption calls.

Not resolved

nordicjm · 2024-09-16T08:31:30Z

boot/zephyr/CMakeLists.txt

+zephyr_library_include_directories(
+  ${ZEPHYR_MBEDTLS_MODULE_DIR}/include
+)


Not resolved

nordicjm · 2024-09-16T08:33:50Z

boot/zephyr/Kconfig

+# Within sdk-nrf NRF security is default
+config NRF_SECURITY
+	bool
+	default y
+	select NRF_OBERON
+	select BOOT_USE_PSA_CRYPTO
+
+# NRF_SECURITY enforces PSA crypt
+config MBEDTLS
+	bool
+	default n if NRF_SECURITY


NRF_SECURITY already exists, it should not be getting defined here again, likewise with MBEDTLS. Default should go in a Kconfig.defconfig file

nordicjm · 2024-09-16T08:34:18Z

boot/zephyr/Kconfig

@@ -27,6 +40,15 @@ config BOOT_USE_MBEDTLS
 	help
 	  Use mbedTLS for crypto primitives.

+config BOOT_USE_PSA_CRYPTO
+	bool
+	# Hidden option


Suggested change

# Hidden option

There should not be a comment for this, put hidden option in the help text only

Not resolved

nordicjm · 2024-09-16T08:34:57Z

boot/zephyr/Kconfig

+config MBEDTLS_HEAP_SIZE
+	int
+	default 2048
+	help
+	  The PSA internals need to be able to allocate memory for operation
+	  and it uses mbedTLS heap for that.
+
+config MBEDTLS_ENABLE_HEAP
+	bool
+	default y


same with these

nordicjm · 2024-09-16T08:35:11Z

boot/zephyr/Kconfig

+	default BOOT_IMG_HASH_ALG_SHA256 if BOOT_IMG_HASH_ALG_SHA256_ALLOW
+	default BOOT_IMG_HASH_ALG_SHA384 if BOOT_IMG_HASH_ALG_SHA384_ALLOW


help text always goes last

boot/zephyr/Kconfig

boot/bootutil/include/bootutil/crypto/aes_ctr.h

nordicjm · 2024-09-16T08:37:17Z

boot/bootutil/src/image_ed25519.c

+    /* Fixme: Fix needed here, as ed25519 should only use sha512 */
+    if (!(hlen == 64 || hlen == 32) || slen != 64) {


should be using defines for these sizes

nvlsianpu · 2024-09-25T06:16:05Z

@nordicjm Please revisit.

nordicjm · 2024-09-25T06:18:16Z

boot/bootutil/zephyr/CMakeLists.txt

+target_include_directories(MCUBOOT_BOOTUTIL INTERFACE
+   ${ZEPHYR_MBEDTLS_MODULE_DIR}/include
+)


Not resolved

nordicjm · 2024-09-25T06:18:23Z

boot/zephyr/CMakeLists.txt

+zephyr_library_include_directories(
+  ${ZEPHYR_MBEDTLS_MODULE_DIR}/include
+)


Not resolved

boot/zephyr/CMakeLists.txt

nordicjm · 2024-09-25T06:18:48Z

boot/zephyr/Kconfig

@@ -27,6 +40,15 @@ config BOOT_USE_MBEDTLS
 	help
 	  Use mbedTLS for crypto primitives.

+config BOOT_USE_PSA_CRYPTO
+	bool
+	# Hidden option


Not resolved

nordicjm · 2024-09-25T06:19:16Z

@nordicjm Please revisit.

None of the comments have been resolved and issues remain

nordicjm

Minor Kconfig nit.

nordicjm · 2024-09-26T15:40:25Z

boot/zephyr/Kconfig

+config BOOT_USE_PSA_CRYPTO
+	bool
+	default y if NRF_SECURITY
+	default n


Suggested change

default n

default n not needed

nordicjm · 2024-09-26T15:40:36Z

boot/zephyr/Kconfig

+
+config BOOT_ED25519_PSA_DEPENDENCIES
+	bool
+	default n


nordicjm · 2024-09-26T15:40:42Z

boot/zephyr/Kconfig

+if BOOT_ENCRYPT_IMAGE
+config BOOT_X25519_PSA_DEPENDENCIES
+	bool
+	default n


nordicjm · 2024-09-26T15:41:35Z

boot/zephyr/Kconfig

+if MBEDTLS_ENABLE_HEAP
+
+config MBEDTLS_HEAP_SIZE
+	int


Suggested change

int

This is an existing symbol already https://github.com/zephyrproject-rtos/zephyr/blob/main/modules/mbedtls/Kconfig#L191

In Mbed TLS 3.1, the private fields in the ASN.1 structure were made private. This breaks code that accesses these private macros. Fix this by changing the ASN.1 specific code to use a new field accessor `ASN1_CONTEXT_MEMBER` that will be conditionally defined based on the version of Mbed TLS that is present. Signed-off-by: David Brown <david.brown@linaro.org> (cherry picked from commit 1d79ef3) Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

Currently, when swap using scratch is used with encrypted images, MCUboot is decrypting the images during the copy from the secondary slot to the scratch area. This means the scratch area contains plaintext image data and therefore that the scratch area must be placed in the MCU's internal flash memory. This commit makes the necessary changes to perform the decryption when copying from the scratch area to the primary slot instead, making possible to place the scratch area in an external flash memory since the scratch area is now encrypted. Note that BOOT_SWAP_SAVE_ENCTLV must be enabled if the scratch area is placed in external flash memory. Signed-off-by: Thomas Altenbach <thomas.altenbach@legrand.com> (cherry picked from commit 08d2d94) Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

In the boot_image_validate_encrypted there was call to flash_area_id_to_multi_image_slot, which tries to figure out slot index from flash area and image index, and the result of the call was not used for anything as slot index is hardcoded in the next call to be 1 (secondary). Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no> (cherry picked from commit 4da4a72)

All of boot_enc_ function follow the same pattern where they take encryption context as the first parameter, and the boot_enc_decrypt stands out here as it does not work around the encryption context, but is rather single-part decryption function only used for decrypting of the image encryption key. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no> (cherry picked from commit 2371c0a)

…_drop. The enc_key_data.valid had been set to true when key has been added to the encryption context, but in case when boot_enc_drop was called, on the same context, the flag remained true, even though the context may no longer hold any valid context nor key. The commit moves the enc_key_data invalidation to enc_key_drop. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no> (cherry picked from commit 3355735)

… of image In all cases where boot_enc_load is called it is known what slot is addressed, so it is better to just pass the slot number instead of making the boot_enc_load figure out slot number from image index and provided flash area object. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no> (cherry picked from commit 7f9ac97)

…mage_to_sram There was not really needed repetition of code in if-else block; common code has been moved out and the block has been reduced. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no> (cherry picked from commit d09112a)

Convert tab to spaces; fix opening brace position. Signed-off-by: Fabio Utzig <utzig@apache.org> (cherry picked from commit d5e0e89)

boot_encrypt required the image_index paired with flash area pointer to be able to figure out which slot it will operate on. Since in most calls the slot is known in advance it can be just passed to the function directly. The commit replaces both parameters with slot number. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no> (cherry picked from commit 3f11286)

Move checking of conditions, which remain the same for the whole loop run, outside of the loop. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no> (cherry picked from commit 6fe259b)

…ndex There is no point for boot_enc_valid to take image index and flash area and use these to figure out slot number. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no> (cherry picked from commit 956311d)

…t_enc_decrypt To be able to implement encryption with API that requires different calls for encryption and encryption, the boot_encrypt needs to be replaced with encryption/decryption specific functions. Upstream PR: mcu-tools/mcuboot#2017 Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

Set of changes to Kconfig, CMakeLists.txt and some of headers that are required for the PSA support to compile. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

The commit provides implementation of image verification with ed25519 and encryption/decryption support where random key is encrypted using x25519. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

de-nordic requested a review from nvlsianpu July 24, 2024 17:06

de-nordic force-pushed the x25519_enc branch from bed190b to b13dc97 Compare July 25, 2024 16:34

de-nordic mentioned this pull request Jul 26, 2024

[noup] ED25519 Vetrification via PSA crypto #319

Closed

de-nordic force-pushed the x25519_enc branch from b13dc97 to 0dd6bfe Compare July 26, 2024 16:29

de-nordic marked this pull request as ready for review July 26, 2024 16:32

de-nordic added the DNM label Jul 26, 2024

NordicBuilder mentioned this pull request Jul 26, 2024

manifest: Add PSA ed25519 image encryption/verification to MCUboot nrfconnect/sdk-nrf#16641

Merged

de-nordic requested review from sigvartmh and frkv July 26, 2024 16:44

nvlsianpu reviewed Aug 6, 2024

View reviewed changes

nvlsianpu requested review from pdunaj and removed request for sigvartmh August 7, 2024 10:15

de-nordic force-pushed the x25519_enc branch from 0dd6bfe to 96d0c68 Compare August 7, 2024 16:56

nvlsianpu reviewed Aug 9, 2024

View reviewed changes

de-nordic mentioned this pull request Aug 9, 2024

[nrf noup] bootutil: Introduce MCUBOOT_ENC_RAW_PRIVATE_KEY #326

Closed

nvlsianpu approved these changes Aug 23, 2024

View reviewed changes

valeriosetti reviewed Aug 30, 2024

View reviewed changes

de-nordic force-pushed the x25519_enc branch 4 times, most recently from e23402b to e13c5f4 Compare September 6, 2024 07:19

de-nordic requested a review from valeriosetti September 11, 2024 08:31

valeriosetti approved these changes Sep 12, 2024

View reviewed changes

de-nordic removed the DNM label Sep 13, 2024

de-nordic mentioned this pull request Sep 13, 2024

ED25519 implementation with SHA512 + support to calculate SHA512 directly on storage + PureEdDSA #325

Merged

nordicjm requested changes Sep 16, 2024

View reviewed changes

nordicjm reviewed Sep 16, 2024

View reviewed changes

de-nordic force-pushed the x25519_enc branch 2 times, most recently from de6a34d to dd4e39f Compare September 18, 2024 07:23

nordicjm reviewed Sep 25, 2024

View reviewed changes

de-nordic force-pushed the x25519_enc branch 5 times, most recently from 41c5c33 to d98fcf0 Compare September 26, 2024 15:33

de-nordic requested a review from nordicjm September 26, 2024 15:35

nordicjm reviewed Sep 26, 2024

View reviewed changes

de-nordic force-pushed the x25519_enc branch 2 times, most recently from e78edce to 329814d Compare September 26, 2024 15:58

d3zd3z and others added 14 commits September 26, 2024 16:59

[nrf fromtree] Fix style issues

0bd46d6

Convert tab to spaces; fix opening brace position. Signed-off-by: Fabio Utzig <utzig@apache.org> (cherry picked from commit d5e0e89)

[nrf fromtree] boot: Simplify copy loop in boot_copy_region

2023198

Move checking of conditions, which remain the same for the whole loop run, outside of the loop. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no> (cherry picked from commit 6fe259b)

[nrf noup] PSA configuration required changes

691d763

Set of changes to Kconfig, CMakeLists.txt and some of headers that are required for the PSA support to compile. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

[nrf noup] PSA implementation of x25519 and ed25519 verification

0d4c91e

The commit provides implementation of image verification with ed25519 and encryption/decryption support where random key is encrypted using x25519. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>

de-nordic force-pushed the x25519_enc branch from 329814d to 0d4c91e Compare September 26, 2024 17:03

de-nordic requested a review from nordicjm September 27, 2024 10:30

nordicjm approved these changes Sep 27, 2024

View reviewed changes

nvlsianpu merged commit 5f95fec into nrfconnect:main Sep 27, 2024
1 check passed

	#error "One crypto backend must be defined: either MBED_TLS or TINYCRYPT"
	#error "One crypto backend must be defined: either MBED_TLS or TINYCRYPT or PSA"

		default BOOT_IMG_HASH_ALG_SHA256 if BOOT_IMG_HASH_ALG_SHA256_ALLOW
		default BOOT_IMG_HASH_ALG_SHA384 if BOOT_IMG_HASH_ALG_SHA384_ALLOW

		/* Fixme: Fix needed here, as ed25519 should only use sha512 */
		if (!(hlen == 64 \|\| hlen == 32) \|\| slen != 64) {

PSA implementation of ed25519 image verification and x25519 random key encryption/decryption #323

PSA implementation of ed25519 image verification and x25519 random key encryption/decryption #323

Conversation

de-nordic commented Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valeriosetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valeriosetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvlsianpu commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordicjm commented Sep 25, 2024

nordicjm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

de-nordic commented Jul 24, 2024 •

edited

Loading