Add URL origin checks for Service Worker (un)registration

Add URL origin checks for Service Worker (un)registration

Now the browser will register or unregister a Service Worker only if document url,
scope, and script url have the same origin.

This patch removes TODOs for adding SW to ChildProcessSecurityPolicy, since it
turns out the only security check needed here is the URL origin one. SW is
expected to be enabled always, not a permission granted per-process.

BUG=311631
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278326

[falken, 2014-06-17 09:26:46]

Michael and Tom, could you please review?

Context: Adding SW to CPSP is a prerequisite for removing the Chromium-side
flag, as per the codereview comment linked to in the bug:
https://codereview.chromium.org/25008006/#msg36

Since SW is expected to be a web plaform feature enabled everywhere, I don't
think CPSP needs to hold per-process state for SW. It seems it only must check
that the registering document has the same origin as the scope and worker.

[Tom Sepez, 2014-06-17 17:43:19]

https://codereview.chromium.org/334413004/diff/1/content/browser/child_proces...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/334413004/diff/1/content/browser/child_proces...
content/browser/child_process_security_policy_impl.cc:893: bool
ChildProcessSecurityPolicyImpl::CanRegisterServiceWorker(
These don't have to live in CPSP because they don't persist any state between
calls.  CPSP should always follow the pattern that there's a grant() call at
some point and later on, a can() call when the renderer wants to do something
that it thinks it is allowed to do.  If it doesn't follow that pattern, then the
check can move up to the caller.

[Tom Sepez, 2014-06-17 23:40:32]

Alternatively, if you want to honor site isolation, you'd need to check
CanAccessCookiesForOrigin() in the caller.

[michaeln, 2014-06-17 23:50:30]

https://codereview.chromium.org/334413004/diff/1/content/browser/child_proces...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/334413004/diff/1/content/browser/child_proces...
content/browser/child_process_security_policy_impl.cc:898:
document_url.GetOrigin() == script_url.GetOrigin();
These same-origin checks are not a matter of 'policy', these are a matter of
spec correctness. The spec correctness parts of this belong directly in the
/service_worker directory.

The 'policy' part of this has to do with chrome's content settings, we should
ask the embedder (aka chrome). Here's what we did for appcache and this is kinda
similar.

bool ChromeAppCacheService::CanCreateAppCache(
    const GURL& manifest_url, const GURL& first_party) {
  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
  return GetContentClient()->browser()->AllowAppCache(
      manifest_url, first_party, resource_context_);
}

Content shell always says 'yes', chrome sometimes decides to say 'no'.

[falken, 2014-06-18 00:01:31]

Thanks for the comments! Let me back up a bit. As reasoned in
http://crbug.com/365201, I'd like to remove the "--enable-service-worker" flag
(but keep the Blink web platform flag that prevents exposing SW in the wild),
and based on your codereview comment (from 7 months ago) we at least need URL
origin checking before doing so. Would it be OK to have just URL origin checks
in the caller for removing the flag?

For SW, Chrome would always just say yes to "CanUseServiceWorker", wouldn't it?

[michaeln1, 2014-06-18 00:36:33]

On 2014/06/18 00:01:31, falken wrote:
> Thanks for the comments! Let me back up a bit. As reasoned in
> http://crbug.com/365201, I'd like to remove the "--enable-service-worker" flag
> (but keep the Blink web platform flag that prevents exposing SW in the wild),
> and based on your codereview comment (from 7 months ago) we at least need URL
> origin checking before doing so. Would it be OK to have just URL origin checks
> in the caller for removing the flag?
> 
> For SW, Chrome would always just say yes to "CanUseServiceWorker", wouldn't
it?

To remove the ---enable-service-worker flag, how about two things.

1) lets check for the --enable-experimental-web-platform-features flag in the
very few places where we currently check for the old flag. So with the flag
turned off, nofin happens.

2) In addition to testing script.origin == scope.origin, also ensure
providerHost.document_url().origin matches as well. The docurl value is
determined differently and testing against that too adds some defense in depth.

I don't think we need to have integration with content settings policy to fiddle
with flags.

[falken, 2014-06-18 01:17:50]

On 2014/06/18 00:36:33, michaeln1 wrote:
> On 2014/06/18 00:01:31, falken wrote:
> > Thanks for the comments! Let me back up a bit. As reasoned in
> > http://crbug.com/365201, I'd like to remove the "--enable-service-worker"
flag
> > (but keep the Blink web platform flag that prevents exposing SW in the
wild),
> > and based on your codereview comment (from 7 months ago) we at least need
URL
> > origin checking before doing so. Would it be OK to have just URL origin
checks
> > in the caller for removing the flag?
> > 
> > For SW, Chrome would always just say yes to "CanUseServiceWorker", wouldn't
> it?
> 
> To remove the ---enable-service-worker flag, how about two things.
> 
> 1) lets check for the --enable-experimental-web-platform-features flag in the
> very few places where we currently check for the old flag. So with the flag
> turned off, nofin happens.
> 
> 2) In addition to testing script.origin == scope.origin, also ensure
> providerHost.document_url().origin matches as well. The docurl value is
> determined differently and testing against that too adds some defense in
depth.
> 
> I don't think we need to have integration with content settings policy to
fiddle
> with flags.

Discussed with Michael, and I think this is a good plan. Some downsides are that
--enable-service-worker has some cache trickery which we'll lose possibly
regressing perf and that we might lose out on learning about perf regressions
earlier if the flag were removed outright. But with the flag just in Blink we
can get in weird states were you can enable the flag, install a SW, then disable
the flag, and the SW would still run.

It looks like CPSP was not needed after all for SW.

So what I'd like to do is:
1) Add URL origin checks (this patch)
2) Replace --enable-service-worker checks with
--enable-experimental-web-platform-features (next patch)

[falken, 2014-06-18 01:34:11]

I've retitled/redescribed this patch. It just adds URL origin checks and removes
TODOs for CPSP that we don't need for SW. PTAL.

[falken, 2014-06-18 09:36:56]

On 2014/06/18 01:34:11, falken wrote:
> I've retitled/redescribed this patch. It just adds URL origin checks and
removes
> TODOs for CPSP that we don't need for SW. PTAL.

Sorry for the disorganized discussion. I reconsidered and still think it's worth
removing the flag entirely. But it doesn't really affect this patch, so let's
continue that discussion on https://codereview.chromium.org/339973003/.

So for this patch, the question for Tom is whether it's OK to remove these TODOs
given that:
1) It seems we don't need CPSP for Service Worker (site isolation seems to be an
experimental feature we don't need to support yet)
2) We'll possibly remove the --enable-service-worker flag checks (the ones next
to the TODOs), and just have --enable-experimental-web-platform-features control
SW. That means SW is not exposed to the web, but a compromised renderer could
send SW IPCs. The effect would be that they circumvent the feature flag.
Alternatively, we can replace the --enable-service-worker checks with
--enable-experimental-web-platform-feature checks.

[Tom Sepez, 2014-06-18 17:18:26]

Thanks. CPSP LGTM since you're no longer modifying it :).

[michaeln, 2014-06-19 00:04:50]

lgtm

https://codereview.chromium.org/334413004/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/334413004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:43:
document_url.GetOrigin() == script_url.GetOrigin();
can you add a todo to respect chrome's content settings

[falken, 2014-06-19 00:23:34]

https://codereview.chromium.org/334413004/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/334413004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:43:
document_url.GetOrigin() == script_url.GetOrigin();
On 2014/06/19 00:04:50, michaeln wrote:
> can you add a todo to respect chrome's content settings

Just to make sure I understand, this means we plan in the future to add
something like AllowServiceWorker to content settings, like we have for
appcache? Geniune/naive question, why would we do this for SW and not other web
platform features? I guess, it's just too powerful (installed state, network
intercept, etc...)?

[falken, 2014-06-19 00:33:35]

The CQ bit was checked by falken@chromium.org

[michaeln, 2014-06-19 00:35:46]

https://codereview.chromium.org/334413004/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/334413004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:43:
document_url.GetOrigin() == script_url.GetOrigin();
On 2014/06/19 00:23:34, falken wrote:
> On 2014/06/19 00:04:50, michaeln wrote:
> > can you add a todo to respect chrome's content settings
> 
> Just to make sure I understand, this means we plan in the future to add
> something like AllowServiceWorker to content settings, like we have for
> appcache? Geniune/naive question, why would we do this for SW and not other
web
> platform features? I guess, it's just too powerful (installed state, network
> intercept, etc...)?

Yes, something like AllowServiceWorker(). And we do do this for lots of other
storage related platform features, its important for privacy.

[commit-bot: I haz the power, 2014-06-19 00:36:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/falken@chromium.org/334413004/80001

[commit-bot: I haz the power, 2014-06-19 11:31:42]

Change committed as 278326