New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 2 users
Status: Fixed
Owner:
OOO
Closed: Sep 22
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux, Windows, Chrome, Mac
Pri: 1
Type: Bug-Security

Blocking:
issue 766253



Sign in to add a comment
Security: privesc to war-extensions with PageState
Project Member Reported by meacer@chromium.org, Sep 18 Back to list
Split from  bug 766253 :

"""
FrameNavigationEntry (FNE) holds a SiteInstance and PageState. If a FNE is navigated to, then SiteInstance determines the process. PageState can override the URL that the renderer navigates to. content/renderer/render_frame_impl.cc:6250 RFI::NavigateInternal():

  std::unique_ptr<HistoryEntry> entry =
    PageStateToHistoryEntry(request_params.page_state);
  ...
  item_for_history_navigation = entry->root();
  ...
  request = frame_->RequestFromHistoryItem(item_for_history_navigation,
                                           cache_policy);

PageState contains a URL that goes into the request. If the SiteInstance belongs to an extension and the url in PageState shouldn't go to extension process, then the transfer logic kicks in. But data: url is loaded fine. Here's the bug: a frame can overwrite the page_state of any other frame in the same WebContents. Using FrameHostMsg_DidCommitProvisionalLoad, which reaches NCI::RendererDidNavigate() in content/browser/frame_host/navigation_controller_impl.cc:946:

  FrameNavigationEntry* frame_entry =
      active_entry->GetFrameEntry(rfh->frame_tree_node());
  ...
  frame_entry->SetPageState(params.page_state);

FNE is looked up based on frame unique names. A compromised frame can lie about its unique name, and set it to the target extension frame using FrameHostMsg_DidChangeName. But GetFrameEntry only looks for frames in the same WebContents. So an attacker must iframe an extension. This is only possible for the web-accessible-resources. Exploit in index.html and sc.cc.
"""
 
index.html
4.9 KB View Download
sc.cc
24.0 KB View Download
Blocking: 766253
Cc: creis@chromium.org nasko@chromium.org dcheng@chromium.org
Cc: gzo...@gmail.com
I was planning on moving unique name calculation to the browser... but I'm not sure I can write a version of that patch that we'd be OK with merging.
Owner: dcheng@chromium.org
Status: Assigned
Cc: awhalley@chromium.org
I wonder what other options we might have in addition (or in place of) moving unique name calculation to the browser.  Would it possibly help if we:

1) modified NavigationEntryImpl::FindFrameEntry so that it complains if there is more than 1 matching entry?

or

2) modified NavigationEntryImpl::TreeNode::MatchesFrame so that it not only compares unique names, but also compares |frame_entry->site_instance()| with |frame_tree_node->current_frame_host()->GetSiteInstance()| ?

or

3) enforced immutability of unique name after a commit (see the comment at the top of RenderFrameImpl::DidChangeName) ?
I don't really know what a good solution would be.

1) I think this wouldn't work because FindFrameEntry looks through FrameNavigationEntries, which still have different unique names. It's the FrameTreeNodes, that will have the same unique name with the exploit.

A similar idea would be to look through the frame_tree_ in RenderFrameHostImpl::OnDidChangeName to check if there is an existing FrameTreeNode with that name. But then a corrupted frame could lie about it's name before the target frame exists, and a non-corrupted frame would probably have to be killed.
 
Or maybe it's possible to validate in OnDidChangeName that the frame name matches to it's position in frame_tree_. Because frame names seem to have the form <!--framePath //<!--frame1-->/<!--frame0-->-->. This CL indicates that frame names are more complicated though: https://chromium-review.googlesource.com/c/chromium/src/+/579031 . I haven't delved into it. I'm also not sure if OnDidChangeName is the only place that sets the name.

2) That's interesting. I was first planning to use WebContentsImpl::UpdateStateForFrame through FrameHostMsg_UpdateState, but it has that site instance check. I suspect that adding the site instance check to MatchesFrame would break stuff. Take a look at NavigationControllerImpl::FindFramesToNavigate. It does pending_entry_->GetFrameEntry(frame) and GetLastCommittedEntry()->GetFrameEntry(frame). I assume that for the pending_entry_ the site instances may not match, if this is for example a back navigation to a different origin. Below, it even checks if the two FrameNavigationEntries have the same site instance to decide if the navigation is same page.

But perhaps add the site instance check to NavigationControllerImpl::RendererDidNavigate, similar to UpdateStateForFrame. Of course, there may be more vulnerable code. I haven't seen any, but who knows.

3) Could this mean that a corrupted frame can still change the name if it does an additional FrameHostMsg_DidCommitProvisionalLoad?
I personally think we should investigate #2 as the immediate fix. The long term fix is to move unique generation completely out of the renderer processes, which we need to do anyway for correctness with OOPIF, but it's unlikely to be a patch we could merge for a security fix.
Project Member Comment 9 by sheriffbot@chromium.org, Sep 19
Labels: M-61
Project Member Comment 10 by sheriffbot@chromium.org, Sep 19
Labels: Pri-1
Very nice report!  I'm just getting up to speed while traveling, but I think I agree with a version of #2 as the right first step.  As the reporter notes, WebContentsImpl::UpdateStateForFrame already has a SiteInstance check which helped.  NavigationControllerImpl::RendererDidNavigate is missing it on both of its SetPageState calls, and those should probably have that.

In general, NavigationController tries to update all the values of FrameNavigationEntry at the same time (so that the SiteInstance would get updated at the same time as the PageState), but clearly this is a spot where we're not doing that.

I haven't looked yet at whether it makes sense to update all of MatchesFrame; will look soon.

I also like the idea of preventing name changes on the browser side once we have any committed entry in the frame.  I think we'll have to allow clearing the name (per spec, issue 706350), but this is worth investigating.

I'm concerned there may be other ways the colliding names may be abused, even if we move name generation to the browser process.  I'll chat with dcheng@ about that.  At the least, we shouldn't let a difference in name between FTN and FNE cause us to only partly update a FNE (i.e., update the PageState but not the SiteInstance).  I'll try to understand more about the exact path that's being taken.
Owner: creis@chromium.org
Status: Started
I have a draft fix and test for it.  I'm cleaning it up and rebasing it now, since it won't compile on trunk with the recent Mojoification of DidCommitProvisionalLoad.

I'll post a link and explanation shortly.  There's some nice subtle behavior in the exploit to get it to work using AUTO_SUBFRAME.  I think we've got a bug with MANUAL_SUBFRAME as well where we're still reading params.frame_unique_name from the renderer, probably allowing the same attack-- I'll confirm and fix that as well.
Ok, the first CL is up for review at https://chromium-review.googlesource.com/c/chromium/src/+/674808.  This takes the "check the SiteInstance before updating PageState" approach, and should be easy to merge.  Explanation and ideas for further fixes below.

One thing that had been bothering me is that NavigationController's various RendererDidNavigate* methods should be updating the FrameNavigationEntry already, including the SiteInstance.  That certainly seemed to be the case in RendererDidNavigateAutoSubframe, which the exploit seemed to be targeting (in the commit function in sc.cc).  That AutoSubframe case called AddOrUpdateFrameEntry with the FrameTreeNode, so it seemed like we would find and update all members of the extension iframe's FrameNavigationEntry (including SiteInstance), and thus the exploit wouldn't work.

The exploit cleverly got around this by sending the forged IPCs from an iframe more deeply nested than the extension iframe.  This took advantage of the algorithm in AddOrUpdateFrameEntry, which first looks up the frame's parent and then searches its immediate children (to make sure it's targeting the right area of the tree).  It doesn't find the existing FNE there, so it creates a new one.  However, future calls to GetFrameEntry (including the one for updating the PageState) will find the old FNE, since it's higher in the tree and doesn't do the same parent lookup.

