If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla33

Status

()

Core
DOM: Security
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Blocks: 1 bug)

unspecified
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
Created attachment 8448119 [details] [diff] [review]
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?
Attachment #8448119 - Flags: review?(sstamm)
(Assignee)

Updated

3 years ago
Assignee: nobody → mozilla
Blocks: 493857
Depends on: 951457
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.
Attachment #8448119 - Flags: review?(sstamm) → review-
(Assignee)

Comment 3

3 years ago
Created attachment 8448307 [details] [diff] [review]
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.
Attachment #8448119 - Attachment is obsolete: true
Attachment #8448307 - Flags: review?(sstamm)
(Assignee)

Comment 4

3 years ago
Created attachment 8448310 [details] [diff] [review]
bug_1032303_v2.patch

rrrh - attached wrong patch previously!
Attachment #8448307 - Attachment is obsolete: true
Attachment #8448307 - Flags: review?(sstamm)
Attachment #8448310 - Flags: review?(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.
Attachment #8448310 - Flags: review?(sstamm) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8448455 [details] [diff] [review]
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.
Attachment #8448455 - Flags: review?(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>
Attachment #8448455 - Flags: review?(sstamm) → review-
(Assignee)

Comment 8

3 years ago
Created attachment 8448887 [details] [diff] [review]
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.
Attachment #8448455 - Attachment is obsolete: true
Attachment #8448887 - Flags: review?(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
Attachment #8448887 - Flags: review?(sstamm) → review-
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).
Attachment #8448887 - Flags: review- → review+
(Assignee)

Comment 11

3 years ago
Created attachment 8448898 [details] [diff] [review]
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!
Attachment #8448887 - Attachment is obsolete: true
Attachment #8448898 - Flags: review+
(Assignee)

Comment 12

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8eccf3af855
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f610977cc08
Flags: in-testsuite+
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/b8eccf3af855
https://hg.mozilla.org/mozilla-central/rev/8f610977cc08
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.