SSL Handshake Error using node >= 8.6.0

[princjef]

• Version: 8.6.0 and 8.7.0 (works in 8.5.0)
• Platform: Windows 10 64 bit, MacOS High Sierra, all docker image variants
• Subsystem: https/crypto

Starting with node 8.6.0, whenever I try to make an https call to an nginx server that I'm running (be it through a library like request or using https.request() directly), I always get the following error:

Error: 101057795:error:14077410:SSL routines:SSL23_GET_SERVER_HELLO:sslv3 alert handshake failure:openssl\ssl\s23_clnt.c:772

I'll admit up front that I'm not the most knowledgeable when it comes to security, but I'm able to talk to my server if I use node 8.5.0 (as well as several previous versions of node 6 and 8), all major web browsers and curl, so it seems like there's something special about more recent versions of node that's causing something strange to happen.

After some digging, I found that Node 8.5.0, Chrome, Firefox, and curl all communicate with my server using the ECDHE-RSA-AES256-GCM-SHA384 cipher, which is present on both the default list of TLS Ciphers and the list of ciphers my server accepts (see below).

However, when communicating using Node 8.6.0/8.7.0 and changing the accepted ciphers on the server to ALL to get the request to complete, I see that cipher ends up being AES256-GCM-SHA384 instead. As far as I can tell (though I don't know much about SSL/TLS ciphers), the Node client and the nginx server are unable to agree on an ECDH curve, so ECDH is dropped from the cipher, which causes an impermissible cipher to be used (and subsequently rejected).

With that in mind, I'm also able to get the request to succeed with Node 8.7.0 if I drop the secp384r1 ECDH curve requirement from my nginx config, which drops it back to the default (auto, I believe). When the ECDH curve requirement is removed, Node 8.7.0 completes the request using the same ECDHE-RSA-AES256-GCM-SHA384 cipher as everything else. So ultimately, this seems to be a change in the ECDH curves Node allows/supports.

Having poked around the commits that went into the 8.6.0 release, I believe this PR made the change that caused this scenario to break. What I'm less sure of is why that change broke it and whether support for the secp384r1 ECDH curve was removed intentionally or not. I'm not necessarily opposed to removing the secp384r1 configuration from my server, but I figure that it's probably best if I can follow the recommendations of https://cipherli.st/ if possible.

Thanks for your time and effort to look at this issue. I'd be more than happy to provide more information and/or test things out on my end. I would also love to help with a fix for this, but I'm afraid I'm not knowledgeable enough about security/crypto to really be able to understand what's going on at a code level.

---

For reference, here's the relevant information about the nginx server (most configuration is lifted from https://cipherli.st/):

• Version: openresty 1.11.2.5 (running from the openresty:1.11.2.5-xenial docker image)
• OpenSSL Version: 1.0.2

Configuration

http {
    # ...
    server {
        # ...
        ssl_protocols TLSv1 TLSv1.1 TLSv1.2;
        ssl_prefer_server_ciphers on; 
        ssl_ciphers ECDHE-RSA-AES256-GCM-SHA512:DHE-RSA-AES256-GCM-SHA512:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA:DHE-RSA-AES256-SHA;
        ssl_ecdh_curve secp384r1;
        ssl_session_tickets off;
        ssl_stapling on;
        ssl_stapling_verify on;
        ssl_session_cache shared:SSL:10m;
        ssl_session_timeout 10m;
    }
}

When

[bnoordhuis]

#15206 is indeed the PR that is responsible for this change. I assume you don't pass an explicit ecdhCurve option to https.request()?

It sounds like #15206 slipped in an accidental bug fix because ecdhCurve is supposed to default to tls.DEFAULT_ECDH_CURVE, which is prime256v1 at the time of writing. secp384r1 shouldn't have worked without a manual override. Regrettably, we don't seem to have test coverage for that.

@nodejs/security What do we want to do?

[princjef]

No I don't pass an explicit curve but I did verify that passing it in works (thanks for pointing that out!):

const req = require('https').request(
    {
        ...require('url').parse('https://my-domain.com'),
        method: 'GET',
        ecdhCurve: 'secp384r1' // 'auto' also works
    },
    res => res.on('data', d => process.stdout.write(d))
);
req.on('error', e => console.error(e));
req.end();

So I guess the question now is the whether this is considered a bugfix (and therefore I should just start passing the ecdhCurve option) or if the variant without the explicit option should continue to work in 8.6.0+.

As a side note: I actually wasn't aware the ecdhCurve could be passed in to https.request(). From the line:

The following additional options from tls.connect() are also accepted when using a custom Agent: pfx, key, passphrase, cert, ca, ciphers, rejectUnauthorized, secureProtocol, servername

I thought that I would only be able to pass the tls.connect() options listed there and, even for those, only when I had a custom agent. Is there a list in the docs somewhere specifying which options to tls.connect() can be used with https.request() with the default agent? Maybe I'm just misreading the docs and all options except the ones listed can be specified with the default agent. The options to https.request() do appear to get passed into tls.connect() (with some modifications) here. I'd be happy to submit an update to the docs to clarify the options if this is the case.

[bnoordhuis]

As a side note: I actually wasn't aware the ecdhCurve could be passed in to https.request().

It's not completely fool-proof at the moment. If you make two requests to the same host using the same agent but with different ecdhCurve options, one or the other might be ignored.

If you'd like to work on that, the logic is in https.Agent#getName() in lib/https.js - it should include the option in the computed key.

Ideally, every option to tls.connect() would be picked up by https.request() automatically but I haven't been able to come up with something that is both robust and efficient.

[rogaps]

Hi all,
Nginx sets ssl_ecdh_curve to auto if it is not specified. How about we do the same? We set tls.DEFAULT_ECDH_CURVE to auto.

[princjef]

@bnoordhuis Ah I see now. Yeah I'll work something up and submit a PR. Looking at the options that are already there, I'm thinking it's reasonable to add support for the rest of the tls.createSecureContext() options since they all have serialization-friendly data types and should be reasonably short (with the possible exception of crl, though I consider that on par with pfx, key, cert and ca, which are already included). Let me know if that sounds unreasonable.

As for supporting all options, I agree that supporting everything through something like getName() would be difficult to do in an efficient/robust way. Everything I can think of would require the signature of https.request() to change such that the user essentially indicates the desired mapping on their end.

[princjef]

@rogaps that sounds reasonable to me. I think the main question would be if there are any security concerns involved with being more permissive in the selection of the curve (I don't actually know if there are, just playing devil's advocate).

[bnoordhuis]

Looking at the options that are already there, I'm thinking it's reasonable to add support for the rest of the tls.createSecureContext() options since they all have serialization-friendly data types and should be reasonably short

@princjef Yes, sounds good.

Nginx sets ssl_ecdh_curve to auto if it is not specified. How about we do the same?

@rogaps That's an option for v9.x, but not v8.x as that branch has been released, is stable and about to go LTS. Related issue: #1495

[Hativ]

The default value of ecdhCurve should be 'auto' or at least stronger curves should be added to the default value.

This breaks the WebSocket client from ws if the https server used for WebSocket.Server uses stronger ecdhCurve's than the default value. See websockets/ws#1227.

[bnoordhuis]

I'll close this out because it looks like we're not going to roll back the change and as a question it's been answered.

@Hativ3 Can you open a new issue or pull request?

[ghost]

For anyone hitting this issue using https with the request lib before #16853 goes out, I was able to resolve by rolling back our node version to 8.5.x (FROM node:8.5 for our Docker image)

[bnoordhuis]

Reopening, more discussion is probably warranted. See nodejs/help#968 where a previously working combination of options no longer works.

@nodejs/crypto Thoughts on making 'auto' the default in node 8? That would restore the previous behavior.

[ghost]

@bnoordhuis #16853 contains that change, and looks to have been fully approved.

[bmonty]

Setting tls.DEFAULT_ECDH_CURVE to auto is a work around for code that I write. Is there a workaround for code that I didn't write (i.e. npm)? I'm using node v8.9.1 on OSX installed using homebrew.

[dandigangi]

I see merges but unsure if this is resolved. Do I still need to roll back to an earlier version of Node 8.5.x for now? Is the merges on a 9.x.x release?

I'm on the same setup as @bmonty

[bnoordhuis]

Is the merges on a 9.x.x release?

@dandigangi You mean #16853? No, that one is semver-major, it's going into node 10.

[dandigangi]

Got it. I should have figured that.

I don't know what happened exactly but I ended up go forward to 9.3.0 today. Not sure if that solved it but it is working again.

[raeesaroj]

adding
ssl_ecdh_curve auto;
in nginx server block fixes my problem

[michaelnisi]

I just ran into this issue with Node v8.12.0. Here’s curl (7.54.0):

% curl -Iv  https://contravariance.rocks/feed.rss                                                             !10025
*   Trying 165.227.254.111...
* TCP_NODELAY set
* Connected to contravariance.rocks (165.227.254.111) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/cert.pem
  CApath: none
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Client hello (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES128-GCM-SHA256
* ALPN, server accepted to use http/1.1
* Server certificate:
*  subject: CN=contravariance.rocks
*  start date: Feb  6 05:50:44 2019 GMT
*  expire date: May  7 05:50:44 2019 GMT
*  subjectAltName: host "contravariance.rocks" matched cert's "contravariance.rocks"
*  issuer: C=US; O=Let's Encrypt; CN=Let's Encrypt Authority X3
*  SSL certificate verify ok.
> HEAD /feed.rss HTTP/1.1
> Host: contravariance.rocks
> User-Agent: curl/7.54.0
> Accept: */*
>
< HTTP/1.1 200 OK
HTTP/1.1 200 OK
< Server: nginx/1.14.0 (Ubuntu)
Server: nginx/1.14.0 (Ubuntu)
< Date: Tue, 26 Feb 2019 13:58:52 GMT
Date: Tue, 26 Feb 2019 13:58:52 GMT
< Content-Type: application/rss+xml
Content-Type: application/rss+xml
< Content-Length: 26430
Content-Length: 26430
< Last-Modified: Thu, 21 Feb 2019 20:07:54 GMT
Last-Modified: Thu, 21 Feb 2019 20:07:54 GMT
< Connection: keep-alive
Connection: keep-alive
< ETag: "5c6f051a-673e"
ETag: "5c6f051a-673e"
< Strict-Transport-Security: max-age=31536000
Strict-Transport-Security: max-age=31536000
< X-Frame-Options: SAMEORIGIN
X-Frame-Options: SAMEORIGIN
< X-Content-Type-Options: nosniff
X-Content-Type-Options: nosniff
< Accept-Ranges: bytes
Accept-Ranges: bytes

<
* Connection #0 to host contravariance.rocks left intact

What is the preferred work around? Globally, like @fabricecolas suggests, or passing request options?

[sam-github]

It's up to your preference. Global makes the behaviour the same across node versions (but if anyone is using specific request options the global default will get ignored).