Closed Bug 1138882 Opened 9 years ago Closed 9 years ago

Create a separate a pref to enable unrestricted RC4 fallback.

Categories

(Core :: Security: PSM, defect)

37 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 + fixed
firefox39 --- fixed

People

(Reporter: emk, Assigned: emk)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

Attached patch unrestricted_rc4_fallback (obsolete) — Splinter Review
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.
Attachment #8571906 - Flags: review?(dkeeler)
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.)
Attachment #8571906 - Flags: review?(dkeeler) → review+
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/5edc84f917cd
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/5edc84f917cd
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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
Attachment #8571906 - Flags: approval-mozilla-aurora?
Attached patch patch for auroraSplinter Review
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
Attachment #8575556 - Flags: review?(dkeeler)
Attachment #8575556 - Flags: approval-mozilla-aurora?
Attachment #8571906 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8575556 - Flags: review?(dkeeler) → review+
Comment on attachment 8571906 [details] [diff] [review]
unrestricted_rc4_fallback

The second patch include this. We don't have to uplift both.
Attachment #8571906 - Attachment is obsolete: true
Attachment #8575556 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.