Closed Bug 1208559 Opened 6 years ago Closed 6 years ago

ServiceWorker registration not governed by CSP

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Keywords: sec-want, Whiteboard: [adv-main44-])

Attachments

(3 files, 6 obsolete files)

No description provided.
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.
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.
Attached patch Tests. r=bholley (obsolete) — Splinter Review
I fixed up the tests so that they pass.
Attachment #8668151 - Flags: review+
Attachment #8666108 - Attachment is obsolete: true
This passes the test, but doesn't properly handle redirects. Christoph and I
discussed another approach that would do that (coming up).
This is incomplete because we don't have a ref to the document. Christoph is going to finish this part up early next week.
Assignee: nobody → mozilla
Flags: needinfo?(mozilla)
Group: core-security → dom-core-security
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?
Attachment #8668153 - Attachment is obsolete: true
Flags: needinfo?(mozilla)
Attachment #8671586 - Flags: review?(jonas)
Attachment #8671586 - Flags: review?(bkelly)
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 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.
Attachment #8671586 - Flags: review?(bkelly) → review+
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.
Attachment #8671586 - Flags: review?(dveditz)
> 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 on attachment 8671586 [details] [diff] [review]
bug_1208559_serviceworker_csp_checks.patch

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

r=dveditz
Attachment #8671586 - Flags: review?(dveditz) → review+
Jonas, did you mean to r+ Christoph's patch here?  ;)
Flags: needinfo?(jonas)
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
Attachment #8668152 - Attachment is obsolete: true
Attachment #8673842 - Flags: review+
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?
Attachment #8673842 - Flags: review?(bkelly)
Attachment #8673842 - Flags: review?(bkelly) → review+
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.
Flags: needinfo?(jonas)
Attachment #8671586 - Flags: review?(jonas)
Blocks: 1045891
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
Group: dom-core-security
Keywords: sec-want
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.
Attachment #8671586 - Flags: approval-mozilla-beta?
Attachment #8671586 - Flags: approval-mozilla-aurora?
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.
Just rebased the patches for aurora.
Attachment #8668151 - Attachment is obsolete: true
Attachment #8676335 - Flags: review+
Just rebased the patches for aurora.
Attachment #8671586 - Attachment is obsolete: true
Attachment #8671586 - Flags: approval-mozilla-beta?
Attachment #8671586 - Flags: approval-mozilla-aurora?
Attachment #8676336 - Flags: review+
Just rebased the patches for aurora.
Attachment #8673842 - Attachment is obsolete: true
Attachment #8676337 - Flags: review+
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).
Attachment #8676335 - Flags: approval-mozilla-aurora?
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).
Attachment #8676336 - Flags: approval-mozilla-aurora?
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).
Attachment #8676337 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1206956
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 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.
Attachment #8676335 - Flags: approval-mozilla-aurora?
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.
Attachment #8676336 - Flags: approval-mozilla-aurora?
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.
Attachment #8676337 - Flags: approval-mozilla-aurora?
Whiteboard: [adv-main44-]
You need to log in before you can comment on or make changes to this bug.