The exploit also had to be careful to do the pushState in the nested subframe instead of the main frame, since back/forward navigations that are same document don't traverse the subtree to see if any subframes need navigating.  If the main frame had been used for the pushState, then going back/forward wouldn't have made the extension iframe load the data URL, even though the data URL was in the FrameNavigationEntry's PageState.

As for the MANUAL_SUBFRAME case, it feels very risky but I don't see an immediate exploit for it.  I was concerned that it's creating a FrameNavigationEntry using params.frame_unique_name instead of the FrameTreeNode's unique name.  That suggests that it might be possible to throw this one aside with a fake name, but have RendererDidNavigate still call SetPageState on the extension's FrameNavigationEntry.  It looks like that's not easily possible, though, since CloneAndReplace does use the FrameTreeNode to find where to put the (corrupted) FrameNavigationEntry.  This means we would probably overwrite the extension's SiteInstance on the FrameNavigationEntry.  Still, we should eliminate the corruption here, whether it can be exploited or not.

--

As for fixes, Nasko and I chatted and think there's a few worthwhile things to do.

1) Start by skipping the PageState update if the SiteInstance doesn't match.  That should be reasonably safe, and easy to merge.  It handles the AUTO_SUBFRAME case, and it would also handle a MANUAL_SUBFRAME case if that did turn out to be exploitable.

2) Get rid of FrameNavigateParams::frame_unique_name.  It's not needed anymore now that we can use FrameTreeNode::unique_name, and it's only creating problems by potentially differing from it.

3) Don't update PageState at the end of RendererDidNavigate.  Instead, do it consistently in each of the various helper methods (alongside a SiteInstance update or verification), and DCHECK that we did that properly at the end.  This avoids the risk that we'll corrupt a (possibly different) FrameNavigationEntry.

4) I still think it's worthwhile to have the browser process enforce that a unique name for a FrameTreeNode can't change (other than being cleared) after any navigation has committed in it.

5) Yes, we should also move unique name computation to the browser process and do a better job detecting non-unique names.

Fixes 1-3 should be done ASAP.  4 would be nice if it's easy, and 5 may take a while.
I am now unsure if defense#4 is sufficient/meaningful - right now the renderer avoids updating the unique name after a first real commit (e.g. see RenderFrameImpl::DidChangeName).  OTOH, the fake name is sent for a frame fully controlled by an attacker, so I think the attacker can work around defense#4 by lying about the unique name *before* a first real commit (i.e. when the initial empty document is still there).

Another short-term fix might be to check (in RenderFrameHostImpl::OnDidChangeName) whether the received |unique_name| is truly unique.  I don't think we can kill a renderer when the name is non-unique (because the non-uniqueness might be caused by a race where 2 renderers independently arrive at the same locally-unique, but globally-duplicated unique name).  But since continuing with a *non*-unique name leads to problems, then maybe we'll get less problems if the browser in this situation generates a truly unique name and uses it instead (but then I am not convinced if this is preferable to just doing defense#5).

FWIW, I've tried the original defense #4 in https://chromium-review.googlesource.com/c/chromium/src/+/676130 - all bots are green, so maybe this is feasible.  (OTOH, I guess I could have used a simpler condition - rather than counting the number of history entries, I probably could have checked if GetLastCommittedURL().is_empty()).
Project Member Comment 15 by bugdroid1@chromium.org, Sep 21
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c050720e317e5223bcbdcaafb816befa789ceaa9

commit c050720e317e5223bcbdcaafb816befa789ceaa9
Author: Charles Reis <creis@chromium.org>
Date: Thu Sep 21 00:40:02 2017

Don't update PageState for a SiteInstance mismatch.

BUG= 766262 
TEST=See bug for repro.

Change-Id: Ifb087b687acd40d8963ef436c9ea82ca2d960117
Reviewed-on: https://chromium-review.googlesource.com/674808
Commit-Queue: Charlie Reis (OOO until 9/25) <creis@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503297}
[modify] https://crrev.com/c050720e317e5223bcbdcaafb816befa789ceaa9/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/c050720e317e5223bcbdcaafb816befa789ceaa9/content/browser/security_exploit_browsertest.cc

$ git find-releases  c050720
commit c050720 was:
  initially in 63.0.3221.0

Shall we ask for merge approval?  Or do we need some more bake time?
Cc: mnissler@chromium.org wfh@chromium.org
Status: Fixed
Great!  Tentatively marking fixed, but I've asked mnissler@ or wfh@ to try to repro the exploit with the fix to see if the fix blocks it.  If we can confirm, then we should request merge.  We'll continue with the followup work.

It does appear that PlzNavigate may also disrupt this stage of the exploit, but I'm not entirely sure and want to confirm why.  (It looks like it's not using the URL from the PageState, which may or may not be a problem in some cases.)  We shouldn't depend on that for M61 since it's still possible to turn off the PlzNavigate field trial, so merging it seems important.
Comment 14: I agree that #4 is not a defense on it's own, but it seems worth enforcing.  For example, the exploit seems to change the frame's unique name and then change it back.  It may be able to work without that step, but no sense in allowing that flexibility if we don't need to.  Thanks for starting the CL for it!

I like the idea of trying to make sure the name is really unique, though I wonder what the right way to handle it is.
Project Member Comment 19 by sheriffbot@chromium.org, Sep 22
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
creis, I tried your CL on M60. It stops the exploit.
Labels: M-62 Merge-Request-62
Great, thank you for confirming!  Requesting merge to M62 first.  (TPMs: r503297 disrupts the attack in  issue 766253 .)

lukasza@: Would you be able to help with the merge?  I won't be available for most of today.
Project Member Comment 22 by sheriffbot@chromium.org, Sep 23
Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: M62 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), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: gov...@chromium.org abdulsyed@chromium.org
abdulsyed@: Ping about the merge request for M62.

Also CC'ing govind@, since we may want to merge this to M61.  (It was a link in a critical exploit chain.  PlzNavigate disrupts this link as well, but I'm not sure we can guarantee that will stay enabled.)
+ awhalley@ for M61 merge review (This change is not yet baked in M62 Beta).
Thanks creis@ - can you confirm if this has been well tested in Canary?I was unclear how this was tested in M60 (comment #20). Is this a safe merge overall?
Comment 25: The change has been on Canary for 4 days and doesn't seem to show any new crashes.  The behavior change itself is safe, and should only affect a case where we were doing the wrong thing in the first place.

As for testing it against the exploit, the CL includes a browsertest that follows the same steps as the relevant portion of the exploit.  See  issue 766253  for context on comment 20; it indicates that the fix is effective against the original exploit.

I think this means it should be a safe and effective merge.
Per offline chat with awhalley@ (Security TPM), this is not a blocker for M61. We can take this merge in for next M61 respin (if any) after change is baked in beta. Thank you.
Labels: -Merge-Review-62 Merge-Approved-62
Thanks for confirming. Approving merge to M62 (branch:3202)
Project Member Comment 29 by bugdroid1@chromium.org, Sep 25
Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2bec05996c7a8001a5d955378e843a6f15028530

commit 2bec05996c7a8001a5d955378e843a6f15028530
Author: Charles Reis <creis@chromium.org>
Date: Mon Sep 25 22:08:12 2017

Don't update PageState for a SiteInstance mismatch.

BUG= 766262 
TEST=See bug for repro.
TBR=lukasza@chromium.org

(cherry picked from commit c050720e317e5223bcbdcaafb816befa789ceaa9)

Change-Id: Ifb087b687acd40d8963ef436c9ea82ca2d960117
Reviewed-on: https://chromium-review.googlesource.com/674808
Commit-Queue: Charlie Reis (OOO until 9/25) <creis@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#503297}
Reviewed-on: https://chromium-review.googlesource.com/682414
Reviewed-by: Charlie Reis (catching up) <creis@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#439}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/2bec05996c7a8001a5d955378e843a6f15028530/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/2bec05996c7a8001a5d955378e843a6f15028530/content/browser/security_exploit_browsertest.cc

Project Member Comment 30 by bugdroid1@chromium.org, Sep 27
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d2a509f392be389697a75061aa6d71df652e7e12

commit d2a509f392be389697a75061aa6d71df652e7e12
Author: Charles Reis <creis@chromium.org>
Date: Wed Sep 27 23:47:48 2017

Remove FrameNavigateParams::frame_unique_name.

The name is already available from the FrameTreeNode and does not need
to be sent up to the browser at commit time.

BUG= 766262 
TEST=No behavior change

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I5f9bc8acd50c868b84514f2ae240b5640ff56a83
Reviewed-on: https://chromium-review.googlesource.com/686174
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Charlie Reis (catching up) <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504806}
[modify] https://crrev.com/d2a509f392be389697a75061aa6d71df652e7e12/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/d2a509f392be389697a75061aa6d71df652e7e12/content/browser/frame_host/navigation_controller_impl_unittest.cc
[modify] https://crrev.com/d2a509f392be389697a75061aa6d71df652e7e12/content/browser/security_exploit_browsertest.cc
[modify] https://crrev.com/d2a509f392be389697a75061aa6d71df652e7e12/content/common/frame_messages.h
[modify] https://crrev.com/d2a509f392be389697a75061aa6d71df652e7e12/content/public/common/frame_navigate_params.h
[modify] https://crrev.com/d2a509f392be389697a75061aa6d71df652e7e12/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/d2a509f392be389697a75061aa6d71df652e7e12/content/test/test_web_contents.cc

Labels: Merge-Request-61
govind@: I'm requesting merge to M61 per your comment #27, since this fix has baked on 	62.0.3202.38 beta for about 5 days now and seems to be doing well.
Cc: asymmetric@chromium.org
+ awhalley@/asymmetric@ (Security TPMs) for M61 merge review
(Note: We're not planning M61 stable respin unless critical issue arise at this point)
Cc: keta...@chromium.org
We're not planning any further M61 releases for Desktop.

+ ketakid@ (Chrome OS TPM) to decide on M61 merge after merge review by awhalley@/asymmetric@.


Getting any of the child bugs of  issue 766253  in 61 for Chrome OS would be superb, and this change does indeed look good, but I know there are exceptional circumstances for the 61 Chrome OS release that I believe have already been deemed sufficient to trump security merges. Deferring to etakid@
Project Member Comment 35 by bugdroid1@chromium.org, Oct 13
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f4448202d63cc64d703637305921239ff789e280

commit f4448202d63cc64d703637305921239ff789e280
Author: Charles Reis <creis@chromium.org>
Date: Fri Oct 13 21:15:03 2017

Set PageState and redirects consistently for FrameNavigationEntries.

Move the update of PageState within each RendererDidNavigate* helper
method, so that it's done on the correct frame.  Due to issue 774637,
there are cases where the PageState was being applied to the wrong
FNE in the past.

BUG= 766262 , 774637

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I78125efdc32d80795ef58afa65be28d374a1366c
Reviewed-on: https://chromium-review.googlesource.com/718080
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508821}
[modify] https://crrev.com/f4448202d63cc64d703637305921239ff789e280/content/browser/frame_host/frame_navigation_entry.cc
[modify] https://crrev.com/f4448202d63cc64d703637305921239ff789e280/content/browser/frame_host/frame_navigation_entry.h
[modify] https://crrev.com/f4448202d63cc64d703637305921239ff789e280/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/f4448202d63cc64d703637305921239ff789e280/content/browser/frame_host/navigation_entry_impl.cc
[modify] https://crrev.com/f4448202d63cc64d703637305921239ff789e280/content/browser/frame_host/navigator_impl.cc

Labels: Release-0-M62
Project Member Comment 37 by sheriffbot@chromium.org, Oct 18
Labels: -M-61
Labels: CVE-2017-15402
Labels: -Restrict-View-SecurityNotify
Labels: allpublic
Sign in to add a comment