I've confirmed this a problem with --enable-browser-side-navigation and --disable-web-security (to simulate a compromised renderer).  If you hardcode RenderFrameImpl to pass something like file://stolen-file in base_url_for_data_url any time that the URL is a data URL, you can exploit this as follows:

f = document.createElement("iframe");
f.src = "data:text/html,I now have access to a file URL.";
document.body.appendChild(f);
f.src = "file://stolen-file";

The iframe will render the contents of stolen-file in the attacker's process, allowing it to steal the contents.

You can also use this technique to open a tab to chrome://settings using window.open.  (That's in a different process, but it still shouldn't be allowed.)

Great catch, Charlie.

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

commit 1a1392222a9b97ffbaa77f3beee093fc81e655ff
Author: nasko <nasko@chromium.org>
Date: Fri May 26 17:30:16 2017

Ensure the renderer doesn't specify base_url_for_data_url in the BeginNavigation IPC.

This should only be set by browser-initiated navigations.

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

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

[modify] https://crrev.com/1a1392222a9b97ffbaa77f3beee093fc81e655ff/content/browser/bad_message.h
[modify] https://crrev.com/1a1392222a9b97ffbaa77f3beee093fc81e655ff/content/browser/browser_side_navigation_browsertest.cc
[modify] https://crrev.com/1a1392222a9b97ffbaa77f3beee093fc81e655ff/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/1a1392222a9b97ffbaa77f3beee093fc81e655ff/tools/metrics/histograms/enums.xml

jam@, I think it is prudent to give at least a canary release a chance to see that all is good before asking for a merge. 

@nasko: it's auto merge at this point, so won't merge till Tuesday. Just added label for now.

UpdatePermissionsForNavigation does gives permission for a few things (in order from code):

1. Grants access to common_params.url.
2. For data URLs, it grants access to common_params.base_url_for_data_url.
3. Grants access to files based on PageState, which should be valid only for history navigations.
4. Grants access to files based on the RequestBody.

(1), (2), and (3) should only be done for browser initiated navigations. Since (4) is required for renderer-initiated navigations, we need to ensure it is either moved out of this method into appropriate places if we stop calling UpdatePermissionsForNavigation for renderer initiated navigations. 

In an offline discussion with creis@ yesterday we were debating whether to extract (4). The alternative, which we think is also reasonable is to continue calling UpdatePermissionsForNavigation in all cases as we do today, however, add explicit checks for which type of navigation it is and only apply the grants that make sense for that case. The main reason for proposing this approach is that all granting permissions code stays in one place instead of being scattered around and when UpdatePermissionsForNavigation is updated, it is easier to see locally to which case any new changes apply.

I like the idea of adding explicit consideration of the navigation type as a parameter in a unified function; that sounds like an easier function to audit.

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

Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

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

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

commit 2b180b660e51d20d6a5d95b9121ca57622bfd4da
Author: John Abd-El-Malek <jam@chromium.org>
Date: Tue May 30 16:19:15 2017

Ensure the renderer doesn't specify base_url_for_data_url in the BeginNavigation IPC.

This should only be set by browser-initiated navigations.

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

