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

extensions/browser/extension_navigation_throttle.cc

Index: extensions/browser/extension_navigation_throttle.cc
diff --git a/extensions/browser/extension_navigation_throttle.cc b/extensions/browser/extension_navigation_throttle.cc
index bb5137148c7c948ae3f85f0053051e87ae883751..d26441f8f357652a6888425b57d7e63fbd57a1ef 100644
--- a/extensions/browser/extension_navigation_throttle.cc
+++ b/extensions/browser/extension_navigation_throttle.cc
@@ -53,14 +53,17 @@ ExtensionNavigationThrottle::WillStartRequest() {
     if (is_nested_url && origin.scheme() == extensions::kExtensionScheme &&
         !is_extension) {
       // Relax this restriction for apps that use <webview>.  See
-      // https://crbug.com/652077.
+      // https://crbug.com/652077.  Be careful to require the request to be
+      // made from a <webview> guest process if the app has a webview
+      // permission (https://crbug.com/656752).
       const extensions::Extension* extension =
           registry->enabled_extensions().GetByID(origin.host());
       bool has_webview_permission =
           extension &&
           extension->permissions_data()->HasAPIPermission(
               extensions::APIPermission::kWebView);
-      if (!has_webview_permission)
+      bool from_guest = guest_view::GuestViewBase::IsGuest(web_contents);
+      if (!has_webview_permission || !from_guest)

[alexmos, 2017/05/10 17:12:19]

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]

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?

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

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:19]

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. 

         return content::NavigationThrottle::CANCEL;
     }