jam@ recently moved the original blocking logic to ExtensionNavigationThrottle::WillStartRequest to make it compatible with PlzNavigate (see r424937).   With that change, it should now be possible to tighten the current check on M56 to actually check whether a guest WebContents is making the request, and whether its owner matches the origin in the blob/filesystem URL.  In fact, the logic to check for a guest and its owner was also recently added to ExtensionNavigationThrottle::WillStartRequest in r425073 by jam@ to fix some other WebView tests.  So it seems we should be able to just reuse this for M56+.  I need to think more about  what we can do (if anything) to also tighten the check on M55/M54.

Thanks.  It might be worth adding that for M56 (for the hopefully rare case that --isolate-extensions is disabled there), while we think about ways we might be able to improve M54/M55.

Alex and I came up with a possible avenue here, that may be mergeable to protect the population of M54 non-oopif users.

For top-level navs to extension blobs, we currently have a policy of allowing if "already in a privileged extension process, or else if the target blob belongs to an extension with the <webview> privilege". We can change that to "already in a privileged extension process, or a guest process of this extension".

With jam@'s code move mentioned in comment #1, the tighter policy ought to be trivial to implement -- we can just look at the WebContents or the SiteInstance to see if it's a chrome guest. We should immediately attempt that, to prototype this approach, and make sure that we don't regress the tests from r422954.

For M54, the challenge is to determine "is this a guest process" on the IO thread. What we realized is that the ChildProcessSecurityPolicyImpl already has the origin "chrome-extension://fhwgads" in the origin_set_ for the guest process, thanks to the GrantOrigin we added to WebViewGuest::CreateWebContents for the oopif fix. If we add a new getter -- say, ChildProcessSecurityPolicyImpl::HasSpecificPermissionForOrigin() -- that queries origin_set_ directly without calling IsWebSafeScheme, we should match only the associated guest process. Since this is consulting existing security state that's already in place, it seems safer to merge.

The hard part of this is that we'll need to develop it on M54 or M55 directly, since the code in question doesn't exist at TOT. But it looks pretty small:  1 new function in ChildProcessSecurityPolicyImpl, plus one call site in ChromeExtensionsNetworkDelegate, and no new Grant ops that could carry potential risk.

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

commit 78760ccdaf6c22e6e436ce21d9d31a6030a5634e
Author: alexmos <alexmos@chromium.org>
Date: Fri Oct 21 18:58:02 2016

Temporarily reintroduce blob/filesystem URL security checks on the IO thread.

These checks were originally introduced in r419019 and r422954.  This
was merged back to M54 and M55.

They were later moved to the UI thread (into
ExtensionNavigationThrottle) in r424937 to make them compatible with
PlzNavigate.  However, that only exists in M56.

Per linked bug, we need to tighten the security check currently in M54
and M55 for apps with a "webview" permission where the problematic
blob/filesystem URL requests aren't made from the guest process.

This is the plan for fixing this:
1.  <This CL> Reintroduce these IO thread checks on M56.
2.  https://codereview.chromium.org/2437753003: Tighten the checks for
    the scenario above using ChildProcessSecurityPolicy.
3.  Merge the CL from step 2 back to M55 and M54.
4.  Remove these checks from M56 and tighten the checks in
    ExtensionNavigationThrottle.

BUG= 656752 

Review-Url: https://chromiumcodereview.appspot.com/2435593007
Cr-Commit-Position: refs/heads/master@{#426855}

[modify] https://crrev.com/78760ccdaf6c22e6e436ce21d9d31a6030a5634e/chrome/browser/net/chrome_extensions_network_delegate.cc

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

commit f7af135c51a99daba0cdb5ef204465ff995802e4
Author: alexmos <alexmos@chromium.org>
Date: Fri Oct 21 20:00:41 2016

Tighten IO thread blob/filesystem URL checks for apps with webview permission.

The IO thread blob/filesystem URL security checks were originally
introduced in r419019 and r422954.  This was merged back to M54 and
M55.

They were later moved to the UI thread (into
ExtensionNavigationThrottle) in r424937 to make them compatible with
PlzNavigate.  However, that only exists in M56.

Per linked bug, we need to tighten the security check currently in M54
and M55 for apps with a "webview" permission where the problematic
blob/filesystem URL requests aren't made from the guest process.

This is the plan for fixing this:
1.  https://codereview.chromium.org/2435593007/: Reintroduce these IO
    thread checks on M56.
2.  <This CL> Tighten the checks for the scenario above using
    ChildProcessSecurityPolicy.
3.  Merge the CL from step 2 back to M55 and M54.
4.  Remove these checks from M56 and tighten the checks in
    ExtensionNavigationThrottle.

The goal of this CL is to determine if a request is being made by a
guest process on the IO thread.  It relies on the fact that
ChildProcessSecurityPolicyImpl already has the origin
"chrome-extension://<appid>" in the origin_set_ for the guest process,
thanks to the GrantOrigin that was added to
WebViewGuest::CreateWebContents in r422976 as part of fixing issue
652784.  Therefore, it's sufficient to check whether the requested
nested URL's origin is in the origin_set_ for the process making the
request.

BUG= 656752 

Review-Url: https://chromiumcodereview.appspot.com/2437753003
Cr-Commit-Position: refs/heads/master@{#426870}

[modify] https://crrev.com/f7af135c51a99daba0cdb5ef204465ff995802e4/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/f7af135c51a99daba0cdb5ef204465ff995802e4/chrome/browser/net/chrome_extensions_network_delegate.cc
[modify] https://crrev.com/f7af135c51a99daba0cdb5ef204465ff995802e4/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/f7af135c51a99daba0cdb5ef204465ff995802e4/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/f7af135c51a99daba0cdb5ef204465ff995802e4/content/public/browser/child_process_security_policy.h

I'm worried that we're still vulnerable here in the non-oopif case, as the following scenario appears to work on stable:

w = window.open("chrome-extension://target-extension/options_page.html");
w.location = "blob:chrome-extension://target-extension/attacker-payload-blob";

I.e., we can do a targeted navigation of a tab that's already hosted in an extension process. For blob-request-time enforcement, the security check needs to be done against the initiator of the navigation, not the target.

We discussed the above comment #9 in team chat. Here's a log:

Nasko: oh, that one is not that easy to solve : ( we don't have initiator info yet

Nick: could we do the enforcement way earlier, on whatever IPC arrives from the initiating process?

Nasko: This would be a navigation through a proxy, right, since the window will be in a new process
so we could try to put validation in the RenderFrameProxyHost
but that feels a bit one off, though that might be ok for a pointed fix

Nick: like here: https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_proxy_host.cc?sq=package:chromium&dr=CSs&rcl=1477066623&l=247 ?

Nasko: Yeah, this is what I had in mind
though that will have to call out to the content client to handle it
or maybe not!
we can't do blobs cross-origin, right?
if we are going through a proxy,then we are definitely cross-origin
possibly block all blob: requests at that point?!

Nick: I really like the idea of blocking blob/filesystem on origin mismatch there. but sadly i don't think that suffices to plug the hole. in non-oopif we have unblessed extension frames.

Nasko: do we even have to check origins?
block blobs outright, no?
also, without oopifs, we won't be going through proxies

Nick: I see. We only hit the proxy case if we're cross process which implies cross origin.

Nasko: Not sure if we can do it for filesystem, but blobs for sure seems applicable
Yeah, proxy guarantees us cross-origin

Nick: oh, but what if you have a same-origin blob URL, and you want to navigate a popup you opened back to your process?

Nasko: hahaha : ) I guess in that case we do need to check the origin

Nick: i think we need to special case extensions here.

Nasko: though we do need the initiator origin in that case

Nick: even that doesn't work, because the initiating process is totally allowed (in non oopif remember) to have chrome extension frames.
unblessed ones. (╯°□°)╯︵ ┻━┻

Nick: i am thinking we take the policy we have implement in chrome_extensions_network_delegate_ and port it to RenderFrameProxyHost.

Nasko: as in extract it in a method in chrome/ and call it through a contentclient interface : )

Nick: Yeah. And it has the benefit of being on the UI thread so we can use the TOT implementation that jam did.

Nasko: is this for --isolate-extensions or not?

Nick: --isolate-extensions is thought to be totally secure right now, since we block the creation.
blocking creation is not possible without --isolate-extensions because of unblessed frames, so the fallback strategy is to try to block top-level navigations to blob/filesystem urls -- but we still have to allow ones that originate from privileged processes.

#9: Nick, thanks for finding this!  This actually might be due to letting popups stay in the same BrowsingInstance from  issue 590068 .  Before fixing that in r423361, I *think* the repro would've failed because we would have broken script references by putting the options page in a new BrowsingInstance (i.e., |w| would've been null after window.open -- that's what happens today if you force a BI swap with window.open("foo.html","","noopener")).  With that fix, both web->extension and extension->web window.open can now script the popup/opener.  I think at the time we merged this, the expectation was still that we would turn on --isolate-extensions in M54, which would've made this a non-issue thanks to other defenses.  But given our change of plans, we indeed need to do something about this for M54.  I like the idea of blocking this in RFPH::OnOpenURL, or we could just revert r423361 from M54 (I still need to verify that the latter would indeed help).

Note that the attack in #9 is also possible if an extensions page opens a (malicious) web popup, and that popup navigates the opener to an extension blob.  (So avoiding BI swap for extension->web but forcing a swap for web->extension transitions isn't going to help.)

Re @alexmos "Note that the attack in #9 is also possible ..." -- interesting. I'm interpreting it overall like a less valuable attack, unless we know of a builtin or extremely popular extension that can be tricked into opening a popup to an arbitrary URL. However, blocking at RFPH::OnOpenURL time ought to suffice here.

I wonder if we can implement this in a pure-content way with the following policy: only allow a blob/filesystem navigation if it would swap to the browsinginstance of the initiator?

Requesting to merge r426870 (#8) to M55.  This has baked on canary over the weekend, and I've checked that so far there are no unexpected crashes and no DumpWithoutCrashing reports that would indicate unintended blocking.  

Note that the discussion in #9-12 will require a separate fix, which I'll work on next.

Your change meets the bar and is auto-approved for M55 (branch: 2883)

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

commit 3299d4da316b8dd3cbf72e49e5b3de4ea806e87f
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Mon Oct 24 18:36:08 2016

Tighten IO thread blob/filesystem URL checks for apps with webview permission.
[Merge to M55]

The IO thread blob/filesystem URL security checks were originally
introduced in r419019 and r422954.  This was merged back to M54 and
M55.

They were later moved to the UI thread (into
ExtensionNavigationThrottle) in r424937 to make them compatible with
PlzNavigate.  However, that only exists in M56.

Per linked bug, we need to tighten the security check currently in M54
and M55 for apps with a "webview" permission where the problematic
blob/filesystem URL requests aren't made from the guest process.

This is the plan for fixing this:
1.  https://codereview.chromium.org/2435593007/: Reintroduce these IO
    thread checks on M56.
2.  <This CL> Tighten the checks for the scenario above using
    ChildProcessSecurityPolicy.
3.  Merge the CL from step 2 back to M55 and M54.
4.  Remove these checks from M56 and tighten the checks in
    ExtensionNavigationThrottle.

The goal of this CL is to determine if a request is being made by a
guest process on the IO thread.  It relies on the fact that
ChildProcessSecurityPolicyImpl already has the origin
"chrome-extension://<appid>" in the origin_set_ for the guest process,
thanks to the GrantOrigin that was added to
WebViewGuest::CreateWebContents in r422976 as part of fixing issue
652784.  Therefore, it's sufficient to check whether the requested
nested URL's origin is in the origin_set_ for the process making the
request.

BUG= 656752 

Review-Url: https://chromiumcodereview.appspot.com/2437753003
Cr-Commit-Position: refs/heads/master@{#426870}
(cherry picked from commit f7af135c51a99daba0cdb5ef204465ff995802e4)

Review URL: https://codereview.chromium.org/2450503002 .

Cr-Commit-Position: refs/branch-heads/2883@{#245}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/3299d4da316b8dd3cbf72e49e5b3de4ea806e87f/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/3299d4da316b8dd3cbf72e49e5b3de4ea806e87f/chrome/browser/net/chrome_extensions_network_delegate.cc
[modify] https://crrev.com/3299d4da316b8dd3cbf72e49e5b3de4ea806e87f/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/3299d4da316b8dd3cbf72e49e5b3de4ea806e87f/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/3299d4da316b8dd3cbf72e49e5b3de4ea806e87f/content/public/browser/child_process_security_policy.h

Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

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

Not entirely fixed yet.  The issue from comments 9-12 still needs to be fixed, and the fix in #8 needs to bake on Beta and be merged to M54.

I've confirmed that the issue in #9-12 doesn't happen on M54 if I revert the BrowsingInstance fix from  issue 590068  (https://chromium.googlesource.com/chromium/src.git/+/50cdccb5bfd420f73e93481184f80446edf79730) from M54.  Things work a bit differently than I expected: after doing "w = window.open(...)", the |w| reference and the corresponding proxy are valid, but when we issue the navigation via "w.location = 'blob:chrome-extension://target-extension/attacker-payload-blob';", it gets blocked by the following check in RenderFrameProxyHost::OnOpenURL:

  // Verify that we are in the same BrowsingInstance as the current
  // RenderFrameHost.
  RenderFrameHostImpl* current_rfh = frame_tree_node_->current_frame_host();
  if (!site_instance_->IsRelatedSiteInstance(current_rfh->GetSiteInstance()))
    return;

Reverting this might be the safest option for M54, but it does change the behavior a bit: before the 590068 fix, web popups opened from extension main frames and vice versa had a null window.opener, after the fix (i.e., when M54 was released) the window.opener started being valid, and reverting the fix would make window.opener null again.  There's a slight risk that developers could start relying on the new window.opener behavior since the initial release of M54, though afaik this wasn't announced or discussed anywhere outside  issue 590068 .

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

commit 01a85bdd1110a2fb19b21dc25e2995fa30f4ba28
Author: nick <nick@chromium.org>
Date: Thu Oct 27 20:17:12 2016

Add test case that navigates to an extension blob from inside of devtools.

BUG= 656752 

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

[modify] https://crrev.com/01a85bdd1110a2fb19b21dc25e2995fa30f4ba28/chrome/browser/devtools/devtools_sanity_browsertest.cc

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

commit 3299d4da316b8dd3cbf72e49e5b3de4ea806e87f
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Mon Oct 24 18:36:08 2016

Tighten IO thread blob/filesystem URL checks for apps with webview permission.
[Merge to M55]

The IO thread blob/filesystem URL security checks were originally
introduced in r419019 and r422954.  This was merged back to M54 and
M55.

They were later moved to the UI thread (into
ExtensionNavigationThrottle) in r424937 to make them compatible with
PlzNavigate.  However, that only exists in M56.

Per linked bug, we need to tighten the security check currently in M54
and M55 for apps with a "webview" permission where the problematic
blob/filesystem URL requests aren't made from the guest process.

This is the plan for fixing this:
1.  https://codereview.chromium.org/2435593007/: Reintroduce these IO
    thread checks on M56.
2.  <This CL> Tighten the checks for the scenario above using
    ChildProcessSecurityPolicy.
3.  Merge the CL from step 2 back to M55 and M54.
4.  Remove these checks from M56 and tighten the checks in
    ExtensionNavigationThrottle.

The goal of this CL is to determine if a request is being made by a
guest process on the IO thread.  It relies on the fact that
ChildProcessSecurityPolicyImpl already has the origin
"chrome-extension://<appid>" in the origin_set_ for the guest process,
thanks to the GrantOrigin that was added to
WebViewGuest::CreateWebContents in r422976 as part of fixing issue
652784.  Therefore, it's sufficient to check whether the requested
nested URL's origin is in the origin_set_ for the process making the
request.

BUG= 656752 

Review-Url: https://chromiumcodereview.appspot.com/2437753003
Cr-Commit-Position: refs/heads/master@{#426870}
(cherry picked from commit f7af135c51a99daba0cdb5ef204465ff995802e4)

Review URL: https://codereview.chromium.org/2450503002 .

Cr-Commit-Position: refs/branch-heads/2883@{#245}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/3299d4da316b8dd3cbf72e49e5b3de4ea806e87f/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/3299d4da316b8dd3cbf72e49e5b3de4ea806e87f/chrome/browser/net/chrome_extensions_network_delegate.cc
[modify] https://crrev.com/3299d4da316b8dd3cbf72e49e5b3de4ea806e87f/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/3299d4da316b8dd3cbf72e49e5b3de4ea806e87f/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/3299d4da316b8dd3cbf72e49e5b3de4ea806e87f/content/public/browser/child_process_security_policy.h

The M55 merge of r426870 has been released in 55.0.2883.28 on 10/26.  Looking at crash/, so far I see no DumpWithoutCrashing reports that would indicate extra blockage due to the tightened security check.  

Omahaproxy shows that the beta with the merge has shipped to all platforms, except ChromeOS?
Given our previous experience in issue 652077, we probably want this to bake on ChromeOS as well.  I'm also reaching out to make sure that the vendors that were affected by issue 652077 retest their software with 55.0.2883.28.  Once those two things are resolved, I'll request a merge to M54.

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

Following up on #22, the M55 merge of r426870 has also been released for ChromeOS in 55.0.2883.29 on 10/31, and as of now it has baked there for 48+ hrs.  I have also received responses from vendors affected by issue 652077, and they have all verified that their software still works with beta releases that have r426870.

Hence, requesting merge of r426870 to M54.

Note that this is only one part of the fix.  We would also need the revert mentioned in #18 to fully fix this issue for M54.

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

commit 452ce50129657e65181973f1e49e3c963a445e0a
Author: alexmos <alexmos@chromium.org>
Date: Thu Nov 03 17:06:16 2016

Clean up source_site_instance usage in Navigator::RequestOpenURL.

Passing source_site_instance is unnecessary/redundant now that proxy
navigations don't use this, and the only call site is
RenderFrameHostImpl::OpenURL, which always passes in the
current RFH's SiteInstance.

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

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

[modify] https://crrev.com/452ce50129657e65181973f1e49e3c963a445e0a/content/browser/frame_host/navigator.h
[modify] https://crrev.com/452ce50129657e65181973f1e49e3c963a445e0a/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/452ce50129657e65181973f1e49e3c963a445e0a/content/browser/frame_host/navigator_impl.h
[modify] https://crrev.com/452ce50129657e65181973f1e49e3c963a445e0a/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/452ce50129657e65181973f1e49e3c963a445e0a/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/452ce50129657e65181973f1e49e3c963a445e0a/content/browser/security_exploit_browsertest.cc

Is there any merge is needed for M55?

#26: the CL for which I requested the merge to M54 in #24 has already been merged to M55 on Oct 24 (see comment #15).

To summarize, here's what's needed to fix this bug on M54/M55/M56:

Part 1: Tighten IO thread blob/filesystem URL checks for apps with webview permission
Status: Landed on M56 (#8), merged to M55 (#15), requested to merge to M54 (#24).

Part 2: Stop allowing blob/filesystem URLs when navigating a tab that's already hosted in an extension process via a proxy.
Status: Fix just landed on M56 (#23). I'll request to merge to M55 once it bakes.  Fix is too complicated for M54, so the only way to stop this in M54 is via the revert discussed in #18.  

I've been discussing the revert from #18 with awhalley@ and bustamante@, but afaik there hasn't been a decision made yet on whether we should go ahead with the revert for M54.

Thank you for the update alexmos@. Please request a merge to M55 once Cl listed at #23 is well baked in canary and safe to merge.

Ccing awhalley@, bustamante@ & ligimole@

Richard, are we planning take the changes in M54?

As per #24 https://chromium.googlesource.com/chromium/src/+/f7af135c51a99daba0cdb5ef204465ff995802e4 is already verified.

And #18 
https://chromium.googlesource.com/chromium/src.git/+/50cdccb5bfd420f73e93481184f80446edf79730 is a revert

I talked to Andrew (who's on a flight now) and we'll likely want to take this - the changes aren't huge and we're doing a refresh anyways.  I'd like to wait until he has a change to review it more before approving it though.

[Automated comment] Request affecting a post-stable build (M54), manual review required.

Let's take it, approved for merge into M54

We are planning to cut Stable RC at 4.00 PM PST today ( Monday - 11/07), Please merge you change ASAP.

alexmos@, how is change landed at #23 looking in Canary?

#32 and #33: there are ongoing discussions regarding whether or not to go ahead with the merge.  Main issues are: (1) two M55 DumpWithoutCrashing reports of an extension that could be affected by this, and (2) whether #15 is worth merging without the revert from #18.

#34: That change has triggered several DumpWithoutCrashing reports on canary, which I need to investigate before requesting a merge to M55.  Issue 662602 has been filed for those.

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

Quick update here: after some internal discussions, we decided to not merge anything for M54 after all, and concentrate on fixing this for M55.

To complete the fix of this issue on M55, I'm now requesting to merge the CL in #23 (r429532) to M55.  This is part 2 of the fix described in #27.  This has been baking on canary since Nov. 3.  While baking, we discovered issue 662602, where we had to relax ShouldAllowOpenURL to always allow chrome schemes to open extensions pages.  That fix (r431074) has also landed and baked on canary, and I will merge it together with r429532.

+awhalley@ for M55 merge review and approval.

Your change meets the bar and is auto-approved for M55 (branch: 2883)

Re #39: Merge approved for M55

There is no bugdroid comment yet, but the merge for #39/#42 landed this morning: https://codereview.chromium.org/2506503003/

Yes, M55 merge is in - https://chromium.googlesource.com/chromium/src.git/+/5a5e4bf10d71da4ec995cb6caf92b6f770dc5d49

Removing "Merge-Approved-55" label.

No need to re-open bug when requesting merge.

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

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