1393329 - Curve25519 point validation strategy does not seem compliant with TLS 1.3

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Benjamin Beurdouche (do not use this account)]

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.

Comment 1

[Franziskus Kiefer [:franziskus]]

> 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.

Comment 2

[Eric Rescorla (:ekr)]

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.

Comment 3

[Franziskus Kiefer [:franziskus]]

https://nss-review.dev.mozaws.net/D420

Comment 4

[Franziskus Kiefer [:franziskus]]

Let's check for all zero result.
https://hg.mozilla.org/projects/nss/rev/4bf658832d8960a070dd0ce2baea49a4276461a1

Comment 5

[Franziskus Kiefer [:franziskus]]

Follow-up fixing a memory leak resulting from this change.
https://hg.mozilla.org/projects/nss/rev/0ba9a65e3f156526e8eb9c121a4c89c6afbea7bc

Comment 6

[Phabricator Automation]

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