Issue 351815: Pwnium: Extension system allows compromised renderer access to crosh

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.

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.

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?

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?

I'm attaching geohot's shellcode.

You should be able to understand the IPC from there.

Deleted: shellcode.c 8.1 KB

shellcode.c 8.1 KB Download

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.

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

(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.)

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.

Deleted: b351815.diff 3.9 KB

b351815.diff 3.9 KB Download

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.

Now that a fix is in, we should merge it to beta and stable.

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

approved for m34/1847.

Merged

$ drover --merge 258521 --branch 1847
Committed revision 258659.

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

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

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

Bulk update: removing view restriction from closed bugs.

36 is now stable.

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

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

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