Masatoshi Kimura [:emk] Assignee
	Description • 10 years ago

      No description provided.

	Masatoshi Kimura [:emk] Assignee
	Comment 1 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c4c98313259

	Masatoshi Kimura [:emk] Assignee
	Comment 3 • 9 years ago

Enclosing it with |#ifdef RELEASE_BUILD| so that site owners can verify their fix using Firefox DevEdition.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 4 • 9 years ago

I suggest that you do things a simpler way: Create a preference that contains a list of domain names ("foo.com,bar.com,baz.com"). Then, load that preference into a hash set. When determining the fallback limit, if the hostname is in the hash set, then set the fallback limit to "TLS 1.0." In particular, I don't think the extra complexity of the per-hostname limit is justified.

	Masatoshi Kimura [:emk] Assignee
	Comment 5 • 9 years ago

I also fixed a bug around mRenegoUnrestrictedSites (it was not cleared when "security.ssl.renego_unrestricted_hosts" was changed from non-empty to empty).

	Masatoshi Kimura [:emk] Assignee
	Comment 7 • 9 years ago

Fixed a build error on gcc.

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

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 8 • 9 years ago

Comment on attachment 8550833 [details] [diff] [review]
Implement TLS intolerance fallback whitelist, v2.1

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

Looks good, but there are some changes we should make. r- for now.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +1673,5 @@
>  
>      if (prefName.EqualsLiteral("security.ssl.renego_unrestricted_hosts")) {
>        nsCString unrestricted_hosts;
>        Preferences::GetCString("security.ssl.renego_unrestricted_hosts", &unrestricted_hosts);
> +      mOwner->setSiteList(mOwner->mRenegoUnrestrictedSites, unrestricted_hosts);

As long as we're touching these lines, I would prefer this be fixed to use the updated style of "unrestrictedHosts".

@@ +1689,5 @@
>                               FALSE_START_REQUIRE_NPN_DEFAULT);
>      } else if (prefName.EqualsLiteral("security.tls.version.fallback-limit")) {
>        mOwner->loadVersionFallbackLimit();
> +    } else if (prefName.EqualsLiteral("security.tls.version.intolerant_hosts")) {
> +      nsCString intolerant_hosts;

nit: intolerantHosts
Although, I think we can use more descriptive names - I might call this "insecureFallbackHosts" and the pref "security.tls.insecure_fallback_hosts".

@@ +1780,3 @@
>    nsCString unrestricted_hosts;
>    Preferences::GetCString("security.ssl.renego_unrestricted_hosts", &unrestricted_hosts);
> +  setSiteList(mRenegoUnrestrictedSites, unrestricted_hosts);

Same here.

@@ +1792,5 @@
>    mFalseStartRequireNPN =
>      Preferences::GetBool("security.ssl.false_start.require-npn",
>                           FALSE_START_REQUIRE_NPN_DEFAULT);
>    loadVersionFallbackLimit();
> +  nsCString intolerant_hosts;

Same here.

@@ +1831,3 @@
>    if (mRenegoUnrestrictedSites) {
>      delete mRenegoUnrestrictedSites;
>      mRenegoUnrestrictedSites = nullptr;

I don't think clearStoredData should be clearing mRenegoUnrestrictedSites, actually (it happens when exiting private browsing, but since mRenegoUnrestrictedSites isn't private/non-private specific, it doesn't need to be cleared).

::: security/manager/ssl/src/nsNSSIOLayer.h
@@ +208,5 @@
>        MOZ_ASSERT(intolerant == 0 || tolerant < intolerant);
>      }
>    };
>    nsDataHashtable<nsCStringHashKey, IntoleranceEntry> mTLSIntoleranceInfo;
> +  nsTHashtable<nsCStringHashKey>* mIntolerantSites;

This doesn't need to be a pointer, and neither does mRenegoUnrestrictedSites.
Also, let's add a little documentation:

// Sites that require insecure fallback to TLS 1.0, set by the pref
// security.tls.insecure_fallback_hosts, which is a comma-delimited
// list of domain names.

Again, I think we can use a more descriptive name than "mIntolerantSites" - how about "mInsecureFallbackSites"?

