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

TLS Module: The default ecdhCurve, prime256v1 (aka NIST P-256) is not safe. #1495

Closed
mattcollier opened this issue Apr 21, 2015 · 36 comments
Closed
Labels
blocked PRs that are blocked by other issues or PRs. openssl Issues and PRs related to the OpenSSL dependency. security Issues and PRs related to security. tls Issues and PRs related to the tls subsystem.

Comments

@mattcollier
Copy link

This document states that the default curve for the ecdhCurve parameter is prime256v1.
https://iojs.org/api/tls.html#tls_tls_createserver_options_secureconnectionlistener

Appendix A of this document indicates that prime256v1 is also known as NIST P-256.
http://www.rfc-editor.org/rfc/rfc4492.txt

This site indicates that NIST P-256 is not secure.
http://safecurves.cr.yp.to/

I recommend that a safe alternative should be chosen as the default and unsafe curves should not be made available.

Also posted to nodejs: nodejs/node-v0.x-archive#18205

@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Apr 22, 2015
@Fishrock123
Copy link
Member

/cc @indutny

@silverwind
Copy link
Contributor

Good catch, now which curve should we choose?

My OpenSSL has these curves defined: https://gist.github.com/silverwind/9e9090833fce8acffa50

I'm having a hard time matching them to the curve names on http://safecurves.cr.yp.to/. Maybe we need to define a custom curve?

@silverwind
Copy link
Contributor

According to http://security.stackexchange.com/a/78624 client support is pretty limited besides prime256v1 and secp384r1, so that would heavily limit our options.

@mattcollier The linked SE answer raises some doubts about the 'not safe' statement.

@shigeki
Copy link
Contributor

shigeki commented Apr 23, 2015

curve25519 is one of the candidates we adopt in the future but it's not been implemented in openssl yet.
We also should care about compatibilities to especially browsers for the default curve. curve25519 is only supported on Safari and Chrome as in http://ianix.com/pub/ed25519-deployment.html while NIST P-256 is included in suiteB and widely deployed at present.
I think we had better to wait and see the ec implementation of openssl and deployment of safer curves in browsers for now.

@bnoordhuis
Copy link
Member

I think we should aim for secure by default. The curve is configurable with the ecdhCurve option or globally through the tls.DEFAULT_ECDH_CURVE property; people that want to revert to prime256v1, can. We just need to call it out in the release notes.

I don't know what a good replacement is, though. I don't think http://safecurves.cr.yp.to/ considers any of the existing curves in OpenSSL safe.

@silverwind
Copy link
Contributor

I think this is a non-issue, let me quote above SE answer:

What they mean is not that some curves are inherently unsafe, but that safe implementation of some curves is easier than for others

Use P-256 to minimize trouble. If you feel that your manhood is threatened by using a 256-bit curve where a 384-bit curve is available, then use P-384

If anything, this should probably be brought up to OpenSSL

@indutny
Copy link
Member

indutny commented Apr 23, 2015

Yeah, exactly. There are nothing we could do about it right now. Perhaps somewhere later when OpenSSL will implement these safe curves.

@bnoordhuis
Copy link
Member

What they mean is not that some curves are inherently unsafe, but that safe implementation of some curves is easier than for others

I don't think that's true. The FIPS mandated curves are suspect because there is reason to believe the NSA may have influenced the decision making process around them.

From https://www.schneier.com/blog/archives/2013/09/the_nsa_is_brea.html#c1675929:

I no longer trust the constants. I believe the NSA has manipulated them through their relationships with industry.

@indutny
Copy link
Member

indutny commented Apr 23, 2015

Let's be straight about it. There are three concerns that are mentioned on safecurves.cr.yp.to:

  • Rigidity - curve was generated using unexplained constant, this is quite questionable and doesn't raise anything except the suspicion
  • Ladder - no ladder scalar multiplication, this may lead to the side-channel attacks. But I believe that OpenSSL mitigates many of them
  • Completeness - requires checks for the points (I believe OpenSSL should be fine with this)
  • Indistinguishability - prevents attackers recognizing the EC curve point in the data stream. This one doesn't really matter for TLS, because it is easy to recognize the protocol anyway.

So only ladder and rigidity actually apply. This is not that great, but not totally bad either IMHO

@shigeki
Copy link
Contributor

shigeki commented Apr 23, 2015

Yes, it is suspicion. How about adding tls.DEFAULT_ECDH_CURVE to doc as 02a51cf ?
That's all we can do now.

@indutny
Copy link
Member

indutny commented Apr 23, 2015

Sounds good to me!

@shigeki
Copy link
Contributor

shigeki commented Apr 23, 2015

@indutny Thanks. Probably it is following to Ben's comment.
@silverwind How do you think it for closing this issue?

@mattcollier
Copy link
Author

I understand there is little to be done at this time because openssl does not currently provide a better alternative. Is there some mechanism in place to keep this issue from being lost and forgotten? Perhaps a "good enough for now" tag?

@shigeki
Copy link
Contributor

shigeki commented Apr 23, 2015

I think it is better to put a comment in the source as

diff --git a/lib/tls.js b/lib/tls.js
index 3ae7a8f..44ea058 100644
--- a/lib/tls.js
+++ b/lib/tls.js
@@ -33,6 +33,7 @@ exports.DEFAULT_CIPHERS = [
   '!CAMELLIA'
 ].join(':');

