Check CSP before registering ServiceWorkers

Check CSP before registering ServiceWorkers

Service Worker registrations should be subject to the same CSP checks as
other workers. The spec doesn't say this explicitly
(https://www.w3.org/TR/CSP2/#directive-child-src-workers says "Worker or
SharedWorker constructors"), but it seems to be in the spirit of things,
and it matches Firefox's behavior.

BUG=579801
Committed: https://crrev.com/5289a5d4c98681e9a0f2d28da0c7aa35e282db57
Cr-Commit-Position: refs/heads/master@{#385775}

[estark, 2016-04-06 21:52:28]

estark@chromium.org changed reviewers:
+ falken@chromium.org, mkwst@chromium.org

[estark, 2016-04-06 21:52:29]

falken, mkwst, PTAL?

[Marijn Kruisselbrink, 2016-04-06 22:01:50]

I wonder how this would/should work for service workers installed via a Link:
rel=serviceworker http header (the code in
content/browser/service_worker/link_header_support.cc HandleServiceWorkerLink).
Maybe it is okay to not apply any kind of CSP checks there because if you can
inject a Link: header in the response you can also modify the CSPs?

[estark, 2016-04-06 23:13:11]

On 2016/04/06 22:01:50, Marijn Kruisselbrink wrote:
> I wonder how this would/should work for service workers installed via a Link:
> rel=serviceworker http header (the code in
> content/browser/service_worker/link_header_support.cc
HandleServiceWorkerLink).
> Maybe it is okay to not apply any kind of CSP checks there because if you can
> inject a Link: header in the response you can also modify the CSPs?

Huh, I didn't know you could install a service worker via an HTTP header... I
agree with you that it's probably outside the threat model of CSP, but I'm not
sure. Mike, maybe CSP3 should say something about how CSP should be applied when
loading the main script for a service worker? (If it doesn't already)

If we wanted to apply CSP checks for a SW loaded from an HTTP header, seems like
that would be tricky with how things are set up right now. We are planning to
move CSP checks into the browser process at some point, at which point it would
probably be much simpler to enforce them for SWs installed via Link.

[falken, 2016-04-07 05:06:08]

lgtm with question

Should we also check CSP on importScripts() in the service worker script? That
may already work.

[Mike West, 2016-04-07 08:42:32]

On 2016/04/07 at 05:06:08, falken wrote:
> lgtm with question

LGTM. I think we'll probably want to split this out into a separate directive at
some point, but matching Firefox's behavior for the moment makes sense.

> Should we also check CSP on importScripts() in the service worker script? That
may already work.

That should be covered by the Service Worker's `script-src` directive. I doubt
we have a test for it, but it Should Totally Work(tm).

> I wonder how this would/should work for service workers installed via a Link:
rel=serviceworker http header

If/when we start processing CSP in the browser, we'll need to process it before
triggering requests via `Link` headers. This is more or less the same thing we
need to do for `frame-ancestor`, just in a different place. That's not something
we're capable of doing today, for better or worse.

[estark, 2016-04-07 15:28:40]

Thanks both.

On 2016/04/07 08:42:32, Mike West wrote:
> On 2016/04/07 at 05:06:08, falken wrote:
> > lgtm with question
> 
> LGTM. I think we'll probably want to split this out into a separate directive
at
> some point, but matching Firefox's behavior for the moment makes sense.
> 
> > Should we also check CSP on importScripts() in the service worker script?
That
> may already work.
> 
> That should be covered by the Service Worker's `script-src` directive. I doubt
> we have a test for it, but it Should Totally Work(tm).

Au contraire!
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

> 
> > I wonder how this would/should work for service workers installed via a
Link:
> rel=serviceworker http header
> 
> If/when we start processing CSP in the browser, we'll need to process it
before
> triggering requests via `Link` headers. This is more or less the same thing we
> need to do for `frame-ancestor`, just in a different place. That's not
something
> we're capable of doing today, for better or worse.

[estark, 2016-04-07 15:28:53]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-04-07 15:29:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861253004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861253004/1

[commit-bot: I haz the power, 2016-04-07 16:14:53]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-07 16:16:53]

Description was changed from

==========
Check CSP before registering ServiceWorkers

Service Worker registrations should be subject to the same CSP checks as
other workers. The spec doesn't say this explicitly
(https://www.w3.org/TR/CSP2/#directive-child-src-workers says "Worker or
SharedWorker constructors"), but it seems to be in the spirit of things,
and it matches Firefox's behavior.

BUG=579801
==========

to

==========
Check CSP before registering ServiceWorkers

Service Worker registrations should be subject to the same CSP checks as
other workers. The spec doesn't say this explicitly
(https://www.w3.org/TR/CSP2/#directive-child-src-workers says "Worker or
SharedWorker constructors"), but it seems to be in the spirit of things,
and it matches Firefox's behavior.

BUG=579801

Committed: https://crrev.com/5289a5d4c98681e9a0f2d28da0c7aa35e282db57
Cr-Commit-Position: refs/heads/master@{#385775}
==========

[commit-bot: I haz the power, 2016-04-07 16:16:54]

Patchset 1 (id:??) landed as
https://crrev.com/5289a5d4c98681e9a0f2d28da0c7aa35e282db57
Cr-Commit-Position: refs/heads/master@{#385775}