Complete UI thread blob/filesystem URL blocking and remove IO thread check.

Complete UI thread blob/filesystem URL blocking and remove IO thread check.

This CL adds one missing bit of our nested URL checks to
ExtensionNavigationThrottle: for apps that have a <webview>
permission, ensuring that only guest processes should be allowed
access to the nested URLs.  Previously this check lived in
ChromeExtensionsNetworkDelegateImpl::OnBeforeURLRequest, which was
incompatible with PlzNavigate and caused a test
(NestedURLNavigationsToAppBlocked) to be disabled with PlzNavigate.

Now that the nested URL checks are in sync on the IO and UI threads,
and there are no differences for these checks in our branches, remove
the IO thread checks in ChromeExtensionsNetworkDelegateImpl.

BUG=656752

[alexmos, 2017-05-09 22:06:35]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 22:07:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 22:49:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 22:49:24]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[alexmos, 2017-05-10 17:08:12]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 17:09:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexmos, 2017-05-10 17:12:18]

alexmos@chromium.org changed reviewers:
+ nick@chromium.org

[alexmos, 2017-05-10 17:12:20]

Nick, can you please take a look?  This should get the UI blob/filesystem checks
up to parity with the IO thread check, which we can now remove.  I've been
meaning (and forgetting) to do this once all the branches got in sync for these
checks, and Nasko reminded me that we should probably do this before PlzNavigate
ships, so it's good to do it now.

https://codereview.chromium.org/2870843003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_extensions_network_delegate.cc (left):

https://codereview.chromium.org/2870843003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:211:
policy->HasSpecificPermissionForOrigin(info->GetChildID(), origin);
Can't remove this from the API, unfortunately, because the Devtools extensions
stuff is also using it now.

https://codereview.chromium.org/2870843003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:221:
base::debug::DumpWithoutCrashing();
I checked the stats on this, and it's rare (40 instances in all versions since
last Oct), so it's unlikely that there were regressions due to this.

https://codereview.chromium.org/2870843003/diff/20001/extensions/browser/exte...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2870843003/diff/20001/extensions/browser/exte...
extensions/browser/extension_navigation_throttle.cc:66: if
(!has_webview_permission || !from_guest)
I was thinking whether we can go one step further and also check that the
extension ID in the origin matches the guest's owner_host() (i.e., the guest can
only open its embedder app's blob URLs and not some other app's or extension's
blob URLs).  But that carries some risk, so probably better to attempt in a
separate CL.

[ncarter (slow), 2017-05-10 17:47:10]

https://codereview.chromium.org/2870843003/diff/20001/extensions/browser/exte...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2870843003/diff/20001/extensions/browser/exte...
extensions/browser/extension_navigation_throttle.cc:66: if
(!has_webview_permission || !from_guest)
On 2017/05/10 17:12:19, alexmos wrote:
> I was thinking whether we can go one step further and also check that the
> extension ID in the origin matches the guest's owner_host() (i.e., the guest
can
> only open its embedder app's blob URLs and not some other app's or extension's
> blob URLs).  But that carries some risk, so probably better to attempt in a
> separate CL.

I'm aware of a few deficiencies with this logic, as currently implemented:
 - Using navigation_handle()->GetStartingSiteInstance() is inherently flawed,
since that origin may actually have nothing to do with the navigation.
 - As you point out, we could do an guest-owner-extension check here. Note that
larger changes are going to be in conflict with
https://codereview.chromium.org/2830893002/ (which I really hope to unblock and
land today).
 - Now that --isolate-extensions is launched, I'm uncertain of the value of
blocking these navigations in the first place. The Create blocking seems pretty
strong, although this can be seen as a defense in depth against that.

So I wonder: if we want to maintain blob/filesystem navigation blocking (maybe
so that it works as a line of defense once we have isolated origins), can we
implement it as a policy that works for all origins, and is not
extension-specific?

[commit-bot: I haz the power, 2017-05-10 18:20:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 18:20:02]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-05-10 20:20:56]

https://codereview.chromium.org/2870843003/diff/20001/extensions/browser/exte...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2870843003/diff/20001/extensions/browser/exte...
extensions/browser/extension_navigation_throttle.cc:66: if
(!has_webview_permission || !from_guest)
On 2017/05/10 17:47:10, ncarter wrote:
> On 2017/05/10 17:12:19, alexmos wrote:
> > I was thinking whether we can go one step further and also check that the
> > extension ID in the origin matches the guest's owner_host() (i.e., the guest
> can
> > only open its embedder app's blob URLs and not some other app's or
extension's
> > blob URLs).  But that carries some risk, so probably better to attempt in a
> > separate CL.
> 
> I'm aware of a few deficiencies with this logic, as currently implemented:
>  - Using navigation_handle()->GetStartingSiteInstance() is inherently flawed,
> since that origin may actually have nothing to do with the navigation.
>  - As you point out, we could do an guest-owner-extension check here. Note
that
> larger changes are going to be in conflict with
> https://codereview.chromium.org/2830893002/ (which I really hope to unblock
and
> land today).
>  - Now that --isolate-extensions is launched, I'm uncertain of the value of
> blocking these navigations in the first place. The Create blocking seems
pretty
> strong, although this can be seen as a defense in depth against that.
> 
> So I wonder: if we want to maintain blob/filesystem navigation blocking (maybe
> so that it works as a line of defense once we have isolated origins), can we
> implement it as a policy that works for all origins, and is not
> extension-specific?


