-
Notifications
You must be signed in to change notification settings - Fork 697
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
PureEdDSA and support for calculating SHA directly on device #2080
base: main
Are you sure you want to change the base?
Conversation
1d63554
to
46a8117
Compare
@taltenbach FYI |
1cef60b
to
a66656b
Compare
rebase needed |
a66656b
to
6fcbb82
Compare
Done |
6fcbb82
to
33d2fd5
Compare
boot/zephyr/Kconfig
Outdated
to address space or RAM area, enabling this option allows hash | ||
calculation functions to directly access the storage through that address | ||
space or using its own DMA. This reduces flash read overhead done | ||
by the MCUboot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by the MCUboot. | |
by MCUboot. |
boot/zephyr/Kconfig
Outdated
by the MCUboot. | ||
Notes: | ||
- not supported when encrypted images are in use, because calculating | ||
SHA requires image to be decrypted first, which is done to RAM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SHA requires image to be decrypted first, which is done to RAM. | |
SHA requires image to be decrypted first, which is done in RAM. |
boot/zephyr/Kconfig
Outdated
SHA requires image to be decrypted first, which is done to RAM. | ||
- currently only supported on internal storage of devices; this | ||
option will not work with devices that use external storage for | ||
either of image slots. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either of image slots. | |
either of the image slots. |
@@ -141,6 +141,13 @@ | |||
#define MCUBOOT_DECOMPRESS_IMAGES | |||
#endif | |||
|
|||
/* Invoke hashing functions directly on storage. This requires for device |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* Invoke hashing functions directly on storage. This requires for device | |
/* Invoke hashing functions directly on storage device. This requires the device |
@@ -141,6 +141,13 @@ | |||
#define MCUBOOT_DECOMPRESS_IMAGES | |||
#endif | |||
|
|||
/* Invoke hashing functions directly on storage. This requires for device | |||
* to be able to map storage to address space or RAM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* to be able to map storage to address space or RAM. | |
* be able to map storage to address space or RAM. |
|
||
/* Search for the TLV */ | ||
rc = bootutil_tlv_iter_next(&it, &off, &len, NULL); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should check the value too and it should only have one valid value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean that the TLV should have a value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Value and length, even if length is 1 and value is 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I also see that layout is not documented.
boot/bootutil/src/image_validate.c
Outdated
@@ -425,6 +461,67 @@ bootutil_img_validate(struct enc_key_data *enc_state, int image_index, | |||
FIH_DECLARE(security_counter_valid, FIH_FAILURE); | |||
#endif | |||
|
|||
#ifdef MCUBOOT_DECOMPRESS_IMAGES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be here
5054696
to
5cc083f
Compare
|
||
/* Search for the TLV */ | ||
rc = bootutil_tlv_iter_next(&it, &off, &len, NULL); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I also see that layout is not documented.
boot/bootutil/src/image_validate.c
Outdated
rc = !image_hash_valid; | ||
if (rc) { | ||
goto out; | ||
} | ||
#elif defined(MCUBOOT_SIGN_PURE) | ||
/* This returns true on EQ, rc is err on non-0 */ | ||
rc = !FIH_EQ(valid_signature, FIH_SUCCESS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use FIH_NOT_EQ()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -268,6 +268,9 @@ struct boot_loader_state { | |||
fih_ret bootutil_verify_sig(uint8_t *hash, uint32_t hlen, uint8_t *sig, | |||
size_t slen, uint8_t key_id); | |||
|
|||
fih_ret bootutil_verify_img(const uint8_t *img, uint32_t size, | |||
uint8_t *sig, size_t slen, uint8_t key_id); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would appreciate code comment on difference to above function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment.
5fa4c21
to
c9d0e3c
Compare
bool "Use Pure signature of image" | ||
depends on BOOT_SIGNATURE_TYPE_PURE_ALLOW | ||
help | ||
The Pure signature is calculated directly over image rather than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have some problems with the wording here. I would argue "This is a more secure signature" is a little misleading. It is unclear to me how this affects the security when doing hardware encryption, because we can still do the signature of the hash by the hardware. The only real thing that PureEdDSA does is make the algorithm more resilient to weaknesses found in the hash function used. It doesn't affect the security of the signature itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is actually a bit of a performance hit to Pure mode as well, as it requires hashing the image twice, whereas Hash EdDSA hashes the image once, and then hashes that has twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have some problems with the wording here. I would argue "This is a more secure signature" is a little misleading. It is unclear to me how this affects the security when doing hardware encryption, because we can still do the signature of the hash by the hardware. The only real thing that PureEdDSA does is make the algorithm more resilient to weaknesses found in the hash function used. It doesn't affect the security of the signature itself.
I can remove the "more secure" info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is actually a bit of a performance hit to Pure mode as well, as it requires hashing the image twice, whereas Hash EdDSA hashes the image once, and then hashes that has twice.
A hardware may access an image directly via address space which improves performance , and I do not think that hash is calculated twice, I suspect it to be cached during operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the "more secure" or confusing, I think, comments.
29bb94b
to
496f44f
Compare
The commit enables SHA512 support, for image hashing, with TinyCrypt. Although on 32bit machines the SHA256 will be faster than SHA512, benefit of enabling the SHA512 is that you have only one algorithm compiled in which reduces size of code. Signed-off-by: Dominik Ermel <[email protected]>
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 with MCUboot configuration option MCUBOOT_HASH_STORAGE_DIRECTLY. Signed-off-by: Dominik Ermel <[email protected]>
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. The feature is enabled with MCUBOOT_SIGN_PURE MCUboot configuration option. Signed-off-by: Dominik Ermel <[email protected]>
Select BOOT_IMG_HASH_ALG_SHA512_ALLOW via BOOT_ED25519_TINYCRYPT. Signed-off-by: Dominik Ermel <[email protected]>
Adds CONFIG_BOOT_IMG_HASH_DIRECTLY_ON_STORAGE, which enables MCUBOOT_HASH_STORAGE_DIRECTLY for Zephyr. Signed-off-by: Dominik Ermel <[email protected]>
Commit adds CONFIG_BOOT_SIGNATURE_TYPE_PURE Kconfig option, which enables MCUBOOT_SIGN_PURE in MCUboot configuration. Signed-off-by: Dominik Ermel <[email protected]>
496f44f
to
75de637
Compare
@d3zd3z Got time to revisit? |
Note on latest change: I have divided commits into bootutil and Zephyr themed so that any problem with Zephyr would not immediately break the feature PR.
Six commits:
Note that mentioned below Kconfig options have proper MCUBOOT_ identifiers in code, but it is easier to communicate change by using them in examples.
Both commits provide support only on devices that are within address space.
There is possibility to extend both commits to work with external devices via features like XIP, where external device can be mapped to internal address space, but this is not approached here.
The PureEdDSA change adds TLV that marks the image as signed with Pure signature and logic in MCUboot that rejects images without the TLV, when expected by MCUboot. The change also slightly improves security of devices, specifically if the verification can be done by dedicated coprocessor.
Both solutions slightly improve performance as they cut on additional processing.
The PureEdDSA commit requires imgtool change: #2063
Notes on stats:
for the 512, the change was in the last config:
Enabling the option
CONFIG_MCUBOOT_HASH_STORAGE_DIRECTLY
, that comes with second commit, reduces code by another ~50 bytes.Enabling the
CONFIG_BOOT_SIGNATURE_TYPE_PURE
, provided by the third commit, shaves off additional ~150 bytes.While SHA directly on device and in RAM works with the same image signed for specified sha, the PureEdDSA requires special signature processing via imgtool and lacks HASH TLV.