Skip to content

ED25519 KMU #335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 24, 2024
+102 −6
Merged

ED25519 KMU #335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions boot/bootutil/src/ed25519_psa.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,28 @@

#include <psa/crypto.h>
#include <psa/crypto_types.h>
#if defined(CONFIG_BOOT_SIGNATURE_USING_KMU)
#include <cracen_psa_kmu.h>
#endif

BOOT_LOG_MODULE_DECLARE(ed25519_psa);

#define SHA512_DIGEST_LENGTH 64
#define EDDSA_KEY_LENGTH 32
#define EDDSA_SIGNAGURE_LENGTH 64

#if defined(CONFIG_BOOT_SIGNATURE_USING_KMU)
/* List of KMU stored key ids available for MCUboot */
#define MAKE_PSA_KMU_KEY_ID(id) PSA_KEY_HANDLE_FROM_CRACEN_KMU_SLOT(CRACEN_KMU_KEY_USAGE_SCHEME_RAW, id)
static psa_key_id_t kmu_key_ids[3] = {
MAKE_PSA_KMU_KEY_ID(226),
MAKE_PSA_KMU_KEY_ID(228),
MAKE_PSA_KMU_KEY_ID(230)
};
#define KMU_KEY_COUNT (sizeof(kmu_key_ids)/sizeof(kmu_key_ids[0]))
#endif

#if !defined(CONFIG_BOOT_SIGNATURE_USING_KMU)
int ED25519_verify(const uint8_t *message, size_t message_len,
const uint8_t signature[EDDSA_SIGNAGURE_LENGTH],
const uint8_t public_key[EDDSA_KEY_LENGTH])
Expand Down Expand Up @@ -69,3 +84,39 @@ int ED25519_verify(const uint8_t *message, size_t message_len,

return ret;
}
#else
int ED25519_verify(const uint8_t *message, size_t message_len,
const uint8_t signature[EDDSA_SIGNAGURE_LENGTH],
const uint8_t public_key[EDDSA_KEY_LENGTH])
{
ARG_UNUSED(public_key);
/* Set to any error */
psa_status_t status = PSA_ERROR_BAD_STATE;
int ret = 0; /* Fail by default */

/* Initialize PSA Crypto */
status = psa_crypto_init();
if (status != PSA_SUCCESS) {
BOOT_LOG_ERR("PSA crypto init failed %d", status);
return 0;
}

status = PSA_ERROR_BAD_STATE;

for (int i = 0; i < KMU_KEY_COUNT; ++i) {
psa_key_id_t kid = kmu_key_ids[i];

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

@nordicjm nordicjm Oct 9, 2024

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

@de-nordic de-nordic Oct 9, 2024

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

@nordicjm nordicjm Oct 9, 2024

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

@de-nordic de-nordic Oct 9, 2024

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

@nordicjm nordicjm Oct 9, 2024

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

@de-nordic de-nordic Oct 9, 2024

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

if (status == PSA_SUCCESS) {
ret = 1;
break;
}

BOOT_LOG_ERR("ED25519 signature verification failed %d", status);
}

return ret;
}
#endif
19 changes: 15 additions & 4 deletions boot/bootutil/src/image_ed25519.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,16 @@
#include "bootutil/crypto/sha.h"

#define EDDSA_SIGNATURE_LENGTH 64

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

extern int ED25519_verify(const uint8_t *message, size_t message_len,
const uint8_t signature[EDDSA_SIGNATURE_LENGTH],
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";

/*
* Parse the public key used for signing.
*/
Expand Down Expand Up @@ -71,21 +73,25 @@ bootutil_import_key(uint8_t **cp, uint8_t *end)

return 0;
}
#endif

