WebCore:
authorweinig@apple.com <weinig@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Nov 2007 00:15:21 +0000 (00:15 +0000)
committerweinig@apple.com <weinig@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Nov 2007 00:15:21 +0000 (00:15 +0000)
        Reviewed by Darin.

        Fix for <rdar://problem/5592988>
        - Enforce tighter restrictions on what frames in other domains
          can be navigated.

        Tests: http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html
               http/tests/security/frameNavigation/xss-ALLOWED-targeted-subframe-navigation-change.html

        * bindings/js/kjs_window.cpp:
        (KJS::Window::put):
        (KJS::Location::put):
        (KJS::LocationProtoFuncReplace::callAsFunction):
        (KJS::LocationProtoFuncAssign::callAsFunction):
        * loader/FrameLoader.cpp:
        (WebCore::FrameLoader::createWindow):
        (WebCore::FrameLoader::load):
        (WebCore::FrameLoader::shouldAllowNavigation): Move and update logic from canTarget().
        * loader/FrameLoader.h:
        * page/FrameTree.cpp:
        (WebCore::FrameTree::isDescendantOf): Make this O(1) in the case when both frames are not
        in the same page.

LayoutTests:

        Reviewed by Darin.

        Tests for <rdar://problem/5592988>

        - Update and add tests for new tighter restrictions on what frames in other domains
          can be navigated.

        * http/tests/security/cross-frame-access-location-expected.txt:
        * http/tests/security/frameNavigation: Added.
        * http/tests/security/frameNavigation/resources: Added.
        * http/tests/security/frameNavigation/resources/iframe-that-performs-parent-navigation.html: Added.
        * http/tests/security/frameNavigation/resources/iframe-with-inner-frame-on-foreign-domain.html: Added.
        * http/tests/security/frameNavigation/resources/navigation-changed-iframe.html: Added.
        * http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change-expected.txt: Added.
        * http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html: Added.
        * http/tests/security/frameNavigation/xss-ALLOWED-targeted-subframe-navigation-change-expected.txt: Added.
        * http/tests/security/frameNavigation/xss-ALLOWED-targeted-subframe-navigation-change.html: Added.

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/security/cross-frame-access-location-expected.txt
LayoutTests/http/tests/security/frameNavigation/resources/iframe-that-performs-parent-navigation.html [new file with mode: 0644]
LayoutTests/http/tests/security/frameNavigation/resources/iframe-with-inner-frame-on-foreign-domain.html [new file with mode: 0644]
LayoutTests/http/tests/security/frameNavigation/resources/navigation-changed-iframe.html [new file with mode: 0644]
LayoutTests/http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html [new file with mode: 0644]
LayoutTests/http/tests/security/frameNavigation/xss-ALLOWED-targeted-subframe-navigation-change-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/security/frameNavigation/xss-ALLOWED-targeted-subframe-navigation-change.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/bindings/js/kjs_window.cpp
WebCore/loader/FrameLoader.cpp
WebCore/loader/FrameLoader.h
WebCore/page/FrameTree.cpp

index e03b5108dd85d54b873de73b26ee1916537a4d41..ee9995ca06b57c78d2e6adbd4e21503aa32d7677 100644 (file)
@@ -1,3 +1,23 @@
+2007-11-26  Sam Weinig  <sam@webkit.org>
+
+        Reviewed by Darin.
+
+        Tests for <rdar://problem/5592988>
+
+        - Update and add tests for new tighter restrictions on what frames in other domains
+          can be navigated.
+
+        * http/tests/security/cross-frame-access-location-expected.txt:
+        * http/tests/security/frameNavigation: Added.
+        * http/tests/security/frameNavigation/resources: Added.
+        * http/tests/security/frameNavigation/resources/iframe-that-performs-parent-navigation.html: Added.
+        * http/tests/security/frameNavigation/resources/iframe-with-inner-frame-on-foreign-domain.html: Added.
+        * http/tests/security/frameNavigation/resources/navigation-changed-iframe.html: Added.
+        * http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change-expected.txt: Added.
+        * http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html: Added.
+        * http/tests/security/frameNavigation/xss-ALLOWED-targeted-subframe-navigation-change-expected.txt: Added.
+        * http/tests/security/frameNavigation/xss-ALLOWED-targeted-subframe-navigation-change.html: Added.
+
 2007-11-26  Dan Bernstein  <mitz@apple.com>
 
         Reviewed by Dave Hyatt.
