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

Message Protection test vector for cipher suite 5 is incorrect #176

Open
RonPeters opened this issue Nov 6, 2023 · 13 comments
Open

Message Protection test vector for cipher suite 5 is incorrect #176

RonPeters opened this issue Nov 6, 2023 · 13 comments

Comments

@RonPeters
Copy link

The spec for HPKE states that the private key size (Nsk) for DHKEM(P-521, HKDF-SHA512) is 66 bytes:
https://datatracker.ietf.org/doc/html/rfc9180#name-key-encapsulation-mechanism

The private key 'signature_priv' for cipher suite 5 in message-protection.json is 65 bytes. This is causing me an error when using that private key. If I prepend a zero byte to make it 66 bytes, I can read the key successfully. So I believe the test vector is incorrect.

Errata report:

In test vector file 'message-protection.json'

"signature_priv": "0beee7d4e812a02538473225803aca13f8dea26718f188f2e1de8357a0037df621230cf4593885f282b858ac301e54c0643f5d07b6e85f237baa13b574000cd821",

should be

"signature_priv": "000beee7d4e812a02538473225803aca13f8dea26718f188f2e1de8357a0037df621230cf4593885f282b858ac301e54c0643f5d07b6e85f237baa13b574000cd821",

@raphaelrobert
Copy link
Member

HPKE doesn't have signatures, signature_priv is the private signature key used to sign public messages for that test vector. I think you are possibly confusing two different things here.

@RonPeters
Copy link
Author

I understand, but the MLS spec seems to defer to the HPKE spec when it comes to key sizes. It was the only documentation I could find that would explain why the 65-byte key would not work. So I'm pretty sure my argument that the serialized key should be 66 bytes is correct.

@ghost

This comment was marked as spam.

@OtaK
Copy link

OtaK commented Nov 16, 2023

HPKE doesn't have signatures, signature_priv is the private signature key used to sign public messages for that test vector. I think you are possibly confusing two different things here.

I had the issue too. P521 secret keys must be 66-bytes long. In the event you have 65-bytes you need to prepend a single null byte.

It means that the implementation that produced the test vectors is misbehaving in their implementation.

@Aurvandill
Copy link
Contributor

FIPS 186-3 defines a P521 Private Key as an integer with 521-bit length.
I don't realy see a problem with it being 65 bytes in the testvector because adding a zero byte doesn't change the integer value itself.
But i guess you can make a pull request and add the missing zero padding to the keys.

with best regards
Aurvandill

@OtaK
Copy link

OtaK commented Nov 17, 2023

So, I've looked around and basically, the answer is that some implementations encode p521 secret keys in a minimum-byte representation (go's crypto/ecdsa is an example of this), meaning that you'll get roughly:

  • 66 bytes 49% of the time
  • 65 bytes 50% of the time
  • 64 bytes 0.2% of the time
  • 63 bytes or below with increasingly decreasing percentages

BUT the HPKE spec mandates that when DHKEM(P-521, HKDF-SHA512) is used, the secret keys must be 66-bytes long.
So I don't know what's the best practice there, is it up to the implementations (for example, RustCrypto's p521) to pre-pad accordingly with 0x00 bytes or is it worth mentioning somewhere that MLS implementations must ensure the prepadding themselves for the sake of consistency with more or less well-behaved other implementations.

@RonPeters
Copy link
Author

RonPeters commented Nov 18, 2023

Thanks for that research @OtaK! In your thread in rust-hpke, I noticed someone said "the NIST/FIPS test vectors for ECDSA are also like that ... with leading zeros removed."

With that in mind, I think the prudent thing is to document in a non-normative section in the MLS spec that some crypto implementations may remove leading zeros, therefore you should check key lengths and prepend zeros if necessary.

@RonPeters
Copy link
Author

Also, these variations in key sizes should be represented in the crypto-basics test vector so we can learn to expect them from the beginning as opposed to deeper into testing

@franziskuskiefer
Copy link
Collaborator

As Raphael said, HPKE is pretty irrelevant here. So are the private keys here as they are not relevant for interoperability.
The relevant part is the signature public key. And they are correctly encoded as uncompressed points. Because of the prime used in P521 the length of the encoding may vary.

@RonPeters
Copy link
Author

The real point I was trying to make is that when developing validation suites for the test vectors, it is not obvious what to do if your encryption library expects all keys to be the correct size. When your library panics because of an incorrect key size, how is one supposed to know they need to prepend zeros if there is no documentation for it? I just took a wild guess that turned out to be correct.

@franziskuskiefer
Copy link
Collaborator

I'd recommend to use a different crypto library in this case. This is something the library should take care of imo.

@RonPeters
Copy link
Author

@franziskuskiefer Easier said than done, I'm afraid. Especially in .NET

@Aurvandill
Copy link
Contributor

@franziskuskiefer i think adding a comment to the test description is a good idea and not that much of a hassle.
A bit more verbosity about the keys being minimum-byte representation should avoid questions of this kind in the future as well :)
@RonPeters do you maybe wanna file a PR for an updated test description?

with best regards
Aurvandill

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

No branches or pull requests

5 participants