Fix security check in ScriptController::canAccessFromCurrentOrigin()
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 20 May 2019 22:53:03 +0000 (22:53 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 20 May 2019 22:53:03 +0000 (22:53 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@245538 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/http/tests/security/showModalDialog-sync-cross-origin-page-load2-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/security/showModalDialog-sync-cross-origin-page-load2.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/ScriptController.cpp
Source/WebCore/bindings/js/ScriptController.h
Source/WebCore/html/HTMLFrameElementBase.cpp

index e634ee1..d725d10 100644 (file)
@@ -1,3 +1,16 @@
+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>
 
         Range getBoundingClientRect returning zero rect on simple text node with <br> before it
diff --git a/LayoutTests/http/tests/security/showModalDialog-sync-cross-origin-page-load2-expected.txt b/LayoutTests/http/tests/security/showModalDialog-sync-cross-origin-page-load2-expected.txt
new file mode 100644 (file)
index 0000000..e6856ee
--- /dev/null
@@ -0,0 +1,2 @@
+This test passes if it does not alert the fail.html's content when clicking the button.
+  
diff --git a/LayoutTests/http/tests/security/showModalDialog-sync-cross-origin-page-load2.html b/LayoutTests/http/tests/security/showModalDialog-sync-cross-origin-page-load2.html
new file mode 100644 (file)
index 0000000..4f9e825
--- /dev/null
@@ -0,0 +1,73 @@
+<!DOCTYPE html>
+<html>
+<body>
+<b>This test passes if it does not alert the fail.html's content when clicking the button.</b><br>
+<input id="testButton" type="button" value="Click me"></input>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.setCanOpenWindows();
+    testRunner.waitUntilDone();
+}
+
+let counter = 0;
+function run(event) {
+  ++counter;
+  if (counter == 2) {
+    event.target.src = "javascript:alert(document.documentElement.outerHTML)";
+  } else if (counter == 3) {
+    frame = event.target;
+
+    a = frame.contentDocument.createElement("a");
+    a.href = cache_frame.src;
+    a.click();
+
+    showModalDialog(URL.createObjectURL(new Blob([`
+      <script>
+        timeout = 0;
+        let intervalID = setInterval(() => {
+          try {
+            opener.frame.contentWindow.foo;
+            timeout++;
+            if (timeout == 200)
+                throw "";
+          } catch (e) {
+            clearInterval(intervalID);
+
+            window.close();
+            if (window.testRunner)
+              testRunner.abortModal();
+          }
+        }, 10);
+      </scr` + "ipt>"], {type: "text/html"})));
+
+      setTimeout(() => {
+        setTimeout(() => {
+          if (window.testRunner)
+            testRunner.notifyDone();
+        }, 0);
+      }, 0);
+  }
+}
+
+testButton.onclick = _ => {
+  frame = document.body.appendChild(document.createElement("iframe"));
+  frame.contentWindow.location = `javascript:'<b><p><iframe`
+      + ` onload="top.run(event)"></iframe></b></p>'`;
+}
+
+cache_frame = document.body.appendChild(document.createElement("iframe"));
+cache_frame.src = "http://localhost:8000/security/resources/fail.html";
+cache_frame.style.display = "none";
+
+onload = function() {
+    if (!window.internals)
+       return;
+
+    internals.withUserGesture(() => {
+        testButton.click();
+    });
+}
+</script>
+</body>
+</html>
index 2abed3a..670279f 100644 (file)
@@ -1,3 +1,27 @@
+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>
 
         Range getBoundingClientRect returning zero rect on simple text node with <br> before it
index 5aaa34b..5290561 100644 (file)
@@ -383,13 +383,15 @@ void ScriptController::disableWebAssembly(const String& errorMessage)
     jsWindowProxy->window()->setWebAssemblyEnabled(false, errorMessage);
 }
 
-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);
 }
index 81eb381..ecde7a0 100644 (file)
@@ -123,7 +123,7 @@ public:
     void disableEval(const String& errorMessage);
     void disableWebAssembly(const String& errorMessage);
 
-    static bool canAccessFromCurrentOrigin(Frame*);
+    static bool canAccessFromCurrentOrigin(Frame*, Document& accessingDocument);
     WEBCORE_EXPORT bool canExecuteScripts(ReasonForCallingCanExecuteScripts);
 
     void setPaused(bool b) { m_paused = b; }
index 3048b49..1526369 100644 (file)
@@ -73,7 +73,7 @@ bool HTMLFrameElementBase::isURLAllowed(const URL& completeURL) const
 
     if (WTF::protocolIsJavaScript(completeURL)) {
         RefPtr<Document> contentDoc = this->contentDocument();
-        if (contentDoc && !ScriptController::canAccessFromCurrentOrigin(contentDoc->frame()))
+        if (contentDoc && !ScriptController::canAccessFromCurrentOrigin(contentDoc->frame(), document()))
             return false;
     }