Currently, only one pref ("security.tls.version.fallback-limit") affects both of version fallback and RC4 fallback. If we decided to reenable RC4 fallback because of too many broken sites, we will also have to reenable version fallback. This patch will give finer control of the fallback.

Comment on attachment 8571906 [details] [diff] [review] unrestricted_rc4_fallback Review of attachment 8571906 [details] [diff] [review]: ----------------------------------------------------------------- Looks good - r=me with comment addressed. Also, it would be a good idea to add a simple test for this. ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +1244,5 @@ > > if ((err == SSL_ERROR_NO_CYPHER_OVERLAP || err == PR_END_OF_FILE_ERROR || > err == PR_CONNECT_RESET_ERROR) && > + (helpers.mUnrestrictedRC4Fallback || > + helpers.isInsecureFallbackSite(socketInfo->GetHostName())) && Just to verify, my understanding is that this change would make it so we wouldn't even use RC4 if the fallback limit has been set to a lower-than-default value when attempting to visit a non-whitelisted site. Is that intended? (In other words, I think we still want to use !fallbackLimitReached here, unless the aim is to really only allow RC4 for whitelisted sites and if the user has flipped the new pref.)

Why is this a global pref rather than a per-site override (like for certs)?

(In reply to Jesse Ruderman from comment #2) > Why is this a global pref rather than a per-site override (like for certs)? The per-site ovverride already exists: "security.tls.insecure_fallback_hosts". I have no plan to separate this list to version fallback exception list and RC4 fallback exception list. Both list have a fair amount of overlap and a list is memory-consuming. Moreover, users will have to know exactly why Firefox fails to connect the server if two lists are present.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #1) > Looks good - r=me with comment addressed. Also, it would be a good idea to > add a simple test for this. Added a test to verify that the pref works. > ::: security/manager/ssl/src/nsNSSIOLayer.cpp > @@ +1244,5 @@ > > > > if ((err == SSL_ERROR_NO_CYPHER_OVERLAP || err == PR_END_OF_FILE_ERROR || > > err == PR_CONNECT_RESET_ERROR) && > > + (helpers.mUnrestrictedRC4Fallback || > > + helpers.isInsecureFallbackSite(socketInfo->GetHostName())) && > > Just to verify, my understanding is that this change would make it so we > wouldn't even use RC4 if the fallback limit has been set to a > lower-than-default value when attempting to visit a non-whitelisted site. Is > that intended? (In other words, I think we still want to use > !fallbackLimitReached here, unless the aim is to really only allow RC4 for > whitelisted sites and if the user has flipped the new pref.) Yes, it was my original intention. But I had second thought and changed it back to !fallbackLimitReached. It should make no difference for the purpose of comment #0 (re-enabling RC4 without enabling the version fallback). https://treeherder.mozilla.org/#/jobs?repo=try&revision=573e79baef75

Comment on attachment 8571906 [details] [diff] [review] unrestricted_rc4_fallback Approval Request Comment [Feature/regressing bug #]: N/A [User impact if declined]: We will have fewer options to deal with the compatibility problems with legacy servers. [Describe test coverage new/current, TreeHerder]: has a test [Risks and why]: very low, default-disabled boolean pref [String/UUID change made/needed]: none

Based on a compatibility test from bug 1124039 comment 58, I decided to enable this pref by default on aurora. Approval Request Comment [Feature/regressing bug #]: 1124039 [User impact if declined]: Many sites are inaccessible. [Describe test coverage new/current, TreeHerder]: Landed in this bug. [Risks and why]: Low, adding a boolean pref [String/UUID change made/needed]: none

Approving for Aurora and tracking, will wait for the review on the second patch before approving.

Comment on attachment 8575556 [details] [diff] [review] patch for aurora Review of attachment 8575556 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. We should update the docs that were added for bug 1124039.

Regarding dev-doc-needed: see comment 10.

Comment on attachment 8571906 [details] [diff] [review] unrestricted_rc4_fallback The second patch include this. We don't have to uplift both.

Updated: https://developer.mozilla.org/en-US/Firefox/Releases/38#Security