Skip to content
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

Merged
merged 1 commit into from
Apr 7, 2016
Merged

Disable SSLv2 (for DROWN) #1395

merged 1 commit into from
Apr 7, 2016

Conversation

chazomaticus
Copy link
Contributor

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:

openssl s_client -ssl2 -starttls smtp -connect localhost:25

(Replace localhost/25 with the hostname and port of haraka.) If you get an error that includes a line like:

140690032785224:error:1406D0B8:SSL routines:GET_SERVER_HELLO:no cipher list:s2_clnt.c:452:

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:

140695119636296:error:1407F0E5:SSL routines:SSL2_WRITE:ssl handshake failure:s2_pkt.c:429:

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.

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;
Copy link
Contributor Author

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.

@msimerson
Copy link
Member

I'd prefer to deal with DROWN by bumping our minimum version of node.js to 0.12.12.

@msimerson
Copy link
Member

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.

@chazomaticus
Copy link
Contributor Author

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.

@msimerson
Copy link
Member

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.

@smfreegard
Copy link
Collaborator

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

+1

@chazomaticus
Copy link
Contributor Author

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.

@msimerson
Copy link
Member

it's far easier to upgrade just Haraka

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.

@msimerson
Copy link
Member

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.

Assessment:

Node.js v0.10 an v0.12 are unaffected, unless manually enabling SSLv2 with --enable-ssl2
Node.js v4 and v5 are unaffected

@chazomaticus
Copy link
Contributor Author

did you test with a version of node > 0.10.29 and discover that Haraka was vulnerable?

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'm seeing a risk of constants.SSL_OP_NO_SSLv2 no longer existing in the future, causing a new maintenance issue.

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.

@msimerson
Copy link
Member

Yes, our haraka ... on node v0.12.9 was vulnerable.

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.

@baudehlo baudehlo merged commit e4feb6b into haraka:master Apr 7, 2016
@baudehlo
Copy link
Collaborator

baudehlo commented Apr 7, 2016

Done.

@chazomaticus
Copy link
Contributor Author

Thanks @msimerson, @baudehlo, and @smfreegard!

@chazomaticus chazomaticus deleted the no-drown branch April 11, 2016 19:03
@msimerson
Copy link
Member

Thank you @chazomaticus!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants