Closed Bug 1229639 Opened 9 years ago Closed 8 years ago

Content Security Policy: matching %-encoded path fails

Categories

(Core :: DOM: Security, defect, P3)

Product:
Core
Component:
DOM: Security
Version:
44 Branch
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: kontakt, Assigned: hchang)

References

(
URL
)

Details

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

Attachments

(3 files)

Screenshot
246.81 KB, image/png
Details
Bug 1229639 - Part 1: Match CSP host source with percent-decoded URI.
58 bytes, text/x-review-board-request
ckerschb
: review+
Details
Bug 1229639 - Part 2: Test case.
58 bytes, text/x-review-board-request
ckerschb
: review+
Details
Attached image 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.
URL: https://demos.scheurle.info/firefox/c...
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: nobody → hchang
Whiteboard: [domsecurity-backlog1] → [domsecurity-backlog1][domsecurity-active]
Still requires a test case but just submit a patch for review first.
Status: NEW → ASSIGNED
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 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 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.
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
https://hg.mozilla.org/mozilla-central/rev/e8f963c5afd2
https://hg.mozilla.org/mozilla-central/rev/f0df0335a049
Status: ASSIGNED → RESOLVED
Closed: 8 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.

Attachment

General

Creator:
Created:
Updated:
Size: