Closed
Bug 1393329
Opened 8 years ago
Closed 8 years ago
Curve25519 point validation strategy does not seem compliant with TLS 1.3
Categories
(NSS :: Libraries, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
3.33
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.
Assignee | ||
Comment 1•8 years ago
|
||
> 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•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
Assignee: nobody → franziskuskiefer
Updated•8 years ago
|
Keywords: sec-moderate
Reporter | ||
Updated•8 years ago
|
Summary: Curve25519 point validation strategy does not seem compliant with RFC-7748 → Curve25519 point validation strategy does not seem compliant with TLS 1.3
Assignee | ||
Comment 4•8 years ago
|
||
Let's check for all zero result.
https://hg.mozilla.org/projects/nss/rev/4bf658832d8960a070dd0ce2baea49a4276461a1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.33
Updated•8 years ago
|
Group: crypto-core-security → core-security-release
Assignee | ||
Comment 5•8 years ago
|
||
Follow-up fixing a memory leak resulting from this change.
https://hg.mozilla.org/projects/nss/rev/0ba9a65e3f156526e8eb9c121a4c89c6afbea7bc
Comment 6•7 years ago
|
||
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+
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•