Changeset 28225 in webkit

Timestamp:

Nov 29, 2007 9:20:39 PM (16 years ago)

Author:

weinig@apple.com

Message:

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:

Location:

trunk/WebCore

Files:

• ChangeLog (modified) (1 diff)
• bindings/js/kjs_window.cpp (modified) (2 diffs)
• loader/FrameLoader.cpp (modified) (1 diff)
• page/FrameTree.cpp (modified) (1 diff)
• page/FrameTree.h (modified) (1 diff)

trunk/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/WebCore/bindings/js/kjs_window.cpp

@@ -1305 +1305 @@
     if (!frame)
         return jsUndefined();
+    Frame* activeFrame = Window::retrieveActive(exec)->impl()->frame();
+    if (!activeFrame)
+        return  jsUndefined();
 
     Page* page = frame->page();
@@ -1316 +1319 @@
         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);
 

trunk/WebCore/loader/FrameLoader.cpp

@@ -2309 +2309 @@
 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
-
-    // Allow if there is no specific target.
+    // 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
+
     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
-
-    // Same security domain case.
+    if (!targetFrame->tree()->parent())
+        return true;
+
     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;
-
-    // Ancestor or descendant case.
-    if (targetFrame->tree()->isDescendantOf(m_frame) || m_frame->tree()->isDescendantOf(targetFrame))
-        return true;
+    for (Frame* ancestorFrame = targetFrame; ancestorFrame; ancestorFrame = ancestorFrame->tree()->parent()) {
+        Document* ancestorDocument = ancestorFrame->document();
+        if (!ancestorDocument)
+            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",

trunk/WebCore/page/FrameTree.cpp

@@ -283 +283 @@
 }
 
-}
+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

trunk/WebCore/page/FrameTree.h

@@ -65 +65 @@
         AtomicString uniqueChildName(const AtomicString& requestedName) const;
 
+        Frame* top() const;
+
     private:
         Frame* deepLastChild() const;