1322111 - importScripts should be governed by script-src in Web Workers

Status: RESOLVED FIXED

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

Description

[gandhi.rahul93]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.99 Safari/537.36

Steps to reproduce:

1) Create a worker with a file which has CSP set to script-src 'self'
2) Inside the Web Worker Script, we use importScripts to import file from other origin.




Actual results:

It works.


Expected results:

According to the spec, importScripts should be governed by script-src in Web Workers. 
However, importScripts seems to be honoring child-src on Firefox (tested on 50.0.2). 

Chrome and Edge are honoring script-src.

https://www.w3.org/TR/CSP2/#directive-script-src

Comment 1

[gandhi.rahul93]

importScripts seems to be honoring child-src. This exposes some vulnerabilities; to block the use of different origin in importScripts, we would have to set child-src to 'self' This would allow the creation of workers in that domain which could otherwise have been blocked if script-src was honored to block importScripts and child-src to null.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

There is probably more to it, but switching this to SCRIPT_SRC_DIRECTIVE lets me pass my WPT test:

https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#233

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

It seems that perhaps we should implement worker-src instead of script-src for workers.  That is what the latest specs mention:

https://w3c.github.io/webappsec-csp/#worker-src

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Oh, that document also says:

"A worker-src directive has been added, deferring to script-src if not present (which likewise defers to default-src in turn)."

So script-src should still be honored it looks like.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add TYPE_INTERNAL_WORKER_IMPORT_SCRIPTS content policy type. r=ckerschb

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make importScripts() use TYPE_INTERNAL_WORKER_IMPORT_SCRIPTS. r=ckerschb

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Update WPT test expectations. r=ckerschb

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1ba0dde7a3eec1d9e43afa9802abfb66ac967d2

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8837217 [details] [diff] [review]
P1 Add TYPE_INTERNAL_WORKER_IMPORT_SCRIPTS content policy type. r=ckerschb

Add an nsIContentPolicy type to distinguish importScripts().

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8837218 [details] [diff] [review]
P2 Make importScripts() use TYPE_INTERNAL_WORKER_IMPORT_SCRIPTS. r=ckerschb

Make worker ScriptLoader use the new nsIContentPolicy type for importScripts() channels.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8837219 [details] [diff] [review]
P3 Update WPT test expectations. r=ckerschb

Update WPT tests to expect new cases to pass.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Update the Cache API schema to account for new nsIContentPolicy type. r=asuth

Since we're adding a new nsIContentPolicy type that can be stored in the Cache db we need to update the schema version to avoid older builds trying to interpret this value.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8837298 [details] [diff] [review]
P4 Update the Cache API schema to account for new nsIContentPolicy type. r=asuth

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

Affirming the downgrade-precluding schema bump is appropriate because the past can't handle enumeration-styled values from the future.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8837217 [details] [diff] [review]
P1 Add TYPE_INTERNAL_WORKER_IMPORT_SCRIPTS content policy type. r=ckerschb

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

looks good, r=me

::: extensions/permissions/nsContentBlocker.cpp
@@ +64,5 @@
> +                                    "", // TYPE_INTERNAL_IMAGE_PRELOAD
> +                                    "", // TYPE_INTERNAL_STYLESHEET
> +                                    "", // TYPE_INTERNAL_STYLESHEET_PRELOAD
> +                                    "", // TYPE_INTERNAL_IMAGE_FAVICON
> +                                    "", // TYPE_INTERNAL_WORKERS_IMPORT_SCRIPTS

thanks for updating.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8837218 [details] [diff] [review]
P2 Make importScripts() use TYPE_INTERNAL_WORKER_IMPORT_SCRIPTS. r=ckerschb

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

thanks, r=me

::: dom/workers/ScriptLoader.cpp
@@ +171,5 @@
>    }
>  
> +  nsContentPolicyType contentPolicyType =
> +    aIsMainScript ? aContentPolicyType
> +                  : nsIContentPolicy::TYPE_INTERNAL_WORKER_IMPORT_SCRIPTS;

I suppose that is a valid distinguisher between regular script and script that is imported using importScripts, but looks reasonable to me.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8837219 [details] [diff] [review]
P3 Update WPT test expectations. r=ckerschb

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

r=me

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30f248917596
P1 Add TYPE_INTERNAL_WORKER_IMPORT_SCRIPTS content policy type. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/1af6b50e212f
P2 Make importScripts() use TYPE_INTERNAL_WORKER_IMPORT_SCRIPTS. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/fca96fba3e51
P3 Update WPT test expectations. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f45618754fe
P4 Update the Cache API schema to account for new nsIContentPolicy type. r=asuth

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/30f248917596
https://hg.mozilla.org/mozilla-central/rev/1af6b50e212f
https://hg.mozilla.org/mozilla-central/rev/fca96fba3e51
https://hg.mozilla.org/mozilla-central/rev/8f45618754fe