Reviewed by Oliver.
authorweinig@apple.com <weinig@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 30 Nov 2007 05:20:39 +0000 (05:20 +0000)
committerweinig@apple.com <weinig@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 30 Nov 2007 05:20:39 +0000 (05:20 +0000)
        Additional fix for <rdar://problem/5592988> / http://bugs.webkit.org/show_bug.cgi?id=15936
        - More closely match IE's policy for frame navigation.

        * bindings/js/kjs_window.cpp:
        (KJS::WindowProtoFuncOpen::callAsFunction):
        * loader/FrameLoader.cpp:
        (WebCore::FrameLoader::shouldAllowNavigation):
        * page/FrameTree.cpp:
        (WebCore::FrameTree::top):
        * page/FrameTree.h:

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

WebCore/ChangeLog
WebCore/bindings/js/kjs_window.cpp
WebCore/loader/FrameLoader.cpp
WebCore/page/FrameTree.cpp
WebCore/page/FrameTree.h

index 38bd01ef48ce25d348c64cbf0419d58ee6989162..8a13a03d974c2e3aa30fab862692b63008780e96 100644 (file)
@@ -1,3 +1,18 @@
+2007-11-29  Sam Weinig  <sam@webkit.org>
+
+        Reviewed by Oliver.
+
+        Additional fix for <rdar://problem/5592988> / http://bugs.webkit.org/show_bug.cgi?id=15936
+        - More closely match IE's policy for frame navigation.
+
+        * bindings/js/kjs_window.cpp:
+        (KJS::WindowProtoFuncOpen::callAsFunction):
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::shouldAllowNavigation):
+        * page/FrameTree.cpp:
+        (WebCore::FrameTree::top):
+        * page/FrameTree.h:
+
 2007-11-29  Dan Bernstein  <mitz@apple.com>
 
         Reviewed by Adam Roben and John Sullivan.
index a123c07b69c3fa0050effdfa0cb72358bccbdc2e..0dfb032c4e8e355efc82764d1c9c9fd0220a5ed7 100644 (file)
@@ -1304,6 +1304,9 @@ JSValue* WindowProtoFuncOpen::callAsFunction(ExecState* exec, JSObject* thisObj,
     Frame* frame = window->impl()->frame();
     if (!frame)
         return jsUndefined();
+    Frame* activeFrame = Window::retrieveActive(exec)->impl()->frame();
+    if (!activeFrame)
+        return  jsUndefined();
 
     Page* page = frame->page();
 
@@ -1315,19 +1318,22 @@ JSValue* WindowProtoFuncOpen::callAsFunction(ExecState* exec, JSObject* thisObj,
     if (!allowPopUp(exec, window) && (frameName.isEmpty() || !frame->tree()->find(frameName)))
         return jsUndefined();
 
-    // Get the target frame for the special cases of _top and _parent
-    if (frameName == "_top")
-        while (frame->tree()->parent())
-              frame = frame->tree()->parent();
-    else if (frameName == "_parent")
-        if (frame->tree()->parent())
-            frame = frame->tree()->parent();
-
-    // In those cases, we can schedule a location change right now and return early
-    if (frameName == "_top" || frameName == "_parent") {
+    // Get the target frame for the special cases of _top and _parent.  In those 
+    // cases, we can schedule a location change right now and return early.
+    bool topOrParent = false;
+    if (frameName == "_top") {
+        frame = frame->tree()->top();
+        topOrParent = true;
+    } else if (frameName == "_parent") {
+        if (Frame* parent = frame->tree()->parent())
+            frame = parent;
+        if (!activeFrame->loader()->shouldAllowNavigation(frame))
+            return jsUndefined();
+        topOrParent = true;
+    }
+    if (topOrParent) {
         String completedURL;
-        Frame* activeFrame = Window::retrieveActive(exec)->impl()->frame();
-        if (!urlString.isEmpty() && activeFrame)
+        if (!urlString.isEmpty())
             completedURL = activeFrame->document()->completeURL(urlString);
 
         const Window* window = Window::retrieveWindow(frame);
index 1c2a1ea54a9825a2e63e729a473fc8e7e28be2e4..4768fec9724c94c84eb2560dc7aadb3fe39f5357 100644 (file)
@@ -2308,41 +2308,35 @@ void FrameLoader::reload()
 
 bool FrameLoader::shouldAllowNavigation(Frame* targetFrame) const
 {
-    // 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
+    // The navigation change is safe if the active frame is:
+    //   - in the same security origin as the target or one of the target's ancestors
+    // Or the target frame is:
+    //   - a top-level frame in the frame hierarchy
 
-    // Allow if there is no specific target.
     if (!targetFrame)
         return true;
+
     if (m_frame == targetFrame)
         return true;
 
-    // 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
+    if (!targetFrame->tree()->parent())
+        return true;
 
-    // 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;
-
-    // Opener case.
-    if (m_frame == targetFrame->loader()->opener())
-        return true;
+    for (Frame* ancestorFrame = targetFrame; ancestorFrame; ancestorFrame = ancestorFrame->tree()->parent()) {
+        Document* ancestorDocument = ancestorFrame->document();
+        if (!ancestorDocument)
+            return true;
 
-    // Ancestor or descendant case.
-    if (targetFrame->tree()->isDescendantOf(m_frame) || m_frame->tree()->isDescendantOf(targetFrame))
-        return true;
+        const SecurityOrigin& ancestorSecurityOrigin = ancestorDocument->securityOrigin();
+        if (activeSecurityOrigin.canAccess(ancestorSecurityOrigin))
+            return true;
+    }
 
     if (!targetFrame->settings()->privateBrowsingEnabled()) {
+        Document* targetDocument = targetFrame->document();
         // 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());
index b50d20495f69beb7bf63fb1b1db3aedf9d6c20bc..92e198d8bab83ae5b7f758e6b66fef0ddaaeebf1 100644 (file)
@@ -282,4 +282,15 @@ Frame* FrameTree::deepLastChild() const
     return result;
 }
 
+Frame* FrameTree::top() const
+{
+    if (Page* page = m_thisFrame->page())
+        return page->mainFrame();
+
+    Frame* frame = m_thisFrame;
+    while (Frame* parent = frame->tree()->parent())
+        frame = parent;
+    return frame;
 }
+
+} // namespace WebCore
index b102468968b13f5d9473a31772c94a691f763017..3f3e20abbd91120fb9844243466c2818c237657b 100644 (file)
@@ -64,6 +64,8 @@ namespace WebCore {
 
         AtomicString uniqueChildName(const AtomicString& requestedName) const;
 
+        Frame* top() const;
+
     private:
         Frame* deepLastChild() const;