configure: disable ssl2/ssl3 by default #8551

Closed
wants to merge 2 commits into
from

Projects

None yet
@indutny
Member
indutny commented Oct 15, 2014

POODLE

@tjfontaine tjfontaine added in progress and removed pr v0.10 labels Oct 15, 2014
@mathiasbynens

Secure defaults ftw. 👍

@jo
jo commented Oct 15, 2014

👍

@3rd-Eden

The biggest downside here is that disabling SSLv3 does impact users who still browse with IE6 (and probably older browsers as well) it might not be obvious to developers on why their SSL connections are failing because SSLv2 and SSLv3 would be disabled by default.

Instead I would just make it easier to disable these protocols when creating a HTTPS or TLS server. Because the current solution is just to damn horrible; https://gist.github.com/3rd-Eden/715522f6950044da45d8

@indutny
Member
indutny commented Oct 15, 2014

@3rd-Eden ok, I think I could replace this patch with a runtime addition of SSL_OP_NO_SSLv3 in lib/tls.js, but this will still leave the people wondering why their IE6 browsers can't connect to node.

Pros: much easier for them to re-enable, cons: will make people copy-paste stuff without actually understanding why it was disabled.

Rebuilding the node.js with enabled ssl3 should give much more time to think about the problem :)

What are your thoughts?

@Swaagie
Swaagie commented Oct 15, 2014

Documentation was also a bit lacking on this subject, added with #8553, on topic: I'm with @3rd-eden here, although I agree security issues are usually a grey area when it comes to force protecting your users at the cost of reduced compatibility

@bnoordhuis
Member

I think I could replace this patch with a runtime addition of SSL_OP_NO_SSLv3 in lib/tls.js, but this will still leave the people wondering why their IE6 browsers can't connect to node.

It's not just old browsers. Many TLS stacks have downgrade paths for when the initial handshake fails. That downgrade path can also kick in when the handshake times out because of a network glitch.

@indutny indutny added a commit that referenced this pull request Oct 15, 2014
@indutny indutny configure: disable ssl2/ssl3 by default
PR-URL: #8551
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
0ec78c9
@indutny indutny added a commit that referenced this pull request Oct 15, 2014
@indutny indutny doc: document why SSL2/SSL3 is disabled
PR-URL: #8551
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
d671291
@indutny
Member
indutny commented Oct 15, 2014

Landed in 0ec78c9, d671291 . @tjfontaine : please do a release

@Fishrock123
Member

Could we also do a 0.11 release? Enough people depend on it.

@indutny
Member
indutny commented Oct 15, 2014
@tjfontaine

I am not sure this is the right solution, it is uncharacteristic of node to remove a feature such that it requires a rebuild of node during a stable release.

We should make the change for 0.11 to disable SSLv3 by default, but doing this for the stable release is not ideal.

Instead we should provide some runtime opt-in solution for users who may require SSLv3 for legacy applications. We have done this in the past with breaking changes in stable releases by using an environment variable. For instance that's we did this for the UTF8 changes.

I would advocate here that instead we actually provide an environment variable or command line flag that is checked at process start to see if we have enabled SSLv3, otherwise prevent the usage of SSLv3 at runtime.

Before we can do a release of 0.10 or 0.11 we must also upgrade openssl to accommodate the other CVEs that were resolved. See https://www.openssl.org/news/secadv_20141015.txt

@3rd-Eden

I would just keep it enabled by default. If we start removing or disabling functionality by default which can be vulnerable to attacks we might as well also delete and disable the fs.read module as malicious paths can be supplied, causing it to read passwords and removing unwanted files. These decisions should be left to the developer. If want a more secure node you should educate them about the risks.

Edit: Also if you're going to disable SSLv3 you might as well start blocking ciphers like RC4 as well which are also vulnerable to attacks. And the list can go on and on and on.

@indutny
Member
indutny commented Oct 15, 2014

