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?