Re-check existence of GuestViewContainer before requesting attach.

extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc

Index: extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
diff --git a/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc b/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
index bec7d1c852016119b6a3df2e7c81ffbda0047867..c060abad4b0c20bb272ab968a4a749c9924fc434 100644
--- a/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
+++ b/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
@@ -166,6 +166,20 @@ void GuestViewInternalCustomBindings::AttachGuest(
           args.Length() == 4 ? args[3].As<v8::Function>()
                              : v8::Local<v8::Function>(),
           args.GetIsolate()));
+
+  // We should be careful that some malicious JS in the GuestView's embedder
+  // hasn't destroyed |guest_view_container| during the enumeration of the
+  // properties of the guest's object during extraction of |params| above
+  // (see https://crbug.com/683523).
+  // TODO(wjmaclean): Evaluate if it's possible for malicious JS to *change*
+  // the |guest_view_container| ... we guard against that below, but a simple
+  // null check might suffice.
+  // TODO(wjmaclean): Evaluate if it's worth removing the early-out above
+  // and just do it here.
+  if (guest_view_container !=
+      guest_view::GuestViewContainer::FromID(element_instance_id)) {

[lfg, 2017/02/16 20:00:58]

GuestViewContainer already has a weak ptr factory, can you use that instead? It
will be safer than comparing pointers like this.

Also, I think this can be moved above just after evaluating javascript for the
params, so we don't need to create the request.

[wjmaclean, 2017/02/16 21:26:16]

Done.

+    return;
+  }
   guest_view_container->IssueRequest(std::move(request));
 
   args.GetReturnValue().Set(v8::Boolean::New(context()->isolate(), true));
@@ -235,9 +249,17 @@ void GuestViewInternalCustomBindings::AttachIframeGuest(
   params->SetBoolean(guest_view::kElementSizeIsLogical, true);
 
   content::RenderFrame* render_frame = GetRenderFrame(args[3]);
+  // It's possible that code executed during the |params| enumeration above

[lfg, 2017/02/16 20:00:58]

Can we use the same approach as above: Get the RenderFrame before executing user
code, grabbing a weak ptr to it, then executing the user code and checking the
weak ptr after?

[wjmaclean, 2017/02/16 21:26:16]

How about if we grab the parent_frame WebLocalFrame* and hang on to that. It's
use later is just as a lookup token.

[lfg, 2017/02/16 22:09:14]

I think this is fine, the WebFrame may be closed while executing javascript, but
oilpan won't delete it immediatelly.

+dcheng@ to confirm this.

+  // could delete the guest view's RenderFrame, so we shouldn't blindly assume
+  // it still exists.
+  if (!render_frame)
+    return;
+
   blink::WebLocalFrame* frame = render_frame->GetWebFrame();
 
   // Parent must exist.
+  // TODO(wjmaclean): Reviewers ... can you confirm that as long as the
+  // GuestView object's RenderFrame exists, that it will have a local parent?

[lfg, 2017/02/16 20:00:57]

The frame comes from getIframeContentWindow in javascript. Even if the parent
could be null, I think it will be just a crash, not a UaF.

[wjmaclean, 2017/02/16 21:26:16]

Acknowledged.

   blink::WebFrame* parent_frame = frame->parent();
   DCHECK(parent_frame);
   DCHECK(parent_frame->isWebLocalFrame());