For this one, we want to determine if filesystem (and/or blob) URLs can be blocked in chrome:// pages entirely, or if there are current important uses of them.

If they can't be blocked entirely, then we should ensure that the browser process enforces that one chrome:// origin cannot create filesystem or blob URLs in other chrome:// origins.

dcheng@: Not sure if you want to look into this as followup to  issue 821613 , or if alexmos@ or I should take a look in terms of filesystem URL checks.  Let me know what you think.  Thanks!

I think it's actually possible to fix the permission checks relatively easily: these have a render frame routing ID associated since the permission checks are plumbed through ContentSettingsObserver. So that's nice.

Unfortunately, this is insufficient: the *actual* IPCs to actually open the resource are not currently scoped to an execution context--we just pass the origin up.

I see two potential alternatives:
1) Validate that the origin passed up in OpenFileSystem is legitimate. I talked with lukasza@ who suggested ChildProcessSecurityPolicy::CanCommitURL might be an appropriate place for this.
2) Convert all this code to be scoped to an execution context with Mojo.

The latter does not seem mergeable so I'm going to focus on the former for now. Independently of this, we should also investigate restricting filesystem:// access on chrome:// URLs.

(As an aside, I think it's a bit difficult to follow the permission checks in this code: we have the permission checks in ContentSettingsObserver... but that's mostly an advisory thing, as the renderer could easily just ignore those checks and choose to do it's own thing. Ideally we'd harden this so the permission checks would be a natural part of getting a handle to access those resources.)

Friendly ping: Any updates on the plan in comment 6?  And did you find that navigations to chrome:// filesystem and blob URLs were necessary to support?  (That seemed like a potentially easier fix, but enforcing origins more effectively could work as well.)

To summarize the earlier discussion today:

We could hardcode kChromeUIScheme as an origin never allowed to access filesystem URLs. I originally explored this, but it made me a bit sad for various reasons:
1. Hardcording kChromeUIScheme to be blocked for filesystem access seems fragile: there might be other sensitive features or schemes in the future that would skirt this block and be similarly problematic.
2. It wouldn't align with the actual permission checks (which are enforced separately... and in a manner that trusts the renderer. Oops)

This being said, it does seem reasonable that we have some way of expressing this in the future (that we have to explicitly opt origins into new web API features rather than having them opted in by default), but that seems somewhat out of scope of mitigating this bug.

The other alternative I explored is strengthening the origin checks. However, the problem is this:
1. chrome://resources/html/cr.html (for example) is not considered a WebUI page. So it doesn't get WebUI bindings. So we should be safe but...
2. When we commit a navigation, we update the permissions for that RFH by calling RFH::UpdatePermissionsForNavigation().
3. This calls GrantRequestURL(), to ensure that the RFH has permissions to request 'various things' from that origin in the future.
4. However, since chrome:// is not a web safe scheme, psuedo scheme, blob, or filesystem scheme, we fall through to the end:
    // When the child process has been commanded to request this scheme,
    // we grant it the capability to request all URLs of that scheme.
   Now we have a renderer that's been granted the chrome:// scheme.

Which means that we now allow this renderer to request *or* commit an chrome:// URL. Which means that HasPermissionsForFileSystemFile() won't early return here:

  // If |filesystem_url.origin()| is not committable in this process, then this
  // page should not be able to place content in that origin via the filesystem
  // API either.
  if (!CanCommitURL(child_id, filesystem_url.origin())) {
    UMA_HISTOGRAM_BOOLEAN("FileSystem.OriginFailedCanCommitURL", true);
    return false;
  }

So if we can get code execution in this renderer, it can request a filesystem for another chrome:// origin...

Maybe we can limit filesystem to websafe schemes, but this also seems fragile: even if we can enforce this (and content:// and externalfile:// and file:// schemes can't use filesystem URLs...), there a number of pieces that happen to all work together so this doesn't fail catastrophically...

Thanks for the explanation!  

It seems to me that we're conflating GrantRequestURL and CanCommitURL.  I think it's probably reasonable that a process that loads a page on chrome://resources (or any chrome:// origin) should be able to request a URL from any other chrome:// origin, if not actually commit it.  In that sense, the GrantRequestURL() thing doesn't seem like big problem at first glance, and might be hard to tighten (since we don't keep track of which WebUI pages are allowed to link to each other).

However, the CPSP::CanCommitURL check could probably be made stronger.  I don't think we let different chrome:// origins share a process with each other, though I don't remember off the top of my head if we lock those processes to origin yet or not.  That might be a place we can tighten.  I assume we're falling back to scheme_judgement in the CanCommitURL helper and saying that any chrome:// URL is fine?  Maybe we should change CanCommitURL to not fall back on which schemes have been granted for requests (which is broader than what's allowed to be committed), but rather find some way to more specifically track what's allowed to be committed?

Friendly ping. :) This is blocking the full resolution of a Critical, so it'd be nice to nail it down.

Hello again! This is your friendly security sheriff here! I wonder if there was an update on this HIGH priority bug?

Daniel has a CL in review to prevent chrome:// URLs from accessing filesystem URLs:
https://chromium-review.googlesource.com/c/chromium/src/+/1103062

We commit ourselves to a 60 day deadline for fixing for high severity vulnerabilities, and have exceeded it here. If you're unable to look into this soon, could you please find another owner or remove yourself so that this gets back into the security triage queue?

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

CL looks abandoned, back to creis for re-assignment.  Thanks.

This is the updated CL, which dcheng@ is still working on:
https://chromium-review.googlesource.com/c/chromium/src/+/1108485

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

commit 4ebba554ef97d679ff229448a90e4f61b498de31
Author: Daniel Cheng <dcheng@chromium.org>
Date: Fri Jul 06 21:43:16 2018

ChildProcessSecurityPolicy: only use granted schemes for requests

Granting a renderer permission to commit any URL with a given scheme is
too powerful. While actual commits are guarded by SiteInstance checks
when site isolation is enabled, there are many other permission checks,
e.g.  the ability to create blob: and filesystem: URLs, that are not
similarly guarded.

With this change, scheme grants (and origin grants) are now separated
into grants for requests and grants for commits. The navigation logic
has been tweaked to grant full commit access to the committing origin,
and only granting limited request access to the scheme of the committed
page. This allows chrome:// URLs to still request (and navigate) to
other chrome:// URLs, but disallows a renderer that hosts chrome://x
from forging a blob or filesystem file that claims an origin of
chrome://y.

Note that the original plan was to completely disallow granting commit
access to an entire scheme; unfortunately, that broke non-standard
schemes such as Android's content: URL scheme, among other things. To
preserve current behavior, if the committing origin is unique,
ChildProcessSecurityPolicyImpl will still grant commit rights to the
scheme.

Bug:  836859 
Change-Id: Ife0f3a254b7b572a0fa6e733f3ca3cd9f4498958
Reviewed-on: https://chromium-review.googlesource.com/1108485
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573079}
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/android_webview/browser/aw_content_browser_client.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/android_webview/browser/aw_contents.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/android_webview/browser/net/aw_url_request_context_getter.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/chrome/browser/chrome_security_exploit_browsertest.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/chrome/browser/devtools/devtools_ui_bindings.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/chrome/browser/extensions/chrome_extension_web_contents_observer.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/content/browser/child_process_security_policy_unittest.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/content/browser/fileapi/browser_file_system_helper.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/content/browser/frame_host/navigation_controller_impl_unittest.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/content/browser/webui/web_ui_mojo_browsertest.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/content/public/browser/child_process_security_policy.h
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/extensions/browser/extension_web_contents_observer.cc
[modify] https://crrev.com/4ebba554ef97d679ff229448a90e4f61b498de31/extensions/browser/guest_view/web_view/web_view_guest.cc

(reward-topanel as part of considering issue 835887)

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

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

+awhalley@ (Security TPM) for merge review.

No merge needed.

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