OpenSSLCertificateOptions doesn’t disallow SSLv2

[Hynek Schlawack]

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.

[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.

[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.

[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.

[Hynek Schlawack]

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

[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.

[Hynek Schlawack]

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.

[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.

[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!

[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!

[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.

[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).

[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.

[Jean-Paul Calderone]

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

[Hynek Schlawack]

Better topfile.

[Tom Prince]

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

[Tom Prince]

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

[Tom Prince]

(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.