Errr... I forgot to mention that the repro also requires getting rid of renderer-side check that is normally happening in extensions::ResourceRequestPolicy::CanRequestResource.

lukasza, I can't tell from your write-up whether this impacts shipping chrome, or is just a nice-to-have add-on feature as demonstrated by the source code change in C#1.  Could you clarify so that we may categorize this properly?  Thanks.

This bug means that a browser will let an exploited renderer (hosting web content) commit non-web-accessible extension resources.  Maybe nasko@ or rdevlin.cronin@ can comment on the impact of this (is this relatively benign, or does it open a possibility of a sandbox escape for an exploited renderer?).

To clarify, this is not something that is currently shipping in Chrome. The bug would exist if we stop checking for extension URLs in the renderer, which is just a proposal at this stage.

This is going to be violating our security model, as we shouldn't allow non-web_acessible_resources to commit in regular processes.

lukasza@, would the test behave differently if instead of using an image file (the png) the URL is to an actual HTML document? I wonder if the reliance on the ResourceRequest type is the cause of this, which can have other implications.

Hmm...

So, if this is already a compromised renderer, then presumably it can not only commit the resource, but also influence it to access scary APIs, which is bad.  Does this require that the extension is hosted in the same process as the web content?  (It sounds like "return false from ChromeExtensionsRendererClient::ShouldFork" might do that.)  If so, this really isn't anything new - if a compromised renderer in the same process as an extension with certain permissions made an IPC request, the browser would do it.

Let me try to clarify what is happening in the repro:

1. I need to relax 2 renderer-side checks:
   - ShouldFork
   - extensions::ResourceRequestPolicy::CanRequestResource
Relaxed checks can be seen as simulation of an exploited renderer.

2. Afterwards, a renderer hosting only web content can navigate to a non-web-accessible extension resource.

As rdevlin.cronin@ pointed above (TIL :-), step 2 shouldn't necessarily grant new privileges to the renderer process (unless it already had them before, which it shouldn't in --isolate-extensions world).  I'd probably want to double-check that (because here this is a top-level navigation, not a subframe navigation), but hopefully this is indeed a false alarm and this won't help with sandbox escapes.

As nasko@ pointed out above, the extension resource being navigated to is a png file - I need to check if the repro will also happen for a html document.

Also - looking at my previous comments I see that I forgot to mention the need to relax ShouldFork renderer-side check :-(  This is not a big deal, because (at least for now) POST requests skip ShouldFork check anyway.

Updating labels based on #4

RE: #4

This *is* something that is currently shipping in Chrome - an exploited renderer won't necessarily go through renderer-side checks in extensions::ResourceRequestPolicy::CanRequestResource and ShouldFork.


RE: would the test behave differently if instead of using an image file (the png) the URL is to an actual HTML document?

No - the commit also (unexpectedly/incorrectly) succeeds for an actual HTML document.

Interestingly, ShouldAllowOpenURL returns false.  I am not yet sure why the page still commits.

On the bright side, the renderer doesn't gain extension privileges AFAICT - the test expectation below passes just fine:

  EXPECT_FALSE(extensions::ProcessMap::Get(profile())->Contains(
      web_contents->GetMainFrame()->GetProcess()->GetID()));

So (as rdevlin.cronin@ pointed out in #c5) this doesn't help an exploited renderer with a sandbox escape.

Cool.  I think this is pretty mild overall, then, because we should have browser-side security checks for anything critical (and compromised renderers can already do crazy stuff).  Maybe with plznavigate we could cut this down a bit more, though?

+alexmos@ (maybe his blob/filesystem-related CLs will also help here?)

Thanks for the CC, Lukasz.  I've confirmed that my draft CL for fixing WAR checks in ShouldAllowOpenURL (https://codereview.chromium.org/2454563003/) fixes this bug.  Here's how I tried this:
1) Installed Charlie's sample extension from https://bugs.chromium.org/p/chromium/issues/detail?id=487872#c4
2) Changed extensions::ResourceRequestPolicy::CanRequestResource to always return true
3) Changed ChromeExtensionsRendererClient::ShouldFork to always return false
4) Browsed to http://csreis.github.io
5) Executed location.href="chrome-extension://bpaekgelgggkcjapdbfgeeobkemgemok/form-post.html" , where form-post.html is not web-accessible (and part of extension from step 1).

This is now blocked in ShouldAllowOpenURL, and we stay at the old page.  Stack trace at the time of block:

#1 0x7fc5c0e4abf6 extensions::ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL()
#2 0x7fc5bf65b2ba ChromeContentBrowserClient::ShouldAllowOpenURL()
#3 0x7fc5b780e49d content::NavigatorImpl::RequestTransferURL()
#4 0x7fc5b785663d content::RenderFrameHostManager::OnCrossSiteResponse()
#5 0x7fc5b77ff50c content::NavigationHandleImpl::MaybeTransferAndProceedInternal()
#6 0x7fc5b77fd329 content::NavigationHandleImpl::MaybeTransferAndProceed()
#7 0x7fc5b77fe3b1 content::NavigationHandleImpl::WillProcessResponse()
#8 0x7fc5b7a6c373 content::(anonymous namespace)::WillProcessResponseOnUIThread()

