Content Security Policy: matching %-encoded path fails

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM: Security
P3
major
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Chris Scheurle, Assigned: hchang)

Tracking

44 Branch
mozilla51
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [domsecurity-backlog1][domsecurity-active], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
Created attachment 8694547 [details]
Screenshot

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

Steps to reproduce:

Visit https://demos.scheurle.info/firefox/csp-url-matching/ .


Actual results:

Of the 2 images whitelistet in the page's CSP only the first is loaded. The second one, having a path containing %-encoded international characters, gets blocked.


Expected results:

Both images should be allowed to load.
(Reporter)

Updated

3 years ago
Severity: normal → major
Component: Untriaged → DOM: Security
OS: Unspecified → Mac OS X
Product: Firefox → Core
Hardware: Unspecified → x86
I can reproduce that problem, we have to investigate closer and fix.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [domsecurity-backlog]
Thanks Chris for filing those issues.
Priority: -- → P1
Priority: P1 → P3
Whiteboard: [domsecurity-backlog] → [domsecurity-backlog1]
(Assignee)

Updated

2 years ago
Assignee: nobody → hchang
Whiteboard: [domsecurity-backlog1] → [domsecurity-backlog1][domsecurity-active]
(Assignee)

Comment 3

2 years ago
Created attachment 8778173 [details]
Bug 1229639 - Part 1: Match CSP host source with percent-decoded URI.

Review commit: https://reviewboard.mozilla.org/r/69526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69526/
Attachment #8778173 - Flags: review?(ckerschb)
(Assignee)

Comment 4

2 years ago
Still requires a test case but just submit a patch for review first.
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED

Comment 5

2 years ago
mozreview-review
Comment on attachment 8778173 [details]
Bug 1229639 - Part 1: Match CSP host source with percent-decoded URI.

https://reviewboard.mozilla.org/r/69526/#review67610

Thanks for fixing; please provide a testcase before landing;

::: dom/security/nsCSPUtils.cpp:662
(Diff revision 1)
>    // http://www.w3.org/TR/CSP11/#source-list-paths-and-redirects
>    if (!aWasRedirected && !mPath.IsEmpty()) {
>      // 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);

It's unfortunate that nsIURL does not provide this functionality for us. Given that, I think your solution is the best option we have.

Please make sure to remove PercentDecodeStr() from nsCSPParser.h file as well.
Attachment #8778173 - Flags: review?(ckerschb) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8788406 [details]
Bug 1229639 - Part 2: Test case.

https://reviewboard.mozilla.org/r/76910/#review75088

looks good, r=me.
I suppose you have tested: https://demos.scheurle.info/firefox/csp-url-matching/ - right?

::: dom/security/test/csp/test_bug1229639.html:14
(Diff revision 1)
> +<body>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +
> +
> +</div>

nit: remove whitespace

::: dom/security/test/csp/test_bug1229639.html:30
(Diff revision 1)
> +
> +examiner.prototype  = {
> +  observe: function(subject, topic, data) {
> +    if (data === 'http://mochi.test:8888/tests/dom/security/test/csp/%24.js') {
> +      is(topic, "specialpowers-http-notify-request");
> +      SimpleTest.finish();

Please call window.examiner.remove() before SimpleTest.finish();
Attachment #8788406 - Flags: review?(ckerschb) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8788406 [details]
Bug 1229639 - Part 2: Test case.

https://reviewboard.mozilla.org/r/76910/#review75088

Thanks for the review :) Besides, I did test https://demos.scheurle.info/firefox/csp-url-matching/ and the image could be loaded successfully.

Comment 11

2 years ago
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8f963c5afd2
Part 1: Match CSP host source with percent-decoded URI. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/f0df0335a049
Part 2: Test case. r=ckerschb

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e8f963c5afd2
https://hg.mozilla.org/mozilla-central/rev/f0df0335a049
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.