Fix web_accesible_resources enforcement for Site Isolation.

Fix web_accesible_resources enforcement for Site Isolation.

When --isolate-extensions or --site-per-process modes are enabled, all
extensions frames run in extension processes and are not mixed in regular
web renderers. This causes a problem with security checks for
web_accessible_resources, which allow all navigations to extension pages
when they are performed in extension process. This is no longer true and
this patch addresses this by using a NavigationThrottle to perform the
proper checks on the UI thread (also PlzNavigate compatible).

BUG=616488
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/b9164c43d2900c967f4fdb5ebfc4812f7e914116
Cr-Commit-Position: refs/heads/master@{#398189}

[nasko, 2016-06-03 21:33:16]

Description was changed from

==========
Fix web_accesible_resources enforcement for Site Isolation.

When --isolate-extensions or --site-per-process modes are enabled, all
extensions frames run in extension processes and are not mixed in regular
web renderers. This causes a problem with security checks for
web_accessible_resources, which allow all navigations to extension pages
when they are performed in extension process. This is no longer true and
this patch addresses this by using a NavigationThrottle to perform the
proper checks on the UI thread (also PlzNavigate compatible).

BUG=616488
==========

to

==========
Fix web_accesible_resources enforcement for Site Isolation.

When --isolate-extensions or --site-per-process modes are enabled, all
extensions frames run in extension processes and are not mixed in regular
web renderers. This causes a problem with security checks for
web_accessible_resources, which allow all navigations to extension pages
when they are performed in extension process. This is no longer true and
this patch addresses this by using a NavigationThrottle to perform the
proper checks on the UI thread (also PlzNavigate compatible).

BUG=616488
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-06-03 21:37:02]

nasko@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[nasko, 2016-06-03 21:37:03]

Hey Devlin,
Can you review this CL for me? It enforces web_accessible_resources through a
NavigationHandle, which ensures it isn't broken in Site Isolation modes.
Thanks in advance!
Nasko

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_protocols.cc:426: net::ERR_BLOCKED_BY_CLIENT);
I've changed this to be consistent with the return value of NavigationThrottle
and avoid this being a difference used for fingerprinting.

[Devlin, 2016-06-03 22:22:46]

Looks like there's some test failures, but nothing too bad.

Also, the NavigationThrottle makes this incredibly clean, I like it a lot!  It's
also something that might just be confined enough to make unittesting easy...
(hint hint ;))  If it turns out to be a pain, we do have existing WAR tests, but
I don't know if any of them would be as clean, clear, and efficient as one here.

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.cc:46:
navigation_handle()->GetWebContents()->GetAllFrames();
nit: I'd probably just inline this on line 47:
for (content::RenderFrameHost* frame :
navigation_handle()->GetWebContents()->GetAllFrames())

