Closed Bug 1330612 Opened 4 years ago Closed 4 years ago

X25519 is the default curve for ECDHE in NSS

Categories

(NSS :: Libraries, defect)

3.28
defect
Not set
normal

Tracking

(firefox52 fixed, firefox53 fixed)

RESOLVED FIXED
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: hkario, Assigned: mt)

References

Details

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161213225121

Steps to reproduce:

When a client that does not advertise supported curves through an extension connects to NSS 3.28.0 server while advertising both ECDHE and DHE ciphersuites, NSS will pick ECDHE key exchange with X25519 curve.


Actual results:

The client doesn't know how to handle X25519 curve ID so the connection fails.


Expected results:

While this is not incorrect behaviour according to RFC 4492 (it is quite
explicit that if client doesn't provide this extension, server can pick any
curve it wants), I'm afraid that this will cause interoperability problems.

Older NSS (3.27.0) will default to P-256 curve in absence of supported_groups extension, vast majority of Internet serves support only P-256 curve, irrespective of advertised curves...

See also: http://openssl.6102.n7.nabble.com/X25519-is-the-default-curve-for-ECDHE-in-OpenSSL-1-1-0-td68400.html (or https://mta.openssl.org/pipermail/openssl-dev/2016-September/008488.html )
I agree with Hubert that this is probably a misfeature in NSS and that it would be safer to pick P-256.
Could this be fixed for 3.29 ?

How much work will it require?

Who should be the assignee?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → 3.29
When we (BoringSSL) added X25519, we opted simply to act as if we had no curves in common if the client sent no supported_groups, aligning with TLS 1.3. (So we'd end up picking a different cipher suite.) We've been running with that logic for around a year now without issues.

If you all prefer implement a P-256 default instead, I would suggest it work something like:

  if version < TLS1_3 && client_supported_groups == nil {
    client_supported_groups = {P256}
  }

  // Do whatever negotiation logic you normally do.

Then you don't have to worry about cases where P-256 was disabled or introduce distinct notions of default vs. preferred curve.
(In reply to David Benjamin from comment #3)
> When we (BoringSSL) added X25519, we opted simply to act as if we had no
> curves in common if the client sent no supported_groups, aligning with TLS
> 1.3. (So we'd end up picking a different cipher suite.) We've been running
> with that logic for around a year now without issues.
> 
> If you all prefer implement a P-256 default instead, I would suggest it work
> something like:
> 
>   if version < TLS1_3 && client_supported_groups == nil {
>     client_supported_groups = {P256}
>   }

actually, TLS1.3 hello without supported_groups extension should result in missing_extension alert from server, so execution shouldn't get this far for TLS1.3 case in the first place...

also, client_supported_groups may not be null but still not include any ECDHE curves - we have FFDHE sharing that extension with ECDHE
This looks like it should be pretty easy to fix. There are a number of possible options, but I think the cleanest is, if the client doesn't send supported_groups, act as if it had sent some "safe" supported groups extension (perhaps P-256 and P-384?). The change would then be to just add a call to ssl3_UpdateSupportedGroups() in ssl3_HandleClientHelloPart2(), perhaps somewhere around here: http://searchfox.org/nss/source/lib/ssl/ssl3con.c#8987, after we decide we're not doing resumption.

That would have the right behavior without changing any of the basic group negotiation logic, and would be easy to change if our view of "safe" changes.

Note that TLS 1.3 enforces that you provide supported_groups (http://searchfox.org/nss/source/lib/ssl/tls13con.c#1099) so no corresponding change is needed there.


The most difficult part about this patch is writing an appropriate test for it. We currently have no way to cause the client not to emit this extension (unlike Bogo, where this is easy). The easiest strategy is to write a filter that removes CH.supported_groups and then write a SKE filter to capture the group. Not very hard but a bit of a PITA. I don't see a problem getting this into 3.29, though I also don't have a proposed assignee.
> actually, TLS1.3 hello without supported_groups extension should result in missing_extension alert from server, so execution shouldn't get this far for TLS1.3 case in the first place...

It depends on the exact negotiation algorithm and whether pure (EC)DHE-less PSK is omitted. A client which only supports (EC)DHE-less PSK with an external PSK may skip sending supported_groups. Of course, this doesn't make sense for a server which only implements certificate-based auth and ticket-based PSK, so, yes, such a server could do this. Or such a server may still, for simplicity, route the "missing extension" case into the "no common groups" case since there's no actual value in separating them.

> also, client_supported_groups may not be null but still not include any ECDHE curves - we have FFDHE sharing that extension with ECDHE

I think a client which sends supported_groups but offers only FFDHE groups should not get a P-256 default. The concern is compatibility with legacy clients that omit the extension altogether, so there's no need to add this complexity in. Trying to subcategorize in an extensible field (so some values are unknown) is a perfect recipe for introducing unknown curve/group intolerance into the ecosystem.
(In reply to Eric Rescorla (:ekr) from comment #5)
> The most difficult part about this patch is writing an appropriate test for
> it. We currently have no way to cause the client not to emit this extension
> (unlike Bogo, where this is easy).

I already have test cases for that in tlsfuzzer, that's how I found it.
(In reply to Hubert Kario from comment #7)
> (In reply to Eric Rescorla (:ekr) from comment #5)
> > The most difficult part about this patch is writing an appropriate test for
> > it. We currently have no way to cause the client not to emit this extension
> > (unlike Bogo, where this is easy).
> 
> I already have test cases for that in tlsfuzzer, that's how I found it.

Sure, but we need a test in the stuff that we run as part of the NSS test suite, which these days means gtests.
(In reply to Eric Rescorla (:ekr) from comment #8)
> (In reply to Hubert Kario from comment #7)
> > (In reply to Eric Rescorla (:ekr) from comment #5)
> > > The most difficult part about this patch is writing an appropriate test for
> > > it. We currently have no way to cause the client not to emit this extension
> > > (unlike Bogo, where this is easy).
> > 
> > I already have test cases for that in tlsfuzzer, that's how I found it.
> 
> Sure, but we need a test in the stuff that we run as part of the NSS test
> suite, which these days means gtests.

sqlite has 3 independent test suites... :) but I understand not wanting to integrate tlsfuzzer for just one test case
I actually would be very interested in seeing a tlsfuzzer integration for taskcluster CI, but I think that this should be tested in the gtests.
I can take this.  It's pretty straightforward.
Assignee: nobody → martin.thomson
https://hg.mozilla.org/projects/nss/rev/1db0933a06a3b7e032cfaea0be485556b97a0d56
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Verified as fixed in 13025:32eaddf94c5d
Blocks: 1337580
You need to log in before you can comment on or make changes to this bug.