CSP erroneously "inherited" into dedicated workers

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

(Blocks 1 bug, {dev-doc-complete, site-compat})

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

Posted patch patch to fix (obsolete) — Splinter Review
The XHR and importScripts code in dedicated workers currently set the wrong loadingPrincipal when creating a channel. That means that we end up using the CSP of the parent document when doing network security checks. I.e. we "inherit" the policy of the parent document into workers when XHR and importScripts are used.

Patch to fix attached
Attachment #8685757 - Flags: review?(mozilla)
Assignee: nobody → jonas
Posted patch Patch v2Splinter Review
Just add missing file_main_worker.js
Attachment #8685757 - Attachment is obsolete: true
Attachment #8685757 - Flags: review?(mozilla)
Attachment #8685770 - Attachment is patch: true
Attachment #8685770 - Flags: review?(mozilla)
Comment on attachment 8685770 [details] [diff] [review]
Patch v2

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

Tests looks great, thanks for fixing. Please just address my nits and r=me.

::: dom/security/test/csp/file_redirects_main.html
@@ +20,5 @@
>                "style-src":  thisSite+page+"?testid=style-src&csp=1",
>                "worker":  thisSite+page+"?testid=worker&csp=1",
>                "xhr-src":  thisSite+page+"?testid=xhr-src&csp=1",
> +              "from-worker": thisSite+page+"?testid=from-worker&csp=1",
> +              "from-blob-worker": thisSite+page+"?testid=from-blob-worker&csp=1",

nit: since you are already modifying those files, can you also remove the redundant: '&csp=1'. This file is the only file that uses file_redirects_page.sjs, you could then also remove the 'if (query["csp"] == 1) {' from file_redirects_page.sjs - thanks.

::: dom/workers/ScriptLoader.cpp
@@ +122,5 @@
>      return NS_ERROR_DOM_SYNTAX_ERR;
>    }
>  
> +  if (parentDoc && parentDoc->NodePrincipal() != principal) {
> +    parentDoc = nullptr;

please add a comment, similar to the one you have in nsXMLHttpRequest.cpp, otherwise this is really confusing.
Attachment #8685770 - Flags: review?(mozilla) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e72c6539d6b5 for Mulet timeouts in dom/workers/test/test_csp.html, https://treeherder.mozilla.org/logviewer.html#?job_id=17113139&repo=mozilla-inbound, and desktop/Fennec timeouts in dom/security/test/csp/test_redirects.html and failures in dom/security/test/csp/test_worker_redirect.html, https://treeherder.mozilla.org/logviewer.html#?job_id=17114818&repo=mozilla-inbound
Comment on attachment 8686227 [details] [diff] [review]
more fixes

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

For the record: We are removing test_worker_redirect.html since it relies on the fact that policies are inherited for workers - which was a false assumption. We are not loosing any testcoverage because we have test_redirects.html.

::: dom/security/test/csp/file_redirects_resource.sjs
@@ +139,5 @@
>    // script that invokes XHR
>    if (query["res"] == "xhr") {
>      response.setHeader("Content-Type", "application/javascript", false);
> +    var resp = 'var x = new XMLHttpRequest();x.open("GET", "' + thisSite +
> +               resource+'?redir=other&res=xhr-resp&id=xhr-src-redir", false);\n' +

Ah I see, so this was not a redirect but tried to access that URL directly, but the CSP error still fired :-( Writing proper CSP tests is hard - thanks.

::: dom/security/test/csp/test_CSP.html
@@ +97,5 @@
>  window.examiner = new examiner();
>  
>  window.testResult = function(testname, result, msg) {
> +  // test already complete.... forget it... remember the first result.
> +  if (window.tests[testname] != -1)

ok - so this you are changing back to what was in the initial test.
Attachment #8686227 - Flags: review?(mozilla) → review+
Turns out we're not just doing the wrong thing for network policies. We also do the wrong thing for JS policies.

This makes us grab the CSP from the result-principal of the channel, rather than grab it from the calling JS.
Flags: needinfo?(jonas)
Attachment #8686752 - Flags: review?(mozilla)
Comment on attachment 8686752 [details] [diff] [review]
Also don't inherit JS policy

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

::: dom/base/nsContentUtils.cpp
@@ -6629,5 @@
> -  }
> -
> -  csp.forget(aCSP);
> -  return true;
> -}

Thanks for removing that piece of code - that is indeed really scary!!

::: dom/workers/test/test_csp.html^headers^
@@ +1,2 @@
>  Cache-Control: no-cache
> +Content-Security-Policy: default-src 'self' blob:

Thanks for adding blob tests here.
Attachment #8686752 - Flags: review?(mozilla) → review+
Posted patch Fix service-workers test (obsolete) — Splinter Review
Apparently we had tests for the wrong inheritance-behavior with SWs as well.

This kills the test which checks that eval() does not work in a SW if it was registered from a page with a CSP. And then applies CSP to the already-existing test which verifies that eval() works in SWs.
Attachment #8686903 - Flags: review?(mozilla)
Modify the test to verify that we actually apply the csp on the page correctly.
Attachment #8686903 - Attachment is obsolete: true
Attachment #8686903 - Flags: review?(mozilla)
Attachment #8686908 - Flags: review?(mozilla)
Attachment #8686949 - Attachment description: cspworker → Roll up of all patches
Attachment #8686908 - Flags: review?(mozilla) → review+

Comment 17

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/12a852867c16
https://hg.mozilla.org/mozilla-central/rev/88025b4a432b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.