It is a bad analogy. Let's say that some feature of HTTP will become insecure, and you can either disable it by default, or leave it working for everyone. What would you prefer?

We should advise people to use secure protocols.

@3rd-Eden

Advising is something else than forcing.

@indutny
Member
indutny commented Oct 15, 2014

There is a way to opt out of it :) It is that the most preferred behaviour is the one that we do provide by default.

@tjfontaine

for reference, this is the version I think we will land in v0.10, and then in v0.12 we will leave sslv3 uncompiiled

@tjfontaine

bah forgot the link #8555

@3rd-Eden

@tjfontaine I don't think it makes sense to start throwing errors in a minor update (if your patch lands in 0.10, SSL servers will suddenly start throwing errors). This breaks backwards compatibility.

@indutny indutny closed this Oct 17, 2014
@tjfontaine tjfontaine removed the in progress label Oct 17, 2014
@indutny indutny deleted the indutny:fix/disable-ssl23 branch Oct 17, 2014
@dougwilson

SSLv3 is sometimes the only way to speak with sites that have TLS1.0 enabled, but are behind a broken F5 (https://www.imperialviolet.org/2013/10/07/f5update.html). Does this mean Node.js 0.10 will suddenly be unable to make outbound SSLv3 connections?

@indutny
Member
indutny commented Oct 17, 2014

@dougwilson yes. Would you prefer to be have insecure connections for modern clients, at the price of supporting old sites?

@dougwilson

They are not always old sites, but companies that don't upgrade their crap. We talk to various modern active sites where the OpenSSL in Node.js 0.10 cannot establish TLS1.0 connections because they are operating behind a broken F5 SSL terminator and so our code is making a special HTTPS agent just for those sites and asking for SSLv3 only, otherwise the connections will hang during SSL negotiation.

We have many teams that deploy these apps to places like Heroku that will suddenly just start pulling down the latest node.js 0.10.x binary and I'm sure there is bound to be a storm is all.

I was just trying to confirm that this is indeed about to happen so I'll know what to tell everyone is all.

@indutny
Member
indutny commented Oct 17, 2014

@dougwilson: are you using secureProtocol option for forcing SSL3? I guess we could enable SSL3 in such cases.

@dougwilson

That's exactly what we're doing. This is a simple example, to be on the same page :)

var agent = new https.Agent({secureProtocol: 'SSLv3_method'});
@indutny
Member
indutny commented Oct 17, 2014

@tjfontaine : thoughts on this? I think we could easily support this.

@bnoordhuis
Member

I'll go out on a limb here and say that maybe it's better if such clients break, if only to make people stop and reconsider if they really need to use SSLv3.

The F5 bug @dougwilson mentions has come up in the past (e.g. #5360) but there has been a fix for over a year now and every network administrator should have applied that by now.

Besides, downgrading the protocol is not a good way to work around buggy TLS terminators. They choke when the TLS handshake is too large but you can reduce it by limiting the list of ciphers in the ClientHello.

@dougwilson

and every network administrator should have applied that by now.

too bad this is untrue, as there is still a good list of domains i know of that still have these broken f5 ssl terminators.

but you can reduce it by limiting the list of ciphers in the ClientHello

What is the http agent code for this, please (node.js 0.10)? this is still going to be a huge suck because Heroku and the like just pull down the latest node.js binaries, even when nothing in the code changed and suddenly apps will be broken, including apps that don't even have owners because the employees left :(

@dougwilson

All I'm asking for is if there is production code deployed out there specifically trying to making client SSL requests with SSLv3 only, it would be nice if it didn't randomly just break when the language runtime does a patch release. Remember, the code is asking to do SSLv3, I'm not even asking for all code to keep being able to downgrade to SSLv3, just code that asks for it, like I posted above.

@indutny
Member
indutny commented Oct 17, 2014

So, my vote would be +1 for @dougwilson inquiry.

@dougwilson

Also, it's hard to actually find real information because of the dumb media hypes from these recent SSL "issues", but as far as I can tell, using OpenSSL as a client is not vulnerable, as the client needs to be vulnerable to a network-based downgrade attack and the server it's connecting to also needs to have SSLv3 on. If the client cannot be tricked into downgrading the protocol due to things like network timeouts, the attack does not work.

The two mitigations for this attack have been:

  1. Disable SSLv3 on servers. This will protect clients that can be tricked by a network attacker into downgraded to SSLv3, since the server won't even have SSLv3 to establish a connection with.
  2. If you are a client that does a special legacy dance (which is not built into OpenSSL, though it is built into NSS), then you can either stop that or use the SCSV extension (which is what OpenSSL 1.0.1j does) if the server is compatible.

Yes, it may be nice to kick Node.js users' HTTPS servers (for those who even terminate SSL using Node.js) off SSLv3, but it doesn't make sense to disable SSLv3 on outgoing TLS connections (especially if the code asked to make a SSLv3 connection, specifically) since the OpenSSL bundled in Node.js is not susceptible to the downgrade attack.

@indutny
Member
indutny commented Oct 17, 2014

@dougwilson I was mostly thinking about servers, when proposing disabling SSL3 completely in node.js.

@dougwilson

Also, just as my closing plea: I wouldn't even be asking this if it wasn't a real problem. I would like to see SSLv3 die jus as much as anyone else. I've been trying to get 3 different companies to upgrade their dumb F5's for over a year now and only one is going to maybe do something at the end of this month, while the other two have no time lines still.

@indutny indutny added a commit to indutny/node that referenced this pull request Oct 18, 2014
@indutny indutny crypto: allow forcing SSLv2/v3 via secureProtocol
Force-enable SSLv2/v3 when `secureProtocol` is explicitly set
to `SSLv2_method` or `SSLv3_method`.

see discussion at #8551
4601da4
@indutny
Member
indutny commented Oct 18, 2014

@dougwilson : please take a look at #8575

@dougwilson

@indutny the change in the description of the docs sounds good to me. It basically says that the default auto negotiation method won't negotiate down to SSLv3 any longer, but (since it didn't require a command line flag when 0.10 came out), specifically asking to only negotiate SSLv3 still still function.

@indutny indutny added a commit that referenced this pull request Oct 20, 2014
@indutny @tjfontaine indutny + tjfontaine crypto: allow forcing SSLv2/v3 via secureProtocol
Force-enable SSLv2/v3 when `secureProtocol` is explicitly set
to `SSLv2_method` or `SSLv3_method`.

see discussion at #8551
1349b68
@misterdjules

@dougwilson After more testing of the latest fixes to mitigate POODLE, @tjfontaine and I found out that they included some regressions regarding the handling of secureOptions/secureProtocols. As a result, we added more tests, made some changes and we believe that we came up with a new set of changes that mitigates POODLE without introducing regressions.

These changes allow you to use SSLv3 if it is specifically set as the desired protocol. We'd like to make sure that as well as mitigating POODLE and not introducing any regression, they cover your use case of using SSLv3 to talk to SSL terminators that can't be upgraded. Would you mind trying it in your environment and let us know if that works for you?

The fix is located in this branch: https://github.com/misterdjules/node/commits/fix-issue-8551, which is based off of joyent/node@v0.10. Depending on how you test it, please make sure that you grab the latest 3 commits of that branch.

@dougwilson

Cool, I see the branch. I'm pulling it down to compile now to try and get you an answer soon, especially since you're letting me check it before release :)

@misterdjules

@dougwilson Thank you, it is very much appreciated :)

@dougwilson

So far some initial tests I was able to do quickly seem positive.

@misterdjules

@dougwilson Without disclosing any confidential info, do you mind sharing with us some details about the tests you've been doing? Especially, I'm interested in the protocols, https and/or tls options and command line switches used by your tests.

Once again, thank you very much for your help!

@dougwilson

Yea, no problem.

So far I just ran one of our apps that essentially does this, boiled down to a command line:

