Re-check existence of GuestViewContainer before requesting attach.

extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/renderer/guest_view/guest_view_internal_custom_bindings.h"
 
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 148 matching lines @@
   // Add flag to |params| to indicate that the element size is specified in
   // logical units.
   params->SetBoolean(guest_view::kElementSizeIsLogical, true);
 
   std::unique_ptr<guest_view::GuestViewRequest> request(
       new guest_view::GuestViewAttachRequest(
           guest_view_container, guest_instance_id, std::move(params),
           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));
 }
 
 void GuestViewInternalCustomBindings::DetachGuest(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   // Allow for an optional callback parameter.
   CHECK(args.Length() >= 1 && args.Length() <= 2);
   // Element Instance ID.
@@ skipping 49 matching lines @@
         converter->FromV8Value(args[2], context()->v8_context()));
     params = base::DictionaryValue::From(std::move(params_as_value));
     CHECK(params);
   }
 
   // Add flag to |params| to indicate that the element size is specified in
   // logical units.
   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());
 
   content::RenderFrame* embedder_parent_frame =
       content::RenderFrame::FromWebFrame(parent_frame);
 
   // Create a GuestViewContainer if it does not exist.
   // An element instance ID uniquely identifies an IframeGuestViewContainer
   // within a RenderView.
@@ skipping 180 matching lines @@
   // EnterFullscreen() and do it directly rather than having a generic "run with
   // user gesture" function.
   blink::WebScopedUserGesture user_gesture(context()->web_frame());
   CHECK_EQ(args.Length(), 1);
   CHECK(args[0]->IsFunction());
   context()->SafeCallFunction(
       v8::Local<v8::Function>::Cast(args[0]), 0, nullptr);
 }
 
 }  // namespace extensions