importScripts should be governed by script-src in Web Workers

RESOLVED FIXED in Firefox 54

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gandhi.rahul93, Assigned: bkelly)

Tracking

(Blocks 1 bug)

50 Branch
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [domsecurity-backlog2])

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Updated

2 years ago
Component: Untriaged → Security
(Reporter)

Comment 1

2 years ago
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.

Updated

2 years ago
Component: Security → DOM: Security
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Whiteboard: [domsecurity-backlog2]
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
(Assignee)

Updated

2 years ago
See Also: → 1337543
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
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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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().
Attachment #8837217 - Flags: review?(ckerschb)
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.
Attachment #8837218 - Flags: review?(ckerschb)
Comment on attachment 8837219 [details] [diff] [review]
P3 Update WPT test expectations. r=ckerschb

Update WPT tests to expect new cases to pass.
Attachment #8837219 - Flags: review?(ckerschb)
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.
Attachment #8837298 - Flags: review?(bugmail)
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.
Attachment #8837298 - Flags: review?(bugmail) → review+
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.
Attachment #8837217 - Flags: review?(ckerschb) → review+
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.
Attachment #8837218 - Flags: review?(ckerschb) → review+
Comment on attachment 8837219 [details] [diff] [review]
P3 Update WPT test expectations. r=ckerschb

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

r=me
Attachment #8837219 - Flags: review?(ckerschb) → review+

Comment 16

2 years ago
Pushed by bkelly@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.