node -pe 'https=require("https");https.get({hostname:"______",agent:(new https.Agent({secureProtocol: "SSLv3_method"}))}, function(res) {console.log(res.headers)})'

If I remove the secureProtocol, then request will hang and eventually an error will be thrown of Error: read ECONNRESET. This is caused by the first TLS handshake just hanging, thanks to our old F5 friend. I'm sure this hanging on the first TLS handshake can be emulated through a raw socket and talking in the binary SSL protocol manually, but I don't have time (right now) to put one together.

When setting secureProtocol to SSLv3_method, it causes OpenSSL to first offer a SSL 3.0 handshake with the server which goes through just fine.

@misterdjules

@dougwilson Great. Please let us know if you do other tests.

@DomT4
DomT4 commented Oct 23, 2014

The new Node's configure script still references the choice to kill off SSLv2 and SSLv3. Is this still correct? Did the default killing the two protocols not land in 0.10.x in the end?

  --without-ssl2        Disable SSL v2
  --without-ssl3        Disable SSL v3
@dougwilson

The two protocols are killed in 0.10 by turning on the no SSLv2 and no SSLv3 flags by default at runtime when the auto-negotiation methods are used, which is the default.

@DomT4
DomT4 commented Oct 24, 2014

I see. So, do the configure flags still work as expected in regards to completely removing that capability or is it advised to let the two runtime flags exercise security in that regard without disabling the protocols entirely?

@dougwilson

Either provides the same exact security; there is no additional security gains by compiling SSLv2/3 out of Node.js, unless you are super paranoid of accidentally enabling it at runtime.

@DomT4
DomT4 commented Oct 24, 2014

Okay, Cheers. Over at Homebrew we pushed through killing SSLv2 everywhere a couple of months ago, and intend to go through the same dance again with SSLv3 in the next few months, so we've started adopting a Debian-esque stance of proactively removing SSLv2 & 3 from software where possible. So my paranoia is more around others accidentally enabling it at runtime, really, heh.

@misterdjules

@DomT4 Yes, --without-ssl2 and --without-ssl3 can still be used the same way as before.

@mscdex mscdex added a commit to mscdex/node that referenced this pull request Dec 25, 2014
@indutny @mscdex indutny + mscdex configure: disable ssl2/ssl3 by default
PR-URL: nodejs#8551
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
7bc948a
@mscdex mscdex added a commit to mscdex/node that referenced this pull request Dec 25, 2014
@indutny @mscdex indutny + mscdex doc: document why SSL2/SSL3 is disabled
PR-URL: nodejs#8551
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
b2106f7
@mscdex mscdex added a commit to mscdex/node that referenced this pull request Dec 25, 2014
@indutny @mscdex indutny + mscdex crypto: allow forcing SSLv2/v3 via secureProtocol
Force-enable SSLv2/v3 when `secureProtocol` is explicitly set
to `SSLv2_method` or `SSLv3_method`.

see discussion at #8551
d8d361b
@piscisaureus piscisaureus added a commit to piscisaureus/io.js that referenced this pull request Jan 9, 2015
@indutny @piscisaureus indutny + piscisaureus configure: disable ssl2/ssl3 by default
PR-URL: nodejs/node-v0.x-archive#8551
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
eb99e0d
@piscisaureus piscisaureus added a commit to piscisaureus/io.js that referenced this pull request Jan 9, 2015
@indutny @piscisaureus indutny + piscisaureus doc: document why SSL2/SSL3 is disabled
PR-URL: nodejs/node-v0.x-archive#8551
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
b8c1e07
@piscisaureus piscisaureus added a commit to nodejs/node that referenced this pull request Jan 10, 2015
@piscisaureus piscisaureus crypto: allow run-time SSLv2/3 opt-in
Squashed commit of the following:

commit 63d21f3e020f4488357629b6303784f0b3a14a7c
Author: Timothy J Fontaine <tjfontaine@gmail.com>
Date:   Wed Oct 22 12:14:10 2014 -0700

    tls: enforce secureOptions on incoming clients

    Reuse the secureProtocol and secureOptions of the server when creating
    the secure context for incoming clients.

commit c67fa4b5370f24669744fc361385d25e8016a3d8
Author: Timothy J Fontaine <tjfontaine@gmail.com>
Date:   Wed Oct 22 10:27:56 2014 -0700

    tls: honorCipherOrder should not degrade defaults

    Specifying honorCipherOrder should not change the SSLv2/SSLv3 defaults
    for a TLS server.

    Use secureOptions logic in both lib/tls.js and lib/crypto.js

commit 4d0c1efa6ecab6a3a5be6001d9a9f508ffd1e2a6
Author: Fedor Indutny <fedor@indutny.com>
Date:   Sat Oct 18 04:47:05 2014 +0400

    crypto: allow forcing SSLv2/v3 via secureProtocol

    Force-enable SSLv2/v3 when `secureProtocol` is explicitly set
    to `SSLv2_method` or `SSLv3_method`.

    see discussion at #8551

commit 9d0af935a10ce2bf27ddc3dd529d832e1a982998
Author: Timothy J Fontaine <tjfontaine@gmail.com>
Date:   Fri Oct 17 15:16:26 2014 -0700

    crypto: move disaling SSLv2/3 into JavaScript

commit 88f34ac3e9b3b9248c52551dac414e4c8aeaf789
Author: Timothy J Fontaine <tjfontaine@gmail.com>
Date:   Fri Oct 17 15:15:45 2014 -0700

    doc: clarify poodle mitigation

commit 1ad00e0cb3e123adc78a4c2ed7f159c06d247dee
Author: Alexis Campailla <alexis@janeasystems.com>
Date:   Thu Oct 16 18:45:47 2014 +0200

    crypto: extra caution in setting ssl options

    Always set ssl2/ssl3 disabled based on whether they are enabled in Node.
    In some corner-case scenario, node with OPENSSL_NO_SSL3 defined could
    be linked to openssl that has SSL3 enabled.

commit cf8a621dd3f5122bce3dabd2c671a6a60203a2c9
Author: Timothy J Fontaine <tjfontaine@gmail.com>
Date:   Wed Oct 15 13:56:40 2014 -0700

    crypto: allow runtime opt in using SSLv2/SSLv3

    This change disables SSLv2/SSLv3 use by default, and introduces a
    command line flag to opt into using SSLv2/SSLv3.

    SSLv2 and SSLv3 are considered unsafe, and should only be used in
    situations where compatibility with other components is required and
    they cannot be upgrade to support newer forms of TLS.

commit 262de7a7f090c59641b8f5b2bba2f8db4ccdab6e
Author: Timothy J Fontaine <tjfontaine@gmail.com>
Date:   Wed Oct 15 14:48:05 2014 -0700

    build: revert change to disable ssl2 and ssl3

commit 564cacb47c1de16698188e8991a0d55eaf36b7a2
Author: Fedor Indutny <fedor@indutny.com>
Date:   Wed Oct 15 19:28:16 2014 +0400

    doc: document why SSL2/SSL3 is disabled

    PR-URL: nodejs/node-v0.x-archive#8551
    Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

commit ac49f04896cbb7564bdd2a8cc955d62de7071f5a
Author: Fedor Indutny <fedor@indutny.com>
Date:   Wed Oct 15 12:58:01 2014 +0400

    configure: disable ssl2/ssl3 by default

    PR-URL: nodejs/node-v0.x-archive#8551
    Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

commit cad51a32b0102a074a85929e98c07b754edaaa54
Author: Swaagie <info@martijnswaagman.nl>
Date:   Wed Oct 15 11:08:33 2014 +0200

    tls add secureOptions documentation

    PR-URL: nodejs/node-v0.x-archive#8553
    Reviewed-By: Fedor Indutny <fedor@indutny.com>
fba4929
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment