1128227 - Add a static TLS insecure fallback whitelist

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: static_fallback_whitelist

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

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: patch v2

Added more test domains.

Comment 2

[Masatoshi Kimura [:emk]]

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

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: patch v3

* Added sites from bug 1128366 and 1128318.
* Added some known sites that require RC4.
* Do not swallow exceptions on failure in genIntolerantFallbackList.js.

Comment 4

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

(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 5

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

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.

Comment 6

[Masatoshi Kimura [:emk]]

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

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: patch v4

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

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: patch v4.1

Constify kIntolerantFallbackList.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 10

[Masatoshi Kimura [:emk]]

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

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: patch v5

* Resolved feedback comments from Brian.
* Removed the following now TLS 1.2 tolerant sites: shop.sekaibunka.com, www.saiyo-dr.jp, www.stocktradersalmanac.com.

Comment 12

[Masatoshi Kimura [:emk]]

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

Comment 13

[Dave Garrett]

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

Comment 14

[Masatoshi Kimura [:emk]]

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.

Comment 15

[Masatoshi Kimura [:emk]]

Ah, special-casing only "www."? It would be worth a try.

Comment 16

[Masatoshi Kimura [:emk]]

Attachment: whitelist update

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

Comment 17

[Dave Garrett]

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 18

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

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.

Comment 19

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

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.

Comment 20

[Masatoshi Kimura [:emk]]

Attachment: Folded patch for checkin

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.

Comment 21

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/508aab34b220

Comment 22

[Masatoshi Kimura [:emk]]

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

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/508aab34b220

Comment 24

[Masatoshi Kimura [:emk]]

Attachment: bug 1114816+1128763+1128227 combined patch for aurora

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

Comment 25

[Lawrence Mandel [:lmandel] (use needinfo)]

(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?

Comment 26

[Masatoshi Kimura [:emk]]

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

Comment 27

[Masatoshi Kimura [:emk]]

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

Comment 28

[Masatoshi Kimura [:emk]]

s/aurora always/aurora already/;

Comment 29

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 30

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e9694bbffaa

Comment 32

[Masatoshi Kimura [:emk]]

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

Comment 33

[Dave Garrett]

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

Comment 34

[Masatoshi Kimura [:emk]]

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

Comment 35

[Yuhong Bao]

developer.palm.com: have HP been notified?

Comment 36

[Masatoshi Kimura [:emk]]

(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

[Yuhong Bao]

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