961528 - HSTS is enforced on apis.google.com sub-domains and hosts yet only works for apis.google.com itself (awstats no longer shows charts)

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Bill Gianopoulos [:WG9s]]

I can no longer see charts for webstast on my server served by awstats using the google charts awstats plugin.  This works fine using Google chrome.

This is a regression caused by https://hg.mozilla.org/mozilla-central/rev/b3c08e6fa790

Comment 1

[Bill Gianopoulos [:WG9s]]

Attachment: Workaround

This just backsout the change.

Comment 2

[Bill Gianopoulos [:WG9s]]

Attachment: Workaround

Actually this seems sufficient.

Comment 3

[Bill Gianopoulos [:WG9s]]

Attachment: Workaround

OK this time the correct patch.

Comment 4

[Bill Gianopoulos [:WG9s]]

Just my opinion, but requiring https to do financial transactions might make sense.  But requiring you to use encryption to draw a graph, not so much!

Comment 5

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

Chrome special-cases chart.apis.google.com and doesn't consider it HSTS (even though apis.google.com is in the preload list with includeSubdomains=true) because for some reason it doesn't have the right certificate.
Apparently chart.apis.google.com is deprecated, and chart.googleapis.com should be used instead.
So, I see a few options here:
1. Replace chart.apis.google.com with chart.googleapis.com in the awstats plugin (it seems like this should be done regardless)
2. Get Google to fix the certificate for chart.apis.google.com
3. Get Google to not specify apis.google.com in the HSTS preload list
4. Change how we process that list to ignore apis.google.com
5. Change Firefox to never consider chart.apis.google.com HSTS (i.e. match Chrome's behavior)

Comment 6

[Bill Gianopoulos [:WG9s]]

(In reply to David Keeler (:keeler) from comment #5)
> Chrome special-cases chart.apis.google.com and doesn't consider it HSTS
> (even though apis.google.com is in the preload list with
> includeSubdomains=true) because for some reason it doesn't have the right
> certificate.
> Apparently chart.apis.google.com is deprecated, and chart.googleapis.com
> should be used instead.
> So, I see a few options here:
> 1. Replace chart.apis.google.com with chart.googleapis.com in the awstats
> plugin (it seems like this should be done regardless)

Making that change on the server seems to have fixed this, so I guess that is the real fix here.

Comment 7

[Bill Gianopoulos [:WG9s]]

Here is the patch I applied:

--- /usr/share/awstats/plugins/graphgooglechartapi.pm.orig	2014-01-19 19:31:22.815246422 -0500
+++ /usr/share/awstats/plugins/graphgooglechartapi.pm	2014-01-19 19:18:55.128080592 -0500
@@ -25,17 +25,17 @@
 #-----------------------------------------------------------------------------
 # <-----
 # ENTER HERE THE MINIMUM AWSTATS VERSION REQUIRED BY YOUR PLUGIN
 # AND THE NAME OF ALL FUNCTIONS THE PLUGIN MANAGE.
 my $PluginNeedAWStatsVersion = "7.0";
 my $PluginHooksFunctions = "Init ShowGraph AddHTMLHeader";
 my $PluginName = "graphgooglechartapi";
 my $ChartProtocol = "http://";
-my $ChartURI = "chart.apis.google.com/chart?";	# Don't put the HTTP part here!
+my $ChartURI = "chart.googleapis.com/chart?";	# Don't put the HTTP part here!
 my $ChartIndex = 0;
 my $title;
 my $type;
 my $imagewidth = 640;			# maximum image width. 
 my $imageratio = .25;			# Height is defaulted to 25% of width
 my $pieratio = .20;				# Height for pie charts should be different
 my $mapratio = .62;				# Height for maps is different
 my $labellength;

Comment 8

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

(In reply to David Keeler (:keeler) from comment #5)
> Chrome special-cases chart.apis.google.com and doesn't consider it HSTS
> (even though apis.google.com is in the preload list with
> includeSubdomains=true) because for some reason it doesn't have the right
> certificate.
> Apparently chart.apis.google.com is deprecated, and chart.googleapis.com
> should be used instead.
> So, I see a few options here:
> 1. Replace chart.apis.google.com with chart.googleapis.com in the awstats
> plugin (it seems like this should be done regardless)

Yes.

> 2. Get Google to fix the certificate for chart.apis.google.com
> 3. Get Google to not specify apis.google.com in the HSTS preload list

Probably not reasonable, or Google would have already done it by now.

> 4. Change how we process that list to ignore apis.google.com
> 5. Change Firefox to never consider chart.apis.google.com HSTS (i.e. match
> Chrome's behavior)

If we're already doing special stuff for Google, and if chart.googleapis.com has the same API as chart.apis.google.com then we could just write a special patch in nsHttpChannel to redirect http://chart.apis.google.com to https://chart.googleapis.com. I guess in terms of end-user benefit, that is going to be the best solution to this particular instance.

I suppose there will be other entries in the Chrome preload list that have "excludes" and we need to deal with that in some way, such as by ignoring the includeSubdomains directive for those entries. It seems HSTS may not be very useful for apis.google.com without includeSubdomains though.

Comment 9

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

(In reply to Bill Gianopoulos [:WG9s] from comment #7)
>  my $PluginName = "graphgooglechartapi";
>  my $ChartProtocol = "http://";
> -my $ChartURI = "chart.apis.google.com/chart?";	# Don't put the HTTP part
> here!
> +my $ChartURI = "chart.googleapis.com/chart?";	# Don't put the HTTP part
> here!

Thanks for making this patch to awstats!

I understand that you don't think it is absolutely necessary to use HTTPS for charts. However, even Google recomends using HTTPS for this API:
https://developers.google.com/chart/image/docs/making_charts

So, I recommend making the change to HTTPS. In my experience, log files and similar often have information that should be private, even if private information isn't supposed to be in the logs. Better safe than sorry.

David:

See https://developers.google.com/chart/image/docs/making_charts#multichart. Google explicitly does not want to use HTTPS for *.chart.apis.google.com.

Comment 10

[Bill Gianopoulos [:WG9s]]

OK.  The issue here is that the servers chart.apis.google.com and the 0.chart.apis.google.com through 9.chart.apis.google.com are running SSL, but are using a cert that is not valid for those names.  So, probably the right thing to do might be to force https for the host apis.google.com, but not for other hosts in that sub-domain.  I am not sure our mechanism allows for that.

Comment 11

[Bill Gianopoulos [:WG9s]]

OK so changed the summary to more correctly reflect the real issue.  We are enforcing HSTS on apis.google.com, all hsots in apis.google.com and all subdomains of apis.google.com when, in fact this should only apply to apis.google.com itslef.  The hosts in that domain and subdomains do permit https connections but do not present a certificate that is valid for their host name.

Comment 12

[Bill Gianopoulos [:WG9s]]

Attachment: Workaround

This is a better workaround.

However, I am NOT taking this bug.

This is NOT the correct fix.

I do not know how to fix this for real because what is required is for the automated system the generates the HSTS **** to generate

  { "apis.google.com", false },

instead of

  { "apis.google.com", true },

I have no idea how to do that.

Comment 13

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

Note that this needs to be uplifted to Firefox 29.

Comment 14

[Bill Gianopoulos [:WG9s]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #13)
> Note that this needs to be uplifted to Firefox 29.

Glad someone else understands this.

Comment 15

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

Attachment: patch

Turns out, holepunching specific hosts/subdomains is not very hard. I'm disappointed that we're doing this rather than Google fixing whatever's wrong about chart.apis.google.com, but I think it's the least-bad option, particularly in terms of protecting users.

Comment 16

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

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

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

David, please re-request review if you don't think it is a good idea to implement my alternative suggestion. My concern is that the "HOLEPUNCH" mechanism adds more complexity than necessary. Also, not good to do things during service startup if we can avoid it.

::: security/manager/boot/src/nsSiteSecurityService.cpp
@@ +114,5 @@
> +                      (uint32_t) nsIPermissionManager::EXPIRE_NEVER,
> +                      0, false);
> +   if (NS_WARN_IF(NS_FAILED(rv))) {
> +     return rv;
> +   }

Instead of adding more abuse to the permission manager, why not just change IsSecureHost and/or IsSecureURI to hard-code "return false" for this host? It seems like it would be much simpler and more obviously correct, while still being the same level of hacky hard-codedness.

Comment 17

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

Attachment: patch v2

Good point. This is simpler (as long as the number of holepunched hosts is small, which I really hope continues to be the case).

Comment 18

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

Comment on attachment 8373733 [details] [diff] [review]
patch v2

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

::: security/manager/boot/src/nsSiteSecurityService.cpp
@@ +468,5 @@
> +      return NS_OK;
> +    }
> +    offset = host.FindChar('.', offset) + 1;
> +  }
> +  while (offset > 0);

Pretty sure you can just check that the string ends with ".chart.apis.google.com" with one call to Length(), one substring construction, and one comparison. (Note the leading dot.)

::: security/manager/ssl/tests/unit/test_sts_holepunch.js
@@ +16,5 @@
> +                                        "sub.chart.apis.google.com", 0));
> +  do_check_true(SSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
> +                                       "example.apis.google.com", 0));
> +  do_check_true(SSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
> +                                       "sub.example.apis.google.com", 0));