That's a really good question.  I agree that with Create blocking it has less
value now that we have --isolate-extensions, but also I'm curious about these
cases:

- could the attacker still guess an filesystem URL that was already created
inside of a dedicated process and attempt to navigate to it?  Or similarly the
blob URL could leak via postMessage, etc.  Or could navigating to an invalid
blob/filesystem URL still cause an undesired process swap that this could
prevent?

- we have some exceptions to the create checks, e.g. the recent drag-and-drop
one for filesystem: or <webview> - should we worry about those?

I also agree that this really should be generalized to also work for isolated
origins, and that it can be a second line of defense for those as well as
extensions.

[alexmos, 2017-05-24 18:48:20]

https://codereview.chromium.org/2870843003/diff/20001/extensions/browser/exte...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2870843003/diff/20001/extensions/browser/exte...
extensions/browser/extension_navigation_throttle.cc:66: if
(!has_webview_permission || !from_guest)
On 2017/05/10 20:20:56, alexmos wrote:
> On 2017/05/10 17:47:10, ncarter wrote:
> > On 2017/05/10 17:12:19, alexmos wrote:
> > > I was thinking whether we can go one step further and also check that the
> > > extension ID in the origin matches the guest's owner_host() (i.e., the
guest
> > can
> > > only open its embedder app's blob URLs and not some other app's or
> extension's
> > > blob URLs).  But that carries some risk, so probably better to attempt in
a
> > > separate CL.
> > 
> > I'm aware of a few deficiencies with this logic, as currently implemented:
> >  - Using navigation_handle()->GetStartingSiteInstance() is inherently
flawed,
> > since that origin may actually have nothing to do with the navigation.
> >  - As you point out, we could do an guest-owner-extension check here. Note
> that
> > larger changes are going to be in conflict with
> > https://codereview.chromium.org/2830893002/ (which I really hope to unblock
> and
> > land today).
> >  - Now that --isolate-extensions is launched, I'm uncertain of the value of
> > blocking these navigations in the first place. The Create blocking seems
> pretty
> > strong, although this can be seen as a defense in depth against that.
> > 
> > So I wonder: if we want to maintain blob/filesystem navigation blocking
(maybe
> > so that it works as a line of defense once we have isolated origins), can we
> > implement it as a policy that works for all origins, and is not
> > extension-specific?
> 
> 
> That's a really good question.  I agree that with Create blocking it has less
> value now that we have --isolate-extensions, but also I'm curious about these
> cases:
> 
> - could the attacker still guess an filesystem URL that was already created
> inside of a dedicated process and attempt to navigate to it?  Or similarly the
> blob URL could leak via postMessage, etc.  Or could navigating to an invalid
> blob/filesystem URL still cause an undesired process swap that this could
> prevent?
> 
> - we have some exceptions to the create checks, e.g. the recent drag-and-drop
> one for filesystem: or <webview> - should we worry about those?
> 
> I also agree that this really should be generalized to also work for isolated
> origins, and that it can be a second line of defense for those as well as
> extensions.

Nick, should we take another look at this before the branch cut?  Just want to
make sure we get this in in time for PlzNavigate shipping if we do decide that
it's necessary.  I think it might be worthwhile to keep as defense-in-depth, as
well as the cases where the attacker guesses or otherwise learns the nested URL.

Just to make sure I understand your GetStartingSiteInstance() concern.  If A is
navigating frame B to C, it gives you B, and that's used to determine
|current_frame_is_extension_process| and |is_guest|.  For is_guest (i.e., the
webview-accessible resource check), that seems like what we want, no?
We want to check if the frame being navigated is a guest, not the frame that
navigates it.

For the extension nested URL check, is your concern that an attacker's (web)
frame A could navigate an extension process for E1 to a nested URL for another
extension E2, and the check ought to block that (but currently it will allow it
because current_frame_is_extension_process will be true)?  That's a problem,
though it requires an attacker to already get access to one extension process. 
It also might be the best we can do until we get initiator plumbing worked out. 
Other than this, it seems it will sometimes cause us to block too much, e.g., if
extension E1 navigates a web frame A to a nested URL for E1, we currently block
that, but technically we could allow it since we'll swap processes anyway.  But
it's been there for a few versions and nobody complained. :)

Assuming we want to keep this as defense-in-depth, I agree we should generalize
this to also work for isolated origins, etc.  We can probably check if the
origin in the nested URL requires a dedicated process and see if the current
site matches that.  But that will be easier to do in a separate CL once we have
this logic centralized here and removed from the IO thread.