Opened 5 years ago

Closed 5 years ago

#6337 defect closed fixed (fixed)

OpenSSLCertificateOptions doesn’t disallow SSLv2

Reported by: Hynek Schlawack Owned by: Tom Prince
Priority: normal Milestone:
Component: core Keywords:
Cc: Hynek Schlawack Branch: branches/disable-sslv2-6337
branch-diff, diff-cov, branch-cov, buildbot
Author: hynek

Description

Our default SSL method in DefaultOpenSSLContextFactory and thus in string endpoints is SSLv23 for maximum compatibility, but we disable SSLv2 because it’s insecure.

However, that doesn’t happen in OpenSSLCertificateOptions: http://twistedmatrix.com/trac/browser/trunk/twisted/internet/_sslverify.py#L743

Probably because it is newer and uses TLSv1 as default?

I suggest to set OP_NO_SSLv2 here too by default. If that’s not possible, at least make it possible to inject it from the outside by a new option. Otherwise we can’t switch string endpoints to OpenSSLCertificateOptions in #6286.

Attachments (2)

disable-sslv2-6337.patch (3.8 KB) - added by Hynek Schlawack 5 years ago.
disable-sslv2-6337-v2.patch (4.3 KB) - added by Hynek Schlawack 5 years ago.
Better topfile.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by Hynek Schlawack

Cc: Hynek Schlawack added

comment:2 Changed 5 years ago by Itamar Turner-Trauring

It should be set by default if someone chooses SSLv23. For other methods it's either irrelevant, or if someone explicitly asks for the SSLv2 method they should be able to get SSLv2. No reason to have external option, SSLv2 is ancient.

comment:3 Changed 5 years ago by Jean-Paul Calderone

You may be interested to know that most distributors of OpenSSL (Linux vendors, largely) have taken this decision out of our hands already. At least a year ago they began building OpenSSL without SSLv2 support. On such builds, SSLv23 does not enable SSLv2, only SSLv3 and TLSv1.

comment:4 Changed 5 years ago by Glyph

I believe that the right thing to do is to always use SSLv23 because that means "more than one specific method" and then use the OP_NO_WHATEVER flags to turn off older protocol versions. As newer TLS versions become available, setting a specific method (as OpenSSLCertificateOptions currently does) might, depending on the whims of the OpenSSL maintainers, mean that we don't support newer, more secure TLS versions, even if the library could do that for us.

comment:5 Changed 5 years ago by Hynek Schlawack

IOW, we should make OpenSSLCertificateOptions’s default identical to DefaultOpenSSLContextFactory and use SSLv23 with disabled SSLv2 as default?

comment:6 in reply to:  5 Changed 5 years ago by Glyph

Replying to hynek:

IOW, we should make OpenSSLCertificateOptions’s default identical to DefaultOpenSSLContextFactory and use SSLv23 with disabled SSLv2 as default?

No; we already require TLS 1.0, so apparently disabling SSLv3 by default is fine too. TLS 1.0 was defined in 1999 so most applications are caught up by now.

I think it would be better to give users a way to express a 'minimum version', and then set all apropriate OP_NO_WHATEVER options, rather than let them set whatever method and then try to bulletproof it for them.

comment:7 Changed 5 years ago by Hynek Schlawack

Type: enhancementdefect

I see.

You are basically saying, we should abstract the method selection away from OpenSSL’s crazy modus operandi as per the discussion on IRC yesterday, right?

That sounds like an extra ticket to me… Would you be cool with fixing this by behaving like DefaultOpenSSLContextFactory for now because I *need* a way to specify SSLv23 w/o SSLv2 for #6286 if we don’t want to make TLSv1 default for str ports?

As for the extra SSL/TLS work I wouldn’t mind to help out after my chains are finally merged. :) I’d be happy to discuss the general future of SSL in Twisted at PyCon too.

comment:8 in reply to:  7 ; Changed 5 years ago by Glyph

Replying to hynek:

I see.

You are basically saying, we should abstract the method selection away from OpenSSL’s crazy modus operandi as per the discussion on IRC yesterday, right?

Right. This would also make it easier to implement with other libraries.

That sounds like an extra ticket to me… Would you be cool with fixing this by behaving like DefaultOpenSSLContextFactory for now because I *need* a way to specify SSLv23 w/o SSLv2 for #6286 if we don’t want to make TLSv1 default for str ports?

Aah, I see. You want to leave it that way so it will be a compatible change, and not change the default version for strports.

I would rather do the minimum-version thing in this ticket. It's really not much work; since you realistically only have 3 options for minimum version; SSLv2 (which hopefully is not even available in your build of OpenSSL; at any rate it shouldn't be), SSLv3, and TLSv1. Which means you can either set OP_NO_SSLv2 or OP_NO_SSLv3... and that's it. One extra conditional does not seem like an onerous additional amount of code :).

As for the extra SSL/TLS work I wouldn’t mind to help out after my chains are finally merged. :) I’d be happy to discuss the general future of SSL in Twisted at PyCon too.

I would love to do that. I'll look forward to seeing you there.

comment:9 in reply to:  8 Changed 5 years ago by Hynek Schlawack

Owner: set to Hynek Schlawack

Replying to glyph:

That sounds like an extra ticket to me… Would you be cool with fixing this by behaving like DefaultOpenSSLContextFactory for now because I *need* a way to specify SSLv23 w/o SSLv2 for #6286 if we don’t want to make TLSv1 default for str ports?

Aah, I see. You want to leave it that way so it will be a compatible change, and not change the default version for strports.

Exactly. That’s currently blocking #6286 from merging.

I would rather do the minimum-version thing in this ticket. It's really not much work; since you realistically only have 3 options for minimum version; SSLv2 (which hopefully is not even available in your build of OpenSSL; at any rate it shouldn't be), SSLv3, and TLSv1. Which means you can either set OP_NO_SSLv2 or OP_NO_SSLv3... and that's it. One extra conditional does not seem like an onerous additional amount of code :).

No it isn’t. I just tend to shy away from adding options. :) Will do.

As for the extra SSL/TLS work I wouldn’t mind to help out after my chains are finally merged. :) I’d be happy to discuss the general future of SSL in Twisted at PyCon too.

I would love to do that. I'll look forward to seeing you there.

Awesome!

comment:10 Changed 5 years ago by Jean-Paul Calderone

I think most of what's written in the comments above is good and interesting. However, if you just want to resolve this ticket, then it's an absolutely trivialy change:

    + ctx.set_options(SSL.OP_NO_SSLv2)

(plus unit tests, obviously). Since this ticket is blocking other more interesting work, I think it's worth considering doing this the simply way and deferring the rest of these improvements to a later ticket.

Thanks!

comment:11 in reply to:  10 Changed 5 years ago by Glyph

Replying to exarkun:

I think most of what's written in the comments above is good and interesting. However, if you just want to resolve this ticket, then it's an absolutely trivialy change:

    + ctx.set_options(SSL.OP_NO_SSLv2)

(plus unit tests, obviously). Since this ticket is blocking other more interesting work, I think it's worth considering doing this the simply way and deferring the rest of these improvements to a later ticket.

I wish I could agree, I like trivial changes. But if upgrading the protocol version for users of SSL endpoints to TLSv1 is an incompatible change, then wouldn't upgrading the protocol version for OpenSSLCertificateOptions users who explicitly specified SSLv23_METHOD to SSL version 3 also be an incompatible change? (Perhaps a worse one, actually, since if some poor idiot set SSLv2_METHOD and then we unconditionally set SSL.OP_NO_SSLv2 wouldn't that mean "no connections ever"?)

If we're going to choose between the two of those, I would rather just bump endpoints users to TLSv1, since it's better security and there's no client I know of that can't handle it.

comment:12 Changed 5 years ago by Jean-Paul Calderone

But if upgrading the protocol version for users of SSL endpoints to TLSv1 is an incompatible change, then wouldn't upgrading the protocol version for OpenSSLCertificateOptions users who explicitly specified SSLv23_METHOD to SSL version 3 also be an incompatible change?

TLSv1 is more incompatible than SSLv3. At some point it would be good to pick an objective metric to inform us whether we're happy to break users depending on SSLv3, but I'm confident that for now we should at least be happy to break users depending on SSLv2 (because SSLv2 is totally broken all by itself, because many installations of OpenSSL in the wild now do not even support SSLv2, because basically everyone can at least silently upgrade to SSLv3).

Also, we already did this to users once when we added OP_NO_SSLv2 to DefaultOpenSSLContextFactory. Not that two wrongs make a right, but there is precedent and I think it was right last time.

(Perhaps a worse one, actually, since if some poor idiot set SSLv2_METHOD and then we unconditionally set SSL.OP_NO_SSLv2 wouldn't that mean "no connections ever"?)

Nope. OP_NO_SSLv2 only changes SSLv23_METHOD behavior, nothing else.

If we're going to choose between the two of those, I would rather just bump endpoints users to TLSv1, since it's better security and there's no client I know of that can't handle it.

I hope we'll get there someday. However, we must consider clients *and* servers, and there are still some SSLv3-only servers out there (I would insert a list here, but it's hard to construct; the only way I know to do so is to do a lot of spidering with something that can recognize this kind of server - I know my position is weaker for not including the list, but I'm confident enough to say "trust me" here, I hope that's enough and we can move on with this very simple change).

comment:13 Changed 5 years ago by Glyph

I'm happy to trust you, as long as we're being explicit that it's taken on faith. Please proceed, I withdraw any objections.

Changed 5 years ago by Hynek Schlawack

Attachment: disable-sslv2-6337.patch added

comment:14 Changed 5 years ago by Hynek Schlawack

Keywords: review added
Owner: Hynek Schlawack deleted

comment:15 Changed 5 years ago by Jean-Paul Calderone

The news fragment in the patch doesn't follow the news fragment style guide.

Changed 5 years ago by Hynek Schlawack

Attachment: disable-sslv2-6337-v2.patch added

Better topfile.

comment:16 Changed 5 years ago by Tom Prince

Author: tomprince
Branch: branches/disable-sslv2-6337

(In [37443]) Branching to disable-sslv2-6337.

comment:17 Changed 5 years ago by Tom Prince

Author: tomprincehynek
Keywords: review removed
Owner: set to Tom Prince

This code looks good and implements the disucssed change. The build results look good. I'll merge.

comment:18 Changed 5 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(In [37445]) Merge disable-sslv2-6337: Disable SSLv2 with t.i.ssl.CertificateOptions.

Author: hynek Reviewers: tom.prince Fixes: #6337

twisted.internet.ssl.CertificateOptions now disables SSLv2 if SSLv23 is selected, allowing only SSLv3 and TLSv1.

Note: See TracTickets for help on using tickets.