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

crypto/mlkem: Encapsulate has wrong order of secret and ciphertext #70950

Open
tmthrgd opened this issue Dec 21, 2024 · 5 comments
Open

crypto/mlkem: Encapsulate has wrong order of secret and ciphertext #70950

tmthrgd opened this issue Dec 21, 2024 · 5 comments
Labels
Critical A critical problem that affects the availability or correctness of production systems built using Go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@tmthrgd
Copy link
Contributor

tmthrgd commented Dec 21, 2024

Go version

go1.24rc1

Output of go env in your module/workspace:

N/A

What did you do?

Used the new crypto/mlkem package from go1.24 (#70122).

What did you see happen?

Noticed that the signature for Encapsulate is Encapsulate() (ciphertext, sharedKey []byte) with ciphertext first and sharedKey second.

What did you expect to see?

As per FIPS 203, the order should be "shared secret key" first and "ciphertext" second:

Algorithm 20 ML-KEM.Encaps(ek)
Uses the encapsulation key to generate a shared secret key and an associated ciphertext.
Checked input: encapsulation key ek ∈ 𝔹384𝑘+32 .
Output: shared secret key 𝐾 ∈ 𝔹32 .
Output: ciphertext 𝑐 ∈ 𝔹32(𝑑𝑢𝑘+𝑑𝑣).
1: 𝑚 ←$− 𝔹32					▷ 𝑚 is 32 random bytes (see Section 3.3)
2: if 𝑚 == NULL then
3: 	return ⊥				▷ return an error indication if random bit generation failed
4: end if
5: (𝐾, 𝑐) ← ML-KEM.Encaps_internal(ek, 𝑚)	▷ run internal encapsulation algorithm
6: return (𝐾, 𝑐)

Because both return types are []byte (so you don't get the type system to help you tell them apart), this seems like a potentially dangerous footgun and unfortunate divergence from the published spec. I also imagine any potential crypto.KEM API is going to want secret first. I think the order should be swapped to match FIPS 203 before go1.24 is released.

For reference, RFC 9180 (HPKE) also specifies it's KEMs to return secret first.

Note: It seems like the original pre-NIST Kyber spec (from round 3) did have the ciphertext first, see #70122 (comment).

@gabyhelp
Copy link

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@mateusz834
Copy link
Member

Marking as a release blocker, see #70122 (comment).

CC @FiloSottile

@dr2chase dr2chase added the Critical A critical problem that affects the availability or correctness of production systems built using Go label Dec 21, 2024
@dr2chase
Copy link
Contributor

@rolandshoemaker

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/638376 mentions this issue: crypto/mlkem: swap order of return values of Encapsulate

@FiloSottile
Copy link
Contributor

I tried and I can't remember why I wrote it this way. It's been like that since the beginning, but I was implementing directly from the ipd, which has the other order. FiloSottile/mlkem768@3294fee

I hate to make such a silent behavior change between release candidates, but I agree that the long term confusion would be significant. The good news is that neither GitHub nor pkg.go.dev know about uses of the package yet.

https://github.com/search?q=%22%5C%22crypto%2Fmlkem%5C%22%22&type=code
https://pkg.go.dev/crypto/mlkem@master?tab=importedby

Also, mixing them up should not be a security risk, beyond stopping the establishment from succeeding: the "secret key" is a hash of a fresh set of random bytes, so it's not exposing anything sensitive, not even the ephemeral PKE message; the ciphertext meanwhile includes enough information to recover the message, so it must be as unpredictable as m, and the attacker can't force a connection to succeed.

Given that, I'm in favor of making the switch.

@cherrymui cherrymui added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical A critical problem that affects the availability or correctness of production systems built using Go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants