Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Add ECDHE support to TLS #4315

Closed
ksdlck opened this issue Nov 26, 2012 · 16 comments
Closed

Add ECDHE support to TLS #4315

ksdlck opened this issue Nov 26, 2012 · 16 comments

Comments

@ksdlck
Copy link

ksdlck commented Nov 26, 2012

Ephemeral ECDH suites require one of {SSL_CTX,SSL}_set_tmp_ecdh{,_callback} to be used. The attached patch is a minimal example that works for me, but I'd suggest expanding this into a function and/or option of tls.Server that takes the EC curve names which should be used. Ideally, the function and option would have the same signature, which is a list of string EC curve names. The NID can be obtained from the string curve name via OBJ_sn2nid(curve_name), like so:

int nid = OBJ_sn2nid("secp521r1");
if (NID_undef == nid)
  handle_error();

Though generating EC keys is extremely fast compared to generating DH params, I'd still suggest only generating one ephemeral key for each curve, when the list of curves is assigned. The callback set using SSL_CTX_set_tmp_ecdh_callback(SSL_CTX *ctx, int export, int bit_length) should iterate through this list until it finds a key with the requested bit length, otherwise returning NULL. The bit length can be retrieved as follows:

EC_GROUP *ecgrp = EC_KEY_get0_group(eckey);
if (NULL == ecgrp)
  handle_error();
int bit_len = EC_GROUP_get_degree(ecgrp);

Obviously, more complex but similarly performant key schedules are possible, such as generating a new key for a curve after a time interval, or some number of uses, but the above represents a robust solution.

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index a2ecfdb..6a69364 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -220,6 +220,11 @@ Handle<Value> SecureContext::Init(const Arguments& args) {

   sc->ctx_ = SSL_CTX_new(method);

+  EC_KEY *ecdh = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
+  if (NULL == ecdh)
+    return ThrowException(Exception::Error(String::New("Unable to generate temp ECDH key")));
+  SSL_CTX_set_tmp_ecdh(sc->ctx_, ecdh);
+
   // SSL session cache configuration
   SSL_CTX_set_session_cache_mode(sc->ctx_,
                                  SSL_SESS_CACHE_SERVER |
@bnoordhuis
Copy link
Member

It's not something we'll be working on in the near future but we take patches. Though they'd need to be a little more full-fledged and include documentation and test cases. :-)

@ksdlck
Copy link
Author

ksdlck commented Nov 27, 2012

According to http://nodejs.org/api/tls.html, the TLS implementation cipher list

Defaults to ECDHE-RSA-AES128-SHA256:AES128-GCM-SHA256:RC4:HIGH:!MD5:!aNULL:!EDH

Given that the highest preference cipher suite in the list won't even be enabled by OpenSSL because you haven't given it a temporary ECDH key, it might be worth it to sneak this in for the time being.

@bnoordhuis
Copy link
Member

Sure, but my earlier points about tests and documentation still stand. Can you submit a PR?

@ksdlck
Copy link
Author

ksdlck commented Nov 28, 2012

#4317

@ksdlck
Copy link
Author

ksdlck commented Feb 1, 2013

Any status updates on whether you are likely to merge this?

@bnoordhuis
Copy link
Member

Not without the aforementioned tests and documentation.

@ksdlck
Copy link
Author

ksdlck commented Feb 3, 2013

Please feel free to choose between the following:
#4709
#4710

@ksdlck
Copy link
Author

ksdlck commented Feb 7, 2013

All joking aside, as someone who regularly has to evaluate the relative merits of softwares from a security standpoint, tools that don't provide what I need are unfortunate, and tools that mislead me into thinking they provide what I need when they don't are unacceptable.

Kindly consider at least updating the documentation with the more neutral patch so users are not mislead into thinking node supports things it doesn't.

@yourcelf
Copy link

Agree with @ksdlck - I spent a long time trying to debug my SSL stack, wondering why ECDHE-RSA-AES128-SHA256 wasn't being used, until I finally found this bug. To make matters worse, tls.getCiphers() lists ECDHE ciphers as supported ciphers even though they aren't usable.

The docs should be clear about the non-support of these ciphers, or the implementation should support them. (Of course I'd prefer the latter, given the increasing visibility of PFS in light of Prism etc). It's unfortunate if the tone of snarky pull requests may have put the devs off; this is important to be clear about.

@bnoordhuis
Copy link
Member

I've added a note in c1bf89d. If someone wants to pick up where the OP left off, the v0.12 merge window is still open.

Of course I'd prefer the latter, given the increasing visibility of PFS in light of Prism etc

ECDHE-RSA doesn't really give you perfect forward secrecy because you re-use the key. If you want that kind of security, you probably want ECDHE-ECDSA which uses ephemeral keys.

@ksdlck
Copy link
Author

ksdlck commented Jun 28, 2013

Actually, ECDHE-RSA gives you the same forward secrecy as ECDHE-ECDSA. The
first item in the cipher suite is used to establish the session secret,
which is what gives you the forward secrecy and will be used to encrypt the
remainder of the session. ECDHE-RSA and ECDHE-ECDSA are equally ephemeral
in this regard.

On Thu, Jun 27, 2013 at 1:41 AM, Ben Noordhuis notifications@git.luolix.topwrote:

I've added a note in c1bf89dhttps://github.com/joyent/node/commit/c1bf89d.
If someone wants to pick up where the OP left off, the v0.12 merge window
is still open.

Of course I'd prefer the latter, given the increasing visibility of PFS in
light of Prism etc

ECDHE-RSA doesn't really give you perfect forward secrecy because you
re-use the key. If you want that kind of security, you probably want
ECDHE-ECDSA which uses ephemeral keys.


Reply to this email directly or view it on GitHubhttps://github.com//issues/4315#issuecomment-20088033
.

@2fours
Copy link

2fours commented Sep 11, 2013

In light of recent NSA revelations when can we expect to see ECDHE support in node's tls implementation?

@baconmania
Copy link

Is there any specific reason why tls.getCiphers() lists ECDHE ciphers as supported, even though they're not? It's incredibly misleading.

@bnoordhuis
Copy link
Member

Actually, ECDHE-RSA gives you the same forward secrecy as ECDHE-ECDSA. The first item in the cipher suite is used to establish the session secret,

Sorry, I seemed to have missed your original reply. That's not how I read OpenSSL's implementation of ECDHE-RSA: the RSA key is ephemeral only in the sense that it's generated when the SSL context is created, it's re-used for all connections after that unless you explicitly enable ephemeral RSA keys (which comes with a pretty hefty performance penalty, probably why it's disabled by default.)

Is there any specific reason why tls.getCiphers() lists ECDHE ciphers as supported, even though they're not?

The list comes straight from OpenSSL, node.js passes it on pretty much verbatim.

@ksdlck
Copy link
Author

ksdlck commented Sep 25, 2013

Both ECDHE-RSA and ECDHE-ECDSA use ephemeral keys to establish the session secret, but not for authentication; in the former case, an RSA key is used for signing, in the latter case an EC key is used with the elliptic curve variant of DSA for signing. In neither of the cases is the signing key ephemeral, nor is there an ephemeral RSA key.

What you say about an ephemeral key (RSA, EC, DH, or otherwise) being reused within an SSL context is typically true, but misses the point. Just because a key is ephemeral does not mean it's single-use—that's up to you, and, even for EC keys which are pretty light to generate compared to RSA or DSA, imposes the aforementioned significant performance penalty if you generate a new one for each connection. If you notice in my patch for EC TLS, keys for the desired curves are generated only once and reused until the application is killed. This usually provides an acceptable level of forward secrecy because each process gets its own key(s) and they only hang around for the lifetime of the process. Naturally, if one were able to access the process' memory directly, there would be plenty of ways to achieve data compromise beyond grabbing the ephemeral key(s).

@senditu
Copy link

senditu commented Feb 17, 2014

I would also like to see proper ECDHE support in node.js.

The documentation currently has the following:

ECDH (Elliptic Curve Diffie-Hellman) ciphers are not yet supported.

But a quick connect with "openssl s_client" shows that a TLSv1.2 connection has been established with ECDH-ECDSA-AES128-GCM-SHA256 (when using "ECDHE:AES128:SHA256:GCM:RC4:HIGH:!MD5:!aNULL:!ED" for ciphers), which is incredibly misleading. ECDH is being implemented correctly, but ECDHE is not.

It took me a little while before figuring out what was going on.


For anyone who reads this: node.js has ECDHE support built into the unreleased versions on git. Version 0.10.21 does not support ECDHE properly, but the version I just grabbed off of the master branch (version 11.12-pre) does.

@indutny indutny closed this as completed Jun 25, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants