Please CC masatokinugawa@gmail.com in this report, who's the original finder of this bug!

Thanks for the report!  I see that BlobDispatcherHost::OnRegisterPublicBlobURL tries to verify the IPC payload received from the renderer, but it does so using CanCommitURL which AFAIK does't consider the origin lock that Site Isolation might have put on the renderer:

void BlobDispatcherHost::OnRegisterPublicBlobURL(const GURL& public_url,
                                                 const std::string& uuid) {
  DCHECK_CURRENTLY_ON(BrowserThread::IO);
  ChildProcessSecurityPolicyImpl* security_policy =
      ChildProcessSecurityPolicyImpl::GetInstance();

  // Blob urls have embedded origins. A frame should only be creating blob URLs
  // in the origin of its current document. Make sure that the origin advertised
  // on the URL is allowed to be rendered in this process.
  if (!public_url.SchemeIsBlob() ||
      !security_policy->CanCommitURL(process_id_, public_url)) {
    DVLOG(1) << "BlobDispatcherHost::OnRegisterPublicBlobURL("
             << public_url.spec() << ", " << uuid
             << "): Invalid or prohibited URL.";
    bad_message::ReceivedBadMessage(this, bad_message::BDH_DISALLOWED_ORIGIN);
    return;
  }

Yes, thanks for the report!  Some of the enforcements for compromised renderers are still works in progress, but this is a nice finding.  We chatted about this and realized the blob code is probably using the wrong check-- it should most likely be using CanAccessDataForOrigin instead of CanCommitURL (which seems to be a more suitable check, and would have avoided this bug even before we tighten CanCommitURL).

We'll sanity check that to make sure we're not regressing anything from Nick's r422474, but I suspect that will be the right way to apply origin lock checks.

Alex or Lukasz, would one of you want to take a closer look to confirm?

Looking closer, I don't think we can just change CanCommitURL to CanAccessDataForOrigin, as CanAccessDataForOrigin only works if the process creating the blob URL is origin-locked.  This won't be the case in some cases, including extensions and WebUI.  The latter is covered  ChromeSecurityExploitBrowserTest.CreateBlobInOtherChromeUIOrigin, which I confirmed starts to fail if we remove CanCommitURL.  So I think we do want to still check the scheme grant performed by CanCommitURL, and additionally check origin locks with CanAccessDataForOrigin.  (Eventually, CanCommitURL should just handle everything.)  I'll also add the enforcements bug here for tracking.

I put together a content_browsertest which seems to repro this, along with this potential fix, at https://chromium-review.googlesource.com/c/chromium/src/+/1235343.  If it's green, I can clean it up and send for review.

Adding mek@, who has worked on blob: navigations code moving to Mojo. I was under the impression that even blob: registration was moved over to Mojo, but the code being referenced in comment 3 and the test Alex has put up all assume classic IPC.

Poking at this, it looks like we do have Mojo based blob: URL registration in third_party/blink/public/mojom/blob/blob_registry.mojom. mek@, can you help clarify what is the current state of blob: mojofication?

blob: mojofication should all be working (and is turned on/required by network servicification). Shipping blob URL mojofication on its own though is still blocked by issue 705114 (which itself is blocked by  issue 819761 ). And I have no idea what the status of either of those is.

Looks like the mojo path has the same problem in BlobURLStoreImpl::Register(), which also checks CanCommitURL() currently:  https://cs.chromium.org/chromium/src/storage/browser/blob/blob_url_store_impl.cc?l=78&rcl=b8d3bfaa306273d85c542c0aca9352d767a95e33

Also, looking at tryjob results from #5, I think we'll need to relax the CanAccessDataForOrigin check for unique origins: e.g., a sandboxed frame on foo.com, locked in a foo.com renderer, should be able to create a blob URL with a null origin.  Not sure if there are any other special cases.

Also, found some relevant discussion that mek@ and I had in: https://chromium-review.googlesource.com/c/chromium/src/+/823828/5/storage/browser/blob/blob_registry_impl.cc#563, where we kind of anticipated that this would be problematic with --site-per-process and --isolate-origins and that we should strengthen the check in a followup - now is a good time for that followup.

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

commit 2078096efde1976b0fa9c820df90cedbfb2b13bc
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Thu Sep 27 23:17:04 2018

Lock down blob/filesystem URL creation with a stronger CPSP::CanCommitURL()

ChildProcessSecurityPolicy::CanCommitURL() is a security check that's
supposed to tell whether a given renderer process is allowed to commit
a given URL.  It is currently used to validate (1) blob and filesystem
URL creation, and (2) Origin headers.  Currently, it has scheme-based
checks that disallow things like web renderers creating
blob/filesystem URLs in chrome-extension: origins, but it cannot stop
one web origin from creating those URLs for another origin.

This CL locks down its use for (1) to also consult
CanAccessDataForOrigin().  With site isolation, this will check origin
locks and ensure that foo.com cannot create blob/filesystem URLs for
other origins.

For now, this CL does not provide the same enforcements for (2),
Origin header validation, which has additional constraints that need
to be solved first (see https://crbug.com/515309).

Bug:  886976 ,  888001 
Change-Id: I743ef05469e4000b2c0bee840022162600cc237f
Reviewed-on: https://chromium-review.googlesource.com/1235343
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594914}
[modify] https://crrev.com/2078096efde1976b0fa9c820df90cedbfb2b13bc/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/2078096efde1976b0fa9c820df90cedbfb2b13bc/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/2078096efde1976b0fa9c820df90cedbfb2b13bc/content/browser/security_exploit_browsertest.cc

I verified the fix from #10 in Win Canary 71.0.3567.0.  I followed the windbg-based repro steps and confirmed that they now lead to a renderer kill (crash id 3c87724eb339387e).

Also double-checked and didn't see any new crashes that might've been introduced by this in 71.0.3564.0+.

*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************

Nice one! The VRP Panel decided to award $8,000 for this report.  Cheers!

Thanks awalley@! As per #c0, could you make sure that this bug is rewarded to Masato? Thanks again!

I'd like to share the bounty with Jun (s.h.h.n....@gmail.com) since he helped me in investigating the details of this bug.
Could you pay Jun and me $4000 each? Thanks!

This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

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

(Already in 71)

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