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

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

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 14 matching lines @@
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "bindings/core/v8/BindingSecurity.h"
 
 #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"
 #include "core/frame/Settings.h"
 #include "core/html/HTMLFrameElementBase.h"
 #include "core/workers/MainThreadWorkletGlobalScope.h"
 #include "platform/weborigin/SecurityOrigin.h"
 
 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
   // origin, depending on the model being used to allocate Frames between
   // processes. See https://crbug.com/601629.
   if (!(accessingWindow && targetWindow && targetWindow->isLocalDOMWindow()))
     return false;
 
   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();
 
   return true;
 }
 
 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)
     exceptionState.throwSecurityError(
         targetWindow->sanitizedCrossDomainAccessErrorMessage(accessingWindow),
         targetWindow->crossDomainAccessErrorMessage(accessingWindow));
   return false;
 }
 
 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 &&
       reportingOption == BindingSecurity::ErrorReportOption::Report)
     accessingWindow->printErrorMessage(
         targetWindow->crossDomainAccessErrorMessage(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.

                                           const EventTarget* target,
                                           ExceptionState& exceptionState) {
   DCHECK(target);
   const DOMWindow* window = target->toDOMWindow();
   if (!window) {
     // We only need to check the access to Window objects which are
     // cross-origin accessible.  If it's not a Window, the object's
     // 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,
                                           const Node* target,
                                           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,
                                           const Node* target,
                                           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,
                                                const DOMWindow* targetWindow) {
   const Frame* accessingFrame = accessingWindow->frame();
   DCHECK(accessingFrame);
   DCHECK(accessingFrame->securityContext());
   const SecurityOrigin* accessingOrigin =
       accessingFrame->securityContext()->getSecurityOrigin();
 
@@ skipping 10 matching lines @@
 
   // Note that there is no need to call back
   // FrameLoader::didAccessInitialDocument() because |targetWindow| must be
   // a child window inside iframe or frame and it doesn't have a URL bar,
   // so there is no need to worry about URL spoofing.
 
   return true;
 }
 
 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