Ooops, wrong file. Here's the right one.

Deleted: PoC.html 1.2 KB

PoC.html 1.2 KB View Download

Nice, thanks for the report!  Indeed, it appears that the browser-side logic to create filesystem URLs also relies on CPSP::CanCommitURL, similarly to blob URLs in  issue 886976 .  More specifically, ChildProcessSecurityPolicyImpl::HasPermissionsForFileSystemFile() checks CanCommitURL() here: https://cs.chromium.org/chromium/src/content/browser/child_process_security_policy_impl.cc?l=1013&rcl=91269b920348317e3f7eef39467dd9fc4750d11f

I'm guessing using a subframe is required since top-level navigations to filesystem URLs are now blocked.  It probably makes sense to expand the fix we're considering in  issue 886976  to also cover this case.  I'll assign to myself for now.

I wrote a SecurityExploitBrowserTest that repros this, mostly along the lines of nick@'s ChromeSecurityExploitBrowserTest.CreateFilesystemURLInExtensionOrigin but for a cross-origin setup rather than an extension one.  

For the fix, perhaps CPSP::CanCommitURL can just call CanAccessDataForOrigin directly?  This is also nice because CPSP::CanCommitURL already handles unique origins correctly for blob/filesystem URLs.  

CPSP::CanCommitURL is used by:
- blob URL creation (two places, one for classic IPC and one for mojo) 
- filesystem URL creation
- Origin header
- CPSP::CanCommitURL itself to deal with nested URLs.

The blob/filesystem cases should check CanAccessDataForOrigin() given these bugs, but I'm not sure about the Origin header.  My attempt at this (PS2 of https://chromium-review.googlesource.com/c/chromium/src/+/1235343) has some layout test failures, showing that a content script should be able to attach an Origin header that doesn't match the origin lock, which probably makes sense... but then do we really want to use CanCommitURL() for checking the origin header?  Charlie, any thoughts on that, since I think you worked on the Origin header enforcements?

Looks like issue 515309 discusses CanCommitURL enforcements for the Origin header.  It also mentions whitelisting chrome-extension:// headers for content scripts.

1) Can we do the origin checks on the UI thread? 

I've talked with Alex about this bug and he pointed out that blob/filesystem creation takes place on the IO thread and therefore doesn't/cannot inspect RenderFrameHostImpl::GetLastCommittedOrigin (which is only available on the UI thread).  This means that tightening of RenderFrameHostImpl::CanCommitOrigin (see  issue 770239 ) wouldn't directly help in the blob/filesystem cases... unless we would consider bouncing to the UI thread when processing blob/filesystem requests.

I wonder if this is worth pursuing further (to do CanCommitOrigin checks only once, on the UI thread, in RenderFrameHostImpl and to avoid imperfectly duplicated CanCommitOrigin checks for the IO thread, in ChildProcessSecurityPolicy [imperfectly, because these IO-thread checks don't have visibility into UI-only data like hosted apps or site urls]).

AFAIR cookies are another example of checks happening on the IO thread, but I think that this case should go away after NetworkService ships (right?).


2) Previous attempts of CanCommitOrigin tightening led to some undesirable kills in the wild.

In https://crbug.com/797968#c12, Alex speculates about a scenario that might have made it difficult to enforce origin-lock-related checks in RenderFrameHostImpl::GetLastCommittedOrigin on the UI-thread.  We've wondered whether this same scenario would also spell trouble for IO-thread checks for blob/filesystem cases.  Maybe we can hope that the problematic scenario doesn't happen as frequently for blob/filesystem cases?

Comments 4-5: I could see using CanAccessDataForOrigin for those cases, and maybe changing how the Origin header enforcement is done if it needs to special case content scripts.

Comment 6: We can't solely rely on a commit-time check on the UI thread, right?  The correct attacker origin can commit, but a later IPC can use a victim origin without attempting to commit it as a navigation.  I agree that being on the IO thread here is tricky, but I don't remember where we left off in discussing how to address it.  (Did it depend on Mojo?)

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

Marking as fixed - see comments 11&12 on  https://crbug.com/886976 .

*** 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.
*********************************

Hi s.h.h.n.j.k@ - we looked into how the fixes landed, and believe were already fixing this issue after your reporting of  issue 886976 . However for the additional test case and info, the VRP panel decided to award $500 for this one. Thanks as ever!

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