index ad213ec3f63b784924c79ed87e0137e29f70510b..b7095c0baef2f06b5ca3da03b0d3bb60bfe2a89f 100644 (file)
@@ -58,8 +58,6 @@ CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http
 
 CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http://localhost:8000/security/resources/cross-frame-iframe-for-get-test.html from frame with URL http://127.0.0.1:8000/security/cross-frame-access-location.html. Domains, protocols and ports must match.
 
-CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http://localhost:8000/security/resources/cross-frame-iframe-for-get-test.html from frame with URL http://127.0.0.1:8000/security/cross-frame-access-location.html. Domains, protocols and ports must match.
-
 
 
 ----- tests for getting/setting window.location and its properties -----
diff --git a/LayoutTests/http/tests/security/frameNavigation/resources/iframe-that-performs-parent-navigation.html b/LayoutTests/http/tests/security/frameNavigation/resources/iframe-that-performs-parent-navigation.html
new file mode 100644 (file)
index 0000000..1180633
--- /dev/null
@@ -0,0 +1,22 @@
+<html>
+<head>
+    <script>
+        function loaded()
+        {
+            document.getElementsByTagName('h4')[0].innerHTML = document.domain;
+            // Allow the user to click the button during manuel runs.
+            if (window.layoutTestController)
+                performTest();
+        }
+
+        function performTest()
+        {
+            parent.location = "http://localhost:8000/security/frameNavigation/resources/navigation-changed-iframe.html";
+        }
+    </script>
+</head>
+<body onload="loaded();">
+    <h4>DOMAIN</h4>
+    <button onclick="performTest();">Perform Test</button>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/security/frameNavigation/resources/iframe-with-inner-frame-on-foreign-domain.html b/LayoutTests/http/tests/security/frameNavigation/resources/iframe-with-inner-frame-on-foreign-domain.html
new file mode 100644 (file)
index 0000000..18ce802
--- /dev/null
@@ -0,0 +1,14 @@
+<html>
+<head>
+    <script>
+        function loaded() 
+        {
+            document.getElementsByTagName('h4')[0].innerHTML = document.domain;
+        }
+    </script>
+</head>
+<body onload="loaded();">
+    <h4>DOMAIN</h4>
+    <iframe name="targetFrame" src="http://localhost:8000/security/resources/cross-frame-iframe.html"></iframe>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/security/frameNavigation/resources/navigation-changed-iframe.html b/LayoutTests/http/tests/security/frameNavigation/resources/navigation-changed-iframe.html
new file mode 100644 (file)
index 0000000..1abe9d9
--- /dev/null
@@ -0,0 +1,16 @@
+<html>
+<head>
+    <script>
+        function fireSentinel()
+        {
+            document.getElementsByTagName('h4')[0].innerHTML = document.domain;
+            if (window.layoutTestController)
+                layoutTestController.notifyDone();
+        }
+    </script>
+</head>
+<body onload="fireSentinel();">
+    <h4>DOMAIN</h4>
+    <p>PASSED: Navigation succeeded.</p>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change-expected.txt b/LayoutTests/http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change-expected.txt
new file mode 100644 (file)
index 0000000..9968bd1
--- /dev/null
@@ -0,0 +1,3 @@
+localhost
+
+PASSED: Navigation succeeded.
diff --git a/LayoutTests/http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html b/LayoutTests/http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html
new file mode 100644 (file)
index 0000000..49d7e57
--- /dev/null
@@ -0,0 +1,23 @@
+<html>
+<head>
+    <style>
+        iframe { width: 400px; height: 200px;}
+    </style>
+    <script>
+        if (window.layoutTestController) {
+            layoutTestController.dumpAsText();
+            layoutTestController.waitUntilDone();
+        }
+
+        function loaded()
+        {
+            document.getElementsByTagName('h4')[0].innerHTML = document.domain;
+        }
+    </script>
+</head>
+<body onload="loaded();">
+    <p>This tests that documents can navigate the location of any of it's parent-frames regardless of domain.</p>
+    <h4>DOMAIN</h4>
+    <iframe src="http://localhost:8000/security/frameNavigation/resources/iframe-that-performs-parent-navigation.html"></iframe>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/security/frameNavigation/xss-ALLOWED-targeted-subframe-navigation-change-expected.txt b/LayoutTests/http/tests/security/frameNavigation/xss-ALLOWED-targeted-subframe-navigation-change-expected.txt
new file mode 100644 (file)
index 0000000..06e8706
--- /dev/null
@@ -0,0 +1,19 @@
+This tests that documents can navigate the location of any of it's sub-frames regardless of domain.
+
+127.0.0.1
+
+  Perform Test
+
+--------
+Frame: '<!--framePath //<!--frame0-->-->'
+--------
+localhost
+
+
+
+--------
+Frame: 'targetFrame'
+--------
+localhost
+
+PASSED: Navigation succeeded.
diff --git a/LayoutTests/http/tests/security/frameNavigation/xss-ALLOWED-targeted-subframe-navigation-change.html b/LayoutTests/http/tests/security/frameNavigation/xss-ALLOWED-targeted-subframe-navigation-change.html
new file mode 100644 (file)
index 0000000..0c1bd62
--- /dev/null
@@ -0,0 +1,44 @@
+<html>
+<head>
+    <style>
+        iframe { width: 400px; height: 200px;}
+    </style>
+    <script>
+        if (window.layoutTestController) {
+            layoutTestController.dumpAsText();
+            layoutTestController.dumpChildFramesAsText();
+            layoutTestController.waitUntilDone();
+        }
+
+        function loaded()
+        {
+            document.getElementsByTagName('h4')[0].innerHTML = document.domain;
+
+            if (window.layoutTestController) {
+                setTimeout(waitForFlag, 1);
+                function waitForFlag() {
+                    if (!layoutTestController.globalFlag) {
+                        setTimeout(waitForFlag, 1);
+                        return;
+                    }
+
+                    layoutTestController.globalFlag = false;
+                    performTest();
+                }
+            }
+        }
+
+        function performTest()
+        {
+            var subFrame = window.frames[0];
+            subFrame.frames[0].location = "http://localhost:8000/security/frameNavigation/resources/navigation-changed-iframe.html";
+        }
+    </script>
+</head>
+<body onload="loaded();">
+    <p>This tests that documents can navigate the location of any of it's sub-frames regardless of domain.</p>
+    <h4>DOMAIN</h4>
+    <iframe src="http://localhost:8000/security/frameNavigation/resources/iframe-with-inner-frame-on-foreign-domain.html"></iframe>
+    <button onclick="performTest()">Perform Test</button>
+</body>
+</html>
index 2d05157c18c0b9548e789546da55a943ff2ea0fa..82e3728f0964bafc03517a46271eca0f9ed7f6ed 100644 (file)
@@ -1,3 +1,28 @@
+2007-11-26  Sam Weinig  <sam@webkit.org>
+
+        Reviewed by Darin.
+
+        Fix for <rdar://problem/5592988>
+        - Enforce tighter restrictions on what frames in other domains
+          can be navigated.
+
+        Tests: http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html
+               http/tests/security/frameNavigation/xss-ALLOWED-targeted-subframe-navigation-change.html
+
+        * bindings/js/kjs_window.cpp:
+        (KJS::Window::put):
+        (KJS::Location::put):
+        (KJS::LocationProtoFuncReplace::callAsFunction):
+        (KJS::LocationProtoFuncAssign::callAsFunction):
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::createWindow):
+        (WebCore::FrameLoader::load):
+        (WebCore::FrameLoader::shouldAllowNavigation): Move and update logic from canTarget().
+        * loader/FrameLoader.h:
+        * page/FrameTree.cpp:
+        (WebCore::FrameTree::isDescendantOf): Make this O(1) in the case when both frames are not
+        in the same page.
+
 2007-11-26  Steve Falkenburg  <sfalken@apple.com>
 
         Build fix.
index 919b2a72fa70e2f759b221d3352b06add8a1a2c2..16f0446948eb837755071adfad6e7c17657e3778 100644 (file)
@@ -731,6 +731,8 @@ void Window::put(ExecState* exec, const Identifier& propertyName, JSValue* value
     case Location_: {
       Frame* p = Window::retrieveActive(exec)->impl()->frame();
       if (p) {
+        if (!p->loader()->shouldAllowNavigation(impl()->frame()))
+          return;
         DeprecatedString dstUrl = p->loader()->completeURL(DeprecatedString(value->toString(exec))).url();
         if (!dstUrl.startsWith("javascript:", false) || isSafeScript(exec)) {
           bool userGesture = static_cast<ScriptInterpreter *>(exec->dynamicInterpreter())->wasRunByUserGesture();
@@ -1960,10 +1962,11 @@ void Location::put(ExecState *exec, const Identifier &p, JSValue *v, int attr)
       switch (entry->value.intValue) {
       case Href: {
           Frame* frame = Window::retrieveActive(exec)->impl()->frame();
-          if (frame)
-              url = frame->loader()->completeURL(str).url();
-          else
-              url = str;
+          if (!frame)
+              return;
+          if (!frame->loader()->shouldAllowNavigation(m_frame))
+              return;
+          url = frame->loader()->completeURL(str).url();
           break;
       } 
       case Hash: {
@@ -2022,9 +2025,11 @@ JSValue* LocationProtoFuncReplace::callAsFunction(ExecState* exec, JSObject* thi
     if (!frame)
         return jsUndefined();
 
-    DeprecatedString str = args[0]->toString(exec);
     Frame* p = Window::retrieveActive(exec)->impl()->frame();
-    if ( p ) {
+    if (p) {
+      if (!p->loader()->shouldAllowNavigation(frame))
+        return jsUndefined();
+      DeprecatedString str = args[0]->toString(exec);
       const Window* window = Window::retrieveWindow(frame);
       if (!str.startsWith("javascript:", false) || (window && window->isSafeScript(exec))) {
         bool userGesture = static_cast<ScriptInterpreter *>(exec->dynamicInterpreter())->wasRunByUserGesture();
@@ -2064,12 +2069,10 @@ JSValue* LocationProtoFuncAssign::callAsFunction(ExecState* exec, JSObject* this
     if (!frame)
         return jsUndefined();
 
-    Window* window = Window::retrieveWindow(frame);
-    if (!window->isSafeScript(exec))
-        return jsUndefined();
-
     Frame *p = Window::retrieveActive(exec)->impl()->frame();
     if (p) {
+        if (!p->loader()->shouldAllowNavigation(frame))
+          return jsUndefined();
         const Window *window = Window::retrieveWindow(frame);
         DeprecatedString dstUrl = p->loader()->completeURL(DeprecatedString(args[0]->toString(exec))).url();
         if (!dstUrl.startsWith("javascript:", false) || (window && window->isSafeScript(exec))) {
index d881f57042ff995ea1ceb29a66c70c7b6d3873f1..1c2a1ea54a9825a2e63e729a473fc8e7e28be2e4 100644 (file)
@@ -306,6 +306,8 @@ Frame* FrameLoader::createWindow(const FrameLoadRequest& request, const WindowFe
 
     if (!request.frameName().isEmpty() && request.frameName() != "_blank")
         if (Frame* frame = m_frame->tree()->find(request.frameName())) {
+            if (!shouldAllowNavigation(frame))
+                return 0;
             if (!request.resourceRequest().url().isEmpty())
                 frame->loader()->load(request, false, true, 0, 0, HashMap<String, String>());
             if (Page* page = frame->page())
@@ -1935,7 +1937,7 @@ void FrameLoader::load(const FrameLoadRequest& request, bool lockHistory, bool u
         referrer = String();
     
     Frame* targetFrame = m_frame->tree()->find(request.frameName());
-    if (!canTarget(targetFrame))
+    if (!shouldAllowNavigation(targetFrame))
         return;
         
     if (request.resourceRequest().httpMethod() != "POST") {
@@ -2304,35 +2306,56 @@ void FrameLoader::reload()
     load(loader.get(), FrameLoadTypeReload, 0);
 }
 
-bool FrameLoader::canTarget(Frame* target) const
+bool FrameLoader::shouldAllowNavigation(Frame* targetFrame) const
 {
-    // This function prevents this exploit:
+    // This function prevents these exploits:
     // <rdar://problem/3715785> multiple frame injection vulnerability reported by Secunia, affects almost all browsers
+    // http://bugs.webkit.org/show_bug.cgi?id=15936 Overly permissive frame navigation allows password theft
 
     // Allow if there is no specific target.
-    if (!target)
+    if (!targetFrame)
+        return true;
+    if (m_frame == targetFrame)
         return true;
 
-    // Allow navigation within the same page/frameset.
-    if (m_frame->page() == target->page())
+    // The navigation change is safe if the active frame is:
+    //   - in the same security domain (satisfies same-origin policy)
+    //   - the opener frame
+    //   - an ancestor or a descendant in frame tree hierarchy
+
+    // Same security domain case.
+    Document* activeDocument = m_frame->document();
+    ASSERT(activeDocument);
+    Document* targetDocument = targetFrame->document();
+    if (!targetDocument)
+        return true;
+    const SecurityOrigin& activeSecurityOrigin = activeDocument->securityOrigin();
+    const SecurityOrigin& targetSecurityOrigin = targetDocument->securityOrigin();
+    if (activeSecurityOrigin.canAccess(targetSecurityOrigin))
         return true;
 
-    // Allow if the request is made from a local file.
-    ASSERT(m_frame->document());
-    String domain = m_frame->document()->domain();
-    if (domain.isEmpty())
+    // Opener case.
+    if (m_frame == targetFrame->loader()->opener())
         return true;
-    
-    // Allow if target is an entire window (top level frame of a window).
-    Frame* parent = target->tree()->parent();
-    if (!parent)
+
+    // Ancestor or descendant case.
+    if (targetFrame->tree()->isDescendantOf(m_frame) || m_frame->tree()->isDescendantOf(targetFrame))
         return true;
+
+    if (!targetFrame->settings()->privateBrowsingEnabled()) {
+        // FIXME: this error message should contain more specifics of why the navigation change is not allowed.
+        String message = String::format("Unsafe JavaScript attempt to initiate a navigation change for frame with URL %s from frame with URL %s.\n",
+                                        targetDocument->URL().utf8().data(), activeDocument->URL().utf8().data());
+
+        if (KJS::Interpreter::shouldPrintExceptions())
+            printf("%s", message.utf8().data());
+
+        // FIXME: should we print to the console of the activeFrame as well?
+        if (Page* page = targetFrame->page())
+            page->chrome()->addMessageToConsole(JSMessageSource, ErrorMessageLevel, message, 1, String());
+    }
     
-    // Allow if the domain of the parent of the targeted frame equals this domain.
-    String parentDomain;
-    if (Document* parentDocument = parent->document())
-        parentDomain = parentDocument->domain();
-    return equalIgnoringCase(parentDomain, domain);
+    return false;
 }
 
 void FrameLoader::stopLoadingSubframes()
index a4d5754fd0e8271af1dff20a2827bafa2c3c0215..2211610032a3de9310563b5ffa745026c4f8d936 100644 (file)
@@ -436,6 +436,9 @@ namespace WebCore {
         bool committingFirstRealLoad() const { return !m_creatingInitialEmptyDocument && !m_committedFirstRealDocumentLoad; }
 
         void iconLoadDecisionAvailable();
+
+        bool shouldAllowNavigation(Frame* targetFrame) const;
+
     private:
         PassRefPtr<HistoryItem> createHistoryItem(bool useOriginal);
         PassRefPtr<HistoryItem> createHistoryItemTree(Frame* targetFrame, bool clipAtTarget);
@@ -536,8 +539,6 @@ namespace WebCore {
 
         void applyUserAgent(ResourceRequest& request);
 
-        bool canTarget(Frame*) const;
-
         void scheduleRedirection(ScheduledRedirection*);
         void startRedirectionTimer();
         void stopRedirectionTimer();
index 1c042b4094c98e0cc4ccb87a663514e294319583..cb8a6a18129b87deb4b288b6ff66b660eea921b0 100644 (file)
@@ -200,6 +200,9 @@ Frame* FrameTree::find(const AtomicString& name) const
 
 bool FrameTree::isDescendantOf(const Frame* ancestor) const
 {
+    if (m_thisFrame->page() != ancestor->page())
+        return false;
+
     for (Frame* frame = m_thisFrame; frame; frame = frame->tree()->parent())
         if (frame == ancestor)
             return true;