CSP activated by default in Service Workers

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: salva, Assigned: jaoo)

Tracking

Trunk
FxOS-S1 (26Jun)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox41 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 2 obsolete attachments)

STR
1- Decompress the attached zip into a folder
2- Run a HTTP server for that folder
3- Observe the console. (See attachment)

Expected:
As there is no active CSP policy in the active page, the should be no active CSP policy in the worker.

Actual:
CSP is being applied on the worker.
Created attachment 8600251 [details]
CSP.png

Look at the bottom of the console to see the CSP warning. The same eval() is in the index.html but there it's not beend prevented.
Created attachment 8600252 [details]
cspsw.zip

You can use python2 -m SimpleHTTPServer or python3 -m http.server to start a simple HTTP server.
Blocks: 1153312
(Assignee)

Updated

3 years ago
OS: Unspecified → All
Hardware: Unspecified → All
Version: 40 Branch → Trunk
(Assignee)

Comment 3

3 years ago
I'll debug here a bit and let you know guys about my findings.
(Assignee)

Comment 4

3 years ago
Bent, after some debugging it seems `eval()` function is not allowed by default on workers (even without any CSP policy at all) (see [1]]. Could you confirm that please? Thanks.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#2237
Flags: needinfo?(bent.mozilla)
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> [1]
> https://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.
> cpp#2237

Sorry for the confusion, the code certainly looks like we set that to false by default, but we change its value here in the absence of CSP: https://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#5035
Flags: needinfo?(bent.mozilla)
(Assignee)

Comment 6

3 years ago
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #5)

Thanks for the answer Ben!
 
> Sorry for the confusion, the code certainly looks like we set that to false
> by default, but we change its value here in the absence of CSP:
> https://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.
> cpp#5035

This is weird. That piece of code is never run as I have seen and moreover there is such report (CSP violation one). I'll keep trying to figure out what's going here. Thanks.
It seems to be specifically related to eval() function. Setting dependency with v2 for tracking purposes, please change it if needed.
Blocks: 1131322

Updated

3 years ago
Whiteboard: [s3]
Target Milestone: --- → NGA S2 (12Jun)

Updated

3 years ago
Assignee: nobody → jaoo
Status: NEW → ASSIGNED
(Assignee)

Comment 8

3 years ago
Created attachment 8614171 [details] [diff] [review]
Part 1: Set both eval allowed and report CSP violation properties in the worker load info.

These bits fix the issue. I'm not totally sure that's the proper fix. Let's see.
(Assignee)

Comment 9

3 years ago
Created attachment 8614183 [details] [diff] [review]
Part 2: mochitest usefull for debugging the issue

Nikhil, the test in this file always passes and it shouldn't unless the part 1 patch is applied. If the part 1 patch is not applied the test passes too but I see this:

[65410] WARNING: '!aInstallEventSuccess', file /Volumes/firefoxos/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp, line 966
[65410] WARNING: 'NS_FAILED(aStatus)', file /Volumes/firefoxos/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp, line 144
[65410] WARNING: ServiceWorkerJob failed with error: NS_ERROR_DOM_ABORT_ERR

I guess the navigator.serviceWorker.register() returned promise shouldn't resolve.

If the part 1 patch is applied the error log above is gone. The test still passes.

So I see here a couple of issue. First one is the eval thing. When creating the service worker at ServiceWorkerManager::CreateServiceWorker() we use our own WorkerLoadInfo so -by default- eval is not allowed. Following the code we never end up calling WorkerPrivate::GetLoadInfo() -as we use our own load info- when the code sets the member mEvalAllowed true in case no CSP policy present. I fixed this issue by setting mEvalAllowed true in the load info we create but I'm not sure if this is a proper fix. Let me know please.

The second issue is the test always passing. I guess that shouldn't happen. Let me know how to deal with this. Should we file a bug for it?
Attachment #8614183 - Flags: feedback?(nsm.nikhil)
Comment on attachment 8614183 [details] [diff] [review]
Part 2: mochitest usefull for debugging the issue

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

Sorry I forgot about this feedback request.

register() is not affected by the install event result since 'install' event is sent to the SW independently of the register() call. If you want to test that eval is allowed in this test you should change the worker script to run eval at the top level instead of in an event handler.
Attachment #8614183 - Flags: feedback?(nsm.nikhil)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #10)
> Comment on attachment 8614183 [details] [diff] [review]
> Part 2: mochitest usefull for debugging the issue
> 
> Review of attachment 8614183 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the feedback!

> register() is not affected by the install event result since 'install' event
> is sent to the SW independently of the register() call.

Oh, I see. Understood.

> If you want to test
> that eval is allowed in this test you should change the worker script to run
> eval at the top level instead of in an event handler.

That worked, thanks.
(Assignee)

Updated

3 years ago
Attachment #8614171 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8614183 - Attachment is obsolete: true
Created attachment 8617892 [details]
MozReview Request: Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm

Bug 1160458 - CSP activated by default in Service Workers. r=nsm,bent
Attachment #8617892 - Flags: review?(nsm.nikhil)
Attachment #8617892 - Flags: review?(bent.mozilla)
Created attachment 8617893 [details]
MozReview Request: Bug 1160458 - Part 2: Test. r=nsm

Bug 1160458 - Test
Attachment #8617893 - Flags: review?(nsm.nikhil)
Attachment #8617893 - Flags: review?(bent.mozilla)
Nikhil, Ben, I asked review at both of you guys because this is related to service workers and the worker load info for the service worker. The fix is quite trivial and might be wrong and this could end r-'ed so if so let me know how to continue please. Thanks!
Comment on attachment 8617893 [details]
MozReview Request: Bug 1160458 - Part 2: Test. r=nsm

Bug 1160458 - Test
Attachment #8617893 - Flags: review?(bent.mozilla)
Comment on attachment 8617892 [details]
MozReview Request: Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm

https://reviewboard.mozilla.org/r/10727/#review9439

::: dom/workers/ServiceWorkerManager.cpp:3418
(Diff revision 1)
> +  info.mEvalAllowed = true;
> +  info.mReportCSPViolations = false;
> +

Just use the CSP of the principal passed to CreateServiceWorker.
Attachment #8617892 - Flags: review?(nsm.nikhil)
Comment on attachment 8617893 [details]
MozReview Request: Bug 1160458 - Part 2: Test. r=nsm

https://reviewboard.mozilla.org/r/10729/#review9441

Please also add another test where the registering page enforces a CSP that prevents eval and ensure that registration fails in that case due to EvalError.

::: dom/workers/test/serviceworkers/csp_worker.js:3
(Diff revision 1)
> +self.addEventListener('install', evt => {

remove this.
Comment on attachment 8617892 [details]
MozReview Request: Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm

Bug 1160458 - CSP activated by default in Service Workers. r=nsm,bent
Attachment #8617892 - Flags: review?(bent.mozilla)
Comment on attachment 8617893 [details]
MozReview Request: Bug 1160458 - Part 2: Test. r=nsm

Bug 1160458 - Test

Updated

3 years ago
Whiteboard: [s3]
Target Milestone: NGA S2 (12Jun) → NGA S3 (26Jun)
Comment on attachment 8617892 [details]
MozReview Request: Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm

Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm
Attachment #8617892 - Attachment description: MozReview Request: Bug 1160458 - CSP activated by default in Service Workers. r=nsm,bent → MozReview Request: Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm
Attachment #8617892 - Flags: review?(nsm.nikhil)
Comment on attachment 8617893 [details]
MozReview Request: Bug 1160458 - Part 2: Test. r=nsm

Bug 1160458 - Part 2: Test. r=nsm
Attachment #8617893 - Attachment description: MozReview Request: Bug 1160458 - Test → MozReview Request: Bug 1160458 - Part 2: Test. r=nsm
Attachment #8617893 - Flags: review?(nsm.nikhil)
Comment on attachment 8617892 [details]
MozReview Request: Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm

https://reviewboard.mozilla.org/r/10727/#review9767

::: dom/workers/ServiceWorkerManager.cpp:3593
(Diff revision 2)
> +    NS_ENSURE_SUCCESS(rv, rv);

NS_WARN_IF form.
Attachment #8617892 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8617893 [details]
MozReview Request: Bug 1160458 - Part 2: Test. r=nsm

https://reviewboard.mozilla.org/r/10729/#review9769

::: dom/workers/test/serviceworkers/test_eval_allowed.html:17
(Diff revision 2)
> +  var registration;

This global can be removed.

::: dom/workers/test/serviceworkers/test_eval_allowed.html:21
(Diff revision 2)
> +      registration = swr;

Instead of setting the registration, just return the Promise, so 

    return navigator.serviceWorker.register("eval_worker.js");

::: dom/workers/test/serviceworkers/test_eval_allowed.html:28
(Diff revision 2)
> +        ok(true, "eval in service worker script didn't cause an issue");

And then you can directly access the registration here since this is chained to register()

::: dom/workers/test/serviceworkers/test_eval_allowed.html:40
(Diff revision 2)
> +    ["dom.messageChannel.enabled", true],

This pref isn't required.

::: dom/workers/test/serviceworkers/test_eval_not_allowed.html:23
(Diff revision 2)
> +      .catch(function() {

There should also be a then() handler that fails the test. Without that this will just timeout.

::: dom/workers/test/serviceworkers/test_eval_not_allowed.html:35
(Diff revision 2)
> +    ["dom.messageChannel.enabled", true],

This can be removed.
Comment on attachment 8617892 [details]
MozReview Request: Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm

Bug 1160458 - Part 1: Use the CSP of the principal passed to CreateServiceWorker. r=nsm
Attachment #8617892 - Flags: review+
Comment on attachment 8617893 [details]
MozReview Request: Bug 1160458 - Part 2: Test. r=nsm

Bug 1160458 - Part 2: Test. r=nsm
Attachment #8617893 - Flags: review+
hi, part 1 failed to apply:

(eg '1-3,5', or 's' to toggle the sort order between id & patch description) 1
adding 1160458 to series file
renamed 1160458 -> 1160458.patch
applying 1160458.patch
patching file dom/workers/ServiceWorkerManager.cpp
Hunk #1 FAILED at 2960
1 out of 1 hunks FAILED -- saving rejects to file dom/workers/ServiceWorkerManager.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh 1160458.patch
Flags: needinfo?(jaoo)
Keywords: checkin-needed
Talked to Carsten on IRC. The patch applies cleanly, he will try again. Thanks!
Flags: needinfo?(jaoo)

Updated

3 years ago
Component: DOM: Workers → DOM: Service Workers
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8614013c4ee4
https://hg.mozilla.org/mozilla-central/rev/1fd17d4e677a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
As NGA Program Manager suggested, let's replace the NGA-X milestones with FxOS-Sx ones (more generic ones), once Bug 1174794 has already landed
Target Milestone: NGA S3 (26Jun) → FxOS-S1 (26Jun)
Attachment #8600252 - Attachment mime type: application/zip → application/x-jar
You need to log in before you can comment on or make changes to this bug.