Closed Bug 1393329 Opened 3 years ago Closed 3 years ago

Curve25519 point validation strategy does not seem compliant with TLS 1.3

Categories

(NSS :: Libraries, defect)

3.33
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benjamin.beurdouche, Assigned: franziskus)

Details

(Keywords: sec-moderate)

Attachments

(1 file)

Curve25519 point validation strategy in NSS doesn't seem to be RFC-7748 compliant.
The current code for ECDH_Derive, calls EC_ValidatePublicKey and it seems that the public point is checked for zero and a set of other specific values (presumably derived from small orders ?).

RFC-7748 and TLS 1.3 require point validation to happen after the scalar-multiplication of the public point with the secret value as the X25519 multiplication will return a zero value if it operates on an input corresponding to a point with small order.

My understanding of the code is that the TLS code is calling the PKCS11 interface to derive the shared secret via NSC_DeriveKey with CKM_ECDH1_DERIVE which directly calls ECDH_Derive. If this is correct, this means that the point validation might not be exhaustive, hence incorrect. 
In this situation, an attacker might forge a public point which is not in the invalid point list and produce a zero value as the result of the scalarmult for example defeating the forward secrecy property expected for TLS.

I might be wrong here but better be certain, can someone please have a look and ping me. Thanks ;) B.
> RFC-7748 and TLS 1.3 require point validation to happen after the scalar-multiplication

Nope it doesn't. It suggests checking for 0 after multiplication (MAY). As the RFC states as well, most implementations don't. I don't think we have to either.
There are 2 points to this:
i) The 0 check is only necessary when you want to enforce contributory behaviour, which is not necessary for TLS (the only use-case of x25519 currently).
ii) If we'd want contributory behaviour, we would have to make sure that the output is not 0. The ValidatePublicKey function checks all known-bad points. I'm not sure if it's possible there being other bad points (I'd have to check further).

So I'm not opposed to a 0-check after the multiplication but I'm not really concerned that i's an issue.
7748 does not require the 0 check, but TLS 1.3 does.

https://tlswg.github.io/tls13-spec/draft-ietf-tls-tls13.html#elliptic-curve-diffie-hellman

"For X25519 and X448, implementations SHOULD use the approach specified in [RFC7748] to calculate the Diffie-Hellman shared secret. Implementations MUST check whether the computed Diffie-Hellman shared secret is the all-zero value and abort if so, as described in Section 6 of [RFC7748]. If implementers use an alternative implementation of these elliptic curves, they SHOULD perform the additional checks specified in Section 7 of [RFC7748]."


WRT point #1, I'm not sure why you say it's not necessary for TLS. It's true that TLS 1.3 has contributory behavior, as does TLS 1.2 with EMS, but not everyone uses EMS.
https://nss-review.dev.mozaws.net/D420
Assignee: nobody → franziskuskiefer
Summary: Curve25519 point validation strategy does not seem compliant with RFC-7748 → Curve25519 point validation strategy does not seem compliant with TLS 1.3
Let's check for all zero result.
https://hg.mozilla.org/projects/nss/rev/4bf658832d8960a070dd0ce2baea49a4276461a1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.33
Group: crypto-core-security → core-security-release
Follow-up fixing a memory leak resulting from this change.
https://hg.mozilla.org/projects/nss/rev/0ba9a65e3f156526e8eb9c121a4c89c6afbea7bc
Comment on attachment 8905580 [details]
Bug 1393329 - x25519 check follow-up, r=ttaubert

Tim Taubert [:ttaubert] has approved the revision.

https://phabricator.services.mozilla.com/D37
Attachment #8905580 - Flags: review+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.