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

configure: disable ssl2/ssl3 by default #8551

Closed
wants to merge 2 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Oct 15, 2014

POODLE

@mathiasbynens
Copy link

Secure defaults ftw. 👍

@jo
Copy link

jo commented Oct 15, 2014

👍

@3rd-Eden
Copy link

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
Copy link
Member Author

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
Copy link

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
Copy link
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 added a commit that referenced this pull request Oct 15, 2014
PR-URL: #8551
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
indutny added a commit that referenced this pull request Oct 15, 2014
PR-URL: #8551
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@indutny
Copy link
Member Author

indutny commented Oct 15, 2014

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

@Fishrock123
Copy link
Member

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

@indutny
Copy link
Member Author

indutny commented Oct 15, 2014

@tjfontaine ^^^

@tjfontaine
Copy link

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
Copy link

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
Copy link
Member Author

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
Copy link

Advising is something else than forcing.

@indutny
Copy link
Member Author

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
Copy link

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
Copy link

bah forgot the link #8555

@3rd-Eden
Copy link

@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
@indutny
Copy link
Member Author

indutny commented Oct 17, 2014

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

@dougwilson
Copy link
Member

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
Copy link
Member Author

indutny commented Oct 17, 2014

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

@dougwilson
Copy link
Member

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 added a commit to indutny/node that referenced this pull request Oct 18, 2014
Force-enable SSLv2/v3 when `secureProtocol` is explicitly set
to `SSLv2_method` or `SSLv3_method`.

see discussion at nodejs#8551
@indutny
Copy link
Member Author

indutny commented Oct 18, 2014

@dougwilson : please take a look at #8575

@dougwilson
Copy link
Member

@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 added a commit that referenced this pull request Oct 20, 2014
Force-enable SSLv2/v3 when `secureProtocol` is explicitly set
to `SSLv2_method` or `SSLv3_method`.

see discussion at #8551
@misterdjules
Copy link

@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
Copy link
Member

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
Copy link

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

@dougwilson
Copy link
Member

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

@misterdjules
Copy link

@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
Copy link
Member

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
Copy link

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

@DomT4
Copy link

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
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link

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
Copy link

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

mscdex pushed a commit to mscdex/node that referenced this pull request Dec 25, 2014
PR-URL: nodejs#8551
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
mscdex pushed a commit to mscdex/node that referenced this pull request Dec 25, 2014
PR-URL: nodejs#8551
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
mscdex pushed a commit to mscdex/node that referenced this pull request Dec 25, 2014
Force-enable SSLv2/v3 when `secureProtocol` is explicitly set
to `SSLv2_method` or `SSLv3_method`.

see discussion at nodejs#8551
piscisaureus added a commit to nodejs/node that referenced this pull request Jan 10, 2015
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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet