Project: chromium Issues People Development process History Sign in
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: Verified
Owner:
out until aug 28th
Closed: Mar 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All, Chrome
Pri: 1
Type: Bug-Security

Blocking:
issue 351788



Sign in to add a comment
Pwnium: Extension system allows compromised renderer access to crosh
Project Member Reported by kenrb@chromium.org, Mar 12 2014 Back to list
This needs further investigation but we have enough information to triage.

A Pwnium 4 entry achieved a sandbox escape by sending messages from a compromised swapped out renderer to a vulnerable extension. The problem seems to be in TabHelper, which is getting state confused by the swap out.

creis' synopsis: TabHelper is not deleting state associated with a swapped out RVH.  It should probably be listening to WebContentsObserver::RenderViewHostChanged in addition to RenderViewHostCreated.

(and I'm initially flagging this as high severity but maybe it should be lower)
 
Blocking: chromium:351788
Comment 2 by meacer@chromium.org, Mar 12 2014
Cc: kalman@chromium.org
Comment 3 by creis@chromium.org, Mar 12 2014
Sounds like this bug was used to send a MSG_ExtensionRequest from a non-extension process, since it had a swapped out RenderView for an extension tab.  TabHelper probably saw the message come in from the swapped out RenderViewHost and accepted it, though it should only accept messages from the active RenderViewHost.
Cc: finnur@chromium.org yoz@chromium.org
Owner: jyasskin@chromium.org
Status: Started
It looks like ExtensionFunctionDispatcher::CreateExtensionFunction is supposed to block this, using the extensions::ProcessMap, which is populated partly from ChromeContentBrowserClient::SiteInstanceGotProcess. I'm still looking.
Comment 6 by kalman@chromium.org, Mar 12 2014
Yeah sounds like we should be watching renderers being swapped. I wonder how many other bugs we have similar to that.

How did the renderer manage to send the request in the first place?
Comment 7 by kenrb@chromium.org, Mar 12 2014
Cc: skuhne@chromium.org
Comment 8 by kenrb@chromium.org, Mar 12 2014
kalman: the initial renderer was compromised by a memory corruption bug. This bug was the key part of the sandbox escape.
TabHelper::OnRequest() uses extensions::ProcessMap::Get(browser_context_)->Contains(extension->id(), web_contents()->GetRenderViewHost()->GetProcess()->GetID()) to check that it's coming from the right process. That looks like it should handle swapped-out RenderViewHosts correctly from the Extensions side. Do we have more details of the WebContents' state when the message comes in? Or the message itself?
Comment 10 by jln@chromium.org, Mar 12 2014
I'm attaching geohot's shellcode.

You should be able to understand the IPC from there.
shellcode.c
8.1 KB Download
Summary: Pwnium: Extension system allows compromised renderer access to crosh (was: Pwnium: TabHelper allows compromised renderer access to crosh)
The shellcode contains:

ViewHostMsg_CreateWindow(routing=0x7fffffff, opener_id=1, ...)
ViewHostMsg_ShowView(routing=1, route_id=3)
ViewHostMsg_OpenURL(routing=3, url="chrome-extension://nkoccljplnhpfnfiajclkommnmllphnl/html/crosh.html")
ViewHostMsg_OpenURL(routing=3, url="chrome-extension://nkoccljplnhpfnfiajclkommnmllphnl/html/crosh.html")

ExtensionHostMsg_Request(routing=4, extension_id="nkoccljplnhpfnfiajclkommnmllphnl", source_url="chrome-extension://nkoccljplnhpfnfiajclkommnmllphnl/html/crosh.html", terminalPrivate.sendInput(pid={2..32768}, input="EXPLOIT"))

I'm not familiar enough with the IPC routing or process-starting code to say anything for sure, but it looks suspicious that a message with a new, higher routing ID, gets routed to an extension that should have been started in an entirely new process. How sure are we that Crosh actually got a dedicated process like extensions are supposed to?

There are also 2-second sleeps (poll(NULL, 0, 2000);) after both OpenURL() calls, so it doesn't look like a race condition.
Owner: ----
Status: Available
I'm lost in the twisty passages of chrome::Navigate(). Can someone who's more familiar with IPC routing and process creation pick it up from here?
Comment 14 by creis@chromium.org, Mar 13 2014
Cc: jam@chromium.org
Comment 9: The web_contents()->GetRenderViewHost() call returns the active RenderViewHost, which probably does not match the swapped out RenderViewHost that the message actually came from.  I suspect that's a bug.

Comment 11: Thanks for spelling those out.  It would help to know what process ID they're coming from if possible, since it's hard to know if 3 and 4 are from the same ID space.

I'm a bit surprised that (1) we allow a web renderer process to navigate to that extension URL given that it shouldn't be a web accessible resource, and (2) that it doesn't end up in a different BrowsingInstance (in ShouldSwapBrowsingInstancesForNavigation), making us block future OpenURL calls to it from the swapped out RenderViewHost.

Anyway, both Nasko and I are eager to take a closer look when we get back into town.  If there's a bigger rush to get this part understood and diagnosed, @jam might have the CreateNewWindow/OpenURL knowledge or can point to someone else who does.  I'm guessing we can probably wait until Monday, though.
Labels: CVE-2014-1709
(FYI, on IRC we had a tangential discussion about how exposing pids was a bad idea because it also enabled this attack, so I've filed issue 352465 to track that.)
Comment 17 by k...@google.com, Mar 14 2014
Cc: kamakshi@chromium.org
Cc: -creis@chromium.org -infe...@chromium.org
Owner: creis@chromium.org
Status: Assigned
Yes, the build is already cut. This one can take its time.
I'm about to attach a patch that repro's this on ToT chrome.  The patch hijacks the Window.alert() method so that visiting a page containing <script>alert()</script> will cause the custom code to be run (rather than popping up a dialog (which is an easy way to get execution ensuring all the setup has happened)).

Note that the sleep() interval is significant.  If it is too low, then the check "Extension API call disallowed - name:" at extension_function_dispatcher.cc:487 will fire instead and report a different PID than when the success message is fired. I expect this is the place where execution is supposed to end when functioning correctly.  

For a debug build, sleeping for 2 secs (as did the exploit in poll()) is too short.

Looking at the log, in the success case, we see a few more renderer process start messages as well.  So it may not be that CROSH and the web contents wind up in the same process. It's possible instead that something is passing the most recent renderer pid down to the extension system for checking.
Attached.
b351815.diff
3.9 KB Download
Comment 21 by creis@chromium.org, Mar 18 2014
Cc: dcheng@chromium.org creis@chromium.org
Owner: nasko@chromium.org
Status: Started
TL;DR: Nasko has been studying the repro and we've been discussing various ways to fix this bug.  There's one obvious oversight that could have disrupted this (which Nasko will fix), and then there's some longer term issues that remain.

---

The exploit is possible because the attacker's renderer process can send a message from a swapped out RenderFrameHost which makes it up into TabHelper.  TabHelper isn't keeping track of who sent the message and assumes it's from WebContents::GetRenderViewHost(), which refers to the current RenderViewHost (legitimately for the extension process) and not the swapped out one (in the attacker's process).

Multiple things are wrong here:

1) A message from a swapped out RFH is making it up to TabHelper.  Swapped out hosts shouldn't be propagating their IPC messages up to observers.  We actually have a check for that in RenderViewHostImpl::OnMessageReceived, where we filter out such IPC messages using CanHandleWhileSwappedOut.  We're missing that check in RenderFrameHostImpl, which is how this snuck through.  Nasko will fix that.

2) TabHelper doesn't know who sent it the message, since that information is not available in OnMessageReceived.  In theory, it would be nice for it not to have to worry about that, since it's easy to miss an access control check.  Fixing (1) means we don't have to worry about messages from swapped out RFHs, for example.  Unfortunately, the problem still exists with current vs pending RFHs.

More concretely, a WebContents can have both a current and a pending RFH at the same time (e.g., one in an attacker's process and one in an extension process).  Until the pending RFH actually commits, IPC messages could come up to WebContentsObservers from either one, and the observers just assume that they're hearing from the current RFH, not the pending one.  That could lead to the same kind of attack.

I'm not sure if it's possible to hide messages from the pending RFH until it commits, since it may need to initialize state (e.g., RenderFrameCreated) before the commit happens.  Queuing the messages up inside WebContents until commit might be possible, but that feels really error prone if it delays acks or other message exchanges.

Another option is to expose who sent the IPC message so that the observer can do an access control check.  Exposing "who sent it" could be in the form of RenderFrameHost, routing ID + process ID, SiteInstance, or some kind of security principal.  Longer term efforts like Mojo might help with that.  In the shorter term, it's less clear how to fix that without exposing concepts like pending RFHs to observers.

Anyway, Nasko is still investigating some of the other unknown aspects of the exploit, like why the sleeps matter.  We can post more as we learn it.
Comment 22 by nasko@chromium.org, Mar 21 2014
Labels: Merge-Requested
Status: Fixed
Now that a fix is in, we should merge it to beta and stable.
Labels: M-33 M-34
Just a fyi, we leave Merge-Requested labels for Sheriffbot to handle. so that it puts all the milestones and stuff.
FYI: This is r258521
Comment 25 by dxie@google.com, Mar 21 2014
Labels: -Merge-Requested Merge-Approved
approved for m34/1847.
Comment 26 by nasko@chromium.org, Mar 21 2014
Merged

$ drover --merge 258521 --branch 1847
Committed revision 258659.
Project Member Comment 27 by bugdroid1@chromium.org, Mar 23 2014
------------------------------------------------------------------
r258521 | nasko@chromium.org | 2014-03-21T11:21:39.145909Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/frame_host/render_frame_host_impl.cc?r1=258521&r2=258520&pathrev=258521

Add filtering of IPC messages when RenderFrameHost is swapped out.

BUG= 351815 

Review URL: https://codereview.chromium.org/205543002
-----------------------------------------------------------------
Project Member Comment 28 by bugdroid1@chromium.org, Mar 23 2014
Labels: -Merge-Approved merge-merged-1847
------------------------------------------------------------------
r258659 | nasko@chromium.org | 2014-03-21T20:24:29.627639Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1847/src/content/browser/frame_host/render_frame_host_impl.cc?r1=258659&r2=258658&pathrev=258659

Merge 258521 "Add filtering of IPC messages when RenderFrameHost..."

> Add filtering of IPC messages when RenderFrameHost is swapped out.
> 
> BUG= 351815 
> 
> Review URL: https://codereview.chromium.org/205543002

TBR=nasko@chromium.org

Review URL: https://codereview.chromium.org/199093004
-----------------------------------------------------------------
Project Member Comment 29 by bugdroid1@chromium.org, Mar 26 2014
------------------------------------------------------------------
r259466 | nasko@chromium.org | 2014-03-26T04:06:39.352221Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/frame_host/navigation_controller_impl_unittest.cc?r1=259466&r2=259465&pathrev=259466
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/renderer_host/render_view_host_impl.cc?r1=259466&r2=259465&pathrev=259466
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/frame_host/render_frame_host_manager_unittest.cc?r1=259466&r2=259465&pathrev=259466
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/frame_host/render_frame_host_manager.cc?r1=259466&r2=259465&pathrev=259466

Add test for filtering of IPC messages when RenderFrameHost is swapped out.

This is a followup CL to add test for the actual fix of filtering IPCs from swapped out RFHs. I've fixed the state transitions in RVH, since we had an illegal one and fixed up the unit tests to account for that.

BUG= 351815 

Review URL: https://codereview.chromium.org/196133032
-----------------------------------------------------------------
Labels: -M-33 Release-0-M34
Project Member Comment 31 by clusterf...@chromium.org, Jun 27 2014
Labels: -Restrict-View-SecurityTeam
Bulk update: removing view restriction from closed bugs.
Comment 32 by krisr@chromium.org, Jul 30 2014
Status: Verified
36 is now stable.
Project Member Comment 33 by sheriffbot@chromium.org, Mar 22 2016
Labels: -security_impact-beta
Labels: OS-All
Project Member Comment 35 by sheriffbot@chromium.org, Oct 1 2016
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
Project Member Comment 36 by sheriffbot@chromium.org, Oct 2 2016
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
Project Member Comment 37 by sheriffbot@chromium.org, Oct 2 2016
Labels: Restrict-View-SecurityNotify
Labels: allpublic
Project Member Comment 39 by sheriffbot@chromium.org, Oct 3 2016
Labels: -Restrict-View-SecurityNotify
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
Sign in to add a comment