947759 - Preload HSTS for Google-specified domains

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

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

I received a request from AGL to preload the HSTS entries for some Google properties into Firefox, even though the websites do not send an 18-week+ max-age in their HSTS, and/or they don't even send an HSTS header at all.

Adam's request was for these domains:

$ cat transport_security_state_static.json | grep "force-https" | grep
'pins": "google"' | grep -v snionly
wallet.google.com
checkout.google.com
chrome.google.com
docs.google.com
sites.google.com
spreadsheets.google.com
appengine.google.com
encrypted.google.com
accounts.google.com
profiles.google.com
mail.google.com
talkgadget.google.com
talk.google.com
hostedtalkgadget.google.com
plus.google.com
plus.sandbox.google.com
script.google.com
history.google.com
security.google.com
goto.google.com
market.android.com
ssl.google-analytics.com
drive.google.com
googleplex.com
groups.google.com
apis.google.com
chromiumcodereview.appspot.com
chrome-devtools-frontend.appspot.com
codereview.appspot.com
codereview.chromium.org
code.google.com
dl.google.com
translate.googleapis.com

Let's create a special exception to pin HSTS for these domain names.

AGL told me he will continue to work with the people in charge of these domains so that we can remove this exception. However, apparently that is something that is likely to continue to take a long time.

Comment 1

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

I agree that this would be an easy thing to do that would immediately improve security for our users. However, I have two concerns:

1. Doing this would remove an incentive for the people in charge of the Google domains to actually send the header. After all, if we're already unconditionally considering them HSTS, why would they bother sending the extra bytes on the wire?
2. If we do this for Google, other sites are going to ask (and reasonably so) if we're going to do it for them as well. I have no time for or interest in dealing with and vetting requests from sites to be added to this exception list (and then being removed again if/when they mess up).

In short, if we go ahead with this, I think we have to be okay with Google sites never sending the header and saying "nope, sorry" to other organizations that ask us to do this with their sites as well.

Comment 2

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

Attachment: patch

Brian - let me know if you have concerns about this approach.
Camilo - I'd appreciate if you had a look at this when you get a chance.
The idea is to keep a bit more metadata about each entry as the script attempts to fetch the header. If it gets a sufficient header, nothing different happens. If it doesn't, if the site should be "treated specially", it adds it to the preload list unconditionally using the includeSubdomains value from the source list.

Comment 3

[Camilo Viecco (:cviecco)]

Comment on attachment 8355626 [details] [diff] [review]
patch

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

I would remove the things not related to google from this patch. Otherwise an r+

::: security/manager/tools/getHSTSPreloadList.js
@@ +137,5 @@
>        error = ERROR_NO_HSTS_HEADER;
>      }
>    }
>  
> +  var treatSpecially = (host.treatSpecially ||

I would rename this to force_include (or similar)

@@ +274,5 @@
>  }
>  
>  function shouldRetry(response) {
>    return (response.error != ERROR_NO_HSTS_HEADER &&
> +          response.error != ERROR_MAX_AGE_TOO_LOW &&

Does this belong in this patch?

Comment 4

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

Comment on attachment 8355626 [details] [diff] [review]
patch

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

IMO, it would have been sufficient and maybe better to simply hard-code the list that AGL gave us.

BTW, I wonder why there is no www.google.com or google.com.

::: security/manager/tools/getHSTSPreloadList.js
@@ +137,5 @@
>        error = ERROR_NO_HSTS_HEADER;
>      }
>    }
>  
> +  var treatSpecially = (host.treatSpecially ||

s/var/let/g

@@ +140,5 @@
>  
> +  var treatSpecially = (host.treatSpecially ||
> +                        (host.pins == "google" && !host.snionly));
> +
> +  if (error == ERROR_NONE && maxAge.value < MINIMUM_REQUIRED_MAX_AGE) {

Add a comment explaining why we bother with this even when treatSpecially is not false.

Comment 5

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

Attachment: patch v2

This patch applies on top of the one from bug 959796 (splitting out the unrelated refactoring).

Comment 6

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

Thanks for the reviews.
https://hg.mozilla.org/mozilla-central/rev/57e19dc6bd3f

Comment 7

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

I think this is a good candidate for uplift to Firefox 28 (not 27) if we can do it this week. If you agree, please ask for uplift on Friday or so.

Comment 8

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

(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #7)
> I think this is a good candidate for uplift to Firefox 28 (not 27) if we can
> do it this week. If you agree, please ask for uplift on Friday or so.

I would agree, but this patch just modified the generation script, so the actual preload list won't change until the automatic update tomorrow (so we won't know if this caused breakage until after then, if at all). I think it would be best to wait until the new list has been on nightly for a few days before uplifting this.

Comment 9

[Bill Gianopoulos [:WG9s]]

This breaks remotely viewing stats on any server using awstats which uses google charts to show the charts.  I think it is odd we would do this on Google's say-so if they are NOT doing this themselves in Chrome.  seems they are just setting us up of a "This works in Chrome but does not work in Firefox" ad campaign.

Comment 10

[Bill Gianopoulos [:WG9s]]

Well I think it does because was broken by the auto update.  The workaround I posted is in bug 961528.

Comment 11

[Adam Langley]

charts.apis.google.com is the old, deprecated domain. chart.googleapis.com is the right one, but some old code still uses the old name.

The Chromium HSTS preload specifically has a cutout for charts.apis.google.com because of this:

  0     // chart.apis.google.com is *not* HSTS because the certificate doesn't match 
  1     // and there are lots of links out there that still use the name. The correct
  2     // hostname for this is chart.googleapis.com.                                
  3     { "name": "chart.apis.google.com", "include_subdomains": true, "pins": "google" },