If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove dead code that handles RC4 fallback

RESOLVED FIXED in Firefox 53

Status

()

Core
Security: PSM
RESOLVED FIXED
3 years ago
7 months ago

People

(Reporter: emk, Assigned: emk)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [psm-blocked])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Followup to bug 1124039. I didn't cleanup in bug 1124039 to make the change minimal.
(Assignee)

Comment 1

3 years ago
Created attachment 8562328 [details] [diff] [review]
patch
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

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=badada465f3b
(Assignee)

Comment 4

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9507662057de
(Assignee)

Comment 5

3 years ago
Backed out because bug 1124039 has been backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee830b93a552
https://hg.mozilla.org/mozilla-central/rev/9507662057de
Assignee: nobody → VYV03354
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
https://hg.mozilla.org/mozilla-central/rev/ee830b93a552
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

a year 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

10 months 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

10 months 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

10 months 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

10 months 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

10 months ago
Summary: Remove dead code that tracks strongCipherStatus → Remove dead code that handles RC4 fallback
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8813568 - Attachment is obsolete: true
Attachment #8813568 - Flags: review?(dolske)
(Assignee)

Updated

10 months ago
Blocks: 1321778
Comment hidden (mozreview-request)

Comment 19

10 months ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/1f7832017dbb
Remove vestigial RC4 fallback backend. r=keeler

Comment 20

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1f7832017dbb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago10 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 21

8 months 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 months ago
Flags: needinfo?(VYV03354)
Duplicate of this bug: 1345869
You need to log in before you can comment on or make changes to this bug.