Closed Bug 1302667 Opened 3 years ago Closed 2 years ago

CSP: Implement 'worker-src'

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [domsecurity-active])

Attachments

(6 files, 5 obsolete files)

17.48 KB, patch
dveditz
: review+
baku
: review+
Details | Diff | Splinter Review
1.50 KB, patch
dveditz
: review+
Details | Diff | Splinter Review
8.24 KB, patch
dveditz
: review+
Details | Diff | Splinter Review
5.26 KB, patch
dveditz
: review+
Details | Diff | Splinter Review
1.97 KB, patch
dveditz
: review+
Details | Diff | Splinter Review
1.89 KB, patch
dveditz
: review+
Details | Diff | Splinter Review
Blocks: csp-w3c-3
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
I've upstreamed Blink's `worker-src` tests to WPT: https://github.com/w3c/web-platform-tests/commit/45185609658ce4ee3576db5a5f362d9e0274bd6b. They require `SecurityPolicyViolationEvent`, however...
Depends on: 1320924
Blocks: 1320924
No longer depends on: 1320924
Any news regarding this issue?

Currently Firefox says 'frame-src' is deprecated and suggests using 'child-src' instead. Chrome deprecated 'child-src' and will remove it in M60 (around August 2017 - next month). Chrome suggests using 'script-src', but the link actually redirects to the 'worker-src' feature status page.

The current situation is less than optimal as it is kind of confusing.
Duplicate of this bug: 1399284
Duplicate of this bug: 1399284
FWIW: I'm waiting on deprecation in Chrome for y'all's implementation. Or Safari's implementation. Or anyone else's. *cough* I believe this reflects what we agreed to in WebAppSec. It would be lovely for y'all to follow through on that to reduce developer confusion. :)
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
Blocks: 1288896
Kate,

CSP3 defines worker-src [0], which is supposed to govern workers. Previously workers were governed by child-src though. To make it a little more complicated, there is also frame-src which governs frames. Previously frames were also governed by child-src though. To sum it up, here is what we have to implement within this bug.

child-src by itself is deprecatd but will be enforced:
  * for workers (if worker-src is not explicitly specified)
  * for frames  (if frame-src is not explicitly specified)

I added parser tests to make sure we can handle worker-src within the CSPParser. In addition I added a mochitest for workers, to make sure workers, shared workers as well as dedicated workers are governed by worker-src. In addition those tests include child-src 'none' to make sure child-src is discarded in case worker-src is specified. In a similar fashion I added an explicit test for frame-src, to make sure frames are governed by frame-src. Please note that I haven't changed any of the child-src tests in our tree, those should keep working even though in CSP2 frame-src was deprecated in favor of child-src ;-)

I was able to update some of the worker-src related web-platform tests, but not all of them, because they rely on Security Policy violation events [1] and we haven't implemented Security Policy violation events yet [2]. Once we have those violation events we can also remove:
* testing/web-platform/meta/content-security-policy/worker-src/dedicated-none.sub.html.ini
* testing/web-platform/meta/content-security-policy/worker-src/service-none.https.sub.html.ini
* testing/web-platform/meta/content-security-policy/worker-src/shared-none.sub.html.ini

I verified the following, if a page ships a CSP including child-src, then we log the following message to the console:
> Content Security Policy: Directive ‘child-src’ has been deprecated. Please use directive ‘worker-src’ to control workers, or directive ‘frame-src’ to control frames respectively.

Finally I manually verified that we log correctly to the console in case when blocking some worker. E.g. you see:
> Content Security Policy: The page’s settings blocked the loading of a resource at https://test1.example.com/tests/dom/security/test/csp/file_service_worker_src.js (“worker-src https://example.com”).

