Remove SSLv3 support #2255
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,9 +59,6 @@ Security::PeerOptions::parse(const char *token) | |
| } | ||
| KeyData &t = certs.back(); | ||
| t.privateKeyFile = SBuf(token + 4); | ||
| } else if (strncmp(token, "min-version=", 12) == 0) { | ||
| tlsMinVersion = SBuf(token + 12); | ||
| optsReparse = true; | ||
|
|
@@ -192,54 +189,6 @@ Security::PeerOptions::updateTlsVersionLimits() | |
|
|
||
| return; | ||
| } | ||
| } | ||
|
|
||
| Security::ContextPointer | ||
|
|
@@ -307,16 +256,6 @@ static struct ssl_option { | |
| "NETSCAPE_REUSE_CIPHER_CHANGE_BUG", SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG | ||
| }, | ||
| #endif | ||
| #if defined(SSL_OP_SSLEAY_080_CLIENT_DH_BUG) | ||
| { | ||
| "SSLEAY_080_CLIENT_DH_BUG", SSL_OP_SSLEAY_080_CLIENT_DH_BUG | ||
|
|
@@ -382,11 +321,6 @@ static struct ssl_option { | |
| "NETSCAPE_DEMO_CIPHER_CHANGE_BUG", SSL_OP_NETSCAPE_DEMO_CIPHER_CHANGE_BUG | ||
| }, | ||
| #endif | ||
| #if defined(SSL_OP_NO_TLSv1) | ||
| { | ||
| "NO_TLSv1", SSL_OP_NO_TLSv1 | ||
|
|
@@ -524,6 +458,11 @@ Security::PeerOptions::parseOptions() | |
| // compliance with RFC 6176: Prohibiting Secure Sockets Layer (SSL) Version 2.0 | ||
| if (SSL_OP_NO_SSLv2) | ||
| op |= SSL_OP_NO_SSLv2; | ||
| #endif | ||
| #if defined(SSL_OP_NO_SSLv3) | ||
| // compliance with RFC 7568: Prohibiting Secure Sockets Layer (SSL) Version 3.0 | ||
| if (SSL_OP_NO_SSLv3) | ||
| op |= SSL_OP_NO_SSLv3; | ||
|
Contributor
There was a problem hiding this comment. I am not convinced we should make this "no SSLv3" decision for Squid admins, especially since Squid is quite often used to "upgrade" legacy clients and servers. The vast majority of existing Squid builds do not support SSLv3 today because OpenSSL builds disable SSLv3 by default since OpenSSL v1.1.0. Folks that use OpenSSL v1.0 today1 probably do not want to disable SSLv3, but even they can already disable SSLv3 in their Squid configurations and/or OpenSSL builds. We could support disabling SSLv3 by default while giving admins an option to overwrite that Squid default, but I am not convinced there is enough demand to implement that feature. And if we are really serious about disabling SSLv3 (where it is supported by an OpenSSL build), then adding that feature would require cooperating on developing a new suitable configuration interface for this -- just using SSL_OP_NO_SSLv3 when no other options were configured would not be good enough; that additional complexity raises the bar for supporting "disabling SSLv3 by default while giving admins an option to overwrite that Squid default" idea. I would support warning admins if their Squid instance allows SSLv3 use (if we can detect that state reliably). Footnotes
Contributor
Author
There was a problem hiding this comment.
You have already imposed the design decision earlier this year that this use-case for Squid is no longer valid, to justify removal of Gopher protocol and other older code.
Contributor
There was a problem hiding this comment.
I have not imposed any design decisions. FWIW, our (IMO, correct) decision to remove Gopher code does not imply we should add code to prohibit SSLv3 support.
Contributor
Author
There was a problem hiding this comment.
You proposed removal of a feature and ensured it was removed for design reasons, despite my objections that said protocol had/has actively growing usage. Ergo you imposed a design decision. Specifically to stop officially supporting translation from legacy protocols to HTTP.
Reasons for code removal should be applied consistently for all code meeting the stated criteria. Ergo, for the "legacy protocol" and "unsolvable security issues" criteria used for removing protocol "Gopher" we have this PR for removing protocol "SSlv3". SSLv3 has:
Contributor
There was a problem hiding this comment. I see the merit in @yadij 's position here. RFC7568 dates back to June 2015; given the age, I would not expect that there are many clients dating so far back in the past that cannot speak to Squid in TLS 1.0 or later. Do we have any hard evidence to the contrary? Furthermore, any change here would get distributed with Squid v7, which should be scheduled to start beta in late 2026 (we didn't explicitly tie up that discussion, but that's for another thread) and is likely to be felt by real-world users probably sometime in mid 2027; 12 years should be enough to deprecate a functionality, isn't it?
Contributor
There was a problem hiding this comment. @kinkie, AFAICT, your comment supports a "position" that nobody is attacking -- we all agree that SSL is old, insecure, and rarely used. SSL is also not supported by a typical Squid installation these days (as detailed in my change request).
Contributor
Author
There was a problem hiding this comment.
So ... Squid has silently stopped supporting SSLv3 since 4 years ago without anyone noticing. One more argument for merging this PR change. |
||
| #endif | ||
| parsedOptions = op; | ||
|
|
||
|
|
||
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.
FWIW, I support removing deprecated and undocumented version=X parameter because it would simplify Squid code quite a bit. I assume that such a removal would still allow admins to achieve the same configuration using an options=Y parameter. That assumption would need to be validated.
That version=X parameter removal can be done in this PR. This PR already has the necessary changes (or most of them -- I did not check carefully).
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.
FTR; This is how SSLv3 is enabled by admin for legacy installs (i.e.
sslversion=3) forces Squid to negotiate SSLv3 regardless of BCP 195 requirements.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.
I am struggling to find a way to connect your statement to my change request. Are you implying that admins would not be able to force SSLv3 negotiations using options=Y parameter after version=X parameter1 is removed?
Footnotes
In the context of this thread, version=X refers to all parameter name variations that configure sslVersion data members. Depending on the directive being parsed, Squid recognizes "version", "tlsversion", "tls-version", and "sslversion" spellings (at least2). AFAICT, those spelling differences are immaterial here. ↩
git grep -E 'strncmp.*"(tls|ssl)-?"'↩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.
To meet your suggested "giving admins an option to overwrite that Squid default" would be to leave this configuration option in place. Just requiring it to be set to re-enable SSLv3 now, instead of auto-enabling with a manual disable.
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.
@yadij how about we do just that in Squid 8 and remove in 9?
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.
I did not suggest that. I actually said that "I am not convinced there is enough demand to implement that feature" and noted implementation complexities. In short, I suggested not to add that feature.
I am still struggling to understand whether
sslversion=Xremoval would still allow admins to achieve the same configuration usingoptions=Yparameter(s). It seems like a simple/straightforward question, but I am probably missing the reason why there is no answer on this change request thread.Uh oh!
There was an error while loading. Please reload this page.
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.
You did. I cut-and-pasted that quote from your comment in the other review thread.
If the only PR action taken is removing
sslversion=the answer would be yes. In official code that option is just appending a few hard-coded values to anyoptions=setting.Removal of support for
options=NO_SSLv3by this PR means that thesslversion=values 4/5/6 are no longer able to be replaced fully byoptions=. But, the change to hard-codeNO_SSLv3restores that.Which leaves
sslversion=3. That officially disables all TLS verisons, forcing SSLv3 as the only potential choice of transport for TLS/SSL connections. without doing anything to ensure that SSLv3 works or is even available from the used openssl.