Add a static TLS insecure fallback whitelist

RESOLVED FIXED in Firefox 37

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

({dev-doc-needed})

unspecified
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox35 unaffected, firefox36 unaffected, firefox37+ fixed, firefox38 fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

4 years ago
Posted patch static_fallback_whitelist (obsolete) — Splinter Review
I made the insecure fallback site list static and added most sites from bug 1084025 comment #96 to minimize the compatibility concern. This list is still shorter than the HSTS preload list and it will only shrink.

I excluded the following sites from the list because Firefox could connect even without the list:
jobs.healthcaresource.com, service.gojokai.com, sps.acc.senshu-u.ac.jp, ssl.ptotjinzaibank.com, www.carejinzaibank.com, www.healthcaresource.com, www.usaswimming.org

and the following sites because Ican not access even with the whitelist nor with Chrome (please double-check):
blogs.luc.edu, login.chicagopolice.org, www.ala.org, www.araggroup.com, www.araglegalcenter.com, www.bbsfonline.com, www.mynpcdata.net
Attachment #8557557 - Flags: review?(dkeeler)
(Assignee)

Comment 1

4 years ago
Posted patch patch v2 (obsolete) — Splinter Review
Added more test domains.
Attachment #8557611 - Flags: review?(dkeeler)
(Assignee)

Updated

4 years ago
Attachment #8557557 - Attachment is obsolete: true
Attachment #8557557 - Flags: review?(dkeeler)
(Assignee)

Updated

4 years ago
Depends on: 1128318
(Assignee)

Updated

4 years ago
Blocks: 1084025
(Assignee)

Comment 3

