Closed Bug 1128227 Opened 6 years ago Closed 6 years ago

Add a static TLS insecure fallback whitelist

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox35 --- unaffected
firefox36 --- unaffected
firefox37 + fixed
firefox38 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 7 obsolete files)

Attached 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)
Attached patch patch v2 (obsolete) — Splinter Review
Added more test domains.
Attachment #8557611 - Flags: review?(dkeeler)
Attachment #8557557 - Attachment is obsolete: true
Attachment #8557557 - Flags: review?(dkeeler)
Depends on: 1128318
Blocks: 1084025
Attached 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)
(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.
Depends on: 1128930
Attached 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
Attached patch patch v4.1 (obsolete) — Splinter Review
Constify kIntolerantFallbackList.
Attachment #8558496 - Attachment is obsolete: true
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+
Blocks: 1129773
(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.
Summary: Make the TLS insecure fallback whitelist static → Add a static TLS insecure fallback whitelist
Blocks: 1128318
Depends on: 1128763
No longer depends on: 1128318
Attached 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 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/ )
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.
Ah, special-casing only "www."? It would be worth a try.
Attached 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)
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+
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/508aab34b220
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Flags: in-testsuite+
(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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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)
(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)
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).
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+
(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)
(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.
(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.
Blocks: 1132399
developer.palm.com: have HP been notified?
(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.
Yes, seeing this in the whitelist is why I asked.
Blocks: 1136091
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.