-
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?
Changes from all commits
a554a08
083b3ef
df7225f
309bf22
10b2b13
5370225
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,12 @@ | |
|
||
/* mcuboot_config.h is needed for MCUBOOT_HW_KEY to work */ | ||
#include "mcuboot_config/mcuboot_config.h" | ||
#ifdef MCUBOOT_IMAGE_MULTI_SIG_SUPPORT | ||
#include <stdbool.h> | ||
#endif /* MCUBOOT_IMAGE_MULTI_SIG_SUPPORT */ | ||
#ifdef MCUBOOT_BUILTIN_KEY | ||
#include "bootutil/fault_injection_hardening.h" | ||
#endif /* MCUBOOT_BUILTIN_KEY */ | ||
|
||
#ifdef __cplusplus | ||
extern "C" { | ||
|
@@ -39,6 +45,17 @@ struct bootutil_key { | |
}; | ||
|
||
extern const struct bootutil_key bootutil_keys[]; | ||
#ifdef MCUBOOT_BUILTIN_KEY | ||
/** | ||
* Verify that the specified key ID is valid for authenticating the given image. | ||
* | ||
* @param[in] image_index Index of the image to be verified. | ||
* @param[in] key_id Identifier of the key to be verified against the image. | ||
* | ||
* @return 0 if the key ID is valid for the image; nonzero on failure. | ||
*/ | ||
fih_ret boot_verify_key_id_for_image(uint8_t image_index, uint32_t key_id); | ||
#endif /* MCUBOOT_BUILTIN_KEY */ | ||
#else | ||
struct bootutil_key { | ||
uint8_t *key; | ||
|
@@ -51,17 +68,41 @@ extern struct bootutil_key bootutil_keys[]; | |
* Retrieve the hash of the corresponding public key for image authentication. | ||
* | ||
* @param[in] image_index Index of the image to be authenticated. | ||
* @param[in] key_index Index of the key to be used. | ||
* @param[out] public_key_hash Buffer to store the key-hash in. | ||
* @param[in,out] key_hash_size As input the size of the buffer. As output | ||
* the actual key-hash length. | ||
* | ||
* @return 0 on success; nonzero on failure. | ||
*/ | ||
int boot_retrieve_public_key_hash(uint8_t image_index, | ||
uint8_t key_index, | ||
uint8_t *public_key_hash, | ||
size_t *key_hash_size); | ||
|
||
#endif /* !MCUBOOT_HW_KEY */ | ||
|
||
#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 */ | ||
Comment on lines
+85
to
+104
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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? |
||
|
||
extern const int bootutil_key_cnt; | ||
|
||
#ifdef __cplusplus | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,151 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* Copyright (c) 2017-2019 Linaro LTD | ||
* Copyright (c) 2016-2019 JUUL Labs | ||
* Copyright (c) 2019-2025 Arm Limited | ||
* Copyright (c) 2025 Nordic Semiconductor ASA | ||
* | ||
* Original license: | ||
* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
#include <stdint.h> | ||
|
||
#include "bootutil/crypto/sha.h" | ||
#include "bootutil/fault_injection_hardening.h" | ||
#include "bootutil/image.h" | ||
#include "bootutil/sign_key.h" | ||
#include "bootutil_priv.h" | ||
#include "mcuboot_config/mcuboot_config.h" | ||
#include "bootutil/bootutil_log.h" | ||
|
||
BOOT_LOG_MODULE_DECLARE(mcuboot); | ||
|
||
#if defined(MCUBOOT_SIGN_RSA) || \ | ||
defined(MCUBOOT_SIGN_EC256) || \ | ||
defined(MCUBOOT_SIGN_EC384) || \ | ||
defined(MCUBOOT_SIGN_EC) || \ | ||
defined(MCUBOOT_SIGN_ED25519) | ||
#define IMAGE_VALIDATION_EXPECTS_KEY | ||
#else | ||
/* no signing, sha256 digest only */ | ||
#endif | ||
|
||
#ifdef IMAGE_VALIDATION_EXPECTS_KEY | ||
#ifdef MCUBOOT_IMAGE_MULTI_SIG_SUPPORT | ||
#define NUM_OF_KEYS MCUBOOT_ROTPK_MAX_KEYS_PER_IMAGE | ||
#else | ||
#define NUM_OF_KEYS 1 | ||
#endif /* MCUBOOT_IMAGE_MULTI_SIG_SUPPORT */ | ||
|
||
#if defined(MCUBOOT_BUILTIN_KEY) | ||
int bootutil_find_key(uint8_t image_index, uint8_t *key_id_buf, uint8_t key_id_buf_len) | ||
{ | ||
uint32_t key_id; | ||
FIH_DECLARE(fih_rc, FIH_FAILURE); | ||
|
||
BOOT_LOG_DBG("bootutil_find_key: image_index %d", image_index); | ||
|
||
/* Key id is passed */ | ||
assert(key_id_buf_len == sizeof(uint32_t)); | ||
memcpy(&key_id, key_id_buf, sizeof(key_id)); | ||
|
||
/* Check if key id is associated with the image */ | ||
FIH_CALL(boot_verify_key_id_for_image, fih_rc, image_index, key_id); | ||
if (FIH_EQ(fih_rc, FIH_SUCCESS)) { | ||
return (int32_t)key_id; | ||
} | ||
|
||
return -1; | ||
} | ||
|
||
#elif defined(MCUBOOT_HW_KEY) | ||
extern unsigned int pub_key_len; | ||
int bootutil_find_key(uint8_t image_index, uint8_t *key, uint16_t key_len) | ||
{ | ||
bootutil_sha_context sha_ctx; | ||
uint8_t hash[IMAGE_HASH_SIZE]; | ||
uint8_t key_hash[IMAGE_HASH_SIZE]; | ||
size_t key_hash_size = sizeof(key_hash); | ||
int rc; | ||
uint8_t key_index; | ||
FIH_DECLARE(fih_rc, FIH_FAILURE); | ||
|
||
BOOT_LOG_DBG("bootutil_find_key: image_index %d", image_index); | ||
|
||
bootutil_sha_init(&sha_ctx); | ||
bootutil_sha_update(&sha_ctx, key, key_len); | ||
bootutil_sha_finish(&sha_ctx, hash); | ||
bootutil_sha_drop(&sha_ctx); | ||
|
||
for(key_index = 0; key_index < NUM_OF_KEYS; key_index++) { | ||
rc = boot_retrieve_public_key_hash(image_index, key_index, key_hash, &key_hash_size); | ||
if (rc) { | ||
return -1; | ||
} | ||
|
||
/* Adding hardening to avoid this potential attack: | ||
* - Image is signed with an arbitrary key and the corresponding public | ||
* key is added as a TLV field. | ||
Comment on lines
+105
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Looks like there’s a mix-up here:
Hope that clears it up—let me know if you have any other questions! |
||
if (FIH_EQ(fih_rc, FIH_SUCCESS)) { | ||
BOOT_LOG_INF("Key %d hash found for image %d", key_index, image_index); | ||
bootutil_keys[0].key = key; | ||
pub_key_len = key_len; | ||
return 0; | ||
} | ||
} | ||
BOOT_LOG_ERR("Key hash NOT found for image %d!", image_index); | ||
|
||
return -1; | ||
} | ||
|
||
#else /* !defined MCUBOOT_BUILTIN_KEY && !defined MCUBOOT_HW_KEY */ | ||
int bootutil_find_key(uint8_t image_index, uint8_t *keyhash, uint8_t keyhash_len) | ||
{ | ||
bootutil_sha_context sha_ctx; | ||
int i; | ||
uint8_t hash[IMAGE_HASH_SIZE]; | ||
const struct bootutil_key *key; | ||
(void)image_index; | ||
|
||
BOOT_LOG_DBG("bootutil_find_key"); | ||
|
||
if (keyhash_len > IMAGE_HASH_SIZE) { | ||
return -1; | ||
} | ||
|
||
for (i = 0; i < bootutil_key_cnt; i++) { | ||
key = &bootutil_keys[i]; | ||
bootutil_sha_init(&sha_ctx); | ||
bootutil_sha_update(&sha_ctx, key->key, *key->len); | ||
bootutil_sha_finish(&sha_ctx, hash); | ||
bootutil_sha_drop(&sha_ctx); | ||
if (!memcmp(hash, keyhash, keyhash_len)) { | ||
return i; | ||
} | ||
} | ||
return -1; | ||
} | ||
#endif | ||
#endif /* IMAGE_VALIDATION_EXPECTS_KEY */ |
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 using
MCUBOOT_BUILTIN_KEY
; other build options don’t touch them.Raw pointers are brittle & unsafe becuase of hard coded flash addresses,
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.
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.I, literally, was not proposing using pointers.
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.
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.
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, 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 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.
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:
Assuming that
map_me_the_key
gives back another key available for a given image, you are able to make this work: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.