Skip to content
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.

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

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

Merged
merged 3 commits into from
Oct 1, 2024

Conversation

de-nordic
Copy link
Contributor

@de-nordic de-nordic commented Aug 2, 2024

Only top four commits are relevant here, the rest are changes the PR is based on ( #323)

The PR provides:

  • usage of SHA512 in ED25519 signature verification where PureEdDSA(sha512(image)) is used
  • support to directly invoke SHA512 calculation on storage, which enabled hardware accelerated SHA512 calculation (when provided by hardware)
  • support for PureEdDSA(image) where signature is directly calculated over image and there is not hash/digest calculation.

Note that there is upstream PR for imgtool that supports this changes:
mcu-tools/mcuboot#2063
and PR that reserves the TLV
mcu-tools/mcuboot#2029
in upstream, so that we do not get in conflict with definitions before we are able to upstream the code.

Placed on top of #323

@michalek-no michalek-no changed the title The proper ED255129 implementation with SHA512 The proper ED25519 implementation with SHA512 Aug 5, 2024
@de-nordic
Copy link
Contributor Author

I was finally able to develop the imgtool version that does the single hash ed25519. The problem is that while imgtool is able to verify signed image fine, the MCUboot is not. I am trying to figure out what is happening.

@de-nordic de-nordic force-pushed the x25519_enc_pure branch 3 times, most recently from 4631e1b to 8c7f38c Compare September 6, 2024 07:22
@de-nordic de-nordic marked this pull request as ready for review September 13, 2024 15:31
@de-nordic de-nordic changed the title The proper ED25519 implementation with SHA512 ED25519 implementation with SHA512 + support to calculate SHA512 directly on storage + PureEdDSA Sep 13, 2024
Comment on lines 38 to 40
target_include_directories(MCUBOOT_BOOTUTIL INTERFACE
${ZEPHYR_MBEDTLS_MODULE_DIR}/include
)
Copy link
Contributor

Choose a reason for hiding this comment

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

2 space indent for cmake

Comment on lines 55 to 57
zephyr_library_include_directories(
${ZEPHYR_MBEDTLS_MODULE_DIR}/include
)
Copy link
Contributor

Choose a reason for hiding this comment

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

etc.

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

config BOOT_USE_PSA_CRYPTO
bool
# Hidden option
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
# Hidden option

@@ -60,6 +70,58 @@ config NRF_CC310_BL
bool
default n

if BOOT_USE_PSA_CRYPTO
config BOOT_PSA_IMG_HASH_ALG_SHA256_DEPENDENCIES
Copy link
Contributor

Choose a reason for hiding this comment

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

newline gap

Dependencies for ed25519 signature

if BOOT_ENCRYPT_IMAGE
config BOOT_X25519_PSA_DEPENDENCIES
Copy link
Contributor

Choose a reason for hiding this comment

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

etc.

Comment on lines 23 to 24
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 from above

&buf[EC_CIPHERKEY_INDEX],
sizeof(iv_and_key) - PSA_CIPHER_IV_LENGTH(PSA_KEY_TYPE_AES, PSA_ALG_CTR));

/* QQQ: are we sure that enckey has the size proper to fit the key ? */
Copy link
Contributor

Choose a reason for hiding this comment

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

should these comments be addressed/looked at?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. The comments are note for improvement, where MCUboot has very poor "management" of various identifiers/constants that are hard to track.

@@ -126,6 +128,15 @@ bootutil_img_hash(struct enc_key_data *enc_state, int image_index,
/* If protected TLVs are present they are also hashed. */
size += hdr->ih_protect_tlv_size;

#ifdef MCUBOOT_HASH_STORAGE_DIRECTLY

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* be directly given to hashing function.
*/
bootutil_sha_update(&sha_ctx, (void *)flash_area_get_off(fap), size);

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

bootutil_sha_update(&sha_ctx, (void *)flash_area_get_off(fap), size);

#else /* MCUBOOT_HASH_STORAGE_DIRECTLY */

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Not resolved

uint8_t *pubkey;
uint8_t *end;

if (slen != 64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

use define

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

uint16_t len;
int32_t rc;

rc = bootutil_tlv_iter_begin(&it, hdr, fap, IMAGE_TLV_SIG_PURE, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

security vulnerability: this can reside in protected or unprotected TLV section, so someone can manipulate an image to either have or not have this from an update that has the inverse - can that cause problems? And why is this not forced to protected area?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only indicates that image is build with pure signature, if you turn this off the MCUboot that was build for pure will just not validate image as it expects "pure image"; if you pass image with this to something that does not understand pure, the chances of validation are slim, because it will just try to validate sha against pure signature.
The real reason for this is to be able to distinguish between images, because otherwise you can not tell the signatures apart, when trying to solve issues.

@de-nordic de-nordic force-pushed the x25519_enc_pure branch 5 times, most recently from c81af6d to 04ae52b Compare September 26, 2024 17:13
Use SHA512 directly calculated over image with the ED25519 signature.

Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>
bootutil_sha_update(&sha_ctx, (void *)flash_area_get_off(fap), size);

#else /* MCUBOOT_HASH_STORAGE_DIRECTLY */

Copy link
Contributor

Choose a reason for hiding this comment

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

Not resolved

@@ -137,8 +137,18 @@
#endif

#ifdef CONFIG_BOOT_DECOMPRESSION

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved after the ifdef, to separate blocks.

@@ -566,8 +613,12 @@ bootutil_img_validate(struct enc_key_data *enc_state, int image_index,
}

image_hash_valid = 1;
break;
}
#endif /*def EXPECTED_HASH_TLV */
Copy link
Contributor

Choose a reason for hiding this comment

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

defined(condition) also missing correct conditions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 147 to 149
#ifdef MCUBOOT_ENC_IMAGES
#error "Direct hash check is currently not supported when encrypted images are enabled"
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

this check can go really, it's already enforced by Kconfig, doesn't make sense to check for something that isn't possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, although I must admit that I do like to check these things also in code, at compile-time.

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.

OK once unneeded check is dropped

The commit add support for passing storage device address space
to hash calculation functions, which allows to use hardware
accelerated hash calculation on storage.
This feature only works when image encryption is not enabled
and all slots are defined within internal storage of device.

The feature is enabled using Kconfig option
 CONFIG_BOOT_IMG_HASH_DIRECTLY_ON_STORAGE

Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>
The commit adds support for PureEdDSA, which validates signature
of image rather than hash. This is most secure, available, ED25519
usage in MCUboot, but due to requirement of PureEdDSA to be able
to calculate signature at whole message at once, here image,
it only works on setups where entire image can be mapped to
device address space, so that PSA functions calculating the
signature can see the whole image at once.

This option is enabled with Kconfig option:
 CONFIG_BOOT_SIGNATURE_TYPE_PURE
when the ED25519 signature type is already selected.

Note that the option will enable SHA512 for calculating public
key hash.

Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>
@nvlsianpu nvlsianpu merged commit de524e9 into nrfconnect:main Oct 1, 2024
1 check passed
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.

3 participants