Closed Bug 1147026 Opened 9 years ago Closed 9 years ago

Content Security Policy path matching doesn't ignore query strings

Categories

(Core :: DOM: Security, defect)

36 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 - wontfix
firefox37 - wontfix
firefox38 + fixed
firefox39 + fixed
firefox-esr31 --- unaffected

People

(Reporter: kontakt, Assigned: ckerschb)

References

()

Details

(Keywords: regression, testcase)

Attachments

(3 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150320202338

Steps to reproduce:

An HTML page containing an iframe with the src https://example.com/test/frame.html?foo=bar is delivered with the following header:
Content-Security-Policy: "default-src 'none'; base-uri https://example.com/test/; frame-ancestors 'none'; child-src https://example.com/test/frame.html; frame-src https://example.com/test/frame.html; plugin-types ; reflected-xss block;"


Actual results:

Output in the developer console:
Content Security Policy: The page's settings blocked the loading of a resource at https://example.com/test/frame.html?foo=bar ("frame-src https://example.com/test/frame.html").1 <unknown>

Same behaviour on Firefox Developer Edition (38.0a2 (2015-03-24))


Expected results:

The iframe should have been loaded.

Imho, this poses a very drastic problem since it makes any use of CSP impossible when you have to rely on information beeing passed to the embeded page (unless you want to risk crucial parts of the website not beeing loaded).
Please fix this before Firefox supports child-src, since then there would be no chance to use CSP path whitelisting for crucial content, ever.
(If it's done in time, one could use domain whitelisting via frame-src and path whitelisting via child-src)
Product: Firefox → Core
Severity: normal → major
Component: Untriaged → DOM: Security
Chris, thank you for filing this bug report. Can you please provide a minimized testcase so I can reproduce what you are seeing?
Severity: major → normal
Flags: needinfo?(mail)
Keywords: testcase-wanted
Flags: needinfo?(mail)
Attached image Screenshot
Keywords: testcase-wanted
Keywords: testcase
Chris, are you able to provide me something that I can load to test myself? A screenshot helps illustrate what you are seeing but does little to help me debug this locally.
QA Whiteboard: [triaged]
*Locally* is a bit tricky, considering we're talking about headers sent by a webserver ;) (I tried using a meta tag, but that didn't work at all (probably another CSP feature missing in Mozilla, but none I'd consider important)). The screenshot is just for demonstrating my point, the testcase can be found in the URL field above.

To be blunt: I am new to this site, I am *not* a Mozilla programmer, I'm just some guy who stumbled uppon a really anoying bug and wanted to share that information.
I don't know about the workflows in here, which seem very confusing, at least to me. I posted a bug. I provided a screenshot to prove my point. I spend some time creating a testcase (that *should* be sufficient for debugging) and linked it here. I explained, why I'd consider this bug *realy* important to fix. I think that should be enough, so please, just *fix* it already (shouldn't be *that* tricky), or at least change the status to NEW.
(In reply to Chris from comment #5)
> the testcase can be found in the URL field above.

Thanks, now I can at least reproduce what you're seeing in my browser. 

I tested a few different browsers and here's my results:
* Chrome 41: both iframes show "Framed"
* Firefox 34: both iframes show "Framed"
* Firefox 35: top iframe shows "Framed", bottom iframe shows blank
* Firefox 36: top iframe shows "Framed", bottom iframe shows blank
* Firefox 39: top iframe shows "Framed", bottom iframe shows blank

Based on this it looks like we might have regressed the behaviour with Firefox 35. I'll see if I can track down a more specific regression range.

FYI, in terms of workflow, we typically don't move a bug from UNCONFIRMED to NEW until it's been validated by a second person. Thanks again for reporting this issue and for providing the testcase. If you're able, please keep that testcase live until this bug has been resolved as we'll need to refer back to it.

[Tracking Requested - why for this release]: nominating to track since this is a regression.
Last Good: 2014-09-19 [c8e325eee9e1]
First Bad: 2014-09-20 [27253887d2cc]
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c8e325eee9e1&tochange=27253887d2cc

Last Good: 2014-09-25 [a6e93c843089]
First Bad: 2014-09-26 [99e24a19bfd6]
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a6e93c843089&tochange=99e24a19bfd6
Shot in the dark but this might have been caused by bug 808292. 

@ckerschb, can you please look in to this?
Flags: needinfo?(mozilla)
Given that we have shipped this regression twice, that is hasn't been a major issue, and that we're at the very end of the 37 cycle, I'm marking 36 and 37 as wontfix. I'm tracking this regression in 38+.
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #6)
> [...] If you're able, please keep that
> testcase live until this bug has been resolved as we'll need to refer back
> to it.

Thanks! No problem, I'll keep it live.
Thanks Chris for filing the bug and providing the testcase. Anthony, thanks for finding the regressing bug.

Interestingly we check for the 'ref' in the url (example.com/test#foo) but not for the query (example.com/test?val=foo) when doing the path-level matching in CSP.
Assignee: nobody → mozilla
Blocks: 808292
Status: NEW → ASSIGNED
Flags: needinfo?(mozilla)
Honestly, I thought that CloneIgnoringRef not only clears the ref but also the query string, but apparently not. Interestingly we have a testcase for ref but not for query.

The attached patch fixes the problem, but I am not sure if we can always QI an nsIURL. For all the testcases, it works. Unfortunately this is the only build in solution I found, otherwise we would have to manually parse the URI to filter out the query string, which by itself sounds error prone to me.
Attachment #8583609 - Flags: review?(dveditz)
Extending test_csp_path_matching.html, so we alternate between resource loads where the url includes a:
* ref (file_csp_path_matching.html), and a
* query (file_csp_path_mathing_incl_query.html.
Attachment #8583610 - Flags: review?(dveditz)
Comment on attachment 8583609 [details] [diff] [review]
bug_1147026_csp_path_ignore_querystr.patch

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

r=dveditz

::: dom/security/nsCSPUtils.cpp
@@ +411,5 @@
> +    // converting aUri into nsIURL so we can strip query and ref
> +    // example.com/test#foo     -> example.com/test
> +    // example.com/test?val=foo -> example.com/test
> +    nsCOMPtr<nsIURL> url = do_QueryInterface(aUri);
> +    if (!url) {

In most cases this will work, but not all nsIURI are nsIURLs. Anything that's a nsISimpleURI (data:, about:) won't be, nor will nsIMozIconURI. Then again you only fall into this block if the CSP says "MUST HAVE A PATH". I think we can safely assume that things that aren't nsStandardURL (which is a nsIURL) won't have paths to match, so failure to QI means it can't match.
Attachment #8583609 - Flags: review?(dveditz) → review+
Attachment #8583610 - Flags: review?(dveditz) → review+
https://hg.mozilla.org/mozilla-central/rev/2b7ca45e0968
https://hg.mozilla.org/mozilla-central/rev/fc15fab6d75b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Untracking for QA verification given we have testsuite coverage.

Thanks again Chris for reporting this issue.
Flags: qe-verify-
Christoph, could you fill the uplift request to aurora? Thanks
Flags: needinfo?(mozilla)
Comment on attachment 8583609 [details] [diff] [review]
bug_1147026_csp_path_ignore_querystr.patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 808292 - Implement path-level host-source matching to CSP

[User impact if declined]:
If path matching is used within CSP of a page and additionaly the path contains a query-string, then CSP will incorrectly *block* the request even though the resource should be allowed to load.

[Describe test coverage new/current, TreeHerder]:
Added a testcase which landed on mc with this patch.

[Risks and why]:
Low - only affects pages with CSP and in addition only when the CSP header relies on path-matching.

[String/UUID change made/needed]:
No
Flags: needinfo?(mozilla)
Attachment #8583609 - Flags: approval-mozilla-aurora?
Comment on attachment 8583609 [details] [diff] [review]
bug_1147026_csp_path_ignore_querystr.patch

[Triage Comment]
Sorry, I meant beta. 
Taking it as we have time to fix potential regressions.
Attachment #8583609 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Should be in 38 beta 3
You need to log in before you can comment on or make changes to this bug.