Closed Bug 961528 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Security, defect, major)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox29 + fixed
firefox30 --- fixed

People

(Reporter: wgianopoulos, Assigned: keeler)

References

Details

(Keywords: regression)

Attachments

(1 file, 6 obsolete files)

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
Attached patch Workaround (obsolete) — Splinter Review
This just backsout the change.
Blocks: 947759
Attached patch Workaround (obsolete) — Splinter Review
Actually this seems sufficient.
Attachment #8362277 - Attachment is obsolete: true
Attached patch Workaround (obsolete) — Splinter Review
OK this time the correct patch.
Attachment #8362284 - Attachment is obsolete: true
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!
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)
(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.
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;
(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.
(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.
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.
Summary: Can no longer see charts when viewing awstats on a server → HSTS is enforces on apis.google.com sub-domains and hosts yet only works for apis.goole.com itself (awstats no longer shows charts)
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.
Summary: HSTS is enforces on apis.google.com sub-domains and hosts yet only works for apis.goole.com itself (awstats no longer shows charts) → HSTS is enforced on apis.google.com sub-domains and hosts yet only works for apis.goole.com itself (awstats no longer shows charts)
Attached patch Workaround (obsolete) — Splinter Review
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.
Attachment #8362286 - Attachment is obsolete: true
Summary: HSTS is enforced on apis.google.com sub-domains and hosts yet only works for apis.goole.com itself (awstats no longer shows charts) → HSTS is enforced on apis.google.com sub-domains and hosts yet only works for apis.google.com itself (awstats no longer shows charts)
Note that this needs to be uplifted to Firefox 29.
Assignee: nobody → dkeeler
Target Milestone: --- → mozilla30
(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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8369137 - Attachment is obsolete: true
Attachment #8373656 - Flags: review?(brian)
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.
Attachment #8373656 - Flags: review?(brian)
Attached patch patch v2 (obsolete) — Splinter Review
Good point. This is simpler (as long as the number of holepunched hosts is small, which I really hope continues to be the case).
Attachment #8373656 - Attachment is obsolete: true
Attachment #8373733 - Flags: review?(brian)
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?
Attachment #8373733 - Flags: review?(brian) → review+
Duplicate of this bug: 970876
Attached patch patch v2.1Splinter Review
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.
Attachment #8373733 - Attachment is obsolete: true
Attachment #8374241 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b95f0e417ba1
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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
Attachment #8374241 - Flags: approval-mozilla-aurora?
Attachment #8374241 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verifying that this fixes the issue that I originally reported.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.