Changeset 245538 in webkit

Timestamp:

May 20, 2019, 3:53:03 PM (7 years ago)

Author:

Chris Dumez

Message:

Fix security check in ScriptController::canAccessFromCurrentOrigin()
​https://bugs.webkit.org/show_bug.cgi?id=196730
<rdar://problem/49731231>

Reviewed by Ryosuke Niwa.

Source/WebCore:

Fix security check in ScriptController::canAccessFromCurrentOrigin() when there is no
current JS exec state. Instead of returning true unconditionally, we now fall back to
using the accessing document's origin for the security check. The new behavior is
aligned with Blink:
​https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/html/html_frame_element_base.cc?rcl=d3f22423d512b45466f1694020e20da9e0c6ee6a&l=62

This fix is based on a patch from Sergei Glazunov <​glazunov@google.com>.

Test: http/tests/security/showModalDialog-sync-cross-origin-page-load2.html

• bindings/js/ScriptController.cpp:

(WebCore::ScriptController::canAccessFromCurrentOrigin):

• bindings/js/ScriptController.h:
• html/HTMLFrameElementBase.cpp:

(WebCore::HTMLFrameElementBase::isURLAllowed const):

LayoutTests:

Add layout test coverage.

• http/tests/security/showModalDialog-sync-cross-origin-page-load2-expected.txt: Added.
• http/tests/security/showModalDialog-sync-cross-origin-page-load2.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/security/showModalDialog-sync-cross-origin-page-load2-expected.txt (added)
• LayoutTests/http/tests/security/showModalDialog-sync-cross-origin-page-load2.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/bindings/js/ScriptController.cpp (modified) (1 diff)
• Source/WebCore/bindings/js/ScriptController.h (modified) (1 diff)
• Source/WebCore/html/HTMLFrameElementBase.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-05-20  Chris Dumez  <cdumez@apple.com>
+
+        Fix security check in ScriptController::canAccessFromCurrentOrigin()
+        https://bugs.webkit.org/show_bug.cgi?id=196730
+        <rdar://problem/49731231>
+
+        Reviewed by Ryosuke Niwa.
+
+        Add layout test coverage.
+
+        * http/tests/security/showModalDialog-sync-cross-origin-page-load2-expected.txt: Added.
+        * http/tests/security/showModalDialog-sync-cross-origin-page-load2.html: Added.
+
 2019-05-20  Gabe Giosia  <giosia@google.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-05-20  Chris Dumez  <cdumez@apple.com>
+
+        Fix security check in ScriptController::canAccessFromCurrentOrigin()
+        https://bugs.webkit.org/show_bug.cgi?id=196730
+        <rdar://problem/49731231>
+
+        Reviewed by Ryosuke Niwa.
+
+        Fix security check in ScriptController::canAccessFromCurrentOrigin() when there is no
+        current JS exec state. Instead of returning true unconditionally, we now fall back to
+        using the accessing document's origin for the security check. The new behavior is
+        aligned with Blink:
+        https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/html/html_frame_element_base.cc?rcl=d3f22423d512b45466f1694020e20da9e0c6ee6a&l=62
+
+        This fix is based on a patch from Sergei Glazunov <glazunov@google.com>.
+
+        Test: http/tests/security/showModalDialog-sync-cross-origin-page-load2.html
+
+        * bindings/js/ScriptController.cpp:
+        (WebCore::ScriptController::canAccessFromCurrentOrigin):
+        * bindings/js/ScriptController.h:
+        * html/HTMLFrameElementBase.cpp:
+        (WebCore::HTMLFrameElementBase::isURLAllowed const):
+
 2019-05-20  Gabe Giosia  <giosia@google.com>
 

trunk/Source/WebCore/bindings/js/ScriptController.cpp

@@ -384 +384 @@
 }
 
-bool ScriptController::canAccessFromCurrentOrigin(Frame* frame)
+bool ScriptController::canAccessFromCurrentOrigin(Frame* frame, Document& accessingDocument)
 {
     auto* state = JSExecState::currentState();
 
-    // If the current state is null we're in a call path where the DOM security check doesn't apply (eg. parser).
-    if (!state)
-        return true;
+    // If the current state is null we should use the accessing document for the security check.
+    if (!state) {
+        auto* targetDocument = frame ? frame->document() : nullptr;
+        return targetDocument && accessingDocument.securityOrigin().canAccess(targetDocument->securityOrigin());
+    }
 
     return BindingSecurity::shouldAllowAccessToFrame(state, frame);

trunk/Source/WebCore/bindings/js/ScriptController.h

@@ -124 +124 @@
     void disableWebAssembly(const String& errorMessage);
 
-    static bool canAccessFromCurrentOrigin(Frame*);
+    static bool canAccessFromCurrentOrigin(Frame*, Document& accessingDocument);
     WEBCORE_EXPORT bool canExecuteScripts(ReasonForCallingCanExecuteScripts);
 

trunk/Source/WebCore/html/HTMLFrameElementBase.cpp

@@ -74 +74 @@
     if (WTF::protocolIsJavaScript(completeURL)) {
         RefPtr<Document> contentDoc = this->contentDocument();
-        if (contentDoc && !ScriptController::canAccessFromCurrentOrigin(contentDoc->frame()))
+        if (contentDoc && !ScriptController::canAccessFromCurrentOrigin(contentDoc->frame(), document()))
             return false;
     }