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

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 27cff1f..1e65d22 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 07695b2..81d2c7c 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 e5fa672..50b9d44 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 6382706..9ce8199 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 c792a6d..28728c3 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<RoutingIDWidgetMap::iterator, bool> 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 2eb7b51..4bb84b1 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<WebContentsImpl*>(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<DOMStorageContextWrapper*>(
+          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 7d04a60..f5d9326 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<SiteInstance> 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 d2c0483..df68db9 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 33332bd..5dbdae3 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 5f338c6..fc6ee3d 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,