Review-Url: https://codereview.chromium.org/2905293002
Cr-Original-Commit-Position: refs/heads/master@{#475043}
Review-Url: https://codereview.chromium.org/2913813002 .
Cr-Commit-Position: refs/branch-heads/3112@{#30}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[modify] https://crrev.com/2b180b660e51d20d6a5d95b9121ca57622bfd4da/content/browser/bad_message.h
[modify] https://crrev.com/2b180b660e51d20d6a5d95b9121ca57622bfd4da/content/browser/browser_side_navigation_browsertest.cc
[modify] https://crrev.com/2b180b660e51d20d6a5d95b9121ca57622bfd4da/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/2b180b660e51d20d6a5d95b9121ca57622bfd4da/tools/metrics/histograms/enums.xml

This is not fixed. It requires more work, so please keep open.

There's a bug in r475043, since it's missing an early return statement.  That means we can still grant access to URLs in base_url_for_data_url, though we'll kill the renderer.  It's my understanding that killing the renderer doesn't clear the grants for the process ID, so the attack would still succeed if the user reloaded the crashed tab.

I'm very frustrated that this CL was rushed and already merged to branch.

btw maybe I'm doing something wrong, but I tried creis@'s steps in comment 1 and the renderer was always getting killed. I passed --disable-web-security, but not sure which code in the renderer is killing it. It's not in the browser as far as I can tell, since none of the breakpoints in bad_message.cc were being hit. I guess this is moot point since a compromised renderer would disable whatever code is doing the killing, but if anyone has tips on how to repro this I'd be curious. (of course we should still fix this, but I was curious).

I don't have a recent enough build to verify this yet, but I was describing what would happen if the user reloaded the tab after the renderer kill.  At that point, the new renderer process would inherit everything that had been granted to it before (since it keeps the same RPH ID and is expected to restore things like files selected in the chooser), so it could access whatever file the attacker chose earlier.

yep, I had reoloaded after the crash.

ok, nvm I was able to repro this. My code was always setting the base_url_for_data_url and that was causing the wrong path to be taken in the renderer the second time.

Charlie: I wonder if to tighten this up we should just reset all capabilities given to a process if it's killed because of a bad message?

Thanks!  (CL in review here: https://codereview.chromium.org/2915813002/)

Re #19: As I noted on the CL, I think it's probably risky to revoke all the privileges after a kill, since that will likely cause an additional round of kills after restart which are harder to diagnose.  (Thanks to Nick for discussing the pros and cons.)  Let's stick with the early return, at least for a CL we want to merge.

Re #8: Yes, I'd feel much better if the first 3 grants in UpdatePermissionsForNavigation were only done for browser-initiated navigations.

I *think* we're ok in the PlzNav case after https://codereview.chromium.org/2915813002/ lands.  Reasoning:
1) common_params.url goes through FilterURL in OnBeginNavigation.
2) common_params.base_url_for_data_url goes through FilterURL in OnBeginNavigation.
3) request_params.page_state can't be set by a BeginNavigation IPC, since it's initialized to an empty PageState in NavigationRequest::CreateRendererInitiated.
4) common_params.post_data goes through CanReadRequestBody in OnBeginNavigation.

This is still risky if anything else gets added to UpdatePermissionsForNavigation, but I think we'll be ok if Nasko makes most of that method dependent on browser-initiated navigations.

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

commit d5226ddf44a52a3f066755e8d32710f6a093aa30
Author: jam <jam@chromium.org>
Date: Thu Jun 01 02:34:52 2017

Add missing return statement after ReceivedBadMessage call.

This prevents the scheme access being granted and used if the renderer is restarted.

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

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

[modify] https://crrev.com/d5226ddf44a52a3f066755e8d32710f6a093aa30/content/browser/browser_side_navigation_browsertest.cc
[modify] https://crrev.com/d5226ddf44a52a3f066755e8d32710f6a093aa30/content/browser/frame_host/render_frame_host_impl.cc

Do we have agreement we can mark the bug as closed now?

There is one more follow up to do, which is to ensure UpdatePermissionsForNavigation checks for browser vs renderer initiated navigations and does not apply all of them in all cases. I'd like to keep this bug open until that is fixed, as it belongs in resolving this bug.

The original issues in this bug, which do block PlzNavigate, have been fixed, so I'm removing Proj-PlzNavigate-Blocking.

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

friendly ping Nasko, are you actively working on this ?

nasko: Uh oh! This issue still open and hasn't been updated in the last 117 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

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

I am not actively working on this. The security issue is fixed, so this should not be considered an active security issue anymore. If it helps, I can resolve this bug as fixed and a file a new one for the followup work I've described in comment 25.

nasko: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

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

nasko@ -- what you state in #c31 is a good idea. can you please do that so that the security issue can be marked as closed? thanks.

I'll do it. :)

https://bugs.chromium.org/p/chromium/issues/detail?id=783017

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