-
Notifications
You must be signed in to change notification settings - Fork 577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate SSLv2 support on v0.12 and v0.10 #80
Comments
I'd like more input on this, so /cc @nodejs/crypto @nodejs/ctc (@nodejs/lts). Reference to read up on if you're not following the details: https://drownattack.com/ Of particular relevance is this:
We are already not enabling it by default for 0.10 and 0.12 (right?) and you have to use However, the difficulty here is that having sslv2 enabled and not even necessarily using it can open you up to attack. At this stage my vote would be to print a verbose warning at start-up if you |
I think I'd prefer if we removed the option by default, but allowed that to be reverted with --security-revert={cvenum}. So if you added --security-revert={CVE-2016-0800} you'd get the warning and be able to use --enable-ssl2. (I think --security-revert was going to warn for each one reverted) It leverages the approach we put in place the last time and makes the bar a little higher in that you have to actively choose to add --security-revert while at the same time providing the customer a work around if they absolutely still need to keep using it. |
I'll be the contrarian today. :-) SSLv2 was superseded - because it was known to be flawed - by SSLv3 twenty years ago, in 1996. It was officially deprecated (in RFC 6176) half a decade ago, in 2011. From the abstract:
I agree with Rod that babying your users is bad but I don't think that's what we're doing if we remove SSLv2 support for good. Honestly, I can't think of a remotely valid use case that would require SSLv2. Even if your application needs to talk to a 2001-era set-top box, it's going to communicate over SSLv3, not SSLv2. |
Do we have any data on whether people are even using the |
Note that the current HEAD of v0.12/v0.10-staging cannot work SSLv2 even if we use According to openssl/openssl@56f1acf, we need to add the following patch. diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 454c9be..754d159 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -391,6 +391,9 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& args) {
SSL_CTX_sess_set_get_cb(sc->ctx_, SSLWrap<Connection>::GetSessionCallback);
SSL_CTX_sess_set_new_cb(sc->ctx_, SSLWrap<Connection>::NewSessionCallback);
+ if (SSL2_ENABLE)
+ SSL_CTX_clear_options(sc->ctx_, SSL_OP_NO_SSLv2);
+
sc->ca_store_ = NULL;
} Furthermore, three SSLv2 ciphers of EXP-RC2-CBC-MD5, DES-CBC-MD5 and DES-CBC3-MD5 are completely disabled by I don't think it is worth while continuing to support SSLv2 even in LTS with paying such costs. |
While I prefer that we avoid breaking changes, we have had to accept them from openssl and pass them along in the past. In this case since we'd have to actively change the Node.js code to enable insecure algs and it would only be a partial fix without floating patches on openssl, I'd have a hard time making a strong case for doing it. |
I'm leaning the same direction as @shigeki and @bnoordhuis. At this point there is exceedingly little reason to continue supporting SSLv2 in any way. |
+1 to removing it unless someone makes a very compelling argument against it |
🔪 Does that mean the flag will be removed, or that it simply won't do anything?
Edit: I just read that the security patch to OpenSSL itself disables it outright. 🔪 🔪 |
Alternatively we could make the existing flag just warn
+1 to this part
… and move the functionality under `--security-revert`
-1 to this part
I would leave it to the user to do this part "manually" if s/he needs the
old SSL v2 functionality for any reason.
|
+1 to removing, but not sure what should the existing flag do — warn or throw? |
+1 to having the flag give a warning. While that is technically a |
I agree that we should not leave the option but have it do nothing. If we just remove it completely the end user would get "bad option:". A warning would allow them to run, but if they are actually using it that could result in a harder to diagnose problem later on. I think I'd lean towards removing completely |
@mhdawson has a point. Maybe detect that flag and fail early with a special error? |
I'll write a patch. |
(cross-posting nodejs/node#5433 (comment)) I'm calling it and adding this: SSLv2 support is being removed Given the additional barriers introduced in OpenSSL 1.0.1s to retaining SSLv2 support and the long list of known SSLv2 vulnerabilities, Node.js v0.10.43 and v0.12.11 will be completely remove SSLv2 support and the I also suggest this means that if you supply the argument then it should bork, not fail silently, it's a breaking change but it needs to be explicit. |
nodejs/node#5529 (v0.10) |
Remove support for SSLv2 because of DROWN (CVE-2016-0800). Use of the `--enable-ssl2` flag is now an error; node will print an error message and exit. Fixes: nodejs/Release#80 PR-URL: nodejs#5529 Reviewed-By: Rod Vagg <rod@vagg.org>
nodejs/node#5536 (v0.12) Quoting from abstract in the paper of DROWN attack,
I close this for hoping that Node makes TLS ecosystem be healthy. |
OpenSSL-1.0.1s disables SSLv2 by default for against DROWN attack(CVE-2016-0800). https://www.openssl.org/news/secadv/20160301.txt
There are no reasons to continue to support SSLv2 on Node LTS.
The text was updated successfully, but these errors were encountered: