Issue metadata
Sign in to add a comment
|
|
||||||||||||||||||||||||
Issue descriptionThis 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) Mar 12 2014,
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. Mar 12 2014,
Mar 12 2014,
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. 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? Mar 12 2014,
Mar 12 2014,kalman: the initial renderer was compromised by a memory corruption bug. This bug was the key part of the sandbox escape. Mar 12 2014,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? Mar 12 2014,I'm attaching geohot's shellcode. You should be able to understand the IPC from there. Mar 12 2014,
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? Mar 12 2014,There are also 2-second sleeps (poll(NULL, 0, 2000);) after both OpenURL() calls, so it doesn't look like a race condition. Mar 12 2014,
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? Mar 13 2014,
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. Mar 14 2014,
Mar 14 2014,(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.) Mar 14 2014,
Mar 14 2014,
Yes, the build is already cut. This one can take its time. Mar 14 2014,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. Mar 14 2014,Attached. Mar 18 2014,
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. Mar 21 2014,
Now that a fix is in, we should merge it to beta and stable. Mar 21 2014,
Just a fyi, we leave Merge-Requested labels for Sheriffbot to handle. so that it puts all the milestones and stuff. Mar 21 2014,FYI: This is r258521 Mar 21 2014,
approved for m34/1847. Mar 21 2014,Merged $ drover --merge 258521 --branch 1847 Committed revision 258659. Mar 23 2014, Project Member------------------------------------------------------------------ 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 ----------------------------------------------------------------- Mar 23 2014, Project Member
------------------------------------------------------------------ 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 ----------------------------------------------------------------- Mar 26 2014, Project Member------------------------------------------------------------------ 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 ----------------------------------------------------------------- Mar 28 2014,
Jun 27 2014, Project Member
Bulk update: removing view restriction from closed bugs. Jul 30 2014,
36 is now stable. Mar 22 2016, Project Member
Jun 7 2016,
Oct 1 2016, Project MemberThis 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 Oct 2 2016, Project MemberThis 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 Oct 2 2016, Project Member
Oct 2 2016,
Oct 3 2016, Project Member
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 Apr 25 2018,
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
Comment 1 by infe...@chromium.org, Mar 12 2014