Closed
Bug 1130670
Opened 10 years ago
Closed 8 years ago
Remove dead code that handles RC4 fallback
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: emk, Assigned: emk)
References
Details
(Whiteboard: [psm-blocked])
Attachments
(2 files, 1 obsolete file)
Followup to bug 1124039. I didn't cleanup in bug 1124039 to make the change minimal.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8562328 -
Flags: review?(dkeeler)
Comment on attachment 8562328 [details] [diff] [review]
patch
Review of attachment 8562328 [details] [diff] [review]:
-----------------------------------------------------------------
Great - r=me.
Attachment #8562328 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Backed out because bug 1124039 has been backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee830b93a552
Comment 6•10 years ago
|
||
Assignee: nobody → VYV03354
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 7•10 years ago
|
||
Status: RESOLVED → REOPENED
status-firefox38:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla38 → ---
I think we'll be able to do this after bug 1268728 has shipped.
Depends on: 1268728
Whiteboard: [psm-blocked]
Assignee | ||
Comment 9•8 years ago
|
||
Bug 1113974 is going to reuse strongCipherStatus.
However, I consider to remove this and offer fallback cipher suites only if we offer TLS <=1.2 when we enable TLS 1.3 by default. Fallback cipher suites are unusable with TLS 1.3 anyway.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•8 years ago
|
||
Bug 1268728 has been shipped, bug 1113974 has been WONTFIXed, and TLS 1.3 has been enabled by default. IMO there is no reason to hold back his bug anymore.
I'm requesting a review again because this patch will remove more code than the previous patch.
Status: REOPENED → ASSIGNED
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8812416 [details]
Bug 1130670 - Remove vestigial RC4 fallback backend.
https://reviewboard.mozilla.org/r/94178/#review94720
Looks good, but I think there are some additional things we should remove at the same time.
I think we can remove the fallback cipher-related code in HandshakeCallback in nsNSSCallbacks.cpp as well, right? (IIRC, that was part of considering DHE weak, which we don't do anymore - it should probably go back to more like what it was before bug 1284840.)
Looks like there is some browser UI we can remove as well:
https://dxr.mozilla.org/mozilla-central/rev/b7f895c1dc2e91530240efbf50ac063a0f8a9cb5/browser/base/content/browser.js#7067
https://dxr.mozilla.org/mozilla-central/rev/b7f895c1dc2e91530240efbf50ac063a0f8a9cb5/browser/base/content/browser.js#2828
(and related...)
::: security/manager/ssl/nsNSSComponent.cpp:1322
(Diff revision 1)
>
> // Update the switch statement in AccumulateCipherSuite in nsNSSCallbacks.cpp
> // when you add/remove cipher suites here.
> static const CipherPref sCipherPrefs[] = {
> { "security.ssl3.ecdhe_rsa_aes_128_gcm_sha256",
> - TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, true },
> + TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 },
I believe these "true" parameters are actually the values of "enabledByDefault", which we want to keep as true.
Attachment #8812416 -
Flags: review?(dkeeler)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8812416 [details]
Bug 1130670 - Remove vestigial RC4 fallback backend.
https://reviewboard.mozilla.org/r/94178/#review94720
I removed usesFallbackCipher and addInsecureFallbackSite that was only used from WeakCryptoOverride.
And I added a new changeset to remove some dead code from the frontend.
> I believe these "true" parameters are actually the values of "enabledByDefault", which we want to keep as true.
Good catch. Reverted the removal.
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8812416 [details]
Bug 1130670 - Remove vestigial RC4 fallback backend.
https://reviewboard.mozilla.org/r/94178/#review96170
Great! r=me
Attachment #8812416 -
Flags: review?(dkeeler) → review+
Assignee | ||
Updated•8 years ago
|
Summary: Remove dead code that tracks strongCipherStatus → Remove dead code that handles RC4 fallback
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8813568 -
Attachment is obsolete: true
Attachment #8813568 -
Flags: review?(dolske)
Comment hidden (mozreview-request) |
Comment 19•8 years ago
|
||
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/1f7832017dbb
Remove vestigial RC4 fallback backend. r=keeler
Comment 20•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 21•8 years ago
|
||
This appeared to cause a rather startling increase in PR_CONNECT_RESET_ERROR being accumulated by SSL_TLS12_INTOLERANCE_REASON_PRE. Is this correct? Expected? Benign?
Alert: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/625/alerts/?from=2016-12-03&to=2016-12-03
Changelog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f65ad27efe839ce9df0283840a1a40b4bbc9ead0&tochange=557548714db55136b51e1129d649e2599797985f
Evolution: https://mzl.la/2kZwl9L
Flags: needinfo?(dkeeler)
Flags: needinfo?(VYV03354)
Thanks for following up on this, Chris. I believe this is due to the following removal from security/manager/ssl/nsNSSIOLayer.cpp:
@@ -1125,38 +1084,16 @@ retryDueToTLSIntolerance(PRErrorCode err
tlsIntoleranceTelemetryBucket(originalReason));
helpers.forgetIntolerance(socketInfo->GetHostName(),
socketInfo->GetPort());
return false;
}
- // Disallow PR_CONNECT_RESET_ERROR if fallback limit reached.
- bool fallbackLimitReached =
- helpers.fallbackLimitReached(socketInfo->GetHostName(), range.max);
- if (err == PR_CONNECT_RESET_ERROR && fallbackLimitReached) {
- return false;
- }
-
- if ((err == SSL_ERROR_NO_CYPHER_OVERLAP || err == PR_END_OF_FILE_ERROR ||
- err == PR_CONNECT_RESET_ERROR) &&
- nsNSSComponent::AreAnyWeakCiphersEnabled()) {
- if (helpers.isInsecureFallbackSite(socketInfo->GetHostName()) ||
- helpers.mUnrestrictedRC4Fallback) {
- if (helpers.rememberStrongCiphersFailed(socketInfo->GetHostName(),
- socketInfo->GetPort(), err)) {
- Telemetry::Accumulate(Telemetry::SSL_WEAK_CIPHERS_FALLBACK,
- tlsIntoleranceTelemetryBucket(err));
- return true;
- }
- Telemetry::Accumulate(Telemetry::SSL_WEAK_CIPHERS_FALLBACK, 0);
- }
- }
-
// When not using a proxy we'll see a connection reset error.
// When using a proxy, we'll see an end of file error.
// Don't allow STARTTLS connections to fall back on connection resets or
// EOF.
if ((err == PR_CONNECT_RESET_ERROR || err == PR_END_OF_FILE_ERROR)
&& socketInfo->GetForSTARTTLS()) {
return false;
Note that previously we would return early if the error was PR_CONNECT_RESET_ERROR and we weren't going to fall back due to reaching the fallback limit. Without this code, we still don't fall back for PR_CONNECT_RESET_ERROR if we've reached the fallback limit, but we do collect telemetry on it ( https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/security/manager/ssl/nsNSSIOLayer.cpp#1137 ). In short, I think this is an expected change (and the code is more correct than it was before the change).
Flags: needinfo?(dkeeler)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(VYV03354)
You need to log in
before you can comment on or make changes to this bug.
Description
•