1130670 - Remove dead code that handles RC4 fallback

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Masatoshi Kimura [:emk]]

Followup to bug 1124039. I didn't cleanup in bug 1124039 to make the change minimal.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: patch

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8562328 [details] [diff] [review]
patch

Review of attachment 8562328 [details] [diff] [review]:
-----------------------------------------------------------------

Great - r=me.

Comment 3

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=badada465f3b

Comment 4

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9507662057de

Comment 5

[Masatoshi Kimura [:emk]]

Backed out because bug 1124039 has been backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee830b93a552

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9507662057de

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee830b93a552

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I think we'll be able to do this after bug 1268728 has shipped.

Comment 9

[Masatoshi Kimura [:emk]]

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 10

[Masatoshi Kimura [:emk]]

Attachment: Bug 1130670 - Remove vestigial RC4 fallback backend.

Review commit: https://reviewboard.mozilla.org/r/94178/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94178/

Comment 11

[Masatoshi Kimura [:emk]]

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.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: Bug 1130670 - Remove vestigial RC4 fallback frontend.

Review commit: https://reviewboard.mozilla.org/r/95014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95014/

Comment 14

[Masatoshi Kimura [:emk]]

Comment on attachment 8812416 [details]
Bug 1130670 - Remove vestigial RC4 fallback backend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94178/diff/1-2/

Comment 15

[Masatoshi Kimura [:emk]]

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

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8812416 [details]
Bug 1130670 - Remove vestigial RC4 fallback backend.

https://reviewboard.mozilla.org/r/94178/#review96170

Great! r=me

Comment 17

[Masatoshi Kimura [:emk]]

Comment on attachment 8812416 [details]
Bug 1130670 - Remove vestigial RC4 fallback backend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94178/diff/2-3/

Comment 18

[Masatoshi Kimura [:emk]]

Comment on attachment 8812416 [details]
Bug 1130670 - Remove vestigial RC4 fallback backend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94178/diff/3-4/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f7832017dbb
Remove vestigial RC4 fallback backend. r=keeler

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1f7832017dbb

Comment 21

[Chris H-C :chutten]

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

Comment 22

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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).