mlswg / mls-implementations

Coordination of implementation and interop specific details
110 stars 14 forks source link

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

Open RonPeters opened 10 months ago

RonPeters commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

You guys please fix this. Is this the reason I'm not receiving texts messages

OtaK commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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:

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

RonPeters commented 9 months ago

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

Aurvandill commented 8 months ago

@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