fih_ret
bootutil_verify_sig(uint8_t *hash, uint32_t hlen, uint8_t *sig, size_t slen,
uint8_t key_id)
{
int rc;
FIH_DECLARE(fih_rc, FIH_FAILURE);
uint8_t *pubkey;
uint8_t *pubkey = NULL;
#if !defined(CONFIG_BOOT_SIGNATURE_USING_KMU)
uint8_t *end;
#endif

if (hlen != IMAGE_HASH_SIZE || slen != EDDSA_SIGNATURE_LENGTH) {
FIH_SET(fih_rc, FIH_FAILURE);
goto out;
}

#if !defined(CONFIG_BOOT_SIGNATURE_USING_KMU)
pubkey = (uint8_t *)bootutil_keys[key_id].key;
end = pubkey + *bootutil_keys[key_id].len;

Expand All @@ -94,6 +100,7 @@ bootutil_verify_sig(uint8_t *hash, uint32_t hlen, uint8_t *sig, size_t slen,
FIH_SET(fih_rc, FIH_FAILURE);
goto out;
}
#endif

rc = ED25519_verify(hash, IMAGE_HASH_SIZE, sig, pubkey);

Expand All @@ -115,14 +122,17 @@ bootutil_verify_img(const uint8_t *img, uint32_t size,
{
int rc;
FIH_DECLARE(fih_rc, FIH_FAILURE);
uint8_t *pubkey;
uint8_t *pubkey = NULL;
#if !defined(CONFIG_BOOT_SIGNATURE_USING_KMU)
uint8_t *end;
#endif

if (slen != EDDSA_SIGNATURE_LENGTH) {
FIH_SET(fih_rc, FIH_FAILURE);
goto out;
}

#if !defined(CONFIG_BOOT_SIGNATURE_USING_KMU)
pubkey = (uint8_t *)bootutil_keys[key_id].key;
end = pubkey + *bootutil_keys[key_id].len;

Expand All @@ -131,6 +141,7 @@ bootutil_verify_img(const uint8_t *img, uint32_t size,
FIH_SET(fih_rc, FIH_FAILURE);
goto out;
}
#endif

rc = ED25519_verify(img, size, sig, pubkey);

Expand Down
6 changes: 6 additions & 0 deletions boot/bootutil/src/image_validate.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ bootutil_img_hash(struct enc_key_data *enc_state, int image_index,
# define KEY_BUF_SIZE (SIG_BUF_SIZE + 24)
#endif /* !MCUBOOT_HW_KEY */

#if !defined(CONFIG_BOOT_SIGNATURE_USING_KMU)
#if !defined(MCUBOOT_HW_KEY)
static int
bootutil_find_key(uint8_t *keyhash, uint8_t keyhash_len)
Expand Down Expand Up @@ -310,6 +311,7 @@ bootutil_find_key(uint8_t image_index, uint8_t *key, uint16_t key_len)
}
#endif /* !MCUBOOT_HW_KEY */
#endif /* !MCUBOOT_BUILTIN_KEY */
#endif /* !defined(CONFIG_BOOT_SIGNATURE_USING_KMU) */
#endif /* EXPECTED_SIG_TLV */

/**
Expand Down Expand Up @@ -626,6 +628,7 @@ bootutil_img_validate(struct enc_key_data *enc_state, int image_index,
break;
}
#endif /* defined(EXPECTED_HASH_TLV) && !defined(MCUBOOT_SIGN_PURE) */
#if !defined(CONFIG_BOOT_SIGNATURE_USING_KMU)
#ifdef EXPECTED_KEY_TLV
case EXPECTED_KEY_TLV:
{
Expand Down Expand Up @@ -656,14 +659,17 @@ bootutil_img_validate(struct enc_key_data *enc_state, int image_index,
break;
}
#endif /* EXPECTED_KEY_TLV */
#endif /* !defined(CONFIG_BOOT_SIGNATURE_USING_KMU) */
#ifdef EXPECTED_SIG_TLV
case EXPECTED_SIG_TLV:
{
#if !defined(CONFIG_BOOT_SIGNATURE_USING_KMU)
/* Ignore this signature if it is out of bounds. */
if (key_id < 0 || key_id >= bootutil_key_cnt) {
key_id = -1;
continue;
}
#endif /* !defined(CONFIG_BOOT_SIGNATURE_USING_KMU) */
if (!EXPECTED_SIG_LEN(len) || len > sizeof(buf)) {
rc = -1;
goto out;
Expand Down
2 changes: 1 addition & 1 deletion boot/zephyr/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ if(CONFIG_MCUBOOT_SERIAL)
endif()
endif()

if(NOT CONFIG_BOOT_SIGNATURE_KEY_FILE STREQUAL "")
if(NOT CONFIG_BOOT_SIGNATURE_USING_KMU OR NOT CONFIG_BOOT_SIGNATURE_KEY_FILE STREQUAL "")
# CONF_FILE points to the KConfig configuration files of the bootloader.
foreach (filepath ${CONF_FILE})
file(READ ${filepath} temp_text)
Expand Down
30 changes: 29 additions & 1 deletion boot/zephyr/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ endchoice # BOOT_IMG_HASH_ALG

config BOOT_SIGNATURE_TYPE_PURE_ALLOW
bool
depends on NRF_SECURITY
help
Hidden option set by configurations that allow Pure variant,
for example ed25519. The pure variant means that image
Expand Down Expand Up @@ -293,6 +294,7 @@ config BOOT_ED25519_MBEDTLS

config BOOT_ED25519_PSA
bool "Use PSA crypto"
depends on NRF_SECURITY
select BOOT_USE_PSA_CRYPTO
select BOOT_ED25519_PSA_DEPENDENCIES
select BOOT_X25519_PSA_DEPENDENCIES if BOOT_ENCRYPT_IMAGE
Expand All @@ -302,6 +304,22 @@ endif

endchoice

config BOOT_SIGNATURE_USING_KMU
bool "Use KMU stored keys for signature verification"
depends on NRF_SECURITY
depends on CRACEN_LIB_KMU
select PSA_WANT_ALG_GCM
select PSA_WANT_KEY_TYPE_AES
select PSA_WANT_AES_KEY_SIZE_256
select PSA_WANT_ALG_SP800_108_COUNTER_CMAC
select PSA_WANT_ALG_CMAC
select PSA_WANT_ALG_ECB_NO_PADDING
help
MCUboot will use keys provisioned to the device key management unit for signature
verification instead of compiling in key data from a file.

if !BOOT_SIGNATURE_USING_KMU

config BOOT_SIGNATURE_KEY_FILE
string "PEM key file"
default "root-ec-p256.pem" if BOOT_SIGNATURE_TYPE_ECDSA_P256
Expand All @@ -319,6 +337,8 @@ config BOOT_SIGNATURE_KEY_FILE
with the public key information will be written in a format expected by
MCUboot.

endif

config MCUBOOT_CLEANUP_ARM_CORE
bool "Perform core cleanup before chain-load the application"
depends on CPU_CORTEX_M
Expand All @@ -335,10 +355,18 @@ config MCUBOOT_CLEANUP_ARM_CORE
start-up code which can cause a module fault and potentially make the
module irrecoverable.

# Disable MBEDTLS from being selected if NRF_SECURITY is enabled, and use default NRF_SECURITY
# configuration file for MBEDTLS
config MBEDTLS
depends on !NRF_SECURITY

config NRF_SECURITY
select MBEDTLS_PROMPTLESS

if MBEDTLS

config MBEDTLS_CFG_FILE
default "mcuboot-mbedtls-cfg.h"
default "mcuboot-mbedtls-cfg.h" if !NRF_SECURITY

endif

Expand Down