+// reconsider this when more safer curve is available.
 exports.DEFAULT_ECDH_CURVE = 'prime256v1';

 exports.getCiphers = function() {

@silverwind
Copy link
Contributor

I wonder if it's worth to explore client compatibilty for the NIST P-384 aka secp384r1 as a better default. It scores a tiny bit better on the linked table.

@indutny
Copy link
Member

indutny commented Apr 23, 2015

@silverwind how is it different?

@silverwind
Copy link
Contributor

@indutny ah, it's the same. I've just misread that table.

@silverwind
Copy link
Contributor

curve25519 has been suggested in the node issue. Still no OpenSSL support, however.

@Fishrock123 Fishrock123 added future openssl Issues and PRs related to the OpenSSL dependency. labels Apr 28, 2015
@ChALkeR
Copy link
Member

ChALkeR commented Nov 8, 2015

I linked to the relevant openssl issue, openssl/openssl#309.

@shigeki
Copy link
Contributor

shigeki commented Jan 28, 2016

Updates for curve25519:

@ChALkeR ChALkeR added the security Issues and PRs related to security. label Feb 28, 2016
@shigeki
Copy link
Contributor

shigeki commented Feb 29, 2016

X25519 is landed in openssl-1.1.0. openssl/openssl@7173624

@indutny
Copy link
Member

indutny commented Feb 29, 2016

Nice! What about browser support?

@shigeki
Copy link
Contributor

shigeki commented Feb 29, 2016

Chrome Canary (50) has already support it. https://code.google.com/p/chromium/issues/detail?id=579716
Microsoft is working on it but it is not yet public.
Firefox is not moving to support it for now. https://bugzilla.mozilla.org/show_bug.cgi?id=957105
Others (e.g. Safari) are still unknown.

@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

What's the current status on this one?

@silverwind
Copy link
Contributor

Isn't it only dependant on the OpenSSL update? Since when are clients our concern? 😉

@bnoordhuis
Copy link
Member

It's about the default curve to use. I don't think we'd change it to something that's not very usable in a practical setting yet. And yes, we'd need to upgrade openssl first.

@indutny
Copy link
Member

indutny commented Apr 4, 2016

And there is no OpenSSL 1.1.0 yet AFAIK

@MichalStaruch
Copy link

OpenSSL 1.1.0 released, things can start moving ;).

@jasnell
Copy link
Member

jasnell commented May 30, 2017

ping @nodejs/crypto ... what can we do on this one?

@shigeki
Copy link
Contributor

shigeki commented May 30, 2017

We are waiting this until upgrading the forthcoming OpenSSL-1.1.1 in Node-v9 or v10.
It will support x25519 for key exchange and Ed25519 in signing and we can be free from NIST-curve at that time.

@Hativ
Copy link
Contributor

Hativ commented Aug 5, 2017

What about supporting multiple curves and defining a preferred order? Like nginx: http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_ecdh_curve

tniessen pushed a commit that referenced this issue Nov 28, 2017
For best out-of-the-box compatibility there should not be one default
`ecdhCurve` for the tls client, OpenSSL should choose them
automatically.

See https://wiki.openssl.org/index.php/Manual:SSL_CTX_set1_curves(3)

PR-URL: #16853
Refs: #16196
Refs: #1495
Refs: #15206
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

Since OpenSSL 1.1.0 just landed, I think this can be looked at again.

@Hativ
Copy link
Contributor

Hativ commented Apr 10, 2018

I think the discussion of the default curve is obsolete since it's default value has changed to 'auto', see af78840.

@silverwind
Copy link
Contributor

Good find, I'll close this.

apapsch pushed a commit to apapsch/debops that referenced this issue Aug 6, 2020
There's some concern over the rigidity of prime256v1 because
it's a NIST curve. See here for the rabbit hole:
nodejs/node#1495

secp521r1 was erroneously copied.

Since Mozilla gives no explanation why they recommend certain
curves and it included prime256v1, the curves are replaced with
NCSC-NL recommendation.
drybjed pushed a commit to debops/debops that referenced this issue Aug 11, 2020
There's some concern over the rigidity of prime256v1 because
it's a NIST curve. See here for the rabbit hole:
nodejs/node#1495

secp521r1 was erroneously copied.

Since Mozilla gives no explanation why they recommend certain
curves and it included prime256v1, the curves are replaced with
NCSC-NL recommendation.

(cherry picked from commit 6792e91)
drybjed pushed a commit to debops/debops that referenced this issue Aug 11, 2020
There's some concern over the rigidity of prime256v1 because
it's a NIST curve. See here for the rabbit hole:
nodejs/node#1495

secp521r1 was erroneously copied.

Since Mozilla gives no explanation why they recommend certain
curves and it included prime256v1, the curves are replaced with
NCSC-NL recommendation.

(cherry picked from commit 6792e91)
drybjed pushed a commit to debops/debops that referenced this issue Aug 11, 2020
There's some concern over the rigidity of prime256v1 because
it's a NIST curve. See here for the rabbit hole:
nodejs/node#1495

secp521r1 was erroneously copied.

Since Mozilla gives no explanation why they recommend certain
curves and it included prime256v1, the curves are replaced with
NCSC-NL recommendation.

(cherry picked from commit 6792e91)
drybjed pushed a commit to debops/debops that referenced this issue Aug 11, 2020
There's some concern over the rigidity of prime256v1 because
it's a NIST curve. See here for the rabbit hole:
nodejs/node#1495

secp521r1 was erroneously copied.

Since Mozilla gives no explanation why they recommend certain
curves and it included prime256v1, the curves are replaced with
NCSC-NL recommendation.

(cherry picked from commit 6792e91)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. openssl Issues and PRs related to the OpenSSL dependency. security Issues and PRs related to security. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests