1160458 - CSP activated by default in Service Workers

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Salvador de la Puente González [:salva]]

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.

Comment 1

[Salvador de la Puente González [:salva]]

Attachment: 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.

Comment 2

[Salvador de la Puente González [:salva]]

Attachment: cspsw.zip

You can use python2 -m SimpleHTTPServer or python3 -m http.server to start a simple HTTP server.

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

I'll debug here a bit and let you know guys about my findings.

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

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

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

(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.

Comment 7

[Noemí Freire (:noemi)]

It seems to be specifically related to eval() function. Setting dependency with v2 for tracking purposes, please change it if needed.

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

Attachment: 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.

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

Attachment: 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?

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

(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.

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

Attachment: 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

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

Attachment: MozReview Request: Bug 1160458 - Part 2: Test. r=nsm

Bug 1160458 - Test

Comment 14

[José Antonio Olivera Ortega [:jaoo]]

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 15

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8617893 [details]
MozReview Request: Bug 1160458 - Part 2: Test. r=nsm

Bug 1160458 - Test

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 18

[José Antonio Olivera Ortega [:jaoo]]

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

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8617893 [details]
MozReview Request: Bug 1160458 - Part 2: Test. r=nsm

Bug 1160458 - Test

Comment 20

[José Antonio Olivera Ortega [:jaoo]]

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

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8617893 [details]
MozReview Request: Bug 1160458 - Part 2: Test. r=nsm

Bug 1160458 - Part 2: Test. r=nsm

Comment 22

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 23

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 24

[José Antonio Olivera Ortega [:jaoo]]

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

Comment 25

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8617893 [details]
MozReview Request: Bug 1160458 - Part 2: Test. r=nsm

Bug 1160458 - Part 2: Test. r=nsm

Comment 26

[José Antonio Olivera Ortega [:jaoo]]

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

Comment 27

[Carsten Book [:Tomcat]]

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

Comment 28

[José Antonio Olivera Ortega [:jaoo]]

Talked to Carsten on IRC. The patch applies cleanly, he will try again. Thanks!

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8614013c4ee4
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd17d4e677a

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8614013c4ee4
https://hg.mozilla.org/mozilla-central/rev/1fd17d4e677a

Comment 31

[Maria Angeles Oteo (:oteo)]

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