I think previously, ShouldAllowOpenURL would also indicate that this should be blocked, but we still allowed the old request to succeed due to a bug in how the blocking behaved on the transfer path (we rewrote the URL to "about:blank", whereas we should have canceled the transfer instead) -- see point 3 in my draft CL description.  I've confirmed that steps 1-5 would load form-post.html successfully without my CL.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c3da4f745a8a4d67573ca386fc9ad24ced878d4

commit 1c3da4f745a8a4d67573ca386fc9ad24ced878d4
Author: alexmos <alexmos@chromium.org>
Date: Thu Nov 03 06:06:11 2016

Fix web accessible resource checks in ShouldAllowOpenURL

(This is joint work with nick@)

When a web page navigates an extension main frame to another extension
URL via a proxy, RenderFrameProxyHost::OnOpenURL calls
NavigatorImpl::RequestTransferURL, which checks whether the
destination URL points to a web-accessible resource in
ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.

This CL fixes several problems with that check:

1. It doesn't work properly for blob/filesystem URL, since it
   references schemes on the destination GURL directly.  This leads to
   a security problem where all blob/filesystem URLs are allowed.  To
   fix this, use url::Origin to handle nested URL cases properly,
   similarly to other fixes in this area (e.g.,  issue 645028 ).

2. It ignores cases where the source SiteInstance's site URL isn't an
   HTTP, HTTPS, or extension scheme.  Thus, a new tab with a data or
   about:blank URL bypasses WAR checks.  This artificial restriction
   apparently was added to make a test pass several years ago.  This
   CL removes it, and fixes one test that was wrong and which relied
   on this logic. The test is
   ExtensionBrowserTest.WindowOpenNoPrivileges; it tried to
   window.open a non-web-accessible resource while incorrectly
   assuming it was web-accessible.  Since it did so from about:blank
   with a blank SiteInstance, this bug allowed the test to work.

3. The things that ShouldAllowOpenURL does block aren't blocked
   correctly for transfers.  RequestTransferURL rewrites the blocked
   URL to about:blank, and later, NavigatorImpl::NavigateToEntry
   decides that this is a transfer to the same RFH
   (is_transfer_to_same == true), so it happily lets the old
   navigation continue and load the *old* (disallowed) URL.  To fix
   this, we return early instead of rewriting dest_url to
   about:blank.

4. The SiteInstance plumbed through to ShouldAllowOpenURL when going
   via RenderFrameProxyHost::OnOpenURL is wrong: it takes the current
   SiteInstance of the frame that's being navigated, rather than the
   SiteInstance in which the navigation on the proxy was started.
   RenderFrameProxyHost::OnOpenURL already passes the right
   SiteInstance as source_site_instance to RequestTransferURL, so it
   just needs to use it when it's available.

Two new tests are added:

NestedURLNavigationsViaProxyBlocked for case 1.
WindowOpenInaccessibleResourceFromDataURL for case 2.

The fixes to ShouldAllowOpenURL also apply to subframe navigations as
well as main frame navigations (since it doesn't distinguish between
the two).  That is, subframe navigations that go through proxies,
transfers, or OpenURL, will now also have the unsafe blob/filesystem
URL blocking enforced, whereas before this was only done for main
frames by other checks.  The test exercising this for subframes,
NestedURLNavigationsViaProxyBlocked, was updated accordingly.

Why don't our other checks work?
--------------------------------

1. ExtensionNavigationThrottle also performs web-accessible resource
checks checks, but that currently happens only for subframes.  Thus,
ShouldAllowOpenURL are the only checks that would work in this
scenario for main frames.  Fundamentally, ExtensionNavigationThrottle
can't perform the WAR checks for main frames until we plumb through
the initiator SiteInstance (i.e., who navigated the proxy, and not the
StartingSiteInstance, which corresponds to the frame that's being
navigated) to NavigationHandle.

2. ChromeExtensionNetworkDelegate also blocks unsafe navigations to
blob/filesystem URLs.  It doesn't apply in this case, since the process
where this navigation is happening is already an extension process.

3. url_request_util::AllowCrossRendererResourceLoad also allows this
navigation, as it explicitly excludes all main frames from WAR
enforcement:
  // Navigating the main frame to an extension URL is allowed, even if not
  // explicitly listed as web_accessible_resource.
  if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
    *allowed = true;
    return true;
  }

BUG= 656752 ,  645028 ,  652887 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2454563003
Cr-Commit-Position: refs/heads/master@{#429532}

[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/browser/extensions/extension_resource_request_policy_apitest.cc
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/browser/extensions/window_open_apitest.cc
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/test/data/extensions/uitest/window_open/manifest.json
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/tools/metrics/histograms/histograms.xml

More to be done here, or can we mark this as fixed?

Marking as fixed.  Please re-open if that's incorrect.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot