916054 - URLs with path are ignored by FF's CSP parser

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Nicolas Golubovic]

The specification of CSP 1.0 states that if a CSP 1.0 compilant Browser encounters a URL with a path it should ignore the path and enforce the Origin of it (see Example 4 at http://www.w3.org/TR/CSP/#sample-policy-definitions).


Example: http://poc.iceqll.eu/csppath/

Firefox fails to parse the URL in the example with the following error message:
"Content Security Policy: Failed to parse unrecognized source http://poc.iceqll.eu/csppath/alert.js"
This leads to a default-src of 'none'.

It should instead fall back to allowing ressources from http://poc.iceqll.eu.



Tested on Firefox Nightly 26.0a1 (2013-09-12).

Comment 1

[Frederik Braun [:freddy]]

Confirmed on Aurora (25) and Nightly (26).

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Hey Garret, I guess we should not only accept a /path/ but probably also a /file/ for CSP. IIRC we do not even have to cut the path and file, because the CSP gets the host for comparison later on anyway.

So I think we should extend [1] with something like this:

const R_PATH       = new RegExp ("todo");
const R_FILE       = new RegEXP ("todo");

const R_HOSTSRC    = new RegExp ("^((((" + R_SCHEME.source + "\\:\\/\\/)?("
                                         +   R_HOST.source + ")"
                                         +   R_PATH.source + "?)"
                                         +   R_FILE.source + "?)"
                                         +   R_PORT.source + "?)$", 'i');

Do you agree, or should we just accept a path but not a file. I think its the better option is to allow a file as well, because otherwise we fall back to 'none' in case a file is provided, which is not the most intuitive option for a security feature.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#69

Comment 4

[Garrett Robinson [:grobinson]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Hey Garret, I guess we should not only accept a /path/ but probably also a
> /file/ for CSP. IIRC we do not even have to cut the path and file, because
> the CSP gets the host for comparison later on anyway.

In the future, when we implement path matching for CSP 1.1, we should support both paths (ending in /) and files (not ending in /) as described in the draft spec [1]. So, you are correct - we should handle (and, for 1.0, ignore) both.

> So I think we should extend [1] with something like this:
> 
> const R_PATH       = new RegExp ("todo");
> const R_FILE       = new RegEXP ("todo");
> 
> const R_HOSTSRC    = new RegExp ("^((((" + R_SCHEME.source + "\\:\\/\\/)?("
>                                          +   R_HOST.source + ")"
>                                          +   R_PATH.source + "?)"
>                                          +   R_FILE.source + "?)"
>                                          +   R_PORT.source + "?)$", 'i');
> 

This is pretty good, but the port should come after the host, not the path/file. I think this is closer to what you want:

const R_FILE = new RegExp("[a-zA-Z0-9-_\.]+"); // refer to RFC 3986 [2]
const R_PATH = new RegExp(R_FILE.source + "\\/");

const R_HOSTSRC    = new RegExp ("^((((" + R_SCHEME.source + "\\:\\/\\/)?("
                                         + R_HOST.source + ")"
                                         + R_PORT.source + "?)",
                                         + R_FILE.source + "?)",
                                         + R_PATH.source + "?)$" 'i');

[1] https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#path-matching
[2] http://tools.ietf.org/html/rfc3986

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_916054_csp_should_ignore_path_tests.patch

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_916054_csp_should_ignore_path.patch

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

When pushing to try [1], I realized that the current regular expressions create a different result then the URI parser [2].

For the URI:
  foo.bar:21

CSPUtils.jsm creates: http://foo.bar:21
URI parser creates:   foo.bar://

The URI parser recognizes foo.bar as a scheme, which I am not sure is correct. Anyone knows?

[1] https://tbpl.mozilla.org/?tree=Try&rev=fe2605f32fd3
[2] http://mxr.mozilla.org/mozilla-central/source/content/base/test/unit/test_csputils.js#250

Comment 8

[Garrett Robinson [:grobinson]]

Comment on attachment 814978 [details] [diff] [review]
bug_916054_csp_should_ignore_path_tests.patch

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

Regarding the test filenames: see this recent thread on firefox-dev [1]. tl;dr test filenames shouldn't just be a bug number. I know this is the convention for a lot of the CSP tests, but let's start moving in the direction of more descriptive test filenames (make sure the bug # is still included in the mochitest HTML's title and comments).

[1] https://mail.mozilla.org/pipermail/firefox-dev/2013-September/000957.html

::: content/base/test/csp/test_CSP_bug916054.html
@@ +16,5 @@
> +  <iframe style="width:100%;" id='frame5'></iframe>
> +  <iframe style="width:100%;" id='frame6'></iframe>
> +  <iframe style="width:100%;" id='frame7'></iframe>
> +  <iframe style="width:100%;" id='frame8'></iframe>
> +  <iframe style="width:100%;" id='frame9'></iframe>

Put these iframes in the div#content above. You should change the style to "visiblity:hidden" instead of "display:none" (otherwise certain things in the iframes won't run). This is more line with the typical Mochitest conventions (but not with some of the CSP test conventions - let's make them better, starting now!)

@@ +24,5 @@
> +SimpleTest.waitForExplicitFinish();
> +
> +function loadNextTest(frame, test) {
> +  document.getElementById(frame).addEventListener('load', test, false);
> +  document.getElementById(frame).src = 'file_CSP_bug916054.sjs';

Can you reuse the more generic "return a file with a given CSP" code from file_CSP_bug888172.sjs? You should cp it to CSPTestServer.sjs or something similar, and extend it so you can also pass the filename of the HTML to load as a query string as well. This would be a good first step to our general goal of "DRY out and develop a resusable framework for CSP tests".

@@ +133,5 @@
> +  catch (e) {
> +    ok(false, "ERROR: could not access content in test 9!");
> +  }
> +  SimpleTest.finish();
> +}

You should be able to DRY all of the test* functions with a for loop. Rather than having each test call the next, solve the async issue with either Promises or Task.js. Let me know if you need any help working with these abstractions (I just learned more about them at the Summit and am eager to share my knowledge!).

Comment 9

[Garrett Robinson [:grobinson]]

Comment on attachment 814980 [details] [diff] [review]
bug_916054_csp_should_ignore_path.patch

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

::: content/base/src/CSPUtils.jsm
@@ +969,5 @@
> +        }
> +        uri = gIoService.newURI(uri, null, null);
> +      }
> +    }
> +    catch (e) {

This kind of logic belonds in the CSPSource constructor (CSPSource.create), not here. Have you checked out CSPSource.fromURI? I don't think it's being used right now, but any special case logic should probably go in/around there.

I'm not convinced that newURI is better than regular expressions. It is not designed for our use case, and there are a lot of edge cases (* in ports and hosts, the weird case you mention in Comment 7) already. I also like the regular expressions because that is how the spec is defined.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_916054_csp_should_ignore_path_tests.patch

(In reply to Garrett Robinson [:grobinson] from comment #8)
> Regarding the test filenames: see this recent thread on firefox-dev [1].
> tl;dr test filenames shouldn't just be a bug number. I know this is the
> convention for a lot of the CSP tests, but let's start moving in the
> direction of more descriptive test filenames (make sure the bug # is still
> included in the mochitest HTML's title and comments).
> 
> [1] https://mail.mozilla.org/pipermail/firefox-dev/2013-September/000957.html

Agreed, we definitely use more descriptive filenames from now on!
 
> ::: content/base/test/csp/test_CSP_bug916054.html
> > +  <iframe style="width:100%;" id='frame9'></iframe>
> 
> Put these iframes in the div#content above. You should change the style to
> "visiblity:hidden" instead of "display:none" (otherwise certain things in
> the iframes won't run). This is more line with the typical Mochitest
> conventions (but not with some of the CSP test conventions - let's make them
> better, starting now!)

One iframe in fact suffices to load and test all the different CSPs used to serve an html file!

> @@ +24,5 @@
> > +SimpleTest.waitForExplicitFinish();
> > +
> > +function loadNextTest(frame, test) {
> > +  document.getElementById(frame).addEventListener('load', test, false);
> > +  document.getElementById(frame).src = 'file_CSP_bug916054.sjs';
> 
> Can you reuse the more generic "return a file with a given CSP" code from
> file_CSP_bug888172.sjs? You should cp it to CSPTestServer.sjs or something
> similar, and extend it so you can also pass the filename of the HTML to load
> as a query string as well. This would be a good first step to our general
> goal of "DRY out and develop a resusable framework for CSP tests".

I added a file file_csp_testserver.sjs, which serves a given file (?file=...) and csp (&csp=...) in the query. This is a very generic server side js file which we can reuse for all our CSP tests from now on. No more ^header^ files, no more sjs files needed in the csp folder :-)
 
> @@ +133,5 @@
> > +  catch (e) {
> > +    ok(false, "ERROR: could not access content in test 9!");
> > +  }
> > +  SimpleTest.finish();
> > +}
> 
> You should be able to DRY all of the test* functions with a for loop. Rather
> than having each test call the next, solve the async issue with either
> Promises or Task.js. Let me know if you need any help working with these
> abstractions (I just learned more about them at the Summit and am eager to
> share my knowledge!).

I think it's almost an overengineering for the purpose of this tests to use Promises. Certainly, Promises are very relevant and useful is some scenarios, but I guess for testing the CSP parser, it suffices to use |load| events.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_916054_csp_should_ignore_path_regexp.patch

(In reply to Garrett Robinson [:grobinson] from comment #9)
> ::: content/base/src/CSPUtils.jsm
> @@ +969,5 @@
> > +        }
> > +        uri = gIoService.newURI(uri, null, null);
> > +      }
> > +    }
> > +    catch (e) {
> 
> This kind of logic belonds in the CSPSource constructor (CSPSource.create),
> not here. Have you checked out CSPSource.fromURI? I don't think it's being
> used right now, but any special case logic should probably go in/around
> there.

Hi Garret, I didn't want to change the current structure of the Regexp in the beginning. If i would have dragged the creation of the URI object into CSPSource.fromURI, I would have had to change the regexp's which I wanted to avoid, because I thought we can eliminate them at some point.
Anyway, this attachment extends the regexp's to accept a path and a file leaving the usage of the URI object for the c++-rewrite :-)

> I'm not convinced that newURI is better than regular expressions. It is not
> designed for our use case, and there are a lot of edge cases (* in ports and
> hosts, the weird case you mention in Comment 7) already. I also like the
> regular expressions because that is how the spec is defined.

Agreed, let's go with an extension of the regexps for now, but we should investigate the problem with the URI-parser in Comment 7 anyway. Maybe, we do want to reuse the usage in the c++ rewrite. I know, that you are convinced that the URI parser is not designed for the use case of CSP-policies, but I think it would make our life a lot easier in the c++ world, but let's leave this discussion open for now.

Thanks for your help, Christoph!

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_916054_csp_should_ignore_path_unit_tests.patch

We can certainly extend the unit tests and also eliminate some of the mochitests which would allow for faster execution of the mochitest suite.

Comment 13

[Garrett Robinson [:grobinson]]

Comment on attachment 816695 [details] [diff] [review]
bug_916054_csp_should_ignore_path_tests.patch

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

::: content/base/test/csp/test_csp_regexp_parsing.html
@@ +86,5 @@
> +  try {
> +    document.getElementById("testframe").removeEventListener('load', test, false);
> +    var testframe = document.getElementById("testframe");
> +    var divcontent = testframe.contentWindow.document.getElementById('testdiv').innerHTML;
> +    is(divcontent, policy[0], "OK: Load should be " + policy[0] + " in test " + (counter - 1) + "!");

Nit: get rid of "OK: ". "should be" is the right language to use (because the same message will be printed whether the test passes or fails).

Comment 14

[Garrett Robinson [:grobinson]]

Comment on attachment 816698 [details] [diff] [review]
bug_916054_csp_should_ignore_path_regexp.patch

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

::: content/base/src/CSPUtils.jsm
@@ +53,5 @@
>  // port            = ":" ( 1*DIGIT / "*" )
>  const R_PORT       = new RegExp ("(\\:([0-9]+|\\*))", 'i');
>  
> +// path
> +const R_PATH       = new RegExp("(\\/(([a-zA-Z0-9\\-\\_]+)\\/?)*)", 'i');

There's a lot of unnecessary escaping in both of these regexes, which makes them hard to read. Since you're using the RegExp constructor, you don't need to escape /. You also don't need to escape - or _ (or special characters in general) in groups.

It looks like this isn't your fault, and you just carried over the convention from the earlier regexes. I don't know why those are written that way, but it's unnecessary.

See https://developer.mozilla.org/en/docs/Web/JavaScript/Guide/Regular_Expressions for reference.

@@ +1331,5 @@
>      // Host regex gets scheme, so remove scheme from aStr. Add 3 for '://'
>      if (schemeMatch)
>        hostMatch = R_HOSTSRC.exec(aStr.substring(schemeMatch[0].length + 3));
>  
> +    // Eliminate possible filename and path

More detail in this comment: "Bug 916054: in CSP 1.0, source-expressions that are paths should have the path after the origin ignored and only the origin enforced."

Comment 15

[Garrett Robinson [:grobinson]]

Patches look great! (sorry for the review delay) Please unbitrot and push to try.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_916054_csp_should_ignore_path_regexp.patch

(In reply to Garrett Robinson [:grobinson] from comment #14)
> @@ +53,5 @@
> >  // port            = ":" ( 1*DIGIT / "*" )
> >  const R_PORT       = new RegExp ("(\\:([0-9]+|\\*))", 'i');
> >  
> > +// path
> > +const R_PATH       = new RegExp("(\\/(([a-zA-Z0-9\\-\\_]+)\\/?)*)", 'i');
> 
> There's a lot of unnecessary escaping in both of these regexes, which makes
> them hard to read. Since you're using the RegExp constructor, you don't need
> to escape /. You also don't need to escape - or _ (or special characters in
> general) in groups.
> 
> It looks like this isn't your fault, and you just carried over the
> convention from the earlier regexes. I don't know why those are written that
> way, but it's unnecessary.

Well, in fact you do need the escaping. Also I would like to keep it in terms of consistency to the other regular expressions which the two new ones are part of.

> > +    // Eliminate possible filename and path
> 
> More detail in this comment: "Bug 916054: in CSP 1.0, source-expressions
> that are paths should have the path after the origin ignored and only the
> origin enforced."

Definitely fixed that.

Try looks good:
https://tbpl.mozilla.org/?tree=Try&rev=b629def09109
This is ready to be checked in.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_916054_csp_should_ignore_path_tests.patch

(In reply to Garrett Robinson [:grobinson] from comment #13)
> > +  try {
> > +    document.getElementById("testframe").removeEventListener('load', test, false);
> > +    var testframe = document.getElementById("testframe");
> > +    var divcontent = testframe.contentWindow.document.getElementById('testdiv').innerHTML;
> > +    is(divcontent, policy[0], "OK: Load should be " + policy[0] + " in test " + (counter - 1) + "!");
> 
> Nit: get rid of "OK: ". "should be" is the right language to use (because
> the same message will be printed whether the test passes or fails).

Definitely fixed that.

Carrying over r+ from grobinson.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_916054_csp_should_ignore_path_unit_tests.patch

Added reviewer to comment. Also carrying over r+ from grobinson.

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8367995 [details] [diff] [review]
bug_916054_csp_should_ignore_path_regexp.patch

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

::: content/base/src/CSPUtils.jsm
@@ +1357,5 @@
> +    // Bug 916054: in CSP 1.0, source-expressions that are paths should have
> +    // the path after the origin ignored and only the origin enforced.
> +    hostMatch[0] = hostMatch[0].replace(R_FILE, "");
> +    hostMatch[0] = hostMatch[0].replace(R_PATH, "");
> +

This might mess with the debugging output a little bit; if the received header has paths, calling toString() on it won't so there might be some confusion with what part of the policy is violated when reading the violation reports.

But since we're rewriting this parser anyway and it doesn't affect policy enforcement, it should be ok.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65dbffad01e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aa13aa6777c
https://hg.mozilla.org/integration/mozilla-inbound/rev/303b316dd325

Comment 21

[Garrett Robinson [:grobinson]]

Comment on attachment 8367995 [details] [diff] [review]
bug_916054_csp_should_ignore_path_regexp.patch

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

::: content/base/src/CSPUtils.jsm
@@ +1357,5 @@
> +    // Bug 916054: in CSP 1.0, source-expressions that are paths should have
> +    // the path after the origin ignored and only the origin enforced.
> +    hostMatch[0] = hostMatch[0].replace(R_FILE, "");
> +    hostMatch[0] = hostMatch[0].replace(R_PATH, "");
> +

Maybe we should be reporting a warning to the Web Console here to help developers?

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/65dbffad01e1
https://hg.mozilla.org/mozilla-central/rev/3aa13aa6777c
https://hg.mozilla.org/mozilla-central/rev/303b316dd325