1032303 - CSP - Keep FULL STOP when matching *.foo.com to disallow loads from foo.com

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

According to the spec a policy of *.foo.com should not allow loads from foo.com, see [1]:

If the first character of the source expression's host is an U+002A ASTERISK character (*) and the remaining characters, including the leading U+002E FULL STOP character (.), are not a case insensitive match for the rightmost characters of uri-host, then return does not match.

[1] http://www.w3.org/TR/CSP11/#matching

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1032303.patch

The attached patch trims off the leading "*" but keeps the following full stop (.). Since there is an assertion right before the Trim() so that the second character is the full stop, I think it makes more sense to use Trim() instead of Substring.

Passes TestCSPParser and mochis locally, do you want me to run it on TRY as well?

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8448119 [details] [diff] [review]
bug_1032303.patch

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

Don't worry about try, this won't affect non-csp tests.

Can you add a test that checks the a.foo.com and foo.com cases for *.foo.com?  I want to see the test fail without this patch applied.

::: content/base/src/nsCSPUtils.cpp
@@ +326,5 @@
>    NS_ENSURE_SUCCESS(rv, false);
>  
>    // Check it the allowed host starts with a wilcard.
>    if (mHost.First() == '*') {
>      // Eliminate leading "*." and check if uriHost ends with defined mHost.

Nit: please fix this comment now that you are not eliminating the dot.

@@ +332,5 @@
>  
>      nsString wildCardHost = mHost;
> +    // Trimming off the leading *, but keeping the FULL STOP (.), according to the spec.
> +    // see http://www.w3.org/TR/CSP11/#matching
> +    wildCardHost.Trim("*");

Substring(wch, 1, wch.Length() - 1) is probably more efficient since you already know the first char is '*'.

Also, the assertion won't cause a failure in opt builds so you need an actual check/return when the '*' is not followed by '.'.  Add a test case for *foo.com (invalid) too.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_925004_v2.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> Comment on attachment 8448119 [details] [diff] [review]
> bug_1032303.patch
> 
> Review of attachment 8448119 [details] [diff] [review]:
> -----------------------------------------------------------------

> Can you add a test that checks the a.foo.com and foo.com cases for
> *.foo.com?  I want to see the test fail without this patch applied.

Tests will follow in a separate patch.
 
> ::: content/base/src/nsCSPUtils.cpp
> @@ +326,5 @@
> >    NS_ENSURE_SUCCESS(rv, false);
> >  
> >    // Check it the allowed host starts with a wilcard.
> >    if (mHost.First() == '*') {
> >      // Eliminate leading "*." and check if uriHost ends with defined mHost.
> 
> Nit: please fix this comment now that you are not eliminating the dot.

Good catch - did that.

> @@ +332,5 @@
> >  
> >      nsString wildCardHost = mHost;
> > +    // Trimming off the leading *, but keeping the FULL STOP (.), according to the spec.
> > +    // see http://www.w3.org/TR/CSP11/#matching
> > +    wildCardHost.Trim("*");
> 
> Substring(wch, 1, wch.Length() - 1) is probably more efficient since you
> already know the first char is '*'.
> 
> Also, the assertion won't cause a failure in opt builds so you need an
> actual check/return when the '*' is not followed by '.'.  Add a test case
> for *foo.com (invalid) too.

In fact, the parser would not allow a policy which starts with a "*" and no (.) thereafter. Anyway, substring is probably the better solution.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1032303_v2.patch

rrrh - attached wrong patch previously!

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8448310 [details] [diff] [review]
bug_1032303_v2.patch

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

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Tests will follow in a separate patch.

Great.

> In fact, the parser would not allow a policy which starts with a "*" and no
> (.) thereafter. Anyway, substring is probably the better solution.

How right you are.  Assertion is a good thing to catch parser bugs, and thus no need to double-check the parsed source.  :)

Looks good.  r=me, do not land without tests.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1032303_tests.patch

I added a description of the tests in test_leading_wildcard.html. Please note that I don't use observers (csp-on-violate-policy) to verify that one script was not executed (blocked by CSP), but rather rely on a div-container that both scripts try to update.

I think we already have enough tests that use the observer notifications. Also, using a variety of different test setups leads to better test coverage overall.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8448455 [details] [diff] [review]
bug_1032303_tests.patch

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

So the only thing that concerns me about this is the remote chance that the scripts won't be done loading/running by the time the load event fires.  That results in an inaccurate "blocked" detection since we're not monitoring "csp-on-violate-policy".

Can you add an observer to the parent to trigger observe blocking situations to differentiate between "not done" and "blocked"?

Also, if you want to avoid adding two trivial files to the tree, you can use file_CSP.sjs to echo back scripts:
<script src="http://example.com/tests/content/base/test/csp/file_CSP.sjs?type=text/javascript&content=document.getElementById(%22testdiv%22).innerHTML%20%2B%3D%20%22blockedScriptRan%22%3B"></script>

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1032303_tests_v2.patch

As per our discussion in person, even though most of our tests follow the same schemate it's better to register observers and listen for CSP violations than using a div container that gets updated or not based on a script was allowed to run or not.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8448887 [details] [diff] [review]
bug_1032303_tests_v2.patch

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

::: content/base/test/csp/file_leading_wildcard.html
@@ +4,5 @@
> +    <title>Bug 1032303 - CSP - Keep FULL STOP when matching *.foo.com to disallow loads from foo.com</title>
> +  </head>
> +  <body>
> +  <script src="http://test1.example.com/tests/content/base/test/csp/leading_wildcard_allowed.js" ></script>
> +  <script src="http://example.com/tests/content/base/test/csp/leading_wildcard_blocked.js" ></script>

you need to update these URLs to point to files that exist.

::: content/base/test/csp/test_leading_wildcard.html
@@ +53,5 @@
> +      if (data.contains("leading_wildcard_allowed.js")) {
> +        ok (true, "CSP should allow file_leading_wildcard_allowed.js!");
> +        finishTest();
> +      }
> +      if (data.contains("leading_wildcard_blocked.js")) {

You'll have to update these .contains() calls too if you're using file_CSP.sjs?type=text/javascript...

@@ +69,5 @@
> +      if (asciiSpec.contains("leading_wildcard_allowed.js")) {
> +        ok (false, "CSP should not block file_leading_wildcard_allowed.js!");
> +        finishTest();
> +      }
> +      if (asciiSpec.contains("leading_wildcard_blocked.js")) {

These too if you're using file_CSP.sjs

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8448887 [details] [diff] [review]
bug_1032303_tests_v2.patch

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

::: content/base/test/csp/file_leading_wildcard.html
@@ +4,5 @@
> +    <title>Bug 1032303 - CSP - Keep FULL STOP when matching *.foo.com to disallow loads from foo.com</title>
> +  </head>
> +  <body>
> +  <script src="http://test1.example.com/tests/content/base/test/csp/leading_wildcard_allowed.js" ></script>
> +  <script src="http://example.com/tests/content/base/test/csp/leading_wildcard_blocked.js" ></script>

Actually, I'm an idiot.  You don't.  This is all fine, but please put a comment in this HTML to say the files don't exist (but it's fine since we are just watching for the request or the violation).

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: updated tests

Added a comment in the file that both scripts do not exists in the file system. Let's roll - ready to go!

Carrying over r+ from Sid!

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8eccf3af855
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f610977cc08

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b8eccf3af855
https://hg.mozilla.org/mozilla-central/rev/8f610977cc08