@@ +232,4 @@
>    bool isRenegoUnrestrictedSite(const nsCString& str);
>    void clearStoredData();
>    void loadVersionFallbackLimit();
> +  void setIntolerantSites(const nsCString& str)

I'm not sure this helper is that helpful - calling setSiteList(mIntolerantSites, str); is basically the same length.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 9 • 9 years ago

Comment on attachment 8550818 [details] [diff] [review]
Add known broken sites to TLS intolerance fallback whitelist, v2

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

Isn't RELEASE_BUILD defined for all releases? What's the benefit of having it not defined for debug/local/etc. builds?

	Masatoshi Kimura [:emk] Assignee
	Comment 10 • 9 years ago

I folded two patches.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> ::: security/manager/ssl/src/nsNSSIOLayer.cpp
> @@ +1831,3 @@
> >    if (mRenegoUnrestrictedSites) {
> >      delete mRenegoUnrestrictedSites;
> >      mRenegoUnrestrictedSites = nullptr;
> 
> I don't think clearStoredData should be clearing mRenegoUnrestrictedSites,
> actually (it happens when exiting private browsing, but since
> mRenegoUnrestrictedSites isn't private/non-private specific, it doesn't need
> to be cleared).

PrivateSSLState() will have a separate instance of nsNSSIOLayerHelpers which will contain mRenegoUnrestrictedSites. It will be reinitialized when a private window is opened again. So clearing it will reduce a bit of footprint during no private windows are open.
I added a code that clears mIntolerantSites (now mInsecureFallbackSites) for the same reason.

> ::: security/manager/ssl/src/nsNSSIOLayer.h
> @@ +232,4 @@
> >    bool isRenegoUnrestrictedSite(const nsCString& str);
> >    void clearStoredData();
> >    void loadVersionFallbackLimit();
> > +  void setIntolerantSites(const nsCString& str)
> 
> I'm not sure this helper is that helpful - calling
> setSiteList(mIntolerantSites, str); is basically the same length.

PrefObserver::Observe can't access mInsecureFallbackSites without this helper because mInsecureFallbackSites is private.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #9)
> Isn't RELEASE_BUILD defined for all releases? What's the benefit of having
> it not defined for debug/local/etc. builds?

RELEASE_BUILD will be defined for all builds whose version number does not contain the letter "a".
https://mxr.mozilla.org/mozilla-central/source/js/src/configure.in#2762
Debug/local/etc. are irrelevant. For our official builds, it will be defined on Release/Beta/ESR and will not be defined on Nightly/DevEdition.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 11 • 9 years ago

(In reply to Masatoshi Kimura [:emk] from comment #10)
> PrivateSSLState() will have a separate instance of nsNSSIOLayerHelpers which
> will contain mRenegoUnrestrictedSites. It will be reinitialized when a
> private window is opened again. So clearing it will reduce a bit of
> footprint during no private windows are open.
> I added a code that clears mIntolerantSites (now mInsecureFallbackSites) for
> the same reason.

Hmmm - seems like that data should be shared so we don't ever have two copies of it anyway, but that's probably something for another bug.

> > ::: security/manager/ssl/src/nsNSSIOLayer.h
> PrefObserver::Observe can't access mInsecureFallbackSites without this
> helper because mInsecureFallbackSites is private.

Oh, right. I guess the patch for bug 1123020 will address this anyway.

> (In reply to David Keeler [:keeler] (use needinfo?) from comment #9)
> > Isn't RELEASE_BUILD defined for all releases? What's the benefit of having
> > it not defined for debug/local/etc. builds?
> 
> RELEASE_BUILD will be defined for all builds whose version number does not
> contain the letter "a".
> https://mxr.mozilla.org/mozilla-central/source/js/src/configure.in#2762
> Debug/local/etc. are irrelevant. For our official builds, it will be defined
> on Release/Beta/ESR and will not be defined on Nightly/DevEdition.

I guess my question is, what's the benefit of doing it this way? Why should we conditionally add those domains?

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 12 • 9 years ago

Please see comment 11 when you get a chance.

	Masatoshi Kimura [:emk] Assignee
	Comment 13 • 9 years ago

See comment #3. If it is not a compelling reason, I'll remove the condition block.

	Patrick McManus [:mcmanus]
	Comment 14 • 9 years ago

nit: bug 1115883 says you need wayfarer.timewarnercable.com too, not just www

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 15 • 9 years ago

(In reply to Masatoshi Kimura [:emk] from comment #13)
> See comment #3.

Wouldn't it be easier to verify with any channel by just changing the pref value?

(In reply to Patrick McManus [:mcmanus] from comment #14)
> nit: bug 1115883 says you need wayfarer.timewarnercable.com too, not just www

This brings up some interesting questions like: What's the plan for administrating this list? It seems clear how we choose to add sites to the list (i.e. people complain and we add whatever sites are broken), but what's the process for removing a site? It would be unfortunate if these domains lived on this list for a long time. I think that for every site on the list, we should be actively pushing the owner/operator towards fixing it, but that's a lot of work. Who's going to be making sure that happens and then coming back to remove those sites from the list? On another track, when the list changes, do we uplift those changes? How far? It might be worth considering shipping with an empty list and telling users that if a site they need to use requires this, they should add it themselves. It's a bummer from a UX perspective, but that would take up the least amount of our time and be the most safe for our users.

	Masatoshi Kimura [:emk] Assignee
	Comment 16 • 9 years ago

If we assume affected users can change the pref, the entire patch is not needed because affected users can just change "security.tls.version.fallback-limit". IMO we can't justify to add more complexity if users are bothered by changing about:config anyway.

Brian do you have any plan to maintain the list?

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 17 • 9 years ago

The main thing that needs to be decided is what percentage of broken connections Firefox is willing to accept as a result of disabling the non-secure fallback. The compatibility impact should be smaller than the impact of disabling SSL 3.0, according to telemetry + some reasonable estimation of the false positive rate for fallback due to connection resets. So, I think even without the whitelist you'll probably be OK. 

However, without putting a bunch of sites on the whitelist initially, you'll likely end up paralyzed by the FUD of the compatibility issue, and you'd probably end up just maintaining the status quo. That's why I suggest you put every TLS intolerant site you can find on the list for the initial release in which you disable TLS version intolerance fallback, even if it is thousands of sites*. Then, you can evaluate whether or not disabling TLS version intolerance fallback is even a viable option with the large whitelist.

After one release, you shouldn't really need to add new sites to the whitelist. I'm guessing that most of the sites that get broken by disabling the fallback will notice the problem and actually fix themselves. So, I don't expect the list to require ongoing maintenance going forward.

After that, you will know how many and which sites are on the whitelist, and the Mozilla community can have an educated discussion about whether or not to keep those sites on the whitelist and for how long. For example, you might decide to only include the top 100 or so sites in the whitelist, or you might decide to drop the whitelist altogether. In order to make those decisions, we'd probably need to measure with telemetry how often the whitelist entries are being used. These are decisions that can be revisited once a year or so. If Mozilla would like, I'd be happy to be responsible for this.

In general, I think you should avoid creating new UI, beyond making an editable pref available for sysadmins. In particular, I think it would ultimately be counterproductive to create a UI similar to the cert error exception UI.

* If there are hundreds or thousands of sites to whitelist, you should probably have a separate whitelist not stored in a pref, but instead stored similarly to the HSTS preload list, and leave the pref for user-added entries.

	Masatoshi Kimura [:emk] Assignee
	Comment 18 • 9 years ago

* Rebased to tip (security-prefs.js has been moved).
* Removed |#ifdef RELEASE_BUILD|.
* Added wayfarer.timewarnercable.com to the whitelist.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 19 • 9 years ago

Comment on attachment 8554121 [details] [diff] [review]
Implement TLS intolerance fallback whitelist, v4

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

Ok - r=me with comments addressed.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +677,5 @@
>  void nsSSLIOLayerHelpers::Cleanup()
>  {
>    mTLSIntoleranceInfo.Clear();
> +  mRenegoUnrestrictedSites.Clear();
> +  mInsecureFallbackSites.Clear();

Don't we need to hold the lock here?

@@ +1823,5 @@
>  
>  void
>  nsSSLIOLayerHelpers::clearStoredData()
>  {
> +  mRenegoUnrestrictedSites.Clear();

Again, don't we need to acquire the lock here?

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 20 • 9 years ago

Going off of bug 1084025 comment 96, we've identified more than 500 sites we might want to put on the list. That's probably too many to put in security-prefs.js.

	Masatoshi Kimura [:emk] Assignee
	Comment 21 • 9 years ago

(In reply to David Keeler [:keeler] (use needinfo?) from comment #19)
> ::: security/manager/ssl/src/nsNSSIOLayer.cpp
> @@ +677,5 @@
> >  void nsSSLIOLayerHelpers::Cleanup()
> >  {
> >    mTLSIntoleranceInfo.Clear();
> > +  mRenegoUnrestrictedSites.Clear();
> > +  mInsecureFallbackSites.Clear();
> 
> Don't we need to hold the lock here?
> 
> @@ +1823,5 @@
> >  
> >  void
> >  nsSSLIOLayerHelpers::clearStoredData()
> >  {
> > +  mRenegoUnrestrictedSites.Clear();
> 
> Again, don't we need to acquire the lock here?

Hm, I don't know why the lock was not acquired before, but it looks like it is certainly needed.
I also whitelisted sites from bug 1126652 and 1126654.

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

	Masatoshi Kimura [:emk] Assignee
	Comment 22 • 9 years ago

(In reply to David Keeler [:keeler] (use needinfo?) from comment #20)
> Going off of bug 1084025 comment 96, we've identified more than 500 sites we
> might want to put on the list. That's probably too many to put in
> security-prefs.js.

IMO we don't have to add all broken sites to the whitelist.
When POODLE was reported, we identified 9690 of 1M sites that require SSLv3 [1]. But we didn't even introduce a whitelist.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1076983#c32

	Masatoshi Kimura [:emk] Assignee
	Comment 23 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ebe8792139d

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 24 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/6ebe8792139d

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 9 years ago

Landed as part of a roll-up patch in bug 1128227.
https://hg.mozilla.org/releases/mozilla-aurora/rev/1e9694bbffaa

	Martin Thomson [:mt:]
	Comment 26 • 9 years ago

:emk, Matt noted that the pref is blank in Firefox 37.  I'm not seeing it in nightly either.  Is there an error in this patch?  Perhaps the extra line break?

	Masatoshi Kimura [:emk] Assignee
	Comment 27 • 9 years ago

(In reply to Martin Thomson [:mt] from comment #26)
> :emk, Matt noted that the pref is blank in Firefox 37.  I'm not seeing it in
> nightly either.  Is there an error in this patch?  Perhaps the extra line
> break?

This pref no longer has value by default. Now the whitelist is built into Firefox binary (bug 1128227). This pref was left for users to add some non-whitelisted sites.

To disable the built-in whitelist, set "security.tls.insecure_fallback_hosts.use_static_list" to false.

	Heiko Giesselmann
	Comment 28 • 9 years ago

The original bug entry is about domain whitelists, while the fix seems to address host a whitelist only. Using firefox in a large corporation with many development systems containing outdated self-signed certificates it's a huge obstacle if individual hosts have to be added to the whitelist. A domain whitelist would be much more efficient, as one usually either trusts all hosts in a specific domain or one doesn't. I.e. it's unlikely that I trust hostA.xyz.com, but not hostB.xyz.com.

	Martin Thomson [:mt:]
	Comment 29 • 9 years ago

Heiko, the nomenclature has domain == host.  You are talking about domain suffixes.

As far as your idea goes, we don't want to expend significant effort on engineering more complex ways to avoid good security.  I expect that we will remove the whitelist in the near future.

	Heiko Giesselmann
	Comment 30 • 9 years ago

Actually, this behavior makes recent Firefox versions unusable for me. I work in a large corporation with many servers with non-secure TLS versions. It's going to take weeks before configuration can be changed for all these servers. Obviously, that's desirable from a security perspective, but doesn't seem to be a strong requirement for well controlled servers that are not exposed externally. Looks like it's going to be chrome or an earlier firefox version for me...

	Martin Thomson [:mt:]
	Comment 31 • 9 years ago

Heiko, see comment 27.