It seems like we should check isSecureURL too.

Also, do we need to worry about case-sensitivity?

Comment 20

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

Attachment: patch v2.1

Thanks for the quick reviews. Carrying over r+.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #18)
> Pretty sure you can just check that the string ends with
> ".chart.apis.google.com" with one call to Length(), one substring
> construction, and one comparison. (Note the leading dot.)

I see what you're saying. I think what I went with in the end is a good compromise between clarity and efficiency.

> ::: security/manager/ssl/tests/unit/test_sts_holepunch.js
> It seems like we should check isSecureURL too.

Ok - added some tests.

> Also, do we need to worry about case-sensitivity?

Turns out, no. I added some test cases to cover it, though.

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b95f0e417ba1

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b95f0e417ba1

Comment 23

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

Comment on attachment 8374241 [details] [diff] [review]
patch v2.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 947759 / chart.apis.google.com doesn't have a valid certificate
User impact if declined: chart.apis.google.com unusable
Testing completed (on m-c, etc.): on m-c, in testsuite
Risk to taking this patch (and alternatives if risky): none 
String or IDL/UUID changes made by this patch: none

Comment 24

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/c4f4eed7a86e

Comment 25

[Bill Gianopoulos [:WG9s]]

Verifying that this fixes the issue that I originally reported.