[0] https://w3c.github.io/webappsec-csp/#directive-worker-src
[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/content-security-policy/support/testharness-helper.js#90
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1037335
Attachment #8911169 - Flags: review?(kmckinley)
Attachment #8911170 - Flags: review?(kmckinley)
Attachment #8911174 - Flags: review?(kmckinley)
Attached patch bug_1302667_test_frame_src.patch (obsolete) — Splinter Review
Attachment #8911175 - Flags: review?(kmckinley)
Attachment #8911176 - Flags: review?(kmckinley)
In the absence of worker-src all workers should actually be governed by script-src. I filed Bug 1402332 to make sure that script-src 'none' actually does not allow to run any scripts in case there is no explicit worker-src directive.
Blocks: 1402332
No longer blocks: 1402332
Blocks: 1402332
Comment on attachment 8911169 [details] [diff] [review]
bug_1302667_implement_worker_src.patch

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

lgtm

::: dom/security/nsCSPUtils.cpp
@@ +1265,5 @@
> +  if (aContentType == nsIContentPolicy::TYPE_INTERNAL_WORKER ||
> +      aContentType == nsIContentPolicy::TYPE_INTERNAL_SHARED_WORKER ||
> +      aContentType == nsIContentPolicy::TYPE_INTERNAL_SERVICE_WORKER) {
> +    return mRestrictWorkers;
> +  }  

whitespace
Attachment #8911169 - Flags: review?(kmckinley) → review+
Attachment #8911170 - Flags: review?(kmckinley) → review+
Attachment #8911174 - Flags: review?(kmckinley) → review+
Attachment #8911175 - Flags: review?(kmckinley) → review+
Attachment #8911176 - Flags: review?(kmckinley) → review+
Dan, as discussed within the 'intent to ship' I think it makes the most sense to implement the following fallback mechanism:
* worker-src
* child-src
* script-src
* default-src

Even though I agree there is some insanity, it probably makes the most sense to implement it that way. Just discussed things with Mike West and Chrome is also shipping that behavior.

Any strong objections (besides the maintenance overhead) to ship it that way?
Flags: needinfo?(dveditz)
Chrome is shipping that way? Compatibility is good, and updating the spec to say that is even better. Can we get Mike to commit to that ordering and updating the spec in one of the github issues even if he doesn't have time to update the spec itself? If so then that sounds fine.

Frames can't work that way though. frame-src => child-src => default-src must be the order there. If we start having script-src control frames just because you don't have a frame-src or child-src that would be pretty bad. Should make sure our tests will check for that case. Should work fine if our fallback is driven by the load type/fetch, but if our fallbacks are tied to directives it could go the wrong way.
Flags: needinfo?(dveditz)
We'll need more tests to flesh out the full workers fallback path, too. You've only got tests for worker-src and child-src here.
(In reply to Daniel Veditz [:dveditz] from comment #14)
> Chrome is shipping that way? Compatibility is good, and updating the spec to
> say that is even better. Can we get Mike to commit to that ordering and
> updating the spec in one of the github issues even if he doesn't have time
> to update the spec itself? If so then that sounds fine.

I added a comment to the github issue you raised:
  https://github.com/w3c/webappsec-csp/issues/239

I'll have to update our patches to reflect that change and also have to add more tests for that fallback mechanism.

> Frames can't work that way though. frame-src => child-src => default-src
> must be the order there.

Yes, and I'll add tests for that as well.
Blocks: 1406083
Blocks: 1409706
Dan,

CSP3 defines worker-src [0], which is supposed to govern all kinds of workers. Previously workers were governed by child-src though. To make it a little more complicated, there is also frame-src which governs frames. Previously frames were also governed by child-src though.

To sum it up, here is what we have to implement within this bug:
child-src by itself is deprecatd but will be enforced
  * for workers (if worker-src is not explicitly specified)
  * for frames  (if frame-src is not explicitly specified)
  * additionaly we have to add a fallback from worker-src to child-src to script-src to default-src (Chrome is going to ship the same behavior, see [3])

I added parser tests to make sure we can handle worker-src within the CSPParser. In addition I added a mochitest for workers, to make sure workers, shared workers as well as dedicated workers are governed by worker-src, child-src or script-src (using all the different fallbacks). In a similar fashion I added an explicit test for frame-src, to make sure frames are governed by frame-src (also testing the fallback to child-src). Please note that had to update test_child-src_worker.html because script-src now governs workers again even though child-src did not fall back to script-src in our previous implementation.

I was able to update some of the worker-src related web-platform tests, but not all of them, because they rely on Security Policy violation events [1] and we haven't implemented Security Policy violation events yet [2]. Once we have those violation events we can also remove:
* testing/web-platform/meta/content-security-policy/worker-src/dedicated-none.sub.html.ini
* testing/web-platform/meta/content-security-policy/worker-src/service-none.https.sub.html.ini
* testing/web-platform/meta/content-security-policy/worker-src/shared-none.sub.html.ini

I verified the following, if a page ships a CSP including child-src, then we log the following message to the console:
> Content Security Policy: Directive ‘child-src’ has been deprecated. Please use directive ‘worker-src’ to control workers, or directive ‘frame-src’ to control frames respectively.

Finally I manually verified that we log correctly to the console in case when blocking some worker. E.g. you see:
> Content Security Policy: The page’s settings blocked the loading of a resource at https://test1.example.com/tests/dom/security/test/csp/file_service_worker_src.js (“worker-src https://example.com”).

Ultimately, I think we need web-platform tests for that fallback behavior, so I filed Bug 1409706 to write those tests.

[0] https://w3c.github.io/webappsec-csp/#directive-worker-src
[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/content-security-policy/support/testharness-helper.js#90
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1037335
[3] https://github.com/w3c/webappsec-csp/issues/239
Attachment #8911169 - Attachment is obsolete: true
Attachment #8911170 - Attachment is obsolete: true
Attachment #8911174 - Attachment is obsolete: true
Attachment #8911175 - Attachment is obsolete: true
Attachment #8911176 - Attachment is obsolete: true
Attachment #8919696 - Flags: review?(dveditz)
Duplicate of this bug: 1402332
Attachment #8919701 - Flags: review?(dveditz) → review+
Attachment #8919697 - Flags: review?(dveditz) → review+
Comment on attachment 8919696 [details] [diff] [review]
bug_1302667_implement_worker_src.patch

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

r=dveditz (with optional l10n comment/suggestion)

::: dom/locales/en-US/chrome/security/csp.properties
@@ +113,5 @@
>  # %1$S is the name of the duplicate directive
>  duplicateDirective = Duplicate %1$S directives detected.  All but the first instance will be ignored.
> +# LOCALIZATION NOTE (deprecatedChildSrcDirective):
> +# %1$S is the value of the deprecated directive.
> +deprecatedChildSrcDirective = Directive ‘%1$S’ has been deprecated. Please use directive ‘worker-src’ to control workers, or directive ‘frame-src’ to control frames respectively.

We never used the old string in more than one place, but in theory it was generic so we could have. This expanded version _only_ works for 'child-src' given the second half of the message. Why not get rid of the %1$S replacement text and just say that?

Thanks for using a new label when you change the text -- the l10n folks appreciate it.

Maybe add to the localization note that 'worker-src' and 'frame-src' should not be translated? I hope it's obvious enough as it is, but I've seen people make those kinds of l10n notes for things I thought were obvious.
Attachment #8919696 - Flags: review?(dveditz) → review+
Attachment #8919698 - Flags: review?(dveditz) → review+
Attachment #8919700 - Flags: review?(dveditz) → review+
Attachment #8919699 - Flags: review?(dveditz) → review+
Comment on attachment 8919696 [details] [diff] [review]
bug_1302667_implement_worker_src.patch

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

Baku, could you sign off on the webidl changes within this patch?
Attachment #8919696 - Flags: review?(amarchesini)
Comment on attachment 8919696 [details] [diff] [review]
bug_1302667_implement_worker_src.patch

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

r+ for the webidl part
Attachment #8919696 - Flags: review?(amarchesini) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70ccfda99dbc
CSP: Implement 'worker-src'. r=baku,dveditz,mckinley
https://hg.mozilla.org/integration/mozilla-inbound/rev/e982481dbf2c
CSP: Add Parser test for 'worker-src'. r=dveditz,mckinley
https://hg.mozilla.org/integration/mozilla-inbound/rev/21c73f9d8fac
CSP: Test 'worker-src'. r=dveditz,mckinley
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ec6b8bf8c6c
CSP: Test 'frame-src'. r=dveditz,mckinley
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff86e185e09d
CSP: Update test_child-src_worker.html because child-src falls back to script-src. r=dveditz,mckinley
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca6ae38c0432
Update wpt tests for worker-src. r=dveditz,mckinley
(In reply to Cristina Coroiu [:ccoroiu] from comment #29)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> e06653ee31c4d9853ddfef1c69deb8ea5cc83f74
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=ca6ae38c0432883413655ac245445d2a9d8bfb6f&filter-
> resultStatus=usercancel&filter-resultStatus=runnable&filter-
> resultStatus=retry&filter-resultStatus=testfailed&filter-
> resultStatus=busted&filter-resultStatus=exception&selectedJob=140653862

Thanks. I l.ooked at it and the tests works locally on mac and linux. I looked through the logs and it seems the tests just requires a little longer timeout. Sometimes we see the second blocked shared worker and sometimes not. Ultimately the test should receive 3 messages of blocked shared workers. Let's see if that causes the problem.
Flags: needinfo?(ckerschb)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbb4d0097263
CSP: Implement 'worker-src'. r=baku,dveditz,mckinley
https://hg.mozilla.org/integration/mozilla-inbound/rev/2219116fd832
CSP: Add Parser test for 'worker-src'. r=dveditz,mckinley
https://hg.mozilla.org/integration/mozilla-inbound/rev/8397d379f6a9
CSP: Test 'worker-src'. r=dveditz,mckinley
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aa2babaaa5d
CSP: Test 'frame-src'. r=dveditz,mckinley
https://hg.mozilla.org/integration/mozilla-inbound/rev/b492ad7ed794
CSP: Update test_child-src_worker.html because child-src falls back to script-src. r=dveditz,mckinley
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6a6606f3ea7
Update wpt tests for worker-src. r=dveditz,mckinley
I've checked on MDN, and we've already documented worker-src:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/worker-src

I've also added a note to the Fx58 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/58#HTTP

Let me know if this all looks OK, or if there's anything else you think needs adding.
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #35)
> I've checked on MDN, and we've already documented worker-src:
> 
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-
> Policy/worker-src

Thanks for adding; could you please update the fallback to something like:

If this directive is absent, the user agent will first look for the child-src directive, then the script-src directive and finally for default-src directive when governing worker execution.
Flags: needinfo?(cmills)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #36)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #35)
> > I've checked on MDN, and we've already documented worker-src:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-
> > Policy/worker-src
> 
> Thanks for adding; could you please update the fallback to something like:
> 
> If this directive is absent, the user agent will first look for the
> child-src directive, then the script-src directive and finally for
> default-src directive when governing worker execution.

OK, updated.
Flags: needinfo?(cmills)
Duplicate of this bug: 1320924
Duplicate of this bug: 1406083
See Also: → 1486331
You need to log in before you can comment on or make changes to this bug.