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

trusted boot: use XMSS signatures instead of a SHA256 hash #148

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

peterohanley
Copy link
Contributor

Describe your changes

firmware.c now either checks that the blob has a valid signature with a given public XMSS key or checks that it has a given SHA256 hash, configurable at build time. The signature verification specs are fairly simple, the signature generation specs are more complicated but I did not continue this effort after a discussion with @spernsteiner about the applicability of XMSS to this part of the code. CN cannot handle many parts of the XMSS library (especially atomics, flexible array members, and unions) and so the code is not currently runnable.

My time log is in xmss.md

Issue ticket number and link

#125

Checklist before requesting a review

  • I have performed a self-review of my code
  • My code matches the coding standards and I have ran the appropriate linters
  • I included documentation updates for my code
  • I extended the test suite and the tests run by the CI to cover my code
  • I assigned a Milestone to this PR
  • I assigned this PR to a Project
  • I assigned this PR appropriate Labels

@peterohanley peterohanley added the application software application software components label Jan 8, 2025
@peterohanley peterohanley added this to the Next PI meeting milestone Jan 8, 2025
@peterohanley peterohanley requested a review from podhrmic January 8, 2025 04:51
Copy link
Collaborator

@podhrmic podhrmic left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together @peterohanley !
What was the reason you included the xmss library directly rather than adding it as a submodule? If CN cannot process it anyway, a submodule would be cleaner (unless you had to make many changes to the library).
Minor comments above and the CI still needs fixing.

@@ -94,6 +339,11 @@ int reset(void *start_address,
&&
(memcmp(last_measure,expected_measure_,MEASURE_SIZE) != 0))
return HASH_MISMATCH;
#else
if (!xmss_verify_signature(&public_key, start_address, region_size, &xmss_signature)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like public_key and xmss_signature are initialized anywhere, so won't this basically always return false?

Comment on lines +268 to +269
static XmssPublicKey public_key;
static XmssSignatureBlob xmss_signature;
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a hardcoded key makes sense, but having a hardcoded signature doesn't (if you're going to hardcode the XMSS signature of a particular input, you could just as well hardcode its SHA256 hash and save yourself a lot of trouble). I think it would make more sense for xmss_signature to be a local inside main/reset, initialized from part of the input (e.g. last N bytes are the signature and everything before that is the code being signed).

@peterohanley
Copy link
Contributor Author

What was the reason you included the xmss library directly rather than adding it as a submodule? If CN cannot process it anyway, a submodule would be cleaner (unless you had to make many changes to the library).

In order to process it we need changes to many xmss headers. Specs could be in a local file that reaches into the submodule but many of the changes are intrusive (flexible array members especially)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application software application software components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants