-
-
Notifications
You must be signed in to change notification settings - Fork 661
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
Disable SSLv2 (for DROWN) #1395
Conversation
This prevents clients from using SSLv2 (for the DROWN attack) *and* SSLv3 (for the POODLE attack) when they issue STARTTLS. See <https://poodle.io/>, <https://drownattack.com/>, and <http://disablessl3.com/#nodejs>.
options.secureOptions = options.secureOptions || | ||
constants.SSL_OP_NO_SSLv3; | ||
options.secureOptions = options.secureOptions | | ||
constants.SSL_OP_NO_SSLv2 | constants.SSL_OP_NO_SSLv3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I'm changing the first |
to ||
||
to |
(sorry, had that backwards the first try). This means that regardless of what the caller passes in secureOptions
, haraka will prevent them from shooting themselves in the foot by allowing SSLv2/v3 connections.
I'm not 100% sure this is always desired, but it seems like good security to me.
I'd prefer to deal with DROWN by bumping our minimum version of node.js to 0.12.12. |
Even better, 0.10.43 was patched to remove SSLv2 as well, so simply requiring at least 0.10.43 would also take care of it. |
That's a great idea too. Might not hurt to do both, at least for now, just in case someone's stuck on an old node version. |
It might be painting with too broad of a brush but in my experience, people who don't upgrade their node are not going to upgrade their Haraka, and vice versa. I think we're better off just removing all of the vestiges of SSLv2, which makes this patch much smaller. |
+1 |
I can't speak for anyone else, but I will say that in our deployment, it's far easier to upgrade just Haraka than node itself. |
That may be, but that only solves one security issue. Everyone needs to upgrade node for a number of other security (mostly openssl) issues as well. Are you using some "slowly if at all" platform like Debian where you have to roll your own node releases to get them them in a timely manner? (if so, sorry, I've felt that pain, and that's a cost of using that distro). It won't be very far into the future when everyone (node.js upstream, us) drops support for 0.10 and then 0.12. Upgrading node might be more work but security-wise, it's really not optional. Patching it locally for your deployment until you upgrade node? Sure. I'm having a hard time seeing any upside to introducing this code into Haraka 2.8 and I'm seeing a risk of constants.SSL_OP_NO_SSLv2 no longer existing in the future, causing a new maintenance issue. |
I looked into this some more. SSLv2 has been disabled-by-default in node since 0.10.29, and was removed entirely in 0.10.43. @chazomaticus , did you test with a version of node > 0.10.29 and discover that Haraka was vulnerable? Bumping to 0.10.43 or 0.12.12 would not resolve drown for node 4 or 5, if that truly is an issue. Based on my reading of nodejs/node#5433, I don't think that downgrade is possible in node 4 or 5.
|
Yes, our haraka (2.7.2-ish plus a few local mods) on node v0.12.9 was vulnerable. Not sure what flags that node was compiled with. I'd just like to reiterate my support for this patch. In my opinion, it's not great security if it can be defeated with a compile flag on the language runtime, which many people who would deploy haraka might not have control over or care about. This patch would keep everyone secure from this threat regardless of the whims of administrators of the machines it's running on.
I'd also like to point out that I don't see a maintenance issue here. I believe js coerces an undefined value, e.g. if SSL_OP_NO_SSLv2 were no longer defined by node, into a 0 in this expression, without warning or error. In other words, I think this code would ensure haraka is doing the right thing regardless of node version or compile flags, and that sounds like a good idea to me. |
I hereby withdraw my objections. I further recommend that if the @haraka/core to wander by here also agrees, they click that green "Merge pull request" button. |
Done. |
Thanks @msimerson, @baudehlo, and @smfreegard! |
Thank you @chazomaticus! |
This prevents clients from using SSLv2 (for the DROWN attack) and
SSLv3 (for the POODLE attack) when they issue STARTTLS. See
https://poodle.io/, https://drownattack.com/, and
http://disablessl3.com/#nodejs.
The only way I know to test is a bit odd--you need an openssl version compiled with SSLv2 support. This can be hard to find on modern systems; for example: the openssl that comes with Ubuntu 14.04 will say
unknown option -ssl2
even though later in the usage docs it has-ssl2 - just use SSLv2
). Run haraka, then:(Replace
localhost
/25
with the hostname and port of haraka.) If you get an error that includes a line like:then it successfully initiated the SSLv2 handshake and just got stymied by a lack of cipher suites, meaning your install is vulnerable to DROWN attacks.
If, however, you get an error like:
then it could not initiate the SSLv2 handshake, and your install is not vulnerable to DROWN.
We've been running this a while now without issue, and 3rd parties have tested and agree that our harakas aren't susceptible to DROWN.