Closed Bug 1130670 Opened 5 years ago Closed 3 years ago

Remove dead code that handles RC4 fallback

Categories

(Core :: Security: PSM, defect)

defect
Not set

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.
Attached patch patchSplinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/9507662057de
Assignee: nobody → VYV03354
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
https://hg.mozilla.org/mozilla-central/rev/ee830b93a552
Status: RESOLVED → REOPENED
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]
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.
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 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 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 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+
Summary: Remove dead code that tracks strongCipherStatus → Remove dead code that handles RC4 fallback
Attachment #8813568 - Attachment is obsolete: true
Attachment #8813568 - Flags: review?(dolske)
Blocks: 1321778
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/1f7832017dbb
Remove vestigial RC4 fallback backend. r=keeler
https://hg.mozilla.org/mozilla-central/rev/1f7832017dbb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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)
Flags: needinfo?(VYV03354)
You need to log in before you can comment on or make changes to this bug.