I'm actually surprised we don't have a method for this on NavigationHandle yet -
is it that rare to want to get the navigating frame?

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.cc:65: if
(!ancestor->GetLastCommittedURL().SchemeIs("chrome-devtools")) {
nit: content::kChromeDevToolsScheme

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.cc:79:
extensions::ExtensionRegistry* registry = extensions::ExtensionRegistry::Get(
no extensions:: prefix needed

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_navigation_throttle.h (right):

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.h:8: #include <string>
not needed

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.h:10: #include
"base/callback.h"
IWYU nit: base/macros.h

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.h:25:
ExtensionNavigationThrottle(content::NavigationHandle* navigation_handle);
explicit

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.h:29: ThrottleCheckResult
WillStartRequest() override;
I'm not very familiar with NavigationThrottle, so I'm not entirely clear on when
this will be called as opposed to our n other WAR checks.  Are they all still
necessary?  Can we change any ifs to dchecks?

[nasko, 2016-06-06 17:22:16]

Patchset #2 (id:20001) has been deleted

[nasko, 2016-06-06 17:40:44]

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.cc:46:
navigation_handle()->GetWebContents()->GetAllFrames();
On 2016/06/03 22:22:45, Devlin wrote:
> nit: I'd probably just inline this on line 47:
> for (content::RenderFrameHost* frame :
> navigation_handle()->GetWebContents()->GetAllFrames())
> 
> I'm actually surprised we don't have a method for this on NavigationHandle yet
-
> is it that rare to want to get the navigating frame?

It isn't rare, but you don't know which RenderFrameHost will be chosen to commit
the navigation until commit time. This is called before the request is even
sent, so you cannot get to RFH this early.
I hoped the comment above captures the notion of why doing it this way is safe,
as we don't really care about the RFH itself, but all of its parents, which will
not be changing.

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.cc:65: if
(!ancestor->GetLastCommittedURL().SchemeIs("chrome-devtools")) {
On 2016/06/03 22:22:45, Devlin wrote:
> nit: content::kChromeDevToolsScheme

Thanks! Already done, but didn't upload.

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.cc:79:
extensions::ExtensionRegistry* registry = extensions::ExtensionRegistry::Get(
On 2016/06/03 22:22:45, Devlin wrote:
> no extensions:: prefix needed

Done.

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_navigation_throttle.h (right):

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.h:8: #include <string>
On 2016/06/03 22:22:46, Devlin wrote:
> not needed

Done.

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.h:10: #include
"base/callback.h"
On 2016/06/03 22:22:46, Devlin wrote:
> IWYU nit: base/macros.h

Done.

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.h:25:
ExtensionNavigationThrottle(content::NavigationHandle* navigation_handle);
On 2016/06/03 22:22:45, Devlin wrote:
> explicit

Done.

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.h:29: ThrottleCheckResult
WillStartRequest() override;
On 2016/06/03 22:22:45, Devlin wrote:
> I'm not very familiar with NavigationThrottle, so I'm not entirely clear on
when
> this will be called as opposed to our n other WAR checks.  Are they all still
> necessary?  Can we change any ifs to dchecks?

This will be called before the request is sent to the network. It is called
after the job is created on the IO thread, which is where all the existing
checks are performed. However, those checks run on the IO thread, where we don't
have the ancestor information, so we can't do it there.

Because the existing checks run ahead of this one, we can't convert ifs into
DCHECKs. Also, if it gets blocked on the IO thread, we end up saving ourselves
creating a request and jumping to the UI thread, so I'd say we keep them for
perf reasons, even though we can technically remove them.

[nasko, 2016-06-06 17:45:40]

On 2016/06/03 22:22:46, Devlin wrote:
> Looks like there's some test failures, but nothing too bad.
> 
> Also, the NavigationThrottle makes this incredibly clean, I like it a lot! 
It's
> also something that might just be confined enough to make unittesting easy...
> (hint hint ;))  If it turns out to be a pain, we do have existing WAR tests,
but
> I don't know if any of them would be as clean, clear, and efficient as one
here.

I'm not sure if we can unit test with arbitrary frame trees outside of content/.
I'll think about it and see if we can do something. Also, it will only cover 
navigations, not subresource loads, which still need tests.

[Devlin, 2016-06-06 22:17:22]

lgtm

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_navigation_throttle.h (right):

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.h:29: ThrottleCheckResult
WillStartRequest() override;
On 2016/06/06 17:40:44, nasko (slow) wrote:
> On 2016/06/03 22:22:45, Devlin wrote:
> > I'm not very familiar with NavigationThrottle, so I'm not entirely clear on
> when
> > this will be called as opposed to our n other WAR checks.  Are they all
still
> > necessary?  Can we change any ifs to dchecks?
> 
> This will be called before the request is sent to the network. It is called
> after the job is created on the IO thread, which is where all the existing
> checks are performed. However, those checks run on the IO thread, where we
don't
> have the ancestor information, so we can't do it there.
> 
> Because the existing checks run ahead of this one, we can't convert ifs into
> DCHECKs. Also, if it gets blocked on the IO thread, we end up saving ourselves
> creating a request and jumping to the UI thread, so I'd say we keep them for
> perf reasons, even though we can technically remove them.

I'm a little worried that this will become (more) confusing between having
extension_protocols, this class, url_request_util, chrome_url_request_util, etc.
 It'd be nice to have a cleaner model.  Unfortunately, I don't know how feasible
that is, given the threading restrictions and whatnot, at least not without a
bit of a refactor.

https://codereview.chromium.org/2042483002/diff/60001/extensions/browser/exte...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/2042483002/diff/60001/extensions/browser/exte...
extensions/browser/extension_protocols.cc:422: // TODO(mpcomplete): better error
code.
Is this considered a better error code? :)

[nasko, 2016-06-06 23:23:03]

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_navigation_throttle.h (right):

https://codereview.chromium.org/2042483002/diff/1/extensions/browser/extensio...
extensions/browser/extension_navigation_throttle.h:29: ThrottleCheckResult
WillStartRequest() override;
On 2016/06/06 22:17:22, Devlin wrote:
> On 2016/06/06 17:40:44, nasko (slow) wrote:
> > On 2016/06/03 22:22:45, Devlin wrote:
> > > I'm not very familiar with NavigationThrottle, so I'm not entirely clear
on
> > when
> > > this will be called as opposed to our n other WAR checks.  Are they all
> still
> > > necessary?  Can we change any ifs to dchecks?
> > 
> > This will be called before the request is sent to the network. It is called
> > after the job is created on the IO thread, which is where all the existing
> > checks are performed. However, those checks run on the IO thread, where we
> don't
> > have the ancestor information, so we can't do it there.
> > 
> > Because the existing checks run ahead of this one, we can't convert ifs into
> > DCHECKs. Also, if it gets blocked on the IO thread, we end up saving
ourselves
> > creating a request and jumping to the UI thread, so I'd say we keep them for
> > perf reasons, even though we can technically remove them.
> 
> I'm a little worried that this will become (more) confusing between having
> extension_protocols, this class, url_request_util, chrome_url_request_util,
etc.
>  It'd be nice to have a cleaner model.  

I think the cleaner model will be once we ship PlzNavigate. We can then enforce
navigation restrictions on the UI thread and subresources on the IO thread.
Also, we should probably remove the redundant checks from renderer side, unless
we think they are providing enough of perf advantage to leave them in.

> Unfortunately, I don't know how feasible
> that is, given the threading restrictions and whatnot, at least not without a
> bit of a refactor.

I'm definitely open to simplifications. Feel free to start a discussion and I'll
contribute. I'm very big fan of simpler codebase.

https://codereview.chromium.org/2042483002/diff/60001/extensions/browser/exte...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/2042483002/diff/60001/extensions/browser/exte...
extensions/browser/extension_protocols.cc:422: // TODO(mpcomplete): better error
code.
On 2016/06/06 22:17:22, Devlin wrote:
> Is this considered a better error code? :)

I don't know : ), hence why I left the comment. I can yank it if you think it is
better now.

[nasko, 2016-06-06 23:48:00]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-06-06 23:48:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2042483002/#ps80001
(title: "Remove stale comment.")

[commit-bot: I haz the power, 2016-06-06 23:48:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2042483002/80001

[commit-bot: I haz the power, 2016-06-07 01:22:05]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-06-07 01:25:12]

Description was changed from

==========
Fix web_accesible_resources enforcement for Site Isolation.

When --isolate-extensions or --site-per-process modes are enabled, all
extensions frames run in extension processes and are not mixed in regular
web renderers. This causes a problem with security checks for
web_accessible_resources, which allow all navigations to extension pages
when they are performed in extension process. This is no longer true and
this patch addresses this by using a NavigationThrottle to perform the
proper checks on the UI thread (also PlzNavigate compatible).

BUG=616488
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix web_accesible_resources enforcement for Site Isolation.

When --isolate-extensions or --site-per-process modes are enabled, all
extensions frames run in extension processes and are not mixed in regular
web renderers. This causes a problem with security checks for
web_accessible_resources, which allow all navigations to extension pages
when they are performed in extension process. This is no longer true and
this patch addresses this by using a NavigationThrottle to perform the
proper checks on the UI thread (also PlzNavigate compatible).

BUG=616488
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/b9164c43d2900c967f4fdb5ebfc4812f7e914116
Cr-Commit-Position: refs/heads/master@{#398189}
==========

[commit-bot: I haz the power, 2016-06-07 01:25:14]

Patchset 4 (id:??) landed as
https://crrev.com/b9164c43d2900c967f4fdb5ebfc4812f7e914116
Cr-Commit-Position: refs/heads/master@{#398189}