971341 - Loading the billing page of Amazon creates infinite amount of tabs

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Armen [:armenzg]]

I've seen this happening to catlee.
I've seen this happening to me for Nightly and Aurora.
It does not happen on Beta.
The URL is: https://console.aws.amazon.com/billing
The tabs are opened at a rate of 2-3 per second.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Hard to debug without a valid account.

Does spoofing your user agent to Firefox 26's make the problem go away? If not, can you find a regression range using nightly builds from the 27 cycle?

Can you identify from the page JS any window.open calls that look suspicious?

Comment 3

[Armen [:armenzg]]

Attachment: CSPUtils.jsm.js

Some things I noticed in the console:
console.aws.amazon.com : server does not support RFC 5746, see CVE-2009-3555

This one repeats as many times as there are tabs opening:
GET https://console.aws.amazon.com/billing/home [HTTP/1.1 200 OK 260ms]
an error occurred while executing regular expression CSPUtils.jsm:1009


  var tokens = aStr.split(/\s+/);
  for (var i in tokens) {
    if (!R_SOURCEEXP.test(tokens[i])) { <----- line 1009
      cspWarn(aCSPRep,
              CSPLocalizer.getFormatStr("failedToParseUnrecognizedSource",
                                        [tokens[i]]));
      continue;
    }
    var src = CSPSource.create(tokens[i], aCSPRep, self, enforceSelfChecks);
    if (!src) {
      cspWarn(aCSPRep,
              CSPLocalizer.getFormatStr("failedToParseUnrecognizedSource",
                                        [tokens[i]]));
      continue;
    }

Comment 4

[Armen [:armenzg]]

I will need info to myself to see if I can test some nightly builds.

Comment 5

[Tanner Filip [:tanner]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #2)
> Hard to debug without a valid account.
> 
> Does spoofing your user agent to Firefox 26's make the problem go away? If
> not, can you find a regression range using nightly builds from the 27 cycle?
> 
> Can you identify from the page JS any window.open calls that look suspicious?

Spoofing to Fx24 doesn't make the problem go away, I'll try to find a regression range.

Comment 6

[Tanner Filip [:tanner]]

I also tried spoofing to 26, that didn't work either. Found this range, not sure if it's right (or even helpful). 

Last good revision: 73eefb421e2a
First bad revision: 604818812338
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=73eefb421e2a&tochange=604818812338

Comment 7

[Chris AtLee [:catlee]]

The weird thing for me is that there's no content in the tab that's opened.

If I go to https://console.aws.amazon.com/billing/home in a new tab, I get an empty 200 response, with a few headers set, including:

x-frame-options:"SAMEORIGIN"
x-ua-compatible:"IE=Edge"

Comment 8

[Armen [:armenzg]]

Thank you for finding a range!

Comment 9

[:Gavin Sharp [email: [e-mail]]]

That range is rather large, unfortunately...

Can someone bisect with local builds? Or provide their account credentials to me temporarily so that I can?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

I thought the CSP errors were a red herring, but apparently not.

I confirmed with local bisection that this was caused by bug 916054:

changeset:   166015:65dbffad01e1
user:        Christoph Kerschbaumer <[e-mail]>
date:        Sun Oct 13 21:12:33 2013 -0700
summary:     Bug 916054 - URLs with path are ignored by FF's CSP parser. r=grobinson

I haven't looked into how that could possibly be!

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Unfortunately I don't have an account, otherwise I would look into that myself right away. Can you provide the CSP-policy that is about to be enforced? You should be able to print it in AppendPolicy [1]. Wondering if they are using a path in any of the urls to be enforced!

[1] https://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js#413

Comment 12

[Garrett Robinson [:grobinson]]

I'll take this, as I reviewed the breaking patch.

Comment 13

[Garrett Robinson [:grobinson]]

Have to wait until tomorrow to look at this, I have two-factor for AWS Billing and my token's at home :P

Comment 14

[Garrett Robinson [:grobinson]]

When I open the same URL in Firefox 27 (not affected by the bug), I get this CSP header:

default-src 'self';script-src https://d8kqaaxn6vwna.cloudfront.net https://d257l1zb7u5fh9.cloudfront.net https://s3.amazonaws.com https://d36cz9buwru1tt.cloudfront.net https://media.amazonwebservices.com https://d2q66yyjeovezo.cloudfront.net 'self' 'unsafe-inline';style-src https://d2q66yyjeovezo.cloudfront.net https://d257l1zb7u5fh9.cloudfront.net https://s3.amazonaws.com https://d8kqaaxn6vwna.cloudfront.net https://s3.amazonaws.com/cdn-prod https://dnnau1efv7r85.cloudfront.net 'self' 'unsafe-inline';img-src https://d257l1zb7u5fh9.cloudfront.net https://s3.amazonaws.com/cdn-prod https://aws.amazon.com https://s3.amazonaws.com https://d34xub3ut0ooib.cloudfront.net https://d1ge0kk1l5kms0.cloudfront.net https://aws-bad-images.s3.amazonaws.com https://g-ecx.images-amazon.com/images/G/01/amazonui/sprites/aui_sprite_0007-1x._V383827579_.png https://images-na.ssl-images-amazon.com https://capi-na.ssl-images-amazon.com data: 'self';font-src https://d257l1zb7u5fh9.cloudfront.net https://dnnau1efv7r85.cloudfront.net https://s3.amazonaws.com/cdn-prod https://d8kqaaxn6vwna.cloudfront.net https://s3.amazonaws.com 'self';frame-src *.s3-external-1.amazonaws.com 'self';report-uri /billing/rest/v1.0/logger/csp

And these errors:

* Content Security Policy: Failed to parse unrecognized source https://s3.amazonaws.com/cdn-prod
* Content Security Policy: Failed to parse unrecognized source https://g-ecx.images-amazon.com/images/G/01/amazonui/sprites/aui_sprite_0007-1x._V383827579_.png

So Amazon's CSP uses paths, and the behavior that bug 916054 was supposed to introduce was to *ignore* the paths - so these "failed to parse unrecognized source" errors would go away, and we would enforce the origins of those urls instead. I'll dig into what's going wrong now.

Comment 15

[Garrett Robinson [:grobinson]]

** token: https://g-ecx.images-amazon.com/images/G/01/amazonui/sprites/aui_sprite_0007-1x._V383827579_.png
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "[JavaScript Error: "an error occurred while executing regular expression" {file: "resource://gre/modules/CSPUtils.jsm" line: 1027}]'[JavaScript Error: "an error occurred while executing regular expression" {file: "resource://gre/modules/CSPUtils.jsm" line: 1027}]' when calling method: [nsIContentSecurityPolicy::appendPolicy]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///home/garrett/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/bin/components/nsPrompter.js :: openModalWindow :: line 394"  data: yes]
************************************************************

Comment 16

[Garrett Robinson [:grobinson]]

The issue here is the . in the path that isn't delimiting the file extension. I'm not 100% sure why that was caused an exception instead of just returning false.

Comment 17

[Garrett Robinson [:grobinson]]

Attachment: 971341-stop-infinite-tabs

The regexes for paths (R_PATH and R_FILE) were missing some valid characters for paths ("unreserved" as defined in RFC 3986 [0]). I've added these, and updated the tests to make sure URLs of the form that caused this bug (which have > 1 ".") parse correctly.

[0] http://www.ietf.org/rfc/rfc3986.txt

Comment 18

[Garrett Robinson [:grobinson]]

CSP test suite passes locally. Try run: https://tbpl.mozilla.org/?tree=Try&rev=0473dc3c456c

Comment 19

[Garrett Robinson [:grobinson]]

Attachment: 971341-stop-infinite-tabs

Update corresponding mochitest (test_csp_regexp_parsing.html) to match changes to parser unit tests.

Try: https://tbpl.mozilla.org/?tree=Try&rev=23ed2d634f49

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8394314 [details] [diff] [review]
971341-stop-infinite-tabs

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

r=me on everything except the regexes.

If we're not doing anything with the path, can we just match ext-host-src (which is :: 'host-source "/" *( <VCHAR except ";" and ","> )') instead of cherry-picking specific char classes?  Or do we need to explicitly look for a-zA-Z0-9-_+.~?

Since we're getting rid of these old parsers soon-ish, a band-aid fix is okay, but I'd prefer to make it simpler and just match on [^;,\s].  Chris: what do you think?

::: content/base/src/CSPUtils.jsm
@@ +68,3 @@
>  
>  // file
> +const R_FILE       = new RegExp("(\\/([a-zA-Z0-9\\-\\_\\+\\.\\~]+)\\.([a-zA-Z0-9]+))", 'i');

Would it be easier/safer to just search for things that are not whitespace, ";" or ","?

@@ +1022,5 @@
>    }
>  
>    var tokens = aStr.split(/\s+/);
>    for (var i in tokens) {
> +    dump("** token: " + tokens[i] + "\n");

Please remove the dump.

::: content/base/test/unit/test_csp_ignores_path.js
@@ +11,5 @@
>  var ioService = Cc["@mozilla.org/network/io-service;1"]
>                    .getService(Ci.nsIIOService);
>  var self = ioService.newURI("http://test1.example.com:80", null, null);
>  
> +function testValidSRCsHostSourceWithScheme() {

Nit: this tests path too, add "andPath".

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8394314 [details] [diff] [review]
971341-stop-infinite-tabs

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

Overall, this looks great.
Don't forget to take out the 'dump' Sid pointed out.
Thanks for fixing this Garret!

::: content/base/src/CSPUtils.jsm
@@ +71,1 @@
>  

Since this is going to be an interim solution, I think we can accept the regular expressions as they are. The new parser is more liberal, and pretty much accepts any (special) characters except whitespace, ";", and "," so that we are not going to introduce regressions

Comment 22

[Garrett Robinson [:grobinson]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #20)
> Comment on attachment 8394314 [details] [diff] [review]
> 971341-stop-infinite-tabs
> 
> Review of attachment 8394314 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me on everything except the regexes.
<snip> 
> If we're not doing anything with the path, can we just match ext-host-src
> (which is :: 'host-source "/" *( <VCHAR except ";" and ","> )') instead of
> cherry-picking specific char classes?  Or do we need to explicitly look for
> a-zA-Z0-9-_+.~?
> 
> Since we're getting rid of these old parsers soon-ish, a band-aid fix is
> okay, but I'd prefer to make it simpler and just match on [^;,\s].  Chris:
> what do you think?
>
> Would it be easier/safer to just search for things that are not whitespace,
> ";" or ","?

I agree that we should make the fix as complete as possible. Your proposal does not work - such a regex would match non-printing characters and null bytes. The CSP spec is clear on what ext-host-src should accept. Here's the best regex I've come up with for it: [a-zA-Z0-9!\"#$%&'()*+-./:<=>?@[\\]^_`{|}]

Comment 23

[Garrett Robinson [:grobinson]]

Attachment: 971341-stop-infinite-tabs

Re-flagging for review due to extensive changes.

I decided to go with the "re-use ext-host-src" approach, and in the process rewrote most of the original "ignore paths" stuff. Per the CSP spec, the allowed character set is VCHAR minus ';' and ','. This is a larger range of allowed characters than previous, and makes some of the "invalid" path tests valid, so they have been removed.

All CSP tests pass.

Comment 24

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8396059 [details] [diff] [review]
971341-stop-infinite-tabs

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

To be clear, this is rolling back the changes Christoph made in bug 916054 (https://bug916054.bugzilla.mozilla.org/attachment.cgi?id=8367995), right?

r=me with either simplification of the regexes as commented below, or at least extensive documentation of how the regexes work so it's easier to read (and fix).

::: content/base/src/CSPUtils.jsm
@@ +70,5 @@
>  
>  // ext-host-source = host-source "/" *( <VCHAR except ";" and ","> )
>  //                 ; ext-host-source is reserved for future use.
> +const R_VCHAR_EXCEPT = new RegExp("[a-zA-Z0-9!\"#$%&'()*+-./:<=>?@[\\]^_`{|}]");
> +const R_EXTHOSTSRC   = new RegExp ("^" + R_HOSTSRC.source.replace(/(^\^)|(\$$)/g, "") + "\\/" + R_VCHAR_EXCEPT.source + "*$", 'i');

These regexes are pretty ugly.  :(

What do you think of:

function STRIP_INPUTDELIM(re) {
  return re.replace(/(^\^)|(\$$)/g, "");
}

R_VCHAR_EXCEPT is "[!-+--:<-~] or [\x21-\x2B\x2D-\x3A\x3C-\x7E]
(ranges exclude , [\x2C] and ; [\x3B])

R_EXTHOSTSRC would then be:
   = RegExp(STRIP_INPUTDELIM(R_HOSTSRC.source) + "\\/" + R_VCHAR_EXCEPT.source + "+");
right?

Goal here is to make it easy to read.  Maybe R_EXTHOSTSRC doesn't need to change (make it multiple lines like R_HOSTSRC), but we should at the very least simplify R_VCHAR_EXCEPT.

@@ +1395,2 @@
>      var hostMatch = R_HOSTSRC.exec(aStr);
> +

nit: extra newline (or if you're adding it on purpose, add one before this line too).

@@ +1411,5 @@
>  
>      sObj._host = CSPHost.fromString(hostMatch[0]);
>      if (!portMatch) {
>        // gets the default port for the given scheme
> +      var defPort = Services.io.getProtocolHandler(sObj._scheme).defaultPort;

Good catch.

Comment 25

[Garrett Robinson [:grobinson]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #24)
> Comment on attachment 8396059 [details] [diff] [review]
> 971341-stop-infinite-tabs
> 
> Review of attachment 8396059 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> To be clear, this is rolling back the changes Christoph made in bug 916054
> (https://bug916054.bugzilla.mozilla.org/attachment.cgi?id=8367995), right?

No - it's extending them so they don't crash the parser when they encounter an unexpected character.

Comment 26

[Garrett Robinson [:grobinson]]

Attachment: 971341-stop-infinite-tabs

Carrying over r+ from sstamm. Clears up regexes as recommended. Also braces some un-braced if's in the vicinity for style.

Try: https://tbpl.mozilla.org/?tree=Try&rev=8a12cefec8a7

Comment 27

[Garrett Robinson [:grobinson]]

Try looked good. Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/4be0c027d106

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4be0c027d106

Comment 29

[Armen [:armenzg]]

Can we please uplift to m-a?

I will test on m-b later (added to my TODO)

Comment 30

[Armen [:armenzg]]

Thank you for fixing this!

Comment 31

[Garrett Robinson [:grobinson]]

Comment on attachment 8400300 [details] [diff] [review]
971341-stop-infinite-tabs

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 916054
User impact if declined: Won't be able to access Amazon AWS billing page, or any other page with a CSP that has paths with unexpected (but valid) characters.
Testing completed (on m-c, etc.): Green try on m-c
Risk to taking this patch (and alternatives if risky): Low, only affects CSP.
String or IDL/UUID changes made by this patch: None.

Comment 32

[Armen [:armenzg]]

I tested that this happens on mozilla-beta as well.

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8400300 [details] [diff] [review]
971341-stop-infinite-tabs

[Triage Comment]
Carrying the nomination approval to beta as well since it's affected - if the patch can't land cleanly there please put up a new one and re-nom.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0f6b063188e
https://hg.mozilla.org/releases/mozilla-beta/rev/fe5d67aa5366

Comment 36

[Fernando Hartmann]

I can confirm that now on FF 29 b6 I can access the bill section of my account in Amazon with no more problems.

Comment 37

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Issue is reproducible on Nightly from February 10 (build ID: 20140210030201).
Verified fixed on Windows 7 64bit, Ubuntu 13.04 64bit and Mac OS X 10.9 using:
- Firefox 29 beta 6 (build ID: 20140407135746)
- latest Firefox 30 Aurora (build ID: 20140409004002)
- latest Firefox 31 Nightly (build ID: 20140409030203)