Opened 9 years ago
Closed 9 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)
Change History (20)
comment:1 Changed 9 years ago by
Cc: | Hynek Schlawack added |
---|
comment:2 Changed 9 years ago by
comment:3 Changed 9 years ago by
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 9 years ago by
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 follow-up: 6 Changed 9 years ago by
IOW, we should make OpenSSLCertificateOptions
’s default identical to DefaultOpenSSLContextFactory
and use SSLv23 with disabled SSLv2 as default?
comment:6 Changed 9 years ago by
Replying to hynek:
IOW, we should make
OpenSSLCertificateOptions
’s default identical toDefaultOpenSSLContextFactory
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 follow-up: 8 Changed 9 years ago by
Type: | enhancement → defect |
---|
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 follow-up: 9 Changed 9 years ago by
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 Changed 9 years ago by
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 follow-up: 11 Changed 9 years ago by
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 Changed 9 years ago by
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 9 years ago by
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 setSSL.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 9 years ago by
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 9 years ago by
Attachment: | disable-sslv2-6337.patch added |
---|
comment:14 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Hynek Schlawack deleted |
comment:15 Changed 9 years ago by
The news fragment in the patch doesn't follow the news fragment style guide.
comment:16 Changed 9 years ago by
Author: | → tomprince |
---|---|
Branch: | → branches/disable-sslv2-6337 |
(In [37443]) Branching to disable-sslv2-6337.
comment:17 Changed 9 years ago by
Author: | tomprince → hynek |
---|---|
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 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
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.