Closed Bug 1106470 Opened 10 years ago Closed 10 years ago

Drop SSLv3 support entirely

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla39
Tracking Status
firefox39 --- verified
relnote-firefox --- 39+

People

(Reporter: emk, Assigned: emk)

References

Details

(Keywords: dev-doc-needed, site-compat)

Attachments

(2 files, 3 obsolete files)

Chrome will drop the support since version 44 which will be released in July 2015.
OS: Windows 7 → All
Hardware: x86 → All
Attached patch patch (obsolete) (deleted) — Splinter Review
Attachment #8567850 - Flags: review?(dkeeler)
This patch should be landed after the aurora merge.
Attached patch patch v2 (obsolete) (deleted) — Splinter Review
Removed more dead code. I didn't remove strings in case we have to backout on future branches.
Attachment #8567850 - Attachment is obsolete: true
Attachment #8567850 - Flags: review?(dkeeler)
Attachment #8568473 - Flags: review?(dkeeler)
Comment on attachment 8568473 [details] [diff] [review] patch v2 Review of attachment 8568473 [details] [diff] [review]: ----------------------------------------------------------------- In general, looks good. I found more things to remove, however. Also, we should keep a very close eye on this. If it doesn't look like SSL 3 is going to be similarly removed from Chrome 44 or if that release date is not close enough to the release date for Firefox 39 (looks like 30 June 2015), we should hold this back at least one release. security/manager/ssl/tests/gtest/TLSIntoleranceTest.cpp should probably be updated to reflect the new minimum supported version. I found some remaining uses of STATE_USES_SSL_3 in toolkit/devtools/webconsole/network-helper.js and toolkit/devtools/webconsole/test/unit/test_security-info-weakness-reasons.js (we should probably have someone from devtools look at these changes as well, since they just added support for showing when a connection uses SSL 3). I think I should have another look, so r- for now. ::: browser/devtools/netmonitor/test/browser_net_security-warnings.js @@ +35,5 @@ > ]}, resolve); > }); > > let cipher = $("#security-warning-cipher"); > let sslv3 = $("#security-warning-sslv3"); Looks like this and related things should be removed, too. ::: netwerk/protocol/http/nsHttpChannel.cpp @@ -1218,5 @@ > if (securityInfo && > NS_SUCCEEDED(securityInfo->GetSecurityState(&state)) && > (state & nsIWebProgressListener::STATE_IS_BROKEN)) { > // Send weak crypto warnings to the web console > - if (state & nsIWebProgressListener::STATE_USES_SSL_3) { Looks like there is still code that sets this flag in nsNSSIOLayer.cpp (around line 1212). Also, it should be removed from uriloader/base/nsIWebProgressListener.idl and docshell/base/nsDocShell.cpp ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ -1297,5 @@ > post = Telemetry::SSL_TLS10_INTOLERANCE_REASON_POST; > break; > - case SSL_LIBRARY_VERSION_3_0: > - pre = Telemetry::SSL_SSL30_INTOLERANCE_REASON_PRE; > - post = Telemetry::SSL_SSL30_INTOLERANCE_REASON_POST; These probes should be removed from toolkit/components/telemetry/Histograms.json as well.
Attachment #8568473 - Flags: review?(dkeeler) → review-
Attached patch patch v2, PSM part (deleted) — Splinter Review
(In reply to David Keeler [:keeler] (use needinfo?) from comment #4) > I found some remaining uses of STATE_USES_SSL_3 in > toolkit/devtools/webconsole/network-helper.js and > toolkit/devtools/webconsole/test/unit/test_security-info-weakness-reasons.js > (we should probably have someone from devtools look at these changes as > well, since they just added support for showing when a connection uses SSL > 3). I divided the devtools part to another patch. > ::: netwerk/protocol/http/nsHttpChannel.cpp > @@ -1218,5 @@ > > if (securityInfo && > > NS_SUCCEEDED(securityInfo->GetSecurityState(&state)) && > > (state & nsIWebProgressListener::STATE_IS_BROKEN)) { > > // Send weak crypto warnings to the web console > > - if (state & nsIWebProgressListener::STATE_USES_SSL_3) { > > Looks like there is still code that sets this flag in nsNSSIOLayer.cpp > (around line 1212). This flag is repurposed to indicate an error when the server tried to negotiate SSLv3 (and we refused). So we can't remove this until we remove the dedicated error page. > Also, it should be removed from uriloader/base/nsIWebProgressListener.idl > and docshell/base/nsDocShell.cpp Even if the flag is no longer used, I will not change the idl to make the late backout possible, just like strings. In theory, constant-only change should not require the iid bump, but now a server-side hook will refuse to push the commit without changing the iid.
Attachment #8568473 - Attachment is obsolete: true
Attachment #8569820 - Flags: review?(dkeeler)
Attached patch patch v2, devtools part (obsolete) (deleted) — Splinter Review
Attachment #8569821 - Flags: review?(vporof)
Attached patch patch v2.1, devtools part (deleted) — Splinter Review
More removal needed.
Attachment #8569821 - Attachment is obsolete: true
Attachment #8569821 - Flags: review?(vporof)
Attachment #8569853 - Flags: review?(vporof)
Comment on attachment 8569820 [details] [diff] [review] patch v2, PSM part Review of attachment 8569820 [details] [diff] [review]: ----------------------------------------------------------------- Ok - r=me. ::: security/manager/ssl/tests/gtest/TLSIntoleranceTest.cpp @@ +20,5 @@ > }; > > TEST_F(TLSIntoleranceTest, Test_Full_Fallback_Process) > { > + helpers.mVersionFallbackLimit = SSL_LIBRARY_VERSION_TLS_1_0; Let's remove this line and assert that we're expecting the fallback limit to be TLS 1.0. @@ +113,5 @@ > SSL_LIBRARY_VERSION_TLS_1_0, > 0)); > } > > TEST_F(TLSIntoleranceTest, Test_Fallback_Limit_Default) Hmmm - I'm not sure this test is testing anything important anymore (i.e. I think Test_Full_Fallback_Process now covers what this test does, particularly if the line asserting what the default limit is gets moved to that test)
Attachment #8569820 - Flags: review?(dkeeler) → review+
We should probably post to dev.platform about this change. :emk, since you're spearheading this, I thought you might want to be the one to tell everyone the good news. If not, I can.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #10) > We should probably post to dev.platform about this change. :emk, since > you're spearheading this, I thought you might want to be the one to tell > everyone the good news. If not, I can. Sent a mail: https://lists.mozilla.org/pipermail/dev-platform/2015-February/008785.html
Blocks: 1137991
Release Note Request (optional, but appreciated) [Why is this notable]: Dropping SSLv3 seems like a notable security change. [Suggested wording]: Removed support for SSLv3 [Links (documentation, blog post, etc)]: Does this warrant a blog post from the sec team?
relnote-firefox: --- → ?
Question for anyone: if this is disabled by default since Fx34, why do we need to remove it entirely? I'm trying to assess the testing impact as well, if any. Thanks.
To remove a footgun. As we removed SSLv2.
Also we can simplify some logic because we can assume SSLv3 will never be negotiated. An example: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSIOLayer.cpp?rev=5801ebeeb3b6&mark=1260-1261,1264-1273#1258
Attachment #8569853 - Flags: review?(vporof) → review+
Depends on: 1141394
Just to make sure, I ran a compatibility pass for this change on builds of Fx39 both before and after the code was removed. I ran the Pulse top sites list. No regressions were found - hence, marking verified.
Status: RESOLVED → VERIFIED
Relnote added with the wording: "Removed support for SSLv3". If there's dev documentation or a blog post on this please need-info me or email me so I can add a link to the release note.
I've confirmed Chrome 44 dropped SSLv3 entirely. https://code.google.com/p/chromium/issues/detail?id=487730
2 things: 1. Removing SSLv3 meant I spent about hours back in July or August trying to connect to a Dell Remote Access Controller (DRAC/MC) for a blade chassis that only supports SSL and not TLS. It used to work in Firefox, but I always had issues with IE already, but then I tried to get it working on IE and Java was a pain about certificates and you have to know the exact URL to add an exeception, which you don't because it's hidden, and these DRACs are pretty unreliable and unresponsive when it comes to connecting to them anyway - so thanks for wasting my time by removing SSLv3 rather than just leaving it disabled by default. (And, no, Dell are not providing an update for this equipment to support TLS, even though it is not that old.) 2. SSLv3 was removed in Firefox 39.0 (in plain English, for the benefit of people trying to work this out to work out which old or ASR version to download). I'll try Firefox 38.4.0 ESR and see if I can finally connect to this chassis!
As far as I can tell, this didn't check into mozilla-esr38 and thus the current 38.x ESR releases should still support SSL 3.0, so that should be a viable workaround. Inevitably, this issue will come up again for you once support for that branch is no longer provided (in about 30 weeks from now).
(In reply to rsx11m from comment #22) > As far as I can tell, this didn't check into mozilla-esr38 and thus the > current 38.x ESR releases should still support SSL 3.0, so that should be a > viable workaround. Inevitably, this issue will come up again for you once > support for that branch is no longer provided (in about 30 weeks from now). Which, as of the time of writing, is fast approaching. The DD-Wrt project still cannot fit TLS onto routers with less than 4MB of storage, of which there are a good number of in operation. The logic of completely disabling SSLv3 for so long as plain HTTP is supported escapes me, unless Mozilla plans to deprecate that too.
> The logic of > completely disabling SSLv3 for so long as plain HTTP is supported escapes > me, unless Mozilla plans to deprecate that too. It appears I spoke too soon: https://blog.mozilla.org/security/2015/04/30/deprecating-non-secure-http/ Lovely.
(In reply to ipatrol from comment #23) > (In reply to rsx11m from comment #22) > > As far as I can tell, this didn't check into mozilla-esr38 and thus the > > current 38.x ESR releases should still support SSL 3.0, so that should be a > > viable workaround. Inevitably, this issue will come up again for you once > > support for that branch is no longer provided (in about 30 weeks from now). > > Which, as of the time of writing, is fast approaching. > > The DD-Wrt project still cannot fit TLS onto routers with less than 4MB of > storage, of which there are a good number of in operation. If you're continuing to use obsolete hardware that requires an obsolete and insecure security protocol that is now 20 years old, it is not exactly a big ask to require an obsolete browser to continue to access it. Have a separate profile for obsolete access and use the last ESR that is capable of doing so, just like we still sadly have the need for old versions of IE for obsolete sites/services in some instances (and my guess is, a router or two). > The logic of > completely disabling SSLv3 for so long as plain HTTP is supported escapes > me, unless Mozilla plans to deprecate that too. It's a 20 year old obsolete insecure security protocol, with shockingly wide continued support on servers where continuing to play along is downright dangerous. It needs to be nuked from orbit and everyone needs to finally join the 21st century. The same is true for insecure HTTP, but that is a much much longer road. Further comments here are not productive. If you wish to discuss interop with obsolete systems, please do so on a Mozilla forum/group/mailinglist and not here (it's a legitimate topic of discussion, but this issue here is closed). If you have a specific RFE or other issue that is appropriate for this system, please file a new bug (taking into account that nobody will be re-adding support for SSL).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: