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.

Sign up for GitHub

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

Jump to bottom

kem: Scheme.DeriveKeyPair length check too strict #486

Open
emersion opened this issue Mar 13, 2024 · 6 comments · May be fixed by #487
Open

kem: Scheme.DeriveKeyPair length check too strict #486

emersion opened this issue Mar 13, 2024 · 6 comments · May be fixed by #487
Labels
needs investigation

Comments

@emersion
Copy link

emersion commented Mar 13, 2024 •
edited
Loading

Scheme.DeriveKeyPair panics if the length of the seed is different from Scheme.SeedSize. However, this is too strict: there are use-cases for passing a seed larger than the hash size, for instance MLS passes a 64-byte value to KEM_X448_HKDF_SHA512 (which has SeedSize of 56).

RFC 9180 section 7.1.3 says:

For a given KEM, the ikm parameter given to DeriveKeyPair() SHOULD have length at least Nsk, and SHOULD have at least Nsk bytes of entropy.

Would you accept a patch which changes the len(seed) == x.SeedSize() check to len(seed) >= x.SeedSize()?
@emersion
Copy link
Author

emersion commented Mar 13, 2024 •
edited
Loading

Actually, a >= check wouldn't be enough for MLS: the official test vectors pass a 64-byte seed to KEM_P521_HKDF_SHA512 (which has SeedSize of 66). We'd need to drop the checks entirely.

emersion added a commit to emersion/circl that referenced this issue Mar 13, 2024
@emersion
hpke: relax seed size check in DeriveKeyPair
7b15fc1 
RFC 9180 section 7.1.3 says:

> For a given KEM, the ikm parameter given to DeriveKeyPair()
> SHOULD have length at least Nsk, and SHOULD have at least Nsk
> bytes of entropy.

Thus, it is not a requirement for HPKE to pass a seed with a fixed
size. Protocols such as MLS rely on this.

Closes: cloudflare#486
@emersion emersion linked a pull request Mar 13, 2024 that will close this issue
Open
@armfazh
Copy link
Contributor

armfazh commented Mar 13, 2024

In Section 7.4, MLS, DeriveKeyPair is called with node_secret, which is the output of DeriveSecret

node_secret[n] = DeriveSecret(path_secret[n], "node")
node_priv[n], node_pub[n] = KEM.DeriveKeyPair(node_secret[n])

Looking at the definition of DeriveSecret in Section 8, MLS, it outputs KDF.Nh bytes.

DeriveSecret(Secret, Label) =
    ExpandWithLabel(Secret, Label, "", KDF.Nh)

There is a mismatch between KDF.Nh and KEM.Nsk only in the following MLS suites:

MLS ID MLS Suite KDF.Nh KEM.Nsk
0x04 MLS_256_DHKEMX448_AES256GCM_SHA512_Ed448 64 56
0x05 MLS_256_DHKEMP521_AES256GCM_SHA512_P521 64 66
0x06 MLS_256_DHKEMX448_CHACHA20POLY1305_SHA512_Ed448 64 56

@emersion , I recommend you to reach the MLS authors to seek guidance, this could be a typo in the spec.

@emersion
Copy link
Author

Good idea. I've sent an email to the mailing list: https://mailarchive.ietf.org/arch/msg/mls/JdrJvjGnVjNHX1xE4kTcsmq_PRc/

@bwesterb
Copy link
Member

The kem.Scheme interface isn't meant to match up precisely with HPKE. Its DeriveKeyPair function is meant to match up exactly with the natural key derivation. HPKE's derivation can be built on top of that, eg. as in X-Wing. I think we might want to add a separate interface for HPKE key derivation.

@bwesterb
Copy link
Member

Also the

For a given KEM, the ikm parameter given to DeriveKeyPair() SHOULD have length at least Nsk, and SHOULD have at least Nsk bytes of entropy.

requirement is problematic for PQ KEMs, which have large secret keys, and is ignored. This requirement is very much written thinking only of elliptic curves.

@emersion
Copy link
Author

That makes sense to me. Do we want a full new interface in the hpke package, or do we want to just add a DeriveKeyPair method to hpke.KEM?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
needs investigation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

hpke: relax seed size check in DeriveKeyPair emersion/circl
3 participants
@bwesterb @emersion @armfazh