4 years ago
Posted patch patch v3 (obsolete) — Splinter Review
* Added sites from bug 1128366 and 1128318.
* Added some known sites that require RC4.
* Do not swallow exceptions on failure in genIntolerantFallbackList.js.
Attachment #8557611 - Attachment is obsolete: true
Attachment #8557611 - Flags: review?(dkeeler)
Attachment #8557914 - Flags: review?(dkeeler)
(In reply to Masatoshi Kimura [:emk] from comment #0)
> and the following sites because Ican not access even with the whitelist nor
> with Chrome (please double-check):
> blogs.luc.edu, login.chicagopolice.org, www.ala.org, www.araggroup.com,
> www.araglegalcenter.com, www.bbsfonline.com, www.mynpcdata.net

Of these, only blogs.luc.edu and www.ala.org fail for me with Chrome 40.0.2214.94 on Fedora.

And, actually, I can load blogs.luc.edu in Nightly.
Comment on attachment 8557914 [details] [diff] [review]
patch v3

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

This patch doesn't apply cleanly for me on mozilla-central. Is there a patch in another bug it depends on? It would be helpful if that could be indicated in the blocks/depends on fields in bugs.
Attachment #8557914 - Flags: review?(dkeeler)
(Assignee)

Comment 6

4 years ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> (In reply to Masatoshi Kimura [:emk] from comment #0)
> > and the following sites because Ican not access even with the whitelist nor
> > with Chrome (please double-check):
> > blogs.luc.edu, login.chicagopolice.org, www.ala.org, www.araggroup.com,
> > www.araglegalcenter.com, www.bbsfonline.com, www.mynpcdata.net
> 
> Of these, only blogs.luc.edu and www.ala.org fail for me with Chrome
> 40.0.2214.94 on Fedora.

Hm, I couldn't still access them except www.bbsfonline.com, but OK, I'll add them back.

> And, actually, I can load blogs.luc.edu in Nightly.

Then whitelisting is not needed anyway.
(Assignee)

Updated

4 years ago
Depends on: 1128930
(Assignee)

Comment 7

4 years ago
Posted patch patch v4 (obsolete) — Splinter Review
* Added a site from bug 1128602.
* Added back sites from comment #0 except blogs.luc.edu and www.ala.org.
* Using mozilla::BinarySearch instead of bsearch.
Attachment #8557914 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Posted patch patch v4.1 (obsolete) — Splinter Review
Constify kIntolerantFallbackList.
Attachment #8558496 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
No longer depends on: 1128930
Comment on attachment 8558505 [details] [diff] [review]
patch v4.1

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

::: security/manager/ssl/src/IntolerantFallbackList.h
@@ +4,5 @@
> +
> +/*****************************************************************************/
> +/* This is an automatically generated file. If you're not                    */
> +/* nsNSSIOLayer.cpp, you shouldn't be #including it.                         */
> +/*****************************************************************************/

Use C++ comments instead of C comments.

Such files should be named with ".inc", not ".h" extensions.

@@ +6,5 @@
> +/* This is an automatically generated file. If you're not                    */
> +/* nsNSSIOLayer.cpp, you shouldn't be #including it.                         */
> +/*****************************************************************************/
> +
> +#include <stdint.h>

Do you still need to include stdint.h?

@@ +9,5 @@
> +
> +#include <stdint.h>
> +#include <mozilla/Array.h>
> +
> +static const mozilla::Array<const char*, 600> kIntolerantFallbackList = {

It would be better to make this "static char const * const kIntolerantFallbackList[]" so that you don't have to give an explicitly length. Note that "600" isn't the correct length.

The brace should go on its own line.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ -1682,5 @@
>        mOwner->loadVersionFallbackLimit();
> -    } else if (prefName.EqualsLiteral("security.tls.insecure_fallback_hosts")) {
> -      nsCString insecureFallbackHosts;
> -      Preferences::GetCString("security.tls.insecure_fallback_hosts", &insecureFallbackHosts);
> -      mOwner->setInsecureFallbackSites(insecureFallbackHosts);

You should keep the pref, and consult both the pref and the static list. That way, (1) users can add sites privately, and (2) the hotfix addon can add sites to the list.
Attachment #8558505 - Flags: feedback+
(Assignee)

Updated

4 years ago
Blocks: 1129773
(Assignee)

Comment 10

4 years ago
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #9)
> @@ +9,5 @@
> > +
> > +#include <stdint.h>
> > +#include <mozilla/Array.h>
> > +
> > +static const mozilla::Array<const char*, 600> kIntolerantFallbackList = {
> 
> It would be better to make this "static char const * const
> kIntolerantFallbackList[]" so that you don't have to give an explicitly
> length. Note that "600" isn't the correct length.

Really? 613 - 14 + 1 = 600, and this number is auto-generated by the script. Am I missing something?
Anyway, I'll change this back to a plain array. I (mis)thought mozilla::BinarySearchIf did not work with plain arrays.
(Assignee)

Updated

4 years ago
Summary: Make the TLS insecure fallback whitelist static → Add a static TLS insecure fallback whitelist
(Assignee)

Updated

4 years ago
Blocks: 1128318
Depends on: 1128763
No longer depends on: 1128318
(Assignee)

Comment 11

4 years ago
Posted patch patch v5 (obsolete) — Splinter Review
* Resolved feedback comments from Brian.
* Removed the following now TLS 1.2 tolerant sites: shop.sekaibunka.com, www.saiyo-dr.jp, www.stocktradersalmanac.com.
Attachment #8558505 - Attachment is obsolete: true
Attachment #8559772 - Flags: review?(dkeeler)

Comment 13

4 years ago
Comment on attachment 8559772 [details] [diff] [review]
patch v5

There's a long list of "www.*" domains in there. It might be better to drop that for all entries and check the list for both the exact domain and "www." plus the domain. I'm worried that sites that load up with either form without redirecting to one would be only whitelisted with one. (e.g. http://www.example.com/ & http://example.com/ )
(Assignee)

Comment 14

4 years ago
Certainly some sites in this list have many subdomains (e.g. kuronekoyamato.jp and whatwg.org). One issue is that I'm self-updating the list by comparing the result with or without use_static_list pref enabled. If the list includes all subdomains, it will no longer be possible.
(Assignee)

Comment 15

4 years ago
Ah, special-casing only "www."? It would be worth a try.
(Assignee)

Comment 16

4 years ago
Posted patch whitelist update (obsolete) — Splinter Review
* Added identity.virginmedia.com from bug 1129887.
* Removed home.apu.edu (now TLS 1.2 tolerant).
* Only 69 sites allowed to access with or without "www.". So I added all of them instead of complicating the logic further.
Attachment #8560463 - Flags: review?(dkeeler)

Comment 17

4 years ago
There are 22 entries in the list for domains of the form bookstore.*.edu. If someone could identify the software they all use for their book stores, then it might be fixable by contacting the vendor rather than the schools.

(In reply to Masatoshi Kimura [:emk] from comment #16)
> * Only 69 sites allowed to access with or without "www.". So I added all of
> them instead of complicating the logic further.

Sounds good. Thanks.
Comment on attachment 8559772 [details] [diff] [review]
patch v5

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

This is great - r=me with comments addressed.

::: security/manager/ssl/src/IntolerantFallbackList.inc
@@ +122,5 @@
> +  "event.kasite.net",
> +  "extra.ytk.fi",
> +  "extranet.eurocontrol.int",
> +  "ez.cityofchesapeake.net",
> +  "fallback.test", // Used by gtest

We should use something like fallback-test.example.com instead.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +1778,5 @@
>    nsCString insecureFallbackHosts;
>    Preferences::GetCString("security.tls.insecure_fallback_hosts", &insecureFallbackHosts);
>    setInsecureFallbackSites(insecureFallbackHosts);
> +  mUseStaticFallbackList =
> +    Preferences::GetBool("security.tls.insecure_fallback_hosts.use_static_list", true);

Looks like we need calls to Preferences::AddStrongObserver / Preferences::RemoveObserver for this pref (or does this just work because security.tls.insecure_fallback_hosts is a substring of this pref? That seems fragile if that's the case.)

::: security/manager/ssl/src/nsNSSIOLayer.h
@@ -234,5 @@
>    void setInsecureFallbackSites(const nsCString& str);
>    bool isInsecureFallbackSite(const nsACString& hostname);
>  
>    bool mFalseStartRequireNPN;
> -  bool mFalseStartRequireForwardSecrecy;

Huh - looks like this was unused. I'm surprised there aren't compiler warnings about it (or maybe there are, but we're ignoring them...)

::: security/manager/ssl/tests/gtest/TLSIntoleranceTest.cpp
@@ +599,5 @@
> +
> +TEST_F(TLSIntoleranceTest, TLS_Static_Fallback_List)
> +{
> +  NS_NAMED_LITERAL_CSTRING(fallback_test, "fallback.test");
> +  NS_NAMED_LITERAL_CSTRING(no_fallback_test, "no.fallback.test");

Let's use fallback-test.example.com and no-fallback-test.example.com, respectively.

::: security/manager/tools/genIntolerantFallbackList.js
@@ +15,5 @@
> +const { FileUtils } = Cu.import("resource://gre/modules/FileUtils.jsm", {});
> +const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
> +const { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> +
> +const SEC_ERROR_UNKNOWN_ISSUER = 0x805a1ff3;

It might be worth it to use nsINSSErrorsService.isNSSErrorCode(...) && nsINSSErrorsService.getErrorClass(...) == nsINSSErrorsService.ERROR_CLASS_BAD_CERT (i.e. that would be true for all overridable errors)

@@ +25,5 @@
> +const REQUEST_TIMEOUT = 30 * 1000;
> +const TEST_DOMAINS = {
> +  "fallback.test": true,
> +  "rc4.example.com": true,
> +  "ssl3rc4.example.com": true,

rc4.example.com and ssl3rc4.example.com aren't on the built-in list, right? They shouldn't need to be here if I'm understanding how TEST_DOMAINS works.

@@ +68,5 @@
> +  // get the fallback status of each host without whitelist
> +  let fallbackStatuses = [];
> +  getFallbackStatuses(gIntolerantFallbackList, fallbackStatuses);
> +
> +  // reesable the current fallback list

typo: reenable

@@ +96,5 @@
> +  throw e;
> +}
> +
> +function readCurrentList(filename) {
> +  var currentHosts = [];

nit: let, not var

@@ +219,5 @@
> +  }
> +}
> +
> +function shouldRetry(response) {
> +  return false;//(response.error != Cr.NS_OK && response.retries > 0);

Does this script not do retries? If so, we could remove the whole retry infrastructure to simplify it.
Attachment #8559772 - Flags: review?(dkeeler) → review+
Comment on attachment 8560463 [details] [diff] [review]
whitelist update

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

You might re-run the script again. There's at least one more site that now appears to work.
Attachment #8560463 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 20

4 years ago
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38b4839a6152

(In reply to David Keeler [:keeler] (use needinfo?) from comment #18)
> ::: security/manager/ssl/src/IntolerantFallbackList.inc
> @@ +122,5 @@
> > +  "event.kasite.net",
> > +  "extra.ytk.fi",
> > +  "extranet.eurocontrol.int",
> > +  "ez.cityofchesapeake.net",
> > +  "fallback.test", // Used by gtest
> 
> We should use something like fallback-test.example.com instead.

".test" is a reserved top level domain just like "example.com" (see RFC 2606). I avoided "example.com" because it has an actual server and the whitelist entry will be included in the shipped binary (not test only).

> ::: security/manager/ssl/src/nsNSSIOLayer.cpp
> @@ +1778,5 @@
> >    nsCString insecureFallbackHosts;
> >    Preferences::GetCString("security.tls.insecure_fallback_hosts", &insecureFallbackHosts);
> >    setInsecureFallbackSites(insecureFallbackHosts);
> > +  mUseStaticFallbackList =
> > +    Preferences::GetBool("security.tls.insecure_fallback_hosts.use_static_list", true);
> 
> Looks like we need calls to Preferences::AddStrongObserver /
> Preferences::RemoveObserver for this pref (or does this just work because
> security.tls.insecure_fallback_hosts is a substring of this pref? That seems
> fragile if that's the case.)

Yes, pref observer will notify of changes for pref names starting with "security.tls.insecure_fallback_hosts". Also, genIntolerantFallbackList.js changes this pref and it works as evidence.

> ::: security/manager/ssl/tests/gtest/TLSIntoleranceTest.cpp
> @@ +599,5 @@
> > +
> > +TEST_F(TLSIntoleranceTest, TLS_Static_Fallback_List)
> > +{
> > +  NS_NAMED_LITERAL_CSTRING(fallback_test, "fallback.test");
> > +  NS_NAMED_LITERAL_CSTRING(no_fallback_test, "no.fallback.test");
> 
> Let's use fallback-test.example.com and no-fallback-test.example.com,
> respectively.

See above.

> ::: security/manager/tools/genIntolerantFallbackList.js
> @@ +15,5 @@
> > +const { FileUtils } = Cu.import("resource://gre/modules/FileUtils.jsm", {});
> > +const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
> > +const { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> > +
> > +const SEC_ERROR_UNKNOWN_ISSUER = 0x805a1ff3;
> 
> It might be worth it to use nsINSSErrorsService.isNSSErrorCode(...) &&
> nsINSSErrorsService.getErrorClass(...) ==
> nsINSSErrorsService.ERROR_CLASS_BAD_CERT (i.e. that would be true for all
> overridable errors)

Sometimes xpcshell fails with SEC_ERROR_UNKNOWN_ISSUER but actual Firefox succeeds to connect the site. I added this check to workaround the discrepancy, not intended to allow overridable errors in general.

> @@ +25,5 @@
> > +const REQUEST_TIMEOUT = 30 * 1000;
> > +const TEST_DOMAINS = {
> > +  "fallback.test": true,
> > +  "rc4.example.com": true,
> > +  "ssl3rc4.example.com": true,
> 
> rc4.example.com and ssl3rc4.example.com aren't on the built-in list, right?
> They shouldn't need to be here if I'm understanding how TEST_DOMAINS works.

Good catch, fixed. It should have been removed since the dynamic pref is restored in v5. Thanks.

Resolved all other comments.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #19)
> You might re-run the script again. There's at least one more site that now
> appears to work.

www.yayoi-kk.co.jp? Removed.
Attachment #8559772 - Attachment is obsolete: true
Attachment #8560463 - Attachment is obsolete: true
Attachment #8560816 - Flags: review+
(Assignee)

Comment 21

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/508aab34b220
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Flags: in-testsuite+
(Assignee)

Comment 22

4 years ago
(In reply to Dave Garrett from comment #17)
> There are 22 entries in the list for domains of the form bookstore.*.edu. If
> someone could identify the software they all use for their book stores, then
> it might be fixable by contacting the vendor rather than the schools.

Filed bug 1130693.
https://hg.mozilla.org/mozilla-central/rev/508aab34b220
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Comment 24

4 years ago
Approval Request Comment
[Feature/regressing bug #]: 1084025
[User impact if declined]: Users cannot access some secure sites.
[Describe test coverage new/current, TreeHerder]: Minimal gtest coverage. Unfortunately our test infrastructure does not support TLS intolerant servers. But the whitelist generater script effectively works as a test.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3773ec71dbb
(note that e10s is not enabled on aurora yet)
[Risks and why]: Low. The patch is seemingly large, the most part is a whitelist definition and a list generater script.
[String/UUID change made/needed]: no
Attachment #8560940 - Flags: approval-mozilla-aurora?
(In reply to Masatoshi Kimura [:emk] from comment #24)
> [User impact if declined]: Users cannot access some secure sites.

With this patch we're hardcoding the insecure fallback for a large number of sites. What is the plan for this list? Will we be working with the sites on the list to remove the need for the insecure fallback at which point we will remove them from the list? Are we simply going to remove the entire list at some point in the future?
Flags: needinfo?(VYV03354)
(Assignee)

Comment 26

4 years ago
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #25)
> With this patch we're hardcoding the insecure fallback for a large number of
> sites. What is the plan for this list? Will we be working with the sites on
> the list to remove the need for the insecure fallback at which point we will
> remove them from the list? Are we simply going to remove the entire list at
> some point in the future?

See bug 1114816 comment 17 about the plan.
Flags: needinfo?(VYV03354)
(Assignee)

Comment 27

4 years ago
Note that aurora always disabled non-secure fallback. If this patch is not uplifted, bug 1084025 must be backed out from aurora. But we should avoid it if at all possible (see bug 1129925 comment #8 for details).
(Assignee)

Comment 28

4 years ago
s/aurora always/aurora already/;
(In reply to Masatoshi Kimura [:emk] from comment #26)
> See bug 1114816 comment 17 about the plan.

The comment does not really contain a plan so much as a number of ideas. We can go forward with the whitelist but I would like to see a plan that addresses the following points

- List owner. Who is responsible for maintenance of the list and making decisions about additions (hopefully none) and removals?
- Expected EOL for the whitelist. This can be time based or based on reaching some other criteria like number of sites that remain on the list.
- Outreach plan. Sites on the list have little incentive to update. How will we provide incentive? Should we disable the whitelist on pre-release channels?
- Review cadence. I would prefer to a review that corresponds with each release in order to remove sites that are fixed from the list.
Flags: needinfo?(VYV03354)
Comment on attachment 8560940 [details] [diff] [review]
bug 1114816+1128763+1128227 combined patch for aurora

I'm approving for Aurora as we are going to want this list to avoid a large number of compat issues in 37. I do want a concrete plan for management of the list as I requested in comment 29. We can figure out management of the list after the list is integrated into the 37 branch.

Aurora+
Attachment #8560940 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 32

4 years ago
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #29)
> - List owner. Who is responsible for maintenance of the list and making
> decisions about additions (hopefully none) and removals?

I'll maintain the list if no other one found.

> - Expected EOL for the whitelist. This can be time based or based on
> reaching some other criteria like number of sites that remain on the list.

No EOL plan at the moment. We have even lists which will basically only grow such as HSTS preload list or public suffix list (as opposed to this list). 

> - Outreach plan. Sites on the list have little incentive to update. How will
> we provide incentive? Should we disable the whitelist on pre-release
> channels?

It was my original plan, but it was declined by the PSM module owner.

> - Review cadence. I would prefer to a review that corresponds with each
> release in order to remove sites that are fixed from the list.

The list generator script will automatically find fixed sites. It already found a few fixed sites in the review cycle. I'll run the script periodically to update the list.
Flags: needinfo?(VYV03354)

Comment 33

4 years ago
(In reply to Masatoshi Kimura [:emk] from comment #32)
> > - Outreach plan. Sites on the list have little incentive to update. How will
> > we provide incentive? Should we disable the whitelist on pre-release
> > channels?
> 
> It was my original plan, but it was declined by the PSM module owner.

After a couple release cycles, an interstitial warning could be added. Users would be forced to click through a page saying that the security of the server is broken and may be insecure. (with no user-facing way to disable) Sites could still be visited, but hopefully dense companies will notice or get enough user complaints to get their act together.
(Assignee)

Comment 34

4 years ago
(In reply to Dave Garrett from comment #33)
> After a couple release cycles, an interstitial warning could be added. Users
> would be forced to click through a page saying that the security of the
> server is broken and may be insecure. (with no user-facing way to disable)
> Sites could still be visited, but hopefully dense companies will notice or
> get enough user complaints to get their act together.

It was declined by a Firefox UI peer (bug 1120942). After all, I had no other choice but to introduce a larger whitelist.

Updated

4 years ago
Blocks: 1132399

Comment 35

4 years ago
developer.palm.com: have HP been notified?
(Assignee)

Comment 36

4 years ago
(In reply to Yuhong Bao from comment #35)
> developer.palm.com: have HP been notified?

Please file an evangelism bug.
Incidentally, it's already in the whitelist and works for me with the latest Nightly.

Comment 37

4 years ago
Yes, seeing this in the whitelist is why I asked.

Updated

4 years ago
Blocks: 1136091
(Assignee)

Updated

4 years ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.