Rework security checks to be based on Window rather than Frame.

third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp

Index: third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp b/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp
index 516ef4dff5498d95b703de67ff7f23e7350fcaca..852e8d1dd51213203d9e23738cdbec3c2357e9a2 100644
--- a/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp
@@ -32,7 +32,10 @@
 
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/V8Binding.h"
+#include "bindings/core/v8/V8Location.h"
+#include "bindings/core/v8/V8Window.h"
 #include "core/dom/Document.h"
+#include "core/frame/DOMWindow.h"
 #include "core/frame/LocalDOMWindow.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/Location.h"
@@ -46,10 +49,11 @@ namespace blink {
 namespace {
 
 bool canAccessFrameInternal(const LocalDOMWindow* accessingWindow,

[Yuki, 2017/03/06 08:42:43]

Should we rename these helper functions canAccessWindow{Internal,}?

[dcheng, 2017/03/07 05:48:09]

Done.

-                            const SecurityOrigin* targetFrameOrigin,

[dcheng, 2017/03/06 06:59:47]

We delay extraction of the security origin to canAccessFrameInternal. We want to
retrieve this from the SecurityContext, but RemoteDOMWindow doesn't have a
direct pointer--it must go via its back pointer to RemoteFrame. However, for
symmetry with the LocalFrame case, it's nice to not have code like this...

... since RemoteDOMWindow needs a special-case anyway, it's OK to just early
return on the special-case and downcast to Document to get the SecurityOrigin on
line 67

                             const DOMWindow* targetWindow) {
+#if 0
   SECURITY_CHECK(!(targetWindow && targetWindow->frame()) ||
                  targetWindow == targetWindow->frame()->domWindow());
+#endif

[dcheng, 2017/03/06 06:59:47]

I guess we can just delete this assert, since it's no longer based on frame at
all. I'm curious if anyone has thoughts for equivalents?

[Yuki, 2017/03/06 08:42:43]

This test is still effective as is, I think.  The test is
"If the window is attached to a frame, the frame's window must be the window."

This doesn't affect to detached windows.  So we can simply keep this test as is,
I think?

[dcheng, 2017/03/07 05:48:09]

Done.

 
   // It's important to check that targetWindow is a LocalDOMWindow: it's
   // possible for a remote frame and local frame to have the same security
@@ -60,11 +64,14 @@ bool canAccessFrameInternal(const LocalDOMWindow* accessingWindow,
 
   const SecurityOrigin* accessingOrigin =
       accessingWindow->document()->getSecurityOrigin();
-  if (!accessingOrigin->canAccessCheckSuborigins(targetFrameOrigin))
+  const LocalDOMWindow* localTargetWindow = toLocalDOMWindow(targetWindow);
+  if (!accessingOrigin->canAccessCheckSuborigins(
+          localTargetWindow->document()->getSecurityOrigin())) {
     return false;
+  }
 
   // Notify the loader's client if the initial document has been accessed.
-  LocalFrame* targetFrame = toLocalDOMWindow(targetWindow)->frame();
+  LocalFrame* targetFrame = localTargetWindow->frame();
   if (targetFrame &&
       targetFrame->loader().stateMachine()->isDisplayingInitialEmptyDocument())
     targetFrame->loader().didAccessInitialDocument();
@@ -73,10 +80,9 @@ bool canAccessFrameInternal(const LocalDOMWindow* accessingWindow,
 }
 
 bool canAccessFrame(const LocalDOMWindow* accessingWindow,
-                    const SecurityOrigin* targetFrameOrigin,
                     const DOMWindow* targetWindow,
                     ExceptionState& exceptionState) {
-  if (canAccessFrameInternal(accessingWindow, targetFrameOrigin, targetWindow))
+  if (canAccessFrameInternal(accessingWindow, targetWindow))
     return true;
 
   if (targetWindow)
@@ -87,10 +93,9 @@ bool canAccessFrame(const LocalDOMWindow* accessingWindow,
 }
 
 bool canAccessFrame(const LocalDOMWindow* accessingWindow,
-                    SecurityOrigin* targetFrameOrigin,
                     const DOMWindow* targetWindow,
                     BindingSecurity::ErrorReportOption reportingOption) {
-  if (canAccessFrameInternal(accessingWindow, targetFrameOrigin, targetWindow))
+  if (canAccessFrameInternal(accessingWindow, targetWindow))
     return true;
 
   if (accessingWindow && targetWindow &&
@@ -100,30 +105,39 @@ bool canAccessFrame(const LocalDOMWindow* accessingWindow,
   return false;
 }
 
+DOMWindow* findWindow(v8::Isolate* isolate,
+                      const WrapperTypeInfo* type,
+                      v8::Local<v8::Object> host) {
+  if (V8Window::wrapperTypeInfo.equals(type)) {
+    v8::Local<v8::Object> windowWrapper =

[Yuki, 2017/03/06 08:42:43]

I think we should use |host| directly (as same as Location).
IIUC, we don't need to use findInstanceInPrototypeChain even when |host| is the
global proxy.

[dcheng, 2017/03/07 05:48:10]

Ah good point, this is leftover from when we didn't have the internal fields set
on the global proxy.

+        V8Window::findInstanceInPrototypeChain(host, isolate);
+    if (windowWrapper.IsEmpty())
+      return 0;

[Yuki, 2017/03/06 08:42:42]

The call site seems not expecting findWindow to return nullptr at all.
Should this be CHECK(!windowWrapper.IsEmpty()) ?

[Yuki, 2017/03/06 08:42:43]

nit: s/0/nullptr/

[dcheng, 2017/03/07 05:48:09]

Done.

[dcheng, 2017/03/07 05:48:09]

Oops, good point. I added a CHECK() in failedAccesCheckFor(), to indicate that
we should never get there (since failing to throw an exception may lead to
security issues)

+    return V8Window::toImpl(windowWrapper);
+  }
+
+  if (V8Location::wrapperTypeInfo.equals(type))
+    return V8Location::toImpl(host)->domWindow();
+
+  // This function can handle only those types listed above.
+  CHECK(false);

[Yuki, 2017/03/06 08:42:43]

NOTREACHED?

[dcheng, 2017/03/07 05:48:09]

Hmm, I guess that's fine here. Done.

+  return nullptr;
+}
+
 }  // namespace
 
 bool BindingSecurity::shouldAllowAccessTo(const LocalDOMWindow* accessingWindow,
                                           const DOMWindow* target,
                                           ExceptionState& exceptionState) {
   DCHECK(target);
-  const Frame* frame = target->frame();
-  if (!frame || !frame->securityContext())
-    return false;
-  return canAccessFrame(accessingWindow,
-                        frame->securityContext()->getSecurityOrigin(), target,
-                        exceptionState);
+  return canAccessFrame(accessingWindow, target, exceptionState);
 }
 
 bool BindingSecurity::shouldAllowAccessTo(const LocalDOMWindow* accessingWindow,
                                           const DOMWindow* target,
                                           ErrorReportOption reportingOption) {
   DCHECK(target);
-  const Frame* frame = target->frame();
-  if (!frame || !frame->securityContext())
-    return false;
-  return canAccessFrame(accessingWindow,
-                        frame->securityContext()->getSecurityOrigin(), target,
-                        reportingOption);
+  return canAccessFrame(accessingWindow, target, reportingOption);
 }
 
 bool BindingSecurity::shouldAllowAccessTo(const LocalDOMWindow* accessingWindow,

[Yuki, 2017/03/06 08:42:43]

Another CL of yours removed the use of shouldAllowAccessTo for EventTarget* from
V8EventTarget.cpp.
We no longer need this version, I think.

[dcheng, 2017/03/07 05:48:09]

Done.

@@ -137,36 +151,21 @@ bool BindingSecurity::shouldAllowAccessTo(const LocalDOMWindow* accessingWindow,
     // origin must always be the same origin (or it already leaked).
     return true;
   }
-  const Frame* frame = window->frame();
-  if (!frame || !frame->securityContext())
-    return false;
-  return canAccessFrame(accessingWindow,
-                        frame->securityContext()->getSecurityOrigin(), window,
-                        exceptionState);
+  return canAccessFrame(accessingWindow, window, exceptionState);
 }
 
 bool BindingSecurity::shouldAllowAccessTo(const LocalDOMWindow* accessingWindow,
                                           const Location* target,
                                           ExceptionState& exceptionState) {
   DCHECK(target);
-  const Frame* frame = target->frame();
-  if (!frame || !frame->securityContext())
-    return false;
-  return canAccessFrame(accessingWindow,
-                        frame->securityContext()->getSecurityOrigin(),
-                        frame->domWindow(), exceptionState);
+  return canAccessFrame(accessingWindow, target->domWindow(), exceptionState);
 }
 
 bool BindingSecurity::shouldAllowAccessTo(const LocalDOMWindow* accessingWindow,
                                           const Location* target,
                                           ErrorReportOption reportingOption) {
   DCHECK(target);
-  const Frame* frame = target->frame();
-  if (!frame || !frame->securityContext())
-    return false;
-  return canAccessFrame(accessingWindow,
-                        frame->securityContext()->getSecurityOrigin(),
-                        frame->domWindow(), reportingOption);
+  return canAccessFrame(accessingWindow, target->domWindow(), reportingOption);
 }
 
 bool BindingSecurity::shouldAllowAccessTo(const LocalDOMWindow* accessingWindow,
@@ -174,8 +173,8 @@ bool BindingSecurity::shouldAllowAccessTo(const LocalDOMWindow* accessingWindow,
                                           ExceptionState& exceptionState) {
   if (!target)
     return false;
-  return canAccessFrame(accessingWindow, target->document().getSecurityOrigin(),
-                        target->document().domWindow(), exceptionState);
+  return canAccessFrame(accessingWindow, target->document().domWindow(),
+                        exceptionState);
 }
 
 bool BindingSecurity::shouldAllowAccessTo(const LocalDOMWindow* accessingWindow,
@@ -183,46 +182,22 @@ bool BindingSecurity::shouldAllowAccessTo(const LocalDOMWindow* accessingWindow,
                                           ErrorReportOption reportingOption) {
   if (!target)
     return false;
-  return canAccessFrame(accessingWindow, target->document().getSecurityOrigin(),
-                        target->document().domWindow(), reportingOption);
+  return canAccessFrame(accessingWindow, target->document().domWindow(),
+                        reportingOption);
 }
 
 bool BindingSecurity::shouldAllowAccessToFrame(
     const LocalDOMWindow* accessingWindow,
-    const Frame* target,
+    const Frame& target,
     ExceptionState& exceptionState) {
-  if (!target || !target->securityContext())
-    return false;
-  return canAccessFrame(accessingWindow,
-                        target->securityContext()->getSecurityOrigin(),
-                        target->domWindow(), exceptionState);
+  return canAccessFrame(accessingWindow, target.domWindow(), exceptionState);
 }
 
 bool BindingSecurity::shouldAllowAccessToFrame(
     const LocalDOMWindow* accessingWindow,
-    const Frame* target,
+    const Frame& target,
     ErrorReportOption reportingOption) {
-  if (!target || !target->securityContext())
-    return false;
-  return canAccessFrame(accessingWindow,
-                        target->securityContext()->getSecurityOrigin(),
-                        target->domWindow(), reportingOption);
-}
-
-bool BindingSecurity::shouldAllowAccessToDetachedWindow(
-    const LocalDOMWindow* accessingWindow,
-    const DOMWindow* target,
-    ExceptionState& exceptionState) {
-  CHECK(target && !target->frame())
-      << "This version of shouldAllowAccessToFrame() must be used only for "
-      << "detached windows.";
-  if (!target->isLocalDOMWindow())
-    return false;
-  Document* document = toLocalDOMWindow(target)->document();
-  if (!document)
-    return false;
-  return canAccessFrame(accessingWindow, document->getSecurityOrigin(), target,
-                        exceptionState);
+  return canAccessFrame(accessingWindow, target.domWindow(), reportingOption);
 }
 
 bool BindingSecurity::shouldAllowNamedAccessTo(const DOMWindow* accessingWindow,
@@ -253,22 +228,17 @@ bool BindingSecurity::shouldAllowNamedAccessTo(const DOMWindow* accessingWindow,
 }
 
 void BindingSecurity::failedAccessCheckFor(v8::Isolate* isolate,
-                                           const Frame* target) {
-  // TODO(dcheng): See if this null check can be removed or hoisted to a
-  // different location.
-  if (!target)
-    return;
-
-  DOMWindow* targetWindow = target->domWindow();
+                                           const WrapperTypeInfo* type,
+                                           v8::Local<v8::Object> host) {
+  DOMWindow* target = findWindow(isolate, type, host);
 
   // TODO(dcheng): Add ContextType, interface name, and property name as
   // arguments, so the generated exception can be more descriptive.
   ExceptionState exceptionState(isolate, ExceptionState::UnknownContext,
                                 nullptr, nullptr);
   exceptionState.throwSecurityError(
-      targetWindow->sanitizedCrossDomainAccessErrorMessage(
-          currentDOMWindow(isolate)),
-      targetWindow->crossDomainAccessErrorMessage(currentDOMWindow(isolate)));
+      target->sanitizedCrossDomainAccessErrorMessage(currentDOMWindow(isolate)),
+      target->crossDomainAccessErrorMessage(currentDOMWindow(isolate)));
 }
 
 }  // namespace blink