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

boot/bootutil: give the hash of the received key to boot_retrieve_public_key_hash #1869

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Olstyle
Copy link

@Olstyle Olstyle commented Nov 30, 2023

MCUBOOT_HW_KEY already allows to put your own signature key management into the bootloader, independent of mcuboot. In the moment boot_retrieve_public_key_hash is limited to exactly one key hash, since it is only called once and there is no way for the called function to determine whether it will be returning an accepted hash.
Adding the hash itself to the call enables the project specific code to look for a fitting hash instead of just returning one. This way two things can be accomplished:

  1. Multiple HW keys can be used since now iterating through a list of hashes is feasible
  2. Automatic revocation (see Support for revoking/invalidating keys #221 ) can be added since the function now knows which key was used and might decide to invalidate other keys based on that fact

The key revocation scheme I plan to use based on this patch is based on a sorted list of key hashes. Normally the first entry is expected to be used for signing. If an entry deeper down the list is used, this indicates that the private keys up to that entry have been compromised and they should be invalidated.
Invalidation itself is HW specfic. For example on an STM32 it is possible to overwrite already written flash with zeros, which can be used to delete a non zero validity flag.

This enables a key revoking/invalidation scheme as mentioned in mcu-tools#221

Signed-off-by: Oliver Mueller <olstyle@gmx.de>
@Olstyle Olstyle changed the title give the hash of the received key to boot_retrieve_public_key_hash boot/bootutil: give the hash of the received key to boot_retrieve_public_key_hash Nov 30, 2023
@d3zd3z d3zd3z self-requested a review December 14, 2023 15:29
Copy link
Member

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

I do think this is slightly better. But, I have a little bit of a concern that we are effectively breaking an API, and this would result in a major version change. It is hard to tell who out there uses this function, so we will need to make sure there are clear directions on how to update this. So, at a minimum, there need to be release notes on this.

But, I really wonder if this whole interface is actually right. Presumably a system with a hardware key wouldn't have the key available (perhaps the public key?), and it seems really weird to me how the key returned from this function is then written to a global. If anything, this makes this code untestable in the simulator (this is not introduced by this change, but already there). I'm not sure how to do this better.

@Olstyle
Copy link
Author

Olstyle commented Jan 12, 2024

To be honest this is just the smallest change I could think of. If we accept this is a breaking change anyway, the interface could be changed to directly return a bool about whether the key hash is accepted.

From what I understood from the original MCUBOOT_HW_KEY setup, the argument for only using hashes was that those might be saved in OTP memory or something similarly expensive. That's not actually what I am doing. I just needed an interface which lets me look at some info about the key (or the key itself) and make a decision instead of giving a key directly.

Maybe bootutil_find_key needs to be rethought for a third time. Originally I expected it to return a (pointer to a) key, but neither version of it does this.

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.

None yet

2 participants