From 04cbd3dca1a6f76e9f6b2ff8ea8091d5e53f4470 Mon Sep 17 00:00:00 2001 From: "nasko@chromium.org" Date: Wed, 4 Dec 2013 04:58:20 +0000 Subject: [PATCH] Prevent the browser process from creating duplicate RenderViewHosts. BUG=312016 Review URL: https://codereview.chromium.org/92873004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@238575 0039d316-1c4b-4281-b951-d872f2087c98 --- .../browser/frame_host/interstitial_page_impl.cc | 1 + .../browser/frame_host/interstitial_page_impl.h | 1 + .../renderer_host/render_view_host_delegate.h | 6 +- .../browser/renderer_host/render_view_host_impl.cc | 5 +- .../renderer_host/render_widget_host_impl.cc | 7 +- content/browser/security_exploit_browsertest.cc | 94 ++++++++++++++++++++++ content/browser/web_contents/web_contents_impl.cc | 22 +++++ content/browser/web_contents/web_contents_impl.h | 1 + content/test/test_web_contents.cc | 1 + content/test/test_web_contents.h | 1 + 10 files changed, 132 insertions(+), 7 deletions(-) diff --git a/content/browser/frame_host/interstitial_page_impl.cc b/content/browser/frame_host/interstitial_page_impl.cc index 27cff1fb2914..1e65d2213f45 100644 --- a/content/browser/frame_host/interstitial_page_impl.cc +++ b/content/browser/frame_host/interstitial_page_impl.cc @@ -697,6 +697,7 @@ gfx::Rect InterstitialPageImpl::GetRootWindowResizerRect() const { } void InterstitialPageImpl::CreateNewWindow( + int render_process_id, int route_id, int main_frame_route_id, const ViewHostMsg_CreateWindow_Params& params, diff --git a/content/browser/frame_host/interstitial_page_impl.h b/content/browser/frame_host/interstitial_page_impl.h index 07695b2eab3a..81d2c7ccecb1 100644 --- a/content/browser/frame_host/interstitial_page_impl.h +++ b/content/browser/frame_host/interstitial_page_impl.h @@ -119,6 +119,7 @@ class CONTENT_EXPORT InterstitialPageImpl virtual WebPreferences GetWebkitPrefs() OVERRIDE; virtual gfx::Rect GetRootWindowResizerRect() const OVERRIDE; virtual void CreateNewWindow( + int render_process_id, int route_id, int main_frame_route_id, const ViewHostMsg_CreateWindow_Params& params, diff --git a/content/browser/renderer_host/render_view_host_delegate.h b/content/browser/renderer_host/render_view_host_delegate.h index e5fa672d45e0..50b9d44de76b 100644 --- a/content/browser/renderer_host/render_view_host_delegate.h +++ b/content/browser/renderer_host/render_view_host_delegate.h @@ -382,8 +382,9 @@ class CONTENT_EXPORT RenderViewHostDelegate { virtual void LostMouseLock() {} // The page is trying to open a new page (e.g. a popup window). The window - // should be created associated with the given route, but it should not be - // shown yet. That should happen in response to ShowCreatedWindow. + // should be created associated with the given |route_id| in process + // |render_process_id|, but it should not be shown yet. That should happen in + // response to ShowCreatedWindow. // |params.window_container_type| describes the type of RenderViewHost // container that is requested -- in particular, the window.open call may // have specified 'background' and 'persistent' in the feature string. @@ -394,6 +395,7 @@ class CONTENT_EXPORT RenderViewHostDelegate { // Note: this is not called "CreateWindow" because that will clash with // the Windows function which is actually a #define. virtual void CreateNewWindow( + int render_process_id, int route_id, int main_frame_route_id, const ViewHostMsg_CreateWindow_Params& params, diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc index 63827068ba66..9ce81997ec5e 100644 --- a/content/browser/renderer_host/render_view_host_impl.cc +++ b/content/browser/renderer_host/render_view_host_impl.cc @@ -1323,8 +1323,9 @@ void RenderViewHostImpl::CreateNewWindow( FilterURL(policy, GetProcess(), true, &validated_params.opener_security_origin); - delegate_->CreateNewWindow(route_id, main_frame_route_id, - validated_params, session_storage_namespace); + delegate_->CreateNewWindow( + GetProcess()->GetID(), route_id, main_frame_route_id, validated_params, + session_storage_namespace); } void RenderViewHostImpl::CreateNewWidget(int route_id, diff --git a/content/browser/renderer_host/render_widget_host_impl.cc b/content/browser/renderer_host/render_widget_host_impl.cc index c792a6dfa703..28728c31fe72 100644 --- a/content/browser/renderer_host/render_widget_host_impl.cc +++ b/content/browser/renderer_host/render_widget_host_impl.cc @@ -212,9 +212,10 @@ RenderWidgetHostImpl::RenderWidgetHostImpl(RenderWidgetHostDelegate* delegate, is_threaded_compositing_enabled_ = IsThreadedCompositingEnabled(); - - g_routing_id_widget_map.Get().insert(std::make_pair( - RenderWidgetHostID(process->GetID(), routing_id_), this)); + std::pair result = + g_routing_id_widget_map.Get().insert(std::make_pair( + RenderWidgetHostID(process->GetID(), routing_id_), this)); + CHECK(result.second) << "Inserting a duplicate item!"; process_->AddRoute(routing_id_, this); // If we're initially visible, tell the process host that we're alive. diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc index 2eb7b51885b4..4bb84b1473dc 100644 --- a/content/browser/security_exploit_browsertest.cc +++ b/content/browser/security_exploit_browsertest.cc @@ -3,11 +3,19 @@ // found in the LICENSE file. #include "base/command_line.h" +#include "base/containers/hash_tables.h" +#include "content/browser/dom_storage/dom_storage_context_wrapper.h" +#include "content/browser/dom_storage/session_storage_namespace_impl.h" +#include "content/browser/renderer_host/render_view_host_factory.h" #include "content/browser/renderer_host/render_view_host_impl.h" #include "content/browser/web_contents/web_contents_impl.h" +#include "content/common/view_messages.h" +#include "content/public/browser/browser_context.h" #include "content/public/browser/notification_service.h" #include "content/public/browser/notification_types.h" +#include "content/public/browser/storage_partition.h" #include "content/public/common/content_switches.h" +#include "content/public/test/browser_test_utils.h" #include "content/public/test/test_utils.h" #include "content/shell/browser/shell.h" #include "content/test/content_browser_test.h" @@ -15,6 +23,61 @@ namespace content { +namespace { + +// This is a helper function for the tests which attempt to create a +// duplicate RenderViewHost or RenderWidgetHost. It tries to create two objects +// with the same process and routing ids, which causes a collision. +// It creates a couple of windows in process 1, which causes a few routing ids +// to be allocated. Then a cross-process navigation is initiated, which causes a +// new process 2 to be created and have a pending RenderViewHost for it. The +// routing id of the RenderViewHost which is target for a duplicate is set +// into |target_routing_id| and the pending RenderViewHost which is used for +// the attempt is the return value. +RenderViewHostImpl* PrepareToDuplicateHosts(Shell* shell, + int* target_routing_id) { + GURL foo("http://foo.com/files/simple_page.html"); + + // Start off with initial navigation, so we get the first process allocated. + NavigateToURL(shell, foo); + + // Open another window, so we generate some more routing ids. + ShellAddedObserver shell2_observer; + EXPECT_TRUE(ExecuteScript( + shell->web_contents(), "window.open(document.URL + '#2');")); + Shell* shell2 = shell2_observer.GetShell(); + + // The new window must be in the same process, but have a new routing id. + EXPECT_EQ(shell->web_contents()->GetRenderViewHost()->GetProcess()->GetID(), + shell2->web_contents()->GetRenderViewHost()->GetProcess()->GetID()); + *target_routing_id = + shell2->web_contents()->GetRenderViewHost()->GetRoutingID(); + EXPECT_NE(*target_routing_id, + shell->web_contents()->GetRenderViewHost()->GetRoutingID()); + + // Now, simulate a link click coming from the renderer. + GURL extension_url("https://bar.com/files/simple_page.html"); + WebContentsImpl* wc = static_cast(shell->web_contents()); + wc->RequestOpenURL( + shell->web_contents()->GetRenderViewHost(), extension_url, + Referrer(), CURRENT_TAB, wc->GetFrameTree()->root()->frame_id(), + false, true); + + // Since the navigation above requires a cross-process swap, there will be a + // pending RenderViewHost. Ensure it exists and is in a different process + // than the initial page. + RenderViewHostImpl* pending_rvh = + wc->GetRenderManagerForTesting()->pending_render_view_host(); + EXPECT_TRUE(pending_rvh != NULL); + EXPECT_NE(shell->web_contents()->GetRenderViewHost()->GetProcess()->GetID(), + pending_rvh->GetProcess()->GetID()); + + return pending_rvh; +} + +} // namespace + + // The goal of these tests will be to "simulate" exploited renderer processes, // which can send arbitrary IPC messages and confuse browser process internal // state, leading to security bugs. We are trying to verify that the browser @@ -52,4 +115,35 @@ IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest, SetWebUIProperty) { terminated.Wait(); } +// This is a test for crbug.com/312016 attempting to create duplicate +// RenderViewHosts. SetupForDuplicateHosts sets up this test case and leaves +// it in a state with pending RenderViewHost. Before the commit of the new +// pending RenderViewHost, this test case creates a new window through the new +// process. +IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest, + AttemptDuplicateRenderViewHost) { + int duplicate_routing_id = MSG_ROUTING_NONE; + RenderViewHostImpl* pending_rvh = + PrepareToDuplicateHosts(shell(), &duplicate_routing_id); + EXPECT_NE(MSG_ROUTING_NONE, duplicate_routing_id); + + // Since this test executes on the UI thread and hopping threads might cause + // different timing in the test, let's simulate a CreateNewWindow call coming + // from the IO thread. + ViewHostMsg_CreateWindow_Params params; + DOMStorageContextWrapper* dom_storage_context = + static_cast( + BrowserContext::GetStoragePartition( + shell()->web_contents()->GetBrowserContext(), + pending_rvh->GetSiteInstance())->GetDOMStorageContext()); + SessionStorageNamespaceImpl* session_storage = + new SessionStorageNamespaceImpl(dom_storage_context); + // Cause a deliberate collision in routing ids. + int main_frame_routing_id = duplicate_routing_id + 1; + pending_rvh->CreateNewWindow( + duplicate_routing_id, main_frame_routing_id, params, session_storage); + + // If the above operation doesn't cause a crash, the test has succeeded! } + +} // namespace content diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc index 7d04a604c1da..f5d932646cc7 100644 --- a/content/browser/web_contents/web_contents_impl.cc +++ b/content/browser/web_contents/web_contents_impl.cc @@ -12,6 +12,7 @@ #include "base/logging.h" #include "base/metrics/histogram.h" #include "base/metrics/stats_counters.h" +#include "base/process/process.h" #include "base/strings/string16.h" #include "base/strings/string_number_conversions.h" #include "base/strings/string_util.h" @@ -73,6 +74,7 @@ #include "content/public/common/content_constants.h" #include "content/public/common/content_switches.h" #include "content/public/common/page_zoom.h" +#include "content/public/common/result_codes.h" #include "content/public/common/url_constants.h" #include "net/base/mime_util.h" #include "net/base/net_util.h" @@ -1252,6 +1254,7 @@ void WebContentsImpl::LostMouseLock() { } void WebContentsImpl::CreateNewWindow( + int render_process_id, int route_id, int main_frame_route_id, const ViewHostMsg_CreateWindow_Params& params, @@ -1262,11 +1265,30 @@ void WebContentsImpl::CreateNewWindow( // SiteInstance in its own BrowsingInstance. bool is_guest = GetRenderProcessHost()->IsGuest(); + // If the opener is to be suppressed, the new window can be in any process. + // Since routing ids are process specific, we must not have one passed in + // as argument here. + DCHECK(!params.opener_suppressed || route_id == MSG_ROUTING_NONE); + scoped_refptr site_instance = params.opener_suppressed && !is_guest ? SiteInstance::CreateForURL(GetBrowserContext(), params.target_url) : GetSiteInstance(); + // A message to create a new window can only come from the active process for + // this WebContentsImpl instance. If any other process sends the request, + // it is invalid and the process must be terminated. + if (GetRenderProcessHost()->GetID() != render_process_id) { + base::ProcessHandle process_handle = + RenderProcessHost::FromID(render_process_id)->GetHandle(); + if (process_handle != base::kNullProcessHandle) { + RecordAction( + UserMetricsAction("Terminate_ProcessMismatch_CreateNewWindow")); + base::KillProcess(process_handle, content::RESULT_CODE_KILLED, false); + } + return; + } + // We must assign the SessionStorageNamespace before calling Init(). // // http://crbug.com/142685 diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h index d2c04839c402..df68db971a39 100644 --- a/content/browser/web_contents/web_contents_impl.h +++ b/content/browser/web_contents/web_contents_impl.h @@ -406,6 +406,7 @@ class CONTENT_EXPORT WebContentsImpl bool last_unlocked_by_target) OVERRIDE; virtual void LostMouseLock() OVERRIDE; virtual void CreateNewWindow( + int render_process_id, int route_id, int main_frame_route_id, const ViewHostMsg_CreateWindow_Params& params, diff --git a/content/test/test_web_contents.cc b/content/test/test_web_contents.cc index 33332bdd87e1..5dbdae34ef97 100644 --- a/content/test/test_web_contents.cc +++ b/content/test/test_web_contents.cc @@ -219,6 +219,7 @@ void TestWebContents::TestDidFailLoadWithError( } void TestWebContents::CreateNewWindow( + int render_process_id, int route_id, int main_frame_route_id, const ViewHostMsg_CreateWindow_Params& params, diff --git a/content/test/test_web_contents.h b/content/test/test_web_contents.h index 5f338c6c8bd6..fc6ee3d620d0 100644 --- a/content/test/test_web_contents.h +++ b/content/test/test_web_contents.h @@ -106,6 +106,7 @@ class TestWebContents : public WebContentsImpl, public WebContentsTester { private: // WebContentsImpl overrides virtual void CreateNewWindow( + int render_process_id, int route_id, int main_frame_route_id, const ViewHostMsg_CreateWindow_Params& params, -- 2.11.4.GIT