-
Notifications
You must be signed in to change notification settings - Fork 780
Add support for multiple same-type signatures with key ID parsing #2305
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
base: main
Are you sure you want to change the base?
Conversation
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.
- Please update
signature
the data member of Image class tosignatures
.
6ce6e96
to
e0027a2
Compare
8301988
to
9322025
Compare
9322025
to
291cf66
Compare
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.
LGTM
Until the PR mcu-tools/mcuboot#2305 is merged, apply the added patches to support multiple signatures in mcuboot. Signed-off-by: Maulik Patel <[email protected]> Change-Id: Id8e62b7f332611858508bd445fab99fd7c0259ab
By default MCUBOOT_IMAGE_MULTI_SIG_SUPPORT is set to OFF, maintaining the existing single-signature behavior. When enabled, RSE secure image is also signed with additional ROTPK key. And both the signaures are verified during boot. This patch works with changes to mcuboot for multi-signature support (PR: mcu-tools/mcuboot#2305) Signed-off-by: Maulik Patel <[email protected]> Change-Id: Ic72d1dcfa3f3ada6c4d275281122f6d919a2d8e1
291cf66
to
6b64341
Compare
Until the PR mcu-tools/mcuboot#2305 is merged, apply the added patches to support multiple signatures in mcuboot. Signed-off-by: Maulik Patel <[email protected]> Change-Id: Id8e62b7f332611858508bd445fab99fd7c0259ab
By default MCUBOOT_IMAGE_MULTI_SIG_SUPPORT is set to OFF, maintaining the existing single-signature behavior. When enabled, RSE secure image is also signed with additional ROTPK key. And both the signaures are verified during boot. This patch works with changes to mcuboot for multi-signature support (PR: mcu-tools/mcuboot#2305) Signed-off-by: Maulik Patel <[email protected]> Change-Id: Ic72d1dcfa3f3ada6c4d275281122f6d919a2d8e1
6b64341
to
92e6949
Compare
Thank you @maulik-arm for all the updates! |
@maulik-arm I forgot to ask you to add a release note snippet for this major change. |
5be07c3
to
f2b3260
Compare
f2b3260
to
99b7b07
Compare
rebase needed |
99b7b07
to
90ff9de
Compare
When MCUBOOT_BUILTIN_KEY is enabled, the key id TLV entry is added to the image. Parse this entry while validating the image to identify the key used to sign the image. This enables future support for scenarios such as multiple built-in keys or multi-signature. Signed-off-by: Maulik Patel <[email protected]> Change-Id: Ibe26bc2b09e63350f4214719606a5aa4bc1be93c
This patch adds support for multiple signatures to single image. This is useful for scenarios where multiple keys are used to sign images, allowing for greater flexibility and security in the image verification process. The tool command line interface is extended to support multiple signatures. The imgtool test suite is updated to test the new functionality. Change-Id: I285b426671f6ad76472f0a2f8fb3a330f8882c3d Signed-off-by: Maulik Patel <[email protected]>
This commit adds functionality to the bootutil library to support multiple sign verfication of same type when 'MCUBOOT_BUILTIN_KEY' or 'MCUBOOT_HW_KEY' is enabled. The image_validate.c file is refactored such that: * bootutil_find_key() find the key is moved to a new file bootutil_find_key.c. * bootutil_image_hash() is moved to a new file bootutil_image_hash.c. * bootutil_img_security_cnt() is moved to a new file bootutil_img_security_cnt.c. This allows common validation code to be reused for multiple signatures. All code specific to multi sign is under the option 'MCUBOOT_IMAGE_MULTI_SIG_SUPPORT'. Furthermore, key id type is updated to uint32_t as per PSA crypto spec. Signed-off-by: Maulik Patel <[email protected]> Change-Id: I05c97ac385c5816c812c51feb010028df8412fe5
Since the key id concept in the PSA specific, rename the variables accordingly. Signed-off-by: Maulik Patel <[email protected]> Change-Id: I8a8a5ceba5554211f185cc4045a6081b6d407507
Signed-off-by: Maulik Patel <[email protected]> Change-Id: I12c087cb399d5be0079e2b9d02b57338f492d37c
For some platorms image_validate.c: In function 'bootutil_img_validate': image_validate.c:358:40: error: 'image_index' undeclared (first use in this function); did you mean 'image_header'? 358 | key_id = bootutil_find_key(image_index, buf, len); | ^~~~~~~~~~~ Resolve imgtool CI errors affecting certain signature verification tests. Change the return type of boot_verify_key_id_for_image to reflect its use as an FIH call. Add new bootutil source files to the Zephyr and espressif CMakeLists.txt files to fix undefined symbols. Update FIH tests to use the latest TFM version. This also requires updating the toolchain version to 14.2 in the Docker image. Signed-off-by: Maulik Patel <[email protected]> Change-Id: Ie75e6c533631b2696a4a41d86b64d4009fac0c54
90ff9de
to
5370225
Compare
@nordicjm |
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.
Passing PSA API key id as TLV is bad idea.
This PR moves a lot of code around and modifies it at the same time, making it barely reviewable.
- bootutil: imgtool: Fix CI failures -- touches a crap load of code here an there and only one change is related to the topic
- imgtool: Rename key-ids to psa-key-ids -- just bloats PR for no reason and could be tackled separately
- imgtool: Add support for multiple signatures -- regarding commit message: as far as I can tell multiple signatures do not improve security, quite the opposite , although they increase flexibility; you basically now have more locks and it is enough to unlock only one. And I do not think that we implement any key revocation at this point.
- bootutil: Parse key id for built in keys -- there is no need, or even desire, to pass psa key id, which is internal API id (equivalent of a pointer, if you will) via TLV
There is no documentation for the new feature, I mean the multiple key thing.
I do not understand why we need to pass this psa ID with image.
I do not think you even need to identify a key required for a signature.
All you need to have is a way to run your image signature through all the keys, preferably, available for that image index; once you have one key verify signature you are good to go.
User does not have to know a key id, nor we have to verify that this is the right key - signature verification process will do that for us.
#ifdef MCUBOOT_IMAGE_MULTI_SIG_SUPPORT | ||
/** | ||
* @brief Checks the key policy for signature verification. | ||
* | ||
* Determines whether a given key might or must be used to sign an image, | ||
* based on the validity of the signature and the key index. Updates the | ||
* provided output parameters to reflect the policy. | ||
* | ||
* @param valid_sig Indicates if the signature is valid. | ||
* @param key The key index to check. | ||
* @param[out] key_might_sign Set to true if the key might be used to sign. | ||
* @param[out] key_must_sign Set to true if the key must be used to sign. | ||
* @param[out] key_must_sign_count Set to the number of keys that must sign. | ||
* | ||
* @return 0 on success, or a negative error code on failure. | ||
*/ | ||
int boot_plat_check_key_policy(bool valid_sig, uint32_t key, | ||
bool *key_might_sign, bool *key_must_sign, | ||
uint8_t *key_must_sign_count); | ||
#endif /* MCUBOOT_IMAGE_MULTI_SIG_SUPPORT */ |
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.
Neither used nor implemented.
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 implemented by the platform to configure respective policy. Example TF-M implementation: https://git.trustedfirmware.org/plugins/gitiles/TF-M/trusted-firmware-m.git/+/refs/heads/main/bl2/src/thin_psa_crypto_core.c#226
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.
That is not called nor used by MCUboot. That is TF-M thing and goes over there.
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.
It is a platform specific hook which is called in image_validate.c in same commit after sign verification (under #ifdef MCUBOOT_IMAGE_MULTI_SIG_SUPPORT). There is already a similar platform specific function 'boot_retrieve_public_key_hash' which is not implemented by MCUboot. So may I know the difference between the two?
* - Image is signed with an arbitrary key and the corresponding public | ||
* key is added as a TLV field. |
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.
Formatting.
* - During public key validation (comparing against key-hash read from | ||
* HW) a fault is injected to accept the public key as valid one. | ||
*/ | ||
FIH_CALL(boot_fih_memequal, fih_rc, hash, key_hash, key_hash_size); |
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.
What is the point of finding a key by key id to then verify it with hash? If you have a hash, you can literally map it to desired key id, without exposing api specific key id to anyone. Hey, you can even change internal key mapping in further release without need to re-sign old images, because key id changes, as knowledge on key id is not needed at the build time.
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.
Looks like there’s a mix-up here:
- MCUBOOT_BUILTIN_KEY
Uses a PSA key ID baked into the image header.
That’s the one mode where an integer handle is attached. - MCUBOOT_HW_KEY
Doesn’t use a numeric key ID in the image.
Instead you ship a hash, which at boot we compare against the platform’s provisioned key.
Hope that clears it up—let me know if you have any other questions!
@@ -98,6 +98,7 @@ extern "C" { | |||
*/ | |||
#define IMAGE_TLV_KEYHASH 0x01 /* hash of the public key */ | |||
#define IMAGE_TLV_PUBKEY 0x02 /* public key */ | |||
#define IMAGE_TLV_KEYID 0x03 /* 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.
I do not think it is good idea. Why not just encode pointer to a key in memory, or data section, if we are already exposing internal api identifiers to user.
Note also that this basically means that you are locked with certain id and have to re-sign images if you find a problem later and figure out that, for some reason, you can not use ids (for example key storage is broken for id [n,m]). Now you not only have to re-build MCUboot but re-issue all app images, even if they do not change internally, just to make them work with mcuboot.
This increases coupling between MCUboot internal implementation and user release process and decrease information hiding, which is not good for coherent and modular design of anything.
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.
Hey, thanks for the feedback! So,
-
PSA key IDs aren’t “internal” magic
They’re part of the public Crypto API—psa_key_id_t
is exactly the handle you pass into everypsa_*
call. It’s not a hidden pointer or private index, but the documented way to name, persist, and reference keys.
Note: Key IDs are only required when usingMCUBOOT_BUILTIN_KEY
; other build options don’t touch them. -
Raw pointers are brittle & unsafe becuase of hard coded flash addresses,
- Build fragility: Hard-coded flash addresses break as soon as you relink.
- Expanded attack surface: Fault-injection or pointer replay attacks can redirect you to any memory.
- Relocation headaches: Juggling build-time offsets is far worse than using a stable numeric handle.
-
Key IDs give you safe indirection
The PSA layer enforces your key’s access policy on every handle. Flip bits in the ID? You get a clean “invalid handle” error, not arbitrary memory corruption. -
What this PR actually does
Right now,MCUBOOT_BUILTIN_KEY
still hard-bakes your image-ID → key-ID 1:1, so changing it forces a full re-sign of all blobs. This PR pulls that magic number into one override point—no more hunting through every header.
That lays the groundwork for a future enhancement - manifest based approach: your image carries only a symbolic tag, and at boot we’ll read a small signed table to resolve “secure-slot-A → key ID 42.” Future key rotations or remaps will then be a one-line table update—not a re-signing marathon.
Hope that clarifies why we picked PSA key IDs over raw pointers—and how we’re already stepping toward a more flexible manifest approach down the road.
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.
Hey, thanks for the feedback! So,
1. **PSA key IDs aren’t “internal” magic** They’re part of the _public_ Crypto API—`psa_key_id_t` is exactly the handle you pass into every `psa_*` call. It’s not a hidden pointer or private index, but the documented way to name, persist, and reference keys. _Note:_ Key IDs are only required when using `MCUBOOT_BUILTIN_KEY`; other build options don’t touch them.
Yes they are, because as far as I can tell when you use psa_import_key
, unless you set required key id as an attribute, that function is able to allocate any value as key id.
2. **Raw pointers are brittle & unsafe** becuase of hard coded flash addresses, * **Build fragility:** Hard-coded flash addresses break as soon as you relink. * **Expanded attack surface:** Fault-injection or pointer replay attacks can redirect you to _any_ memory. * **Relocation headaches:** Juggling build-time offsets is far worse than using a stable numeric handle.
I, literally, was not proposing using pointers.
3. **Key IDs give you safe indirection** The PSA layer enforces your key’s access policy on every handle. Flip bits in the ID? You get a clean “invalid handle” error, not arbitrary memory corruption.
That does not mean I have to have them glued to an image, I can do the same stuff internally. The same thing will happen with "bit flip" there.
4. **What this PR actually does** Right now, `MCUBOOT_BUILTIN_KEY` still hard-bakes your image-ID → key-ID 1:1, so changing it forces a full re-sign of all blobs. This PR pulls that magic number into one override point—no more hunting through every header. That lays the groundwork for a future enhancement - manifest based approach: your image carries only a symbolic tag, and at boot we’ll read a small signed table to resolve “secure-slot-A → key ID 42.” Future key rotations or remaps will then be a one-line table update—not a re-signing marathon.
Right. When a key leaks we have to re-issue applications anyway.
But you still do not have to glue API specific ID to the image. PSA is not going to be the only supported backend ever and we should avoid directly exposing api specific stuff to signing or encryption.
Hope that clarifies why we picked PSA key IDs over raw pointers—and how we’re already stepping toward a more flexible manifest approach down the road.
Oh...
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.
Yes they are, because as far as I can tell when you use psa_import_key, unless you set required key id as an attribute, that function is able to allocate any value as key id.
Yes, but we can set deterministic repeatable handle/id using psa_set_key_id so that shouldn't be any issue. right?
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.
But you still do not have to glue API specific ID to the image. PSA is not going to be the only supported backend ever and we should avoid directly exposing api specific stuff to signing or encryption.
But there is already 1:1 image to key id mapping in current code ([https://github.com/mcu-tools/mcuboot/blob/9f209100336455c21f9f127f5bb9f2b5a394bb4f/boot/bootutil/include/bootutil/crypto/ecdsa.h#L397]). So wouldn't this PR add flexibility?
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.
But you still do not have to glue API specific ID to the image. PSA is not going to be the only supported backend ever and we should avoid directly exposing api specific stuff to signing or encryption.
But there is already 1:1 image to key id mapping in current code ([https://github.com/mcu-tools/mcuboot/blob/9f209100336455c21f9f127f5bb9f2b5a394bb4f/boot/bootutil/include/bootutil/crypto/ecdsa.h#L397]). So wouldn't this PR add flexibility?
Yes, and I want out of that not only for ecdsa.
You do not need to have the key_id glued to the image to have flexibility.
You only need pair <image, purpose> to request a psa_key_id_t id (or other type, depending on backend) to get a key id that is usable for your backend.
You can have as many keys as you want. At this point we are not discussing policies applied by backend regarding key rejection or revocation, or how to inform backend on key status.
Lets assume the function is identified as boot map_me_the_key(uint32_t image_id, int purpose, key_type *t)
, purpose is optional I just try to make the thing work with encryption, that returns true and available key id if there is, another, one available.
Now you have something like:
verified = 0;
while (map_me_the_key(image, SIGNATURE. &key)) {
ret = psa_verify_me_the_signature(key, stuff_to_verify);
if (ret == PSA_SUCCESS) { // weakest point probably
verified++; // this can not be simple int, this is just an example
}
}
// here check verified and do stuff
Assuming that map_me_the_key
gives back another key available for a given image, you are able to make this work:
- if you expect only one key to verify image
- if you expect at least 1 of N keys to verify an image
- if you expect M of N keys to verify the image, as long as you check that
verified
afterwards against your expected 1 or M.
map_me_the_key
can have different lists of key ids for every image, for some images or just have one and ignore image number at all.
You do not need to specify PSA key id in image.
Drawback and advantage is that every key is run against the image, so if you are mapping 1000 keys, that may take time, but also you do not know which key worked. You can also just exit loop the first time you are satisfied with number of verification attempts.
(you still need to second check the verification "counter" later...).
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 literally want a function that will check whether given key ID may be used with given image; you may as well just have a function that will give you a key id for a given image.
Hey, Thank you for comments. Let me try to address the concerns:
Policy-driven multi-sig security
You implement your desired policy in For a real-world example, see TF-M’s thin PSA core at line 226 of
Why scanning all slots isn’t enough? Imagine you’ve got two images (secure vs non-secure) and three provisioned keys. If you naively loop over every key slot and accept the first one that “fits,” an attacker could re-sign the secure image with a key that’s valid in PSA but wasn’t meant for that image. You’d happily verify it and boot “secure” code with the wrong signer. By embedding the exact psa_key_id_t in the TLV, we force the bootloader to call psa_verify_hash(handle, …) on that handle only. No guessing, no stray-slot wins—only the intended key can pass. I hope that clarifies many things, but review is highly appreciated so please let me know if any comments. |
Before I answer anything of that: you know that I will have to maintain that changes after you? That is default assumption in the review, because I can not assume that, after you get the things in, you will still going to be interested in doing any other changes.
I get the refactor, just avoid doing it with addition of a feature in a single commit. I can diff changes commit by commit, but not when you move-and-modify the stuff. If that can not be done in separate PR, at least do that in separate commit.
I am not implementing this, because it is not used by MCUboot anywhere. That is requirement of TF-M not MCUboot.
I agree with the costly argument. Note, though, that to avoid it it would be better to use key fingerprinting, as it can be totally abstract (and can be done in any way) and reveal nothing of how keys are mapped to backend. Additionally same fingerprinting may be used with any backend.
The only thing that saves you here is
Look up.
|
This PR adds support for signing and verifying images with multiple signatures of the same type (e.g., multiple EC256 signatures), enhancing flexibility in secure boot scenarios. It also introduces Key ID TLV parsing to enable the bootloader to select the correct key from a set of built-in keys.
Motivation
Previously, MCUboot only allowed a single signature per image per signature type. This limited use cases where multiple stakeholders need to sign the same image or when fallback keys are required.
This PR removes that limitation by allowing multiple signatures of the same type.
Use Cases
Changes Included
1. bootutil: Parse key ID TLV for built-in keys
MCUBOOT_BUILTIN_KEY
is enabled.2. imgtool: Add support for multiple signatures and key ID TLVs
--key
arguments.3. bootutil: Add support for verifying multiple same-type signatures
MCUBOOT_BUILTIN_KEY
orMCUBOOT_HW_KEY
is enabled, the key ID is used to select the appropriate key for verification.Notes