New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 644966 link

Starred by 1 user
Status: Fixed
Owner:
█ nick@chromium.org
not working at Google anymore
Closed: Oct 2016
Cc:
jsb...@chromium.org
creis@chromium.org
alex...@chromium.org
dmu...@chromium.org
█ michaeln@chromium.org
█ nasko@chromium.org
wfh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug

Blocking:
issue 644426



Sign in to add a comment

Process transfers with --isolate-extensions need to handle blob: and filesystem: URLs

Project Member Reported by alex...@chromium.org, Sep 8 2016 
Repro steps: with --isolate-extensions and --disable-web-security:
(1) Go to http://csreis.github.io/tests/cross-site-iframe-simple.html
(2) Execute document.querySelector("iframe").src = "blob:chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/"

Normally, the load in (2) is disallowed by the renderer (by SecurityOrigin::canDisplay()), but that can be bypassed in a compromised renderer, as simulated by --disable-web-security here.

Assuming the load proceeds, it seems we should be kicking it into a separate subframe process with --isolate-extensions enabled, but this is not happening.  When checking if a transfer is needed, ChromeContentBrowserClientExtensionsPart::DoesSiteRequireDedicatedProcess receives an effective_site_url of blob:chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/, and decides against a transfer since its scheme is "blob" and not "chrome-extension".  A similar problem likely exists for filesystem URLs.  Instead, this should probably look at the inner URL, if one is available.  I noticed how url::Origin handles these two cases in its constructor: 

  if (url.SchemeIsFileSystem()) {
    tuple_ = SchemeHostPort(*url.inner_url());
  } else if (url.SchemeIsBlob()) {
    // If we're dealing with a 'blob:' URL, https://url.spec.whatwg.org/#origin
    // defines the origin as the origin of the URL which results from parsing
    // the "path", which boils down to everything after the scheme. GURL's
    // 'GetContent()' gives us exactly that.
    tuple_ = SchemeHostPort(GURL(url.GetContent()));
  } else {
    tuple_ = SchemeHostPort(url);
  }

so one way to fix it could be to check the scheme of a url::Origin created from the effective_site_url (or extract this logic somewhere that can be easily used here).  Or perhaps SiteInstanceImpl::GetEffectiveURL should be doing this, to catch more places with this potential confusion.

Note how all this isn't a problem for --site-per-process, as it always returns true from SiteInstanceImpl::DoesSiteRequireDedicatedProcess().

Nick, do want to take a look, since you've worked on this and blob URLs not too long ago?

Comment 1 by nick@chromium.org, Sep 8 2016 

The blob URL in the original description is invalid and ought to error (possibly via 404). The effective_site_url of a blob or filesystem scheme should be the embedded origin, so that, at least, is one bug.

There may be stuff going on at other levels too. I just sketched up the following browsertest that does A(B(C)), and then has B navigates C to a valid blob URL in A's origin. What's weird is that it FAILS when run without --site-per-process; it PASSES with --site-per-process and (most surprisingly) also PASSES in --disable-web-security. 

IN_PROC_BROWSER_TEST_F(FrameTreeBrowserTest, NavigateChildToCrossOriginBlob) {
  WebContents* contents = shell()->web_contents();
  FrameTreeNode* root =
      static_cast<WebContentsImpl*>(contents)->GetFrameTree()->root();

  // First, snapshot the FrameTree for a normal A(B(A)) case where all frames
  // are served over http. The blob test should result in the same structure.
  EXPECT_TRUE(NavigateToURL(
      shell(), embedded_test_server()->GetURL(
                   "a.com", "/cross_site_iframe_factory.html?a(b(a))")));
  std::string reference_tree = FrameTreeVisualizer().DepictFrameTree(root);

  GURL main_url(embedded_test_server()->GetURL(
      "a.com", "/cross_site_iframe_factory.html?a(b(c))"));
  EXPECT_TRUE(NavigateToURL(shell(), main_url));

  // The middle node will initiate the navigation on the grandchild when it gets
  // a postmessage from the root; its grandchild node will be the target of the
  // navigation.
  FrameTreeNode* initiator = root->child_at(0);
  FrameTreeNode* target = root->child_at(0)->child_at(0);

  // Install a message handler in the middle node that navigates the child.
  EXPECT_TRUE(ExecuteScript(
    initiator,
    "function receiveMessage(event) {"
    "  if (event.source == window.parent) {"
    "    frames[0].location.assign(event.data);"
    "  }"
    "}"
    "window.addEventListener('message', receiveMessage, false);"));

  // Create a blob in the root, post its URL to the middle node, invoking the
  // receiveMessage above, which navigates the grandchild from the middle node.
  // Wait until the root node receives a message from the loaded blob page.
  std::string blob_url_string;
  EXPECT_TRUE(ExecuteScriptAndExtractString(
      root,
      "function receiveMessage(event) {"
      "  document.body.appendChild(document.createTextNode(event.data));"
      "  domAutomationController.send(event.source.location.href);"
      "}"
      "window.addEventListener('message', receiveMessage, false);"
      "var blob = new Blob(["
      "    '<html><body><div>This is blob content.</div><script>"
      "         window.parent.parent.postMessage(\"HI\", document.origin);"
      "     </script></body></html>'], {type: 'text/html'});"
      "var blob_url = URL.createObjectURL(blob);"
      "frames[0].postMessage(blob_url, '*');",
      &blob_url_string));
  // TODO(nick): What if we use an invalid blob that 404's?
  EXPECT_EQ(GURL(blob_url_string), target->current_url());
  EXPECT_EQ(url::kBlobScheme, target->current_url().scheme());
  EXPECT_FALSE(target->current_origin().unique());
  EXPECT_EQ("a.com", target->current_origin().host());
  EXPECT_EQ(url::kHttpScheme, target->current_origin().scheme());

  std::string document_body;
  EXPECT_TRUE(ExecuteScriptAndExtractString(
      target,
      "domAutomationController.send(document.body.children[0].innerHTML);",
      &document_body));
  EXPECT_EQ("This is blob content.", document_body);

  // Make sure we did a process transfer back to A.
  EXPECT_EQ(reference_tree, FrameTreeVisualizer().DepictFrameTree(root));
}

Comment 2 by nick@chromium.org, Sep 8 2016 

What's becoming clear is that there is an apparent policy, implemented in blink, that is supposed to prevent navigation to blob URLs unless they are initiated by the blob's origin.

Somehow, --site-per-process short-circuits this check; our fix may involve enforcing this on the browser side somehow.

Comment 3 by creis@chromium.org, Sep 8 2016

Cc: jsb...@chromium.org dmu...@chromium.org
Note that we appear to not transfer processes for valid blob URLs either (in 55.0.2853.0, where --isolate-extensions is on by default).  I tested it by creating a blob URL within chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/index.html (in DevTools) and then loading that into an iframe of a web page.  No subframe process was created.

As for invalid blob URLs, I'm wondering if it might be sufficient (or preferable) to show a real error page (which uses "data:text/html,chromewebdata" under the hood).  That means we don't end up with a document with the destination origin in the process, as we do today for invalid blob URLs.  (I suppose that doesn't matter if the attacker has full code execution in the renderer process, though.)

Another interesting note is that invalid filesystem URLs don't behave the same way.  There's 3 different failure modes:
1) Invalid extension URLs show a real error page (data URL under the hood).
2) Invalid blob URLs have an empty document with the destination origin in document.origin and destination URL in location.href.  (Easily exploitable.)
3) Invalid filesystem URLs have about:blank as their location.href and the parent page's origin in document.origin.


Anyway, we should definitely fix the process transfer, but we may want something more consistent for the error pages as well.

Comment 4 by alex...@chromium.org, Sep 9 2016 

I found another problematic aspect of this, which is we attempt to do transfers on blob URLs when a transfer shouldn't happen.

Repro steps on canary with --isolate-extensions on by default:
1) Navigate tab to chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/index.html
2) From Devtools:
var blob = new Blob(['<html><body>foo</body></html>'], {type: 'text/html'});
var u=URL.createObjectURL(blob)
window.open(u)

This creates a new tab and then just hangs with the spinner active.

In this case, the current RFH and the destination URL have the same origin, but IsRendererTransferForNavigaution concludes that a transfer is needed, due to 
IsCurrentlySameSite(rfh, dest_url) returning false.  This later leads us to take a rare path in NavigateToEntry where we think that a transfer ended up in the same SiteInstance it started in [1].  (I'm not sure whether it's ok that things hang after that point.)

If I replace dest_url with effective_url, and also put in a fix that Nick is experimenting with, where effective_url is computed using GetSiteForURL instead of GetEffectiveURL, things start working as expected.

[1] https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_impl.cc?l=405
      // No need to navigate again.  Just resume the deferred request.
      dest_render_frame_host->GetProcess()->ResumeDeferredNavigation(
          entry.transferred_global_request_id());

Comment 5 by nick@chromium.org, Sep 9 2016 

I wasn't able to repro a failure in the scenario in #4 using the --isolate-sites-for-testing=*.is trick; in that analogous case, IsCurrentlySameSite(frame, "blob:") returns false even though it's same-origin, but thinks work out okay.

I'm suspecting that extensions may require a browsing instance swap, i.e., maybe ShouldSwapBrowsingInstancesForNavigation is troublesome here too.

It's starting to look to me like we may be suffering from blob-origin confusion in many other of the contentbrowserclient calls: ShouldUseProcessPerSite, ShouldSwapBrowsingInstancesForNavigation, and maybe more.

Definitely, fixing IsCurrentlySameSite seems proper. But I suspect there may be variant of this where we still do the wrong thing and wind up cross process from our extension, maybe triggerable via back/forward nav.

I'll switch to working on a chrome repro + test so that we can iron out all the kinks. In TDI, we lean pretty hard on the aforementioned 'rare path in NavigateToEntry'.
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 9 2016 

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

commit db193a1b105de523fd0bb089c9769a71ed287d9e
Author: nick <nick@chromium.org>
Date: Fri Sep 09 23:09:23 2016

Fix process transfers for blob urls of sites requiring dedicated processes.

RenderFrameHostManager::IsRendererTransferNeededForNavigation had a bug
where it passed an effective url, instead of an effective SITE url, to
a function that was expecting the latter.

Add a test that exercises this case. Add a CHECK to content shell browser
client to verify that we're actually getting site urls all the time.

BUG= 644966 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/db193a1b105de523fd0bb089c9769a71ed287d9e/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/db193a1b105de523fd0bb089c9769a71ed287d9e/content/browser/frame_host/frame_tree_browsertest.cc
[modify] https://crrev.com/db193a1b105de523fd0bb089c9769a71ed287d9e/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/db193a1b105de523fd0bb089c9769a71ed287d9e/content/browser/site_instance_impl.cc
[modify] https://crrev.com/db193a1b105de523fd0bb089c9769a71ed287d9e/content/browser/site_instance_impl.h
[modify] https://crrev.com/db193a1b105de523fd0bb089c9769a71ed287d9e/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/db193a1b105de523fd0bb089c9769a71ed287d9e/content/public/browser/content_browser_client.h
[modify] https://crrev.com/db193a1b105de523fd0bb089c9769a71ed287d9e/content/shell/browser/shell_content_browser_client.cc
[modify] https://crrev.com/db193a1b105de523fd0bb089c9769a71ed287d9e/content/shell/browser/shell_content_browser_client.h
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 12 2016 

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

commit 07fd7e19e0095aeb30bd2c99109d083bb67732cb
Author: nick <nick@chromium.org>
Date: Mon Sep 12 18:54:06 2016

Fix IsolateIcelandFrameTreeBrowserTest.ProcessSwitchForIsolatedBlob so that it's not flaky under --site-per-process.

BUG= 644966 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/07fd7e19e0095aeb30bd2c99109d083bb67732cb/content/browser/frame_host/frame_tree_browsertest.cc

Comment 8 by creis@chromium.org, Sep 20 2016

Status: Started (was: Assigned)
Note on status: The original repro steps for this bug are fixed by r417752, such that we transfer processes for invalid blob URLs now.  (Nick, do we still need to fix filesystem URLs?)

We're leaving this open since Nick is also trying to prevent non-extension processes from creating blobs (and filesystem URLs) for extension origins, using arbitrary IPCs.  The fix in r417752 just makes it hard to create them from Javascript, since the iframe gets transferred to a different process.  It does not defend against a compromised renderer.

Comment 9 by dmu...@chromium.org, Sep 20 2016 

Question: Did that cause this issue here:
https://bugs.chromium.org/p/chromium/issues/detail?id=647839

Or is that possibly a similar security concern?

Comment 10 by creis@chromium.org, Sep 20 2016 

Comment 9: I think Nick discovered that while working on this one, so it's unlikely this fix caused that.  However, I'm having trouble reproing issue 647839, so I'll follow up with Nick.

Comment 11 by nick@chromium.org, Sep 28 2016 

Fix is almost ready at https://codereview.chromium.org/2364633004/
Project Member

Comment 12 by bugdroid1@chromium.org, Sep 29 2016 

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

commit a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e
Author: nick <nick@chromium.org>
Date: Thu Sep 29 22:50:40 2016

Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme
is considered "web safe" to be requestable from any process, but only
"web safe" to commit in extension processes.

In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when
seeing blob and filesystem urls, make a security decision based
on the inner origin rather than the scheme.

When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a
RenderFrame being created in an extension SiteInstance, grant that process
permission to commit chrome-extension:// URLs.

In BlobDispatcherHost, only allow creation of blob URLs from processes that
would be able to commit them.

Add a security exploit browsertest that verifies the above mechanisms working
together.

BUG= 644966 

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

[modify] https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e/chrome/browser/DEPS
[modify] https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e/chrome/browser/chrome_security_exploit_browsertest.cc
[modify] https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e/chrome/browser/devtools/devtools_sanity_browsertest.cc
[modify] https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e/chrome/browser/devtools/devtools_ui_bindings.cc
[modify] https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e/content/browser/bad_message.h
[modify] https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e/content/browser/blob_storage/blob_dispatcher_host.cc
[modify] https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e/content/public/browser/child_process_security_policy.h
[modify] https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e/extensions/browser/extension_web_contents_observer.cc
Project Member

Comment 13 by bugdroid1@chromium.org, Sep 30 2016 

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

commit bd3b7a4c91c8dc7a28e197c3a88a2885121c3042
Author: tsergeant <tsergeant@chromium.org>
Date: Fri Sep 30 00:42:19 2016

Revert of Lock down the registration of blob:chrome-extension:// URLs (patchset #13 id:230001 of https://codereview.chromium.org/2364633004/ )

Reason for revert:
Speculative revert to fix failing test http/tests/xmlhttprequest/xhr-to-blob-in-isolated-world.html in webkit_tests.

See failures in:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/46369
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/builds/24388

Original issue's description:
> Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme
> is considered "web safe" to be requestable from any process, but only
> "web safe" to commit in extension processes.
>
> In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when
> seeing blob and filesystem urls, make a security decision based
> on the inner origin rather than the scheme.
>
> When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a
> RenderFrame being created in an extension SiteInstance, grant that process
> permission to commit chrome-extension:// URLs.
>
> In BlobDispatcherHost, only allow creation of blob URLs from processes that
> would be able to commit them.
>
> Add a security exploit browsertest that verifies the above mechanisms working
> together.
>
> BUG= 644966 
>
> Committed: https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e
> Cr-Commit-Position: refs/heads/master@{#421964}

TBR=reillyg@chromium.org,creis@chromium.org,thestig@chromium.org,rdevlin.cronin@chromium.org,nick@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 644966 

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

[modify] https://crrev.com/bd3b7a4c91c8dc7a28e197c3a88a2885121c3042/chrome/browser/DEPS
[modify] https://crrev.com/bd3b7a4c91c8dc7a28e197c3a88a2885121c3042/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/bd3b7a4c91c8dc7a28e197c3a88a2885121c3042/chrome/browser/chrome_security_exploit_browsertest.cc
[modify] https://crrev.com/bd3b7a4c91c8dc7a28e197c3a88a2885121c3042/chrome/browser/devtools/devtools_sanity_browsertest.cc
[modify] https://crrev.com/bd3b7a4c91c8dc7a28e197c3a88a2885121c3042/chrome/browser/devtools/devtools_ui_bindings.cc
[modify] https://crrev.com/bd3b7a4c91c8dc7a28e197c3a88a2885121c3042/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/bd3b7a4c91c8dc7a28e197c3a88a2885121c3042/content/browser/bad_message.h
[modify] https://crrev.com/bd3b7a4c91c8dc7a28e197c3a88a2885121c3042/content/browser/blob_storage/blob_dispatcher_host.cc
[modify] https://crrev.com/bd3b7a4c91c8dc7a28e197c3a88a2885121c3042/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/bd3b7a4c91c8dc7a28e197c3a88a2885121c3042/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/bd3b7a4c91c8dc7a28e197c3a88a2885121c3042/content/public/browser/child_process_security_policy.h
[modify] https://crrev.com/bd3b7a4c91c8dc7a28e197c3a88a2885121c3042/extensions/browser/extension_web_contents_observer.cc
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 3 2016 

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

commit 2a8ba8c4c186e5ea0a2ed938cc5d41441af64228
Author: nick <nick@chromium.org>
Date: Mon Oct 03 18:51:39 2016

Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme
is considered "web safe" to be requestable from any process, but only
"web safe" to commit in extension processes.

In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when
seeing blob and filesystem urls, make a security decision based
on the inner origin rather than the scheme.

When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a
RenderFrame being created in an extension SiteInstance, grant that process
permission to commit chrome-extension:// URLs.

In BlobDispatcherHost, only allow creation of blob URLs from processes that
would be able to commit them.

Add a security exploit browsertest that verifies the above mechanisms working
together.

BUG= 644966 

Committed: https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e
Review-Url: https://codereview.chromium.org/2364633004
Cr-Original-Commit-Position: refs/heads/master@{#421964}
Cr-Commit-Position: refs/heads/master@{#422474}

[modify] https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228/chrome/browser/DEPS
[modify] https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228/chrome/browser/chrome_security_exploit_browsertest.cc
[modify] https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228/chrome/browser/devtools/devtools_sanity_browsertest.cc
[modify] https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228/chrome/browser/devtools/devtools_ui_bindings.cc
[modify] https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228/content/browser/bad_message.h
[modify] https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228/content/browser/blob_storage/blob_dispatcher_host.cc
[modify] https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228/content/public/browser/child_process_security_policy.h
[modify] https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228/extensions/browser/extension_web_contents_observer.cc
[modify] https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xhr-to-blob-in-isolated-world.html

Comment 15 by creis@chromium.org, Oct 3 2016

Status: Fixed (was: Started)
I'll mark this fixed now that the blob creation defense has landed.  We may have discovered a case that it's too conservative for V2 apps that load their own URLs inside <webview> tags, but we'll follow up in a separate bug for that.
Project Member

Comment 16 by sheriffbot@chromium.org, Oct 4 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 17 by creis@chromium.org, Oct 4 2016 

Following up on comment 15, we did indeed find a case where the check fails with <webview> tags.  See issue 652784 for details.

Once that's fixed, we can evaluate whether to merge both to M54, since this is a blockin security fix for issue 644426.
Project Member

Comment 18 by bugdroid1@chromium.org, Oct 4 2016 

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

commit 5edda59b0b1cb8fff058b47567ac32e58be5168a
Author: nasko <nasko@chromium.org>
Date: Tue Oct 04 22:44:50 2016

Allow <webview> to access URLs in the origin of the app embedding it.

With r422474 creation of blob: URLs with origin of a chrome-extension://
was locked down. However, the case of a <webview> loading an
accessible_resource from its embedder and creating a blob: is disallowed.
This CL adds permission for <webview> to create such URLs in the origin
of its embedder.

This CL is based on work by nick@chromium.org.

BUG=652784,  644966 

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

[modify] https://crrev.com/5edda59b0b1cb8fff058b47567ac32e58be5168a/chrome/test/data/extensions/platform_apps/web_view/guest_focus_test/guest.js
[modify] https://crrev.com/5edda59b0b1cb8fff058b47567ac32e58be5168a/extensions/browser/guest_view/web_view/web_view_guest.cc

Comment 19 by nasko@chromium.org, Oct 5 2016 

Now that the kill in <webview> cases is fixed, shall we merge these two CLs in M54?

Comment 20 by nick@chromium.org, Oct 5 2016

Status: Started (was: Fixed)
This isn't fixed yet; we need 2387173005 too.

The merge will need to be a manual one; there's a conflict with jww's suborigins CL.
Project Member

Comment 21 by bugdroid1@chromium.org, Oct 6 2016 

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

commit e2e627390daf45140560b17c3e5f20fe73544a4f
Author: nick <nick@chromium.org>
Date: Thu Oct 06 00:08:22 2016

Disallow file api access from processes that lack permissions for the scheme of the origin.

BUG= 644966 

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

[modify] https://crrev.com/e2e627390daf45140560b17c3e5f20fe73544a4f/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/e2e627390daf45140560b17c3e5f20fe73544a4f/tools/metrics/histograms/histograms.xml

Comment 22 by nick@chromium.org, Oct 6 2016

Labels: Merge-Request-54
Now, it's fixed.

Merge plan is two merges.

Today, I'll merge the following three diffs:
 - Partial merge of jww's r420848 (pull in the ChildProcessSecurityPolicyImpl::CanSetAsOriginHeader entry point, but not the suborigin behavior).
 - Merge of r422474 (blob:chrome-extension lockdown)
 - Merge of r422976 (webview fix)

On Monday, assuming canary looks good as measured by no samples for the FileSystem.OriginFailedCanCommitURL metric here: https://uma.googleplex.com/p/chrome/timeline_v2/?sid=3734aa69bf5be2ba595fa770a347953f, we will also do:
 - Merge of r423348 (filesystem:chrome-extension lockdown)

Comment 23 by dimu@chromium.org, Oct 6 2016

Labels: -Merge-Request-54 Merge-Review-54 Hotlist-Merge-Review
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.

Comment 24 by bustamante@google.com, Oct 6 2016

Labels: -Merge-Review-54 Merge-Approved-54
Approving for merge into M54, since it looks like a high priority security fix / functional with <webview>.

We're planning on cutting a the first RC for 54 stable on monday 10/10, please be careful about breakages.
Project Member

Comment 25 by bugdroid1@chromium.org, Oct 6 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2

commit 4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2
Author: creis <creis@chromium.org>
Date: Thu Oct 06 23:33:56 2016

Merges six security fixes to M54, related to blobs.

Merge patch created pair programming style with creis@ and nick@.
Several manual fixups were required to get the tests passing on M54.

BUG= 644966 , 646278 ,652784
TEST=Manual testing included:
 - Verifying exploit steps w/ chrome w/ --isolate-extensions
 - content_browsertests and content_unittests
 - The following browser_tests subsets, both w/ and w/o --isolate-extensions:
   *ProcessManager*
   *Grants*
   *Exploit*
   *TouchFocuses*
NOPRESUBMIT=true
NOTRY=true
TBR=nick@chromium.org

The following six fixes are included in this diff:

1. https://codereview.chromium.org/2322673005:
  > Fix process transfers for blob urls of sites requiring dedicated processes
  >
  > RenderFrameHostManager::IsRendererTransferNeededForNavigation had a bug
  > where it passed an effective url, instead of an effective SITE url, to
  > a function that was expecting the latter.
  >
  > Add a test that exercises this case. Add a CHECK to content shell browser
  > client to verify that we're actually getting site urls all the time.
  >
  > Committed: https://crrev.com/db193a1b105de523fd0bb089c9769a71ed287d9e
  > Cr-Commit-Position: refs/heads/master@{#417752}

2. https://codereview.chromium.org/2331063002:
  > Fix IsolateIcelandFrameTreeBrowserTest.ProcessSwitchForIsolatedBlob so
  > that it's not flaky under --site-per-process.
  >
  > Committed: https://crrev.com/07fd7e19e0095aeb30bd2c99109d083bb67732cb
  > Cr-Commit-Position: refs/heads/master@{#417987}

3. https://codereview.chromium.org/2365433002:
  > (re-land) Disallow navigations to blob URLs with non-canonical origins.
  >
  > Re-landing this with a fix for xhr-to-blob-in-isolated-world.html
  >
  > Review-Url: https://codereview.chromium.org/2365433002
  > Cr-Commit-Position: refs/heads/master@{#420436}

4. https://codereview.chromium.org/2332263002
   [partial merge, just for the helper function it added, used by later CLs]
  > Updated suborigin serialization to latest spec proposal
  >
  > This modifiest the serialization format of suborigins so they are now
  > represented in the form https-so://suboriginname.host.name (or,
  > alternatively, with the scheme http-so). This change removes collisions
  > with potentially valid URLs that were being deserialized as suborigins.
  >
  > Additionally, this adds suborigins back as an experimental web platform
  > feature rather than a testing feature.
  >
  > Review-Url: https://codereview.chromium.org/2332263002
  > Cr-Commit-Position: refs/heads/master@{#420828}
  > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

5. https://codereview.chromium.org/2364633004:
  > Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme
  > is considered "web safe" to be requestable from any process, but only
  > "web safe" to commit in extension processes.
  >
  > In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when
  > seeing blob and filesystem urls, make a security decision based
  > on the inner origin rather than the scheme.
  >
  > When the extensions ProcessManager (via ExtensionWebContentsObserver)
  > notices a RenderFrame being created in an extension SiteInstance,
  > grant that process permission to commit chrome-extension:// URLs.
  >
  > In BlobDispatcherHost, only allow creation of blob URLs from processes
  > that would be able to commit them.
  >
  > Add a security exploit browsertest that verifies the above mechanisms
  > working together.
  >
  > Committed: https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e
  > Committed: https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228
  > Cr-Original-Commit-Position: refs/heads/master@{#421964}
  > Cr-Commit-Position: refs/heads/master@{#422474}

6. https://codereview.chromium.org/2396533003:
  > Allow <webview> to access URLs in the origin of the app embedding it.
  >
  > With r422474 creation of blob: URLs with origin of a chrome-extension://
  > was locked down. However, the case of a <webview> loading an
  > accessible_resource from its embedder and creating a blob: is disallowed.
  > This CL adds permission for <webview> to create such URLs in the origin
  > of its embedder.
  >
  > This CL is based on work by nick@chromium.org.
  >
  > Committed: https://crrev.com/5edda59b0b1cb8fff058b47567ac32e58be5168a
  > Cr-Commit-Position: refs/heads/master@{#422976}
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2399853003
Cr-Commit-Position: refs/branch-heads/2840@{#672}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/DEPS
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/chrome_security_exploit_browsertest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/devtools/devtools_sanity_browsertest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/devtools/devtools_ui_bindings.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/test/data/extensions/platform_apps/web_view/guest_focus_test/guest.js
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/bad_message.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/blob_storage/blob_dispatcher_host.cc
[add] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/blob_storage/blob_url_browsertest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/child_process_security_policy_unittest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/frame_host/frame_tree_browsertest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/site_instance_impl.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/site_instance_impl.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/content_tests.gypi
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/public/browser/child_process_security_policy.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/public/browser/content_browser_client.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/shell/browser/shell_content_browser_client.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/shell/browser/shell_content_browser_client.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/extensions/browser/extension_web_contents_observer.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xhr-to-blob-in-isolated-world.html

Comment 26 by nick@chromium.org, Oct 7 2016

Status: Fixed (was: Started)
Project Member

Comment 28 by bugdroid1@chromium.org, Oct 10 2016 

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

commit 7fd5f88a9dbe923676a8b97caf3aad08eed631fd
Author: nick <nick@chromium.org>
Date: Mon Oct 10 18:34:25 2016

Revert "[Merge to M54] Disallow file api access from processes that lack permissions for the scheme of the origin."

This reverts commit c9ac10a66c02609504c34849f8bb17e6f60d3a5f.

Reverting because of bug 654479: data from canary suggests that this fix may
be having unintended behavior.

BUG= 644966 ,654479
TBR=creis@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2405933002
Cr-Commit-Position: refs/branch-heads/2840@{#702}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/7fd5f88a9dbe923676a8b97caf3aad08eed631fd/content/browser/child_process_security_policy_impl.cc

Comment 29 by alex...@chromium.org, Oct 10 2016

Cc: michaeln@chromium.org
Project Member

Comment 30 by bugdroid1@chromium.org, Oct 18 2016 

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

commit 389119fcb0c2906c964023d4585792af8b7c23cd
Author: nick <nick@chromium.org>
Date: Tue Oct 18 20:07:05 2016

[re-land in M54] Disallow file api access from processes that lack permissions for the scheme of the origin.

BUG= 644966 

Review-Url: https://codereview.chromium.org/2387173005
Cr-Commit-Position: refs/heads/master@{#423348}
TBR=creis@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2428723002
Cr-Commit-Position: refs/branch-heads/2840@{#750}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/389119fcb0c2906c964023d4585792af8b7c23cd/content/browser/child_process_security_policy_impl.cc
Project Member

Comment 31 by bugdroid1@chromium.org, Oct 27 2016 

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

commit e2e627390daf45140560b17c3e5f20fe73544a4f
Author: nick <nick@chromium.org>
Date: Thu Oct 06 00:08:22 2016

Disallow file api access from processes that lack permissions for the scheme of the origin.

BUG= 644966 

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

[modify] https://crrev.com/e2e627390daf45140560b17c3e5f20fe73544a4f/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/e2e627390daf45140560b17c3e5f20fe73544a4f/tools/metrics/histograms/histograms.xml
Project Member

Comment 32 by bugdroid1@chromium.org, Oct 27 2016 

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

commit 87d0b2931a2ab2c4a50d220a51e36aea9a351d44
Author: nick <nick@chromium.org>
Date: Thu Oct 06 19:08:06 2016

BlobDispatcherHost: don't rely on NOTREACHED() checks in ParamTraits::Read

Explicitly handle all possible storage::DataElement cases, and add comments
explaining the purpose of existing security checks.

BUG= 644966 
TEST=content_browsertests

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

[modify] https://crrev.com/87d0b2931a2ab2c4a50d220a51e36aea9a351d44/content/browser/blob_storage/blob_dispatcher_host.cc
[modify] https://crrev.com/87d0b2931a2ab2c4a50d220a51e36aea9a351d44/content/common/resource_messages.cc
Project Member

Comment 33 by bugdroid1@chromium.org, Oct 27 2016 

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

commit 4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2
Author: creis <creis@chromium.org>
Date: Thu Oct 06 23:33:56 2016

Merges six security fixes to M54, related to blobs.

Merge patch created pair programming style with creis@ and nick@.
Several manual fixups were required to get the tests passing on M54.

BUG= 644966 , 646278 ,652784
TEST=Manual testing included:
 - Verifying exploit steps w/ chrome w/ --isolate-extensions
 - content_browsertests and content_unittests
 - The following browser_tests subsets, both w/ and w/o --isolate-extensions:
   *ProcessManager*
   *Grants*
   *Exploit*
   *TouchFocuses*
NOPRESUBMIT=true
NOTRY=true
TBR=nick@chromium.org

The following six fixes are included in this diff:

1. https://codereview.chromium.org/2322673005:
  > Fix process transfers for blob urls of sites requiring dedicated processes
  >
  > RenderFrameHostManager::IsRendererTransferNeededForNavigation had a bug
  > where it passed an effective url, instead of an effective SITE url, to
  > a function that was expecting the latter.
  >
  > Add a test that exercises this case. Add a CHECK to content shell browser
  > client to verify that we're actually getting site urls all the time.
  >
  > Committed: https://crrev.com/db193a1b105de523fd0bb089c9769a71ed287d9e
  > Cr-Commit-Position: refs/heads/master@{#417752}

2. https://codereview.chromium.org/2331063002:
  > Fix IsolateIcelandFrameTreeBrowserTest.ProcessSwitchForIsolatedBlob so
  > that it's not flaky under --site-per-process.
  >
  > Committed: https://crrev.com/07fd7e19e0095aeb30bd2c99109d083bb67732cb
  > Cr-Commit-Position: refs/heads/master@{#417987}

3. https://codereview.chromium.org/2365433002:
  > (re-land) Disallow navigations to blob URLs with non-canonical origins.
  >
  > Re-landing this with a fix for xhr-to-blob-in-isolated-world.html
  >
  > Review-Url: https://codereview.chromium.org/2365433002
  > Cr-Commit-Position: refs/heads/master@{#420436}

4. https://codereview.chromium.org/2332263002
   [partial merge, just for the helper function it added, used by later CLs]
  > Updated suborigin serialization to latest spec proposal
  >
  > This modifiest the serialization format of suborigins so they are now
  > represented in the form https-so://suboriginname.host.name (or,
  > alternatively, with the scheme http-so). This change removes collisions
  > with potentially valid URLs that were being deserialized as suborigins.
  >
  > Additionally, this adds suborigins back as an experimental web platform
  > feature rather than a testing feature.
  >
  > Review-Url: https://codereview.chromium.org/2332263002
  > Cr-Commit-Position: refs/heads/master@{#420828}
  > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

5. https://codereview.chromium.org/2364633004:
  > Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme
  > is considered "web safe" to be requestable from any process, but only
  > "web safe" to commit in extension processes.
  >
  > In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when
  > seeing blob and filesystem urls, make a security decision based
  > on the inner origin rather than the scheme.
  >
  > When the extensions ProcessManager (via ExtensionWebContentsObserver)
  > notices a RenderFrame being created in an extension SiteInstance,
  > grant that process permission to commit chrome-extension:// URLs.
  >
  > In BlobDispatcherHost, only allow creation of blob URLs from processes
  > that would be able to commit them.
  >
  > Add a security exploit browsertest that verifies the above mechanisms
  > working together.
  >
  > Committed: https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e
  > Committed: https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228
  > Cr-Original-Commit-Position: refs/heads/master@{#421964}
  > Cr-Commit-Position: refs/heads/master@{#422474}

6. https://codereview.chromium.org/2396533003:
  > Allow <webview> to access URLs in the origin of the app embedding it.
  >
  > With r422474 creation of blob: URLs with origin of a chrome-extension://
  > was locked down. However, the case of a <webview> loading an
  > accessible_resource from its embedder and creating a blob: is disallowed.
  > This CL adds permission for <webview> to create such URLs in the origin
  > of its embedder.
  >
  > This CL is based on work by nick@chromium.org.
  >
  > Committed: https://crrev.com/5edda59b0b1cb8fff058b47567ac32e58be5168a
  > Cr-Commit-Position: refs/heads/master@{#422976}
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2399853003
Cr-Commit-Position: refs/branch-heads/2840@{#672}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/DEPS
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/chrome_security_exploit_browsertest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/devtools/devtools_sanity_browsertest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/devtools/devtools_ui_bindings.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/chrome/test/data/extensions/platform_apps/web_view/guest_focus_test/guest.js
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/bad_message.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/blob_storage/blob_dispatcher_host.cc
[add] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/blob_storage/blob_url_browsertest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/child_process_security_policy_unittest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/frame_host/frame_tree_browsertest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/site_instance_impl.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/browser/site_instance_impl.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/content_tests.gypi
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/public/browser/child_process_security_policy.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/public/browser/content_browser_client.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/shell/browser/shell_content_browser_client.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/content/shell/browser/shell_content_browser_client.h
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/extensions/browser/extension_web_contents_observer.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/4cf1e7bf7a79dcd98967ffe8db93490614a8d4b2/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xhr-to-blob-in-isolated-world.html
Project Member

Comment 34 by bugdroid1@chromium.org, Oct 27 2016 

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

commit c9ac10a66c02609504c34849f8bb17e6f60d3a5f
Author: nick <nick@chromium.org>
Date: Fri Oct 07 19:17:31 2016

[Merge to M54] Disallow file api access from processes that lack permissions for the scheme of the origin.

BUG= 644966 
TBR=creis@chromium.org
NOTRY=true
NOPRESUBMIT=true

Original commit info:
 > Review-Url: https://codereview.chromium.org/2387173005
 > Cr-Commit-Position: refs/heads/master@{#423348}

Review-Url: https://codereview.chromium.org/2399593007
Cr-Commit-Position: refs/branch-heads/2840@{#683}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/c9ac10a66c02609504c34849f8bb17e6f60d3a5f/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/c9ac10a66c02609504c34849f8bb17e6f60d3a5f/tools/metrics/histograms/histograms.xml
Project Member

Comment 35 by bugdroid1@chromium.org, Oct 27 2016 

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

commit 7fd5f88a9dbe923676a8b97caf3aad08eed631fd
Author: nick <nick@chromium.org>
Date: Mon Oct 10 18:34:25 2016

Revert "[Merge to M54] Disallow file api access from processes that lack permissions for the scheme of the origin."

This reverts commit c9ac10a66c02609504c34849f8bb17e6f60d3a5f.

Reverting because of bug 654479: data from canary suggests that this fix may
be having unintended behavior.

BUG= 644966 ,654479
TBR=creis@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2405933002
Cr-Commit-Position: refs/branch-heads/2840@{#702}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/7fd5f88a9dbe923676a8b97caf3aad08eed631fd/content/browser/child_process_security_policy_impl.cc
Project Member

Comment 36 by bugdroid1@chromium.org, Oct 27 2016 

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

commit 389119fcb0c2906c964023d4585792af8b7c23cd
Author: nick <nick@chromium.org>
Date: Tue Oct 18 20:07:05 2016

[re-land in M54] Disallow file api access from processes that lack permissions for the scheme of the origin.

BUG= 644966 

Review-Url: https://codereview.chromium.org/2387173005
Cr-Commit-Position: refs/heads/master@{#423348}
TBR=creis@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2428723002
Cr-Commit-Position: refs/branch-heads/2840@{#750}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/389119fcb0c2906c964023d4585792af8b7c23cd/content/browser/child_process_security_policy_impl.cc

Comment 37 by gov...@chromium.org, Oct 31 2016 

Any of these changes need merge to M55? If yes, please request a merge to M55 ASAP. Thank you.

Comment 38 by gov...@chromium.org, Oct 31 2016 

FYI: M55 was branched on Oct 6th, Branched Chromium at revision: 423768.

Comment 39 by creis@chromium.org, Oct 31 2016 

govind: It looks like the last actual fix here (comment 32, r423621) landed just before the M55 branch cut, so I don't think any merges are needed.  The rest of the work was about M54 merges.  Thanks for checking!
Project Member

Comment 40 by bugdroid1@chromium.org, Dec 2 2016 

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

commit bfaea4ee6b138d958efc97e16da9624dc28545e6
Author: nick <nick@chromium.org>
Date: Fri Dec 02 20:59:31 2016

browser_test that creates filesystem resources in extension origin

Implement a security exploit test for  bug 644966 , and improve
DEPS layering by using browser_test_util trampoline functions
instead of content messages from chrome directly.

Synchronizing the CreateFile operations requires installing a
temporary FileUpdateObserver in the FileSystemContext. It turns
out that this is quite hard to do, so it's encapsulated in
a utility class.

BUG= 644966 

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

[modify] https://crrev.com/bfaea4ee6b138d958efc97e16da9624dc28545e6/chrome/browser/DEPS
[modify] https://crrev.com/bfaea4ee6b138d958efc97e16da9624dc28545e6/chrome/browser/chrome_security_exploit_browsertest.cc
[modify] https://crrev.com/bfaea4ee6b138d958efc97e16da9624dc28545e6/content/browser/fileapi/mock_file_update_observer.h
[modify] https://crrev.com/bfaea4ee6b138d958efc97e16da9624dc28545e6/content/public/test/browser_test_utils.cc
[modify] https://crrev.com/bfaea4ee6b138d958efc97e16da9624dc28545e6/content/public/test/browser_test_utils.h
[add] https://crrev.com/bfaea4ee6b138d958efc97e16da9624dc28545e6/content/public/test/test_fileapi_operation_waiter.cc
[add] https://crrev.com/bfaea4ee6b138d958efc97e16da9624dc28545e6/content/public/test/test_fileapi_operation_waiter.h
[modify] https://crrev.com/bfaea4ee6b138d958efc97e16da9624dc28545e6/content/test/BUILD.gn
Project Member

Comment 41 by sheriffbot@chromium.org, Jan 14 2017

Labels: -Restrict-View-SecurityNotify allpublic
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
►

Sign in to add a comment