Make window.open() IPCs be frame-based

content/browser/renderer_host/render_widget_helper.cc

Index: content/browser/renderer_host/render_widget_helper.cc
diff --git a/content/browser/renderer_host/render_widget_helper.cc b/content/browser/renderer_host/render_widget_helper.cc
index fce786a8ccae10e00b86b151a5d429a7b6b879c2..47feb6a5d5ab73096e8a0b1584cef9db3b6f9c87 100644
--- a/content/browser/renderer_host/render_widget_helper.cc
+++ b/content/browser/renderer_host/render_widget_helper.cc
@@ -11,6 +11,8 @@
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "content/browser/dom_storage/session_storage_namespace_impl.h"
+#include "content/browser/frame_host/frame_tree_node.h"
+#include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
@@ -90,8 +92,7 @@ void RenderWidgetHelper::OnResumeDeferredNavigation(
 void RenderWidgetHelper::CreateNewWindow(
     mojom::CreateNewWindowParamsPtr params,
     bool no_javascript_access,
-    base::ProcessHandle render_process,
-    int32_t* route_id,
+    int32_t* render_view_route_id,
     int32_t* main_frame_route_id,
     int32_t* main_frame_widget_route_id,
     SessionStorageNamespace* session_storage_namespace) {
@@ -100,17 +101,17 @@ void RenderWidgetHelper::CreateNewWindow(
     // open the window in a new BrowsingInstance, and thus a new process. That
     // means the current renderer process will not be able to route messages to
     // it. Because of this, we will immediately show and navigate the window
-    // in OnCreateWindowOnUI, using the params provided here.
-    *route_id = MSG_ROUTING_NONE;
+    // in OnCreateNewWindowOnUI, using the params provided here.
+    *render_view_route_id = MSG_ROUTING_NONE;
     *main_frame_route_id = MSG_ROUTING_NONE;
     *main_frame_widget_route_id = MSG_ROUTING_NONE;
   } else {
-    *route_id = GetNextRoutingID();
+    *render_view_route_id = GetNextRoutingID();
     *main_frame_route_id = GetNextRoutingID();
     // TODO(avi): When RenderViewHostImpl has-a RenderWidgetHostImpl, this
     // should be updated to give the widget a distinct routing ID.
     // https://crbug.com/545684
-    *main_frame_widget_route_id = *route_id;
+    *main_frame_widget_route_id = *render_view_route_id;
     // Block resource requests until the frame is created, since the HWND might
     // be needed if a response ends up creating a plugin. We'll only have a
     // single frame at this point. These requests will be resumed either in
@@ -121,24 +122,45 @@ void RenderWidgetHelper::CreateNewWindow(
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      base::Bind(&RenderWidgetHelper::OnCreateWindowOnUI, this,
-                 base::Passed(&params), *route_id, *main_frame_route_id,
-                 *main_frame_widget_route_id,
+      base::Bind(&RenderWidgetHelper::OnCreateNewWindowOnUI, this,
+                 base::Passed(&params), *render_view_route_id,
+                 *main_frame_route_id, *main_frame_widget_route_id,
                  base::RetainedRef(session_storage_namespace)));
 }
 
-void RenderWidgetHelper::OnCreateWindowOnUI(
+void RenderWidgetHelper::OnCreateNewWindowOnUI(
     mojom::CreateNewWindowParamsPtr params,
-    int32_t route_id,
+    int32_t render_view_route_id,
     int32_t main_frame_route_id,
     int32_t main_frame_widget_route_id,
     SessionStorageNamespace* session_storage_namespace) {
-  RenderViewHostImpl* host =
-      RenderViewHostImpl::FromID(render_process_id_, params->opener_id);
-  if (host) {
-    host->CreateNewWindow(route_id, main_frame_route_id,
-                          main_frame_widget_route_id, *params,
-                          session_storage_namespace);
+  RenderFrameHostImpl* opener = RenderFrameHostImpl::FromID(
+      render_process_id_, params->opener_render_frame_id);
+  if (opener && opener->IsRenderFrameLive() &&

[alexmos, 2016/12/13 18:41:40]

I suppose it makes sense to check whether the frame died right before opening a
new window.  But I'm kind of curious how you arrived at putting it in?  Was that
something that happened in practice or in a test?

[ncarter (slow), 2016/12/15 00:33:16]

I didn't write a test for this, but I do think it's possible in practice.
OnMessageReceived ignores messages from dead RFH's, it seemed to make sense to
do that here too for the forwarded message.

Ignoring messages from dead RFH's is a general defense against the attack
scenario "start with two tabs sharing a RPH, trigger a deliberate crash, get the
user to reload one tab, then take over the renderer exploited; the exploited
process sends messages to the second tab's (still-dead) RFH, causing invariant
violations or UaFs that help you take over the browser process"

[alexmos, 2016/12/15 18:36:48]

Acknowledged.

+      opener->frame_tree_node()->current_frame_host() == opener) {

[alexmos, 2016/12/13 18:41:40]

Similarly, can you use opener->is_active() here? This might also let you remove
the dependency on frame_tree_node.  Also, looks like you're also checking this
in RFHI::OnCreateNewWindow, so do we really need both checks?

[ncarter (slow), 2016/12/15 00:33:16]

Done.

+    opener->OnCreateNewWindow(render_view_route_id, main_frame_route_id,
+                              main_frame_widget_route_id, *params,
+                              session_storage_namespace);
+  }
+
+  // If we did not create a WebContents to house the renderer-created
+  // RenderFrame/RenderView/RenderWidget; we must send a message to destroy
+  // those objects.
+  RenderProcessHost* rph = RenderProcessHost::FromID(render_process_id_);
+  if (main_frame_route_id != MSG_ROUTING_NONE && rph != nullptr) {

[alexmos, 2016/12/13 18:41:40]

Why do we need to look up and null-check the rph, rather than using
render_process_id_ directly below?

[ncarter (slow), 2016/12/15 00:33:16]

Looking up the rph is nececessary for the rph->Send() below.

The RPH could be null because nothing stops a MessageFilter from outliving the
RPH that nominally owns it. Again, it's an exceedingly rare case.

[alexmos, 2016/12/15 18:36:48]

Acknowledged.

+    bool succeeded = RenderWidgetHostImpl::FromID(
+                         rph->GetID(), main_frame_widget_route_id) != nullptr;
+    if (!succeeded) {
+      DCHECK(!RenderFrameHostImpl::FromID(rph->GetID(), main_frame_route_id));
+      DCHECK(!RenderViewHostImpl::FromID(rph->GetID(), render_view_route_id));
+
+      rph->Send(new ViewMsg_Close(render_view_route_id));
+    } else {
+      // If a RenderWidgetHost was created, there should also be a
+      // RenderFrameHost and RenderViewHost.
+      DCHECK(RenderFrameHostImpl::FromID(rph->GetID(), main_frame_route_id));
+      DCHECK(RenderViewHostImpl::FromID(rph->GetID(), render_view_route_id));
+    }
   }

[alexmos, 2016/12/13 18:41:40]

Is it worth adding an else and DCHECKing that render_view_routing_id is also
MSG_ROUTING_NONE when main_frame_route_id is, and also mentioning how we get
there?  IIUC, this happens when the intention is to open the new window in a new
BI (such as when suppressing the opener), and in that case the browser and not
the renderer initiates creation of the corresponding RV/RF, so nothing to clean
up.

[ncarter (slow), 2016/12/15 00:33:16]

I added this DCHECK to the top of the WebContentsImpl function. For some reason
I thought it already existed.

 }