Kill renderers that register Service Workers from non-secure origins

Kill renderers that register Service Workers from non-secure origins

Service Workers are only available on "secure origins" per
[1]. There's an API-level check in
ServiceWorkerContainer::registerServiceWorker,
unregisterServiceWorker. To defend against a compromised renderer
circumventing the policy, this adds a check that the origin is secure
in browser, where the registration takes place.

[1] http://www.chromium.org/Home/chromium-security/prefer-secure-origins-for-powerful-new-features

BUG=394213

[dominicc (has gone to gerrit), 2014-08-08 07:23:49]

PTAL

creis: content/browser/DEPS
falken: content/browser/service_worker

This depends on https://codereview.chromium.org/452793002/ going first.

[falken, 2014-08-08 09:06:15]

lgtm with nit

https://codereview.chromium.org/452013002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/452013002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_dispatcher_host.cc:41:
blink::WebURL webUrl(url);
We're in chromium, so use snake_case

https://codereview.chromium.org/452013002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_dispatcher_host.cc:43:
blink::WebString unusedErrorMessage;
ditto

[Charlie Reis, 2014-08-08 16:26:35]

[-creis1,+creis,+scottmg]

Adding Scott to look at the DEPS change, since I think those were removed
intentionally.

https://codereview.chromium.org/452013002/diff/1/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/452013002/diff/1/content/browser/DEPS#newcode51
content/browser/DEPS:51: "+third_party/WebKit/public/platform/WebURL.h",
These were pulled out in
https://src.chromium.org/viewvc/chrome?revision=208318&view=revision and
http://src.chromium.org/viewvc/chrome?view=revision&revision=214159.  I'm
guessing we don't want to add them back.  @scottmg, can you confirm?

[michaeln, 2014-08-08 20:15:33]

https://codereview.chromium.org/452013002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/452013002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_dispatcher_host.cc:42:
blink::WebSecurityOrigin origin = blink::WebSecurityOrigin::create(webUrl);
chromium /browser libs can't depend on blink like this

if theres to be common code for this funcion, mayber blink should use a function
provided by chromium thru blink::Platform?

[scottmg, 2014-08-08 21:57:13]

On 2014/08/08 16:26:35, Charlie Reis wrote:
> [-creis1,+creis,+scottmg]
> 
> Adding Scott to look at the DEPS change, since I think those were removed
> intentionally.
> 
> https://codereview.chromium.org/452013002/diff/1/content/browser/DEPS
> File content/browser/DEPS (right):
> 
>
https://codereview.chromium.org/452013002/diff/1/content/browser/DEPS#newcode51
> content/browser/DEPS:51: "+third_party/WebKit/public/platform/WebURL.h",
> These were pulled out in
> https://src.chromium.org/viewvc/chrome?revision=208318&view=revision and
> http://src.chromium.org/viewvc/chrome?view=revision&revision=214159.  I'm
> guessing we don't want to add them back.  @scottmg, can you confirm?

We don't want any blink code in the browser process because the browser/renderer
are separated into 2 dlls on windows. If the headers are just enums or simple
POD types it's OK, but otherwise they shouldn't be re-added.

[dominicc (has gone to gerrit), 2014-08-11 02:29:29]

On 2014/08/08 21:57:13, scottmg wrote:
> On 2014/08/08 16:26:35, Charlie Reis wrote:
> > [-creis1,+creis,+scottmg]
> > 
> > Adding Scott to look at the DEPS change, since I think those were removed
> > intentionally.
> > 
> > https://codereview.chromium.org/452013002/diff/1/content/browser/DEPS
> > File content/browser/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/452013002/diff/1/content/browser/DEPS#newcode51
> > content/browser/DEPS:51: "+third_party/WebKit/public/platform/WebURL.h",
> > These were pulled out in
> > https://src.chromium.org/viewvc/chrome?revision=208318&view=revision and
> > http://src.chromium.org/viewvc/chrome?view=revision&revision=214159.  I'm
> > guessing we don't want to add them back.  @scottmg, can you confirm?

Thanks for the archaeology. Was WebSecurityOrigin removed because it was unused?
Or was it made to be unused and then removed?

I could live without WebURL if it's OK to just be passing a String there.

> We don't want any blink code in the browser process because the
browser/renderer
> are separated into 2 dlls on windows.

What's the issue with that? Bloat, DLL loading, or ...?

> If the headers are just enums or simple
> POD types it's OK, but otherwise they shouldn't be re-added.

Super high level, it feels reasonable for Blink to define things like which
schemes are secure and so on for the embedder, since that's web platform stuff,
which is half of this check. If I can't call that in browser, it will be tough
to implement this policy without duplication.

Chrome already tells Blink additional schemes to treat as secure; it could tell
it all of them. But it seems weird.

Adam--do you have an opinion or guidance?

[palmer, 2014-08-11 18:24:04]

> Thanks for the archaeology. Was WebSecurityOrigin removed because it was
unused?
> Or was it made to be unused and then removed?

WebSecurityOrigin is still present...

> > We don't want any blink code in the browser process because the
> > browser/renderer
> > are separated into 2 dlls on windows.
> 
> What's the issue with that? Bloat, DLL loading, or ...?

Hmm, we are going to have to duplicate the logic in Chromium code, then. I was
hoping to avoid having to do that. Inevitable bugs...

[michaeln1, 2014-08-11 20:03:47]

> What's the issue with that? Bloat, DLL loading, or ...?

There is more than one reasons:
- To keep the size of build targets smaller.
- To avoid Threading landmines in blink (initting the static collections used by
WebSecurityOrigin for example).
- To help with having a single process WebView package for Android.

> Hmm, we are going to have to duplicate the logic in Chromium code, then. I was
> hoping to avoid having to do that. Inevitable bugs...

My vote would be for blink to use a function provided to it by chromium thru
blink::Platform.
We move the impl of the function to chromium and put it in chromium/common.

[dominicc (has gone to gerrit), 2014-08-15 06:51:07]

On 2014/08/11 18:24:04, Chromium Palmer wrote:
> > Thanks for the archaeology. Was WebSecurityOrigin removed because it was
> unused?
> > Or was it made to be unused and then removed?
> 
> WebSecurityOrigin is still present...
> 
> > > We don't want any blink code in the browser process because the
> > > browser/renderer
> > > are separated into 2 dlls on windows.
> > 
> > What's the issue with that? Bloat, DLL loading, or ...?
> 
> Hmm, we are going to have to duplicate the logic in Chromium code, then. I was
> hoping to avoid having to do that. Inevitable bugs...

I'll close this issue. Could someone opine on Issue 362214 about where this
should live in Chrome?