1208559 - ServiceWorker registration not governed by CSP

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: csp_serviceworker_proof_of_concept_test.patch

Attaching a proof of concept patch that service worker registration does not consult CSP of a page. We are missing a call to NS_CheckContentLoadPolicy somewhere.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I wrote a spec issue, but this is actually already spec'd.

The SW Update algorithm should be running the equivalent of Fetch when downloading the sw script.  Fetch includes the CSP check.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Tests. r=bholley

I fixed up the tests so that they pass.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Do a CSP Check in ServiceWorkerManager::Register. v1

This passes the test, but doesn't properly handle redirects. Christoph and I
discussed another approach that would do that (coming up).

Comment 5

[Bobby Holley (:bholley)]

Attachment: Use AsyncOpen2 in ServiceWorkerScriptCache. WIP

This is incomplete because we don't have a ref to the document. Christoph is going to finish this part up early next week.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1208559_serviceworker_csp_checks.patch

Jonas, Ben, I think it's the best solution if we just have service workers use asyncOpen2(). If so, then I think there is no need in having the additional check within
> dom/workers/ServiceWorkerManager.cpp
which would result in consulting CSP twice for the same load.

Kate created CSP tests for service workers including redirects (see Bug 1045891) which work fine with that approach.

If you are fine with that patch I can have dveditz review the bits within dom/security where we needed to add a fallback mechanism to use the handed in principal in case the loadingDocument is null (which is the case for service workers).



Thoughts?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

AsyncOpen2 is generally about network policies, so semantically it's not a great fit here. However, I can't see anything that would cause it not to work here, so it's up to you, as the CSP-owner, and the SW-owners.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8671586 [details] [diff] [review]
bug_1208559_serviceworker_csp_checks.patch

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

r=me, although I think we should consider the case where ScriptLoader loads the service worker script instead of ServiceWorkerScriptCache.

Also, I don't really understand Jonas's comment 7.  These are doing network loads...

::: dom/security/nsContentSecurityManager.cpp
@@ +124,5 @@
>        break;
>      }
>  
> +    case nsIContentPolicy::TYPE_SCRIPT: {
> +      mimeTypeGuess = NS_LITERAL_CSTRING("application/javascript");

I'm not sure how this guess is used, but service workers are required to be one of:

  text/javascript
  application/x-javascript
  application/javascript

We check for this in service worker code explicitly.

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +111,5 @@
>      }
>  
>      // Note that because there is no "serviceworker" RequestContext type, we can
>      // use the TYPE_INTERNAL_SCRIPT content policy types when loading a service
>      // worker.

This comment seems wrong now.

@@ +115,5 @@
>      // worker.
>      rv = NS_NewChannel(getter_AddRefs(mChannel),
>                         uri, aPrincipal,
> +                       nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,
> +                       nsIContentPolicy::TYPE_INTERNAL_SERVICE_WORKER,

In theory we should ensure these values are set in ScriptLoader.cpp as well, right?

The ScriptLoader will attempt a network download of the service worker script if the Cache does not have it.  That can happen due to storage eviction, etc.  See:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp?case=true&from=ScriptLoader.cpp#1500
  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#863
  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#168

Is that case covered somehow?  To be honest, it would probably be hard to trigger with a test.

We could also just make this trigger a load error if the top level service worker script is not already in the Cache:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#1498

Its kind of corner casey, so I guess it could wait until ScriptLoader is converted to AsyncOpen2() as well.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8671586 [details] [diff] [review]
bug_1208559_serviceworker_csp_checks.patch

(In reply to Ben Kelly [:bkelly] from comment #8)
> r=me, although I think we should consider the case where ScriptLoader loads
> the service worker script instead of ServiceWorkerScriptCache.

With ScriptLoader you mean dom/workers/ScriptLoader, right? Within dom/workers/ScriptLoader we perform a CSP check anyway, so that should be fine.

> > +    case nsIContentPolicy::TYPE_SCRIPT: {
> > +      mimeTypeGuess = NS_LITERAL_CSTRING("application/javascript");
> 
> I'm not sure how this guess is used, but service workers are required to be
> one of:
> 
>   text/javascript
>   application/x-javascript
>   application/javascript
> 
> We check for this in service worker code explicitly.

I already had that discussion with Jonas a while ago. I think I remember that we think it's fine to just use "applicaton/javascript", but then again, I am also not completely sure how important the mime type guess is. We don't consume it internally, but addons rely on it in some way. Dan probably can tell us whether we can just use "application/javascript" for all kinds of TYPE_SCRIPT or not. He has to review the patch anyway, since we need a dom/security peer to review the CSP changes.

 > @@ +115,5 @@
> >      // worker.
> >      rv = NS_NewChannel(getter_AddRefs(mChannel),
> >                         uri, aPrincipal,
> > +                       nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,
> > +                       nsIContentPolicy::TYPE_INTERNAL_SERVICE_WORKER,
> 
> In theory we should ensure these values are set in ScriptLoader.cpp as well,
> right?

Yes, you are right, we are going to do that within Bug 1206955, but that's fine for now. As long as we haven't converted the callsite to use asyncOpen2() we perform security checks on the callsite. But soon we should be able to convert that callsite to use AsyncOpen2() anyway. Definitely safe for now.

> The ScriptLoader will attempt a network download of the service worker
> script if the Cache does not have it.  That can happen due to storage
> eviction, etc.  See:
> ...
> Is that case covered somehow?  To be honest, it would probably be hard to
> trigger with a test.

So, that's another service worker specific question. When registering a service worker I assume it's fine to perform a CSP check only if the script code is fetched from the network. I think this patch should cover everything we need, but then again I could be convinced otherwise.

Let's wait for Dan's input.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Also, I don't really understand Jonas's comment 7.  These are doing network loads...

What I'm saying is that AsyncOpen2 generally protects against three things:

* That website A can't read sensitive personal data from website B. (For example, read the users login
  credentials for website B)
* That website A can't create requests which to website B which would cause B to take inappropriate
  actions with user data that website B holds. (For example transfer the user's money to an
  attacker-controlled account.)
* That website A can't be tricked into sending data to an untrusted party or over an insecure connection.
  (Trick website A into sending the users credentials for website A to an attacker controlled server).

Limiting the scope of a SW doesn't seem to fit into any of these categories.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 8671586 [details] [diff] [review]
bug_1208559_serviceworker_csp_checks.patch

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

r=dveditz

Comment 12

[Andrew Overholt [:overholt]]

Jonas, did you mean to r+ Christoph's patch here?  ;)

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug-1208559---Do-a-CSP-Check-in-ServiceWorkerManag.patch

As discussed with bkelly, we should also land the CSP check within the service worker registration code. Taking bholly's patch but using TYPE_INTERNAL_SERVICE_WORKER as the contentType.

Ben also filed:
https://github.com/slightlyoff/ServiceWorker/issues/755#issuecomment-148142978

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8673842 [details] [diff] [review]
Bug-1208559---Do-a-CSP-Check-in-ServiceWorkerManag.patch

Ben, as agreed, can you green light that patch?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8671586 [details] [diff] [review]
bug_1208559_serviceworker_csp_checks.patch

Chatted with Jonas over IRC; he is fine with other people reviewing those bits. Clearing his r? and ni? flags.

Comment 16

[Daniel Veditz [:dveditz]]

I don't think this needs to be hidden (or require sec-approval to land) because ServiceWorkers aren't enabled yet and it's already known we are incomplete in CSP wrt governing workers in general

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e497ed4a5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd458f2077e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/eca8be0e5336

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e2e497ed4a5c
https://hg.mozilla.org/mozilla-central/rev/dd458f2077e8
https://hg.mozilla.org/mozilla-central/rev/eca8be0e5336

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e2e497ed4a5c
https://hg.mozilla.org/mozilla-central/rev/dd458f2077e8
https://hg.mozilla.org/mozilla-central/rev/eca8be0e5336

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8671586 [details] [diff] [review]
bug_1208559_serviceworker_csp_checks.patch

Approval Request Comment
[Feature/regressing bug #]:
Similar to Bug 1198078, we have had this bug for a while.
[User impact if declined]: ServiceWorker registration bypasses CSP of a page because ContentPolicy checks were missing.
[Describe test coverage new/current, TreeHerder]: Has a test and has been on try server.
[Risks and why]: Low, because only pages using service workers and shipping with a CSP are affected.
[String/UUID change made/needed]: No.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Note, service workers are not enabled by default on beta.  Also, we have decided not to ship service workers in 43.  So not sure if this really needs to be uplifted.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug-1208559---Tests-rbholley.patch

Just rebased the patches for aurora.

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1208559_serviceworker_csp_checks.patch

Just rebased the patches for aurora.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug-1208559---Do-a-CSP-Check-in-ServiceWorkerManag.patch

Just rebased the patches for aurora.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8676335 [details] [diff] [review]
Bug-1208559---Tests-rbholley.patch

See comment 20 (once approved and landed on aurora I can rebase them for beta).

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8676336 [details] [diff] [review]
bug_1208559_serviceworker_csp_checks.patch

See comment 20 (once approved and landed on aurora I can rebase them for beta).

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8676337 [details] [diff] [review]
Bug-1208559---Do-a-CSP-Check-in-ServiceWorkerManag.patch

See comment 20 (once approved and landed on aurora I can rebase them for beta).

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Christoph, do we really need this on aurora/beta? It is more or less too late for beta 42 at this point unless this is very critical.  
Since Service Workers isn't shipping in 43, do we need this to uplift to 43?

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8676335 [details] [diff] [review]
Bug-1208559---Tests-rbholley.patch

Clearing uplift request as Service workers are not going to be shipped in FF43.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8676336 [details] [diff] [review]
bug_1208559_serviceworker_csp_checks.patch

Clearing uplift request as Service workers are not going to be shipped in FF43.

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8676337 [details] [diff] [review]
Bug-1208559---Do-a-CSP-Check-in-ServiceWorkerManag.patch

Clearing uplift request as Service workers are not going to be shipped in FF43.