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

[signing] Support sphincs+ PreHashedSha256 mode #25871

Closed
wants to merge 2 commits into from

Conversation

cfrantz
Copy link
Contributor

@cfrantz cfrantz commented Jan 14, 2025

  1. Redefine the config field of key_{ecdsa,sphicns_plus} rules to be
    a dictionary of parameters.
  2. Allow setting parameters in the key nickname field of keyset rules
    (e.g. {"//key:label": "foo:domain=Pure"}).
  3. When generating pre-signing artifacts, use only the public key to
    prevent opentitantool from immediately signing the artifact.
  4. In the _local_sign and _hsmtool_sign helper functions, construct
    appropriate command line arguments considering the signing mode and
    byte-reversal settings.
  5. Fix the image manifest update command to use the normal SHA256 byte
    ordering. Add a command line argument to reverse the hash to deal
    with existing implementations with a reversed hash.
  6. Allow the spx sign|verify commands to optionally reverse their
    input.
  7. Support verification of owner code using the SPHINCS+
    PreHashedSha256 mode. Since the OpenTitan HMAC peripheral
    produces hashes in "little-endian" order, reverse the bytes
    before verification so that the correct byte ordering is used.
  8. Add a test for PreHashedSha256 mode using the fake app DEV key.

@cfrantz cfrantz requested review from a team as code owners January 14, 2025 22:48
@cfrantz cfrantz requested review from jwnrt, a team, jon-flatley and vbendeb and removed request for a team, jwnrt and jon-flatley January 14, 2025 22:48
1. Redefine the `config` field of `key_{ecdsa,sphicns_plus}` rules to be
   a dictionary of parameters.
2. Allow setting parameters in the key nickname field of `keyset` rules
   (e.g. `{"//key:label": "foo:domain=Pure"}`).
3. When generating pre-signing artifacts, use only the public key to
   prevent opentitantool from immediately signing the artifact.
4. In the `_local_sign` and `_hsmtool_sign` helper functions, construct
   appropriate command line arguments considering the signing mode and
   byte-reversal settings.
5. Fix the `image manifest update` command to use the normal SHA256 byte
   ordering.  Add a command line argument to reverse the hash to deal
   with existing implementations with a reversed hash.
6. Allow the `spx sign|verify` commands to optionally reverse their
   input.

Signed-off-by: Chris Frantz <[email protected]>
@@ -406,6 +406,7 @@ static rom_error_t rom_ext_spx_verify(
expected_root.data[i] ^= shares[i];
}

dbg_printf("spx_verify: %C\r\n", key_alg);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this debug print intended to be left in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I forgot to remove it. Fixed.

config = "Sha2128s",
config = {"domain": "Pure"},
Copy link
Contributor

Choose a reason for hiding this comment

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

How does the implementation now know, for example, to use SHA2 parameters instead of SHAKE? I've lost track of where this information comes from now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the host, the key files store the algorithm information. In the device, the key "algorithm" configuration encodes both the algorithm and domain.

$ cat ./sw/device/silicon_creator/lib/ownership/keys/fake/app_prod_spx.pub.pem
-----BEGIN RAW:SPHINCS+_SHA2_128s_simple PUBLIC KEY-----
J4pnFBeX/Xc3VvOIqL4XxdM9EXvi4rlcCFwfQuru5MY=
-----END RAW:SPHINCS+_SHA2_128s_simple PUBLIC KEY-----
enum ownership_key_alg {
  ...
  /** Key algorithm Hybrid P256 & SPX+ Prehashed SHA256: `H+S2` */
  kOwnershipKeyAlgHybridSpxPrehash = 0x32532b48,
}

FYI, the prior value of the "config" attribute in the bazel rules was unused - it was never used in a conditional, nor passed to any programs (like opentitantool or hsmtool). A set of key/value pairs indicating additional information that may be needed by the signer seemed like a better use of that field.

1. Support verification of owner code using the SPHINCS+
   `PreHashedSha256` mode.  Since the OpenTitan HMAC peripheral
   produces hashes in "little-endian" order, reverse the bytes
   before verification so that the correct byte ordering is used.
2. Add a test for PreHashedSha256 mode using the fake app DEV key.

Signed-off-by: Chris Frantz <[email protected]>
@vbendeb
Copy link

vbendeb commented Jan 15, 2025

Nit:

Is supporting both digest byte orders necessary be able to keep building for ROM validation?

The command line argument name indicating 'correct' vs 'incorrect' byte order would be clearer.

@cfrantz
Copy link
Contributor Author

cfrantz commented Jan 15, 2025

Is supporting both digest byte orders necessary be able to keep building for ROM validation?

Yes - the ROM's implementation of pre-hashed validation erroneously uses the reversed byte-order Sha256 hash. In order to validate an SPX signed ROM_EXT using Prehashed mode, we need to reverse the byte order of the hash.

The command line argument name indicating 'correct' vs 'incorrect' byte order would be clearer.

I created #25870 to fix the byte ordering emitted and used by our tooling. Fixing that issue will deal with fixing and/or renaming flags.

@cfrantz
Copy link
Contributor Author

cfrantz commented Jan 15, 2025

Closed as these commits were submitted with #25872.

@cfrantz cfrantz closed this Jan 15, 2025
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.

3 participants