Attachment 358516 Details for Bug 193076 – WIP Patch

[patch] WIP Patch

193076_prevent_third_party_redirects_wip.patch (text/plain), 16.16 KB, created by Chris Dumez on 2019-01-07 12:51:41 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-01-07 12:51:41 PST

Size: 16.16 KB

patch

obsolete

>diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
>index f752db87356..c4c8208121b 100644
>--- a/Source/WebCore/dom/Document.cpp
>+++ b/Source/WebCore/dom/Document.cpp
>@@ -456,12 +456,12 @@ static bool canAccessAncestor(const SecurityOrigin& activeSecurityOrigin, Frame*
>     return false;
> }
> 
>-static void printNavigationErrorMessage(Frame* frame, const URL& activeURL, const char* reason)
>+static void printNavigationErrorMessage(Frame& frame, const URL& activeURL, const char* reason)
> {
>-    String message = "Unsafe JavaScript attempt to initiate navigation for frame with URL '" + frame->document()->url().string() + "' from frame with URL '" + activeURL.string() + "'. " + reason + "\n";
>+    String message = "Unsafe JavaScript attempt to initiate navigation for frame with URL '" + frame.document()->url().string() + "' from frame with URL '" + activeURL.string() + "'. " + reason + "\n";
> 
>     // FIXME: should we print to the console of the document performing the navigation instead?
>-    frame->document()->domWindow()->printErrorMessage(message);
>+    frame.document()->domWindow()->printErrorMessage(message);
> }
> 
> uint64_t Document::s_globalTreeVersion = 0;
>@@ -3337,7 +3337,7 @@ SocketProvider* Document::socketProvider()
>     return m_socketProvider.get();
> }
>     
>-bool Document::canNavigate(Frame* targetFrame)
>+bool Document::canNavigate(Frame* targetFrame, const URL& destinationURL)
> {
>     if (!m_frame)
>         return false;
>@@ -3348,30 +3348,45 @@ bool Document::canNavigate(Frame* targetFrame)
>     if (!targetFrame)
>         return true;
> 
>+    if (!canNavigateInternal(*targetFrame))
>+        return false;
>+
>+    if (isNavigationBlockedByThirdPartyIFrameRedirectBlocking(*targetFrame, destinationURL)) {
>+        printNavigationErrorMessage(*targetFrame, url(), "The frame attempting navigation of the top-level window is cross-origin and the user has never interacted with the frame."_s);
>+        return false;
>+    }
>+
>+    return true;
>+}
>+
>+bool Document::canNavigateInternal(Frame& targetFrame)
>+{
>+    ASSERT(m_frame);
>+
>     // Cases (i), (ii) and (iii) pass the tests from the specifications but might not pass the "security origin" tests.
> 
>     // i. A frame can navigate its top ancestor when its 'allow-top-navigation' flag is set (sometimes known as 'frame-busting').
>-    if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top())
>+    if (!isSandboxed(SandboxTopNavigation) && &targetFrame == &m_frame->tree().top())
>         return true;
> 
>     // ii. A frame can navigate its top ancestor when its 'allow-top-navigation-by-user-activation' flag is set and navigation is triggered by user activation.
>-    if (!isSandboxed(SandboxTopNavigationByUserActivation) && UserGestureIndicator::processingUserGesture() && targetFrame == &m_frame->tree().top())
>+    if (!isSandboxed(SandboxTopNavigationByUserActivation) && UserGestureIndicator::processingUserGesture() && &targetFrame == &m_frame->tree().top())
>         return true;
> 
>     // iii. A sandboxed frame can always navigate its descendants.
>-    if (isSandboxed(SandboxNavigation) && targetFrame->tree().isDescendantOf(m_frame))
>+    if (isSandboxed(SandboxNavigation) && targetFrame.tree().isDescendantOf(m_frame))
>         return true;
> 
>     // From https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate.
>     // 1. If A is not the same browsing context as B, and A is not one of the ancestor browsing contexts of B, and B is not a top-level browsing context, and A's active document's active sandboxing
>     // flag set has its sandboxed navigation browsing context flag set, then abort these steps negatively.
>-    if (m_frame != targetFrame && isSandboxed(SandboxNavigation) && targetFrame->tree().parent() && !targetFrame->tree().isDescendantOf(m_frame)) {
>+    if (m_frame != &targetFrame && isSandboxed(SandboxNavigation) && targetFrame.tree().parent() && !targetFrame.tree().isDescendantOf(m_frame)) {
>         printNavigationErrorMessage(targetFrame, url(), "The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors."_s);
>         return false;
>     }
> 
>     // 2. Otherwise, if B is a top-level browsing context, and is one of the ancestor browsing contexts of A, then:
>-    if (m_frame != targetFrame && targetFrame == &m_frame->tree().top()) {
>+    if (m_frame != &targetFrame && &targetFrame == &m_frame->tree().top()) {
>         bool triggeredByUserActivation = UserGestureIndicator::processingUserGesture();
>         // 1. If this algorithm is triggered by user activation and A's active document's active sandboxing flag set has its sandboxed top-level navigation with user activation browsing context flag set, then abort these steps negatively.
>         if (triggeredByUserActivation && isSandboxed(SandboxTopNavigationByUserActivation)) {
>@@ -3387,7 +3402,7 @@ bool Document::canNavigate(Frame* targetFrame)
> 
>     // 3. Otherwise, if B is a top-level browsing context, and is neither A nor one of the ancestor browsing contexts of A, and A's Document's active sandboxing flag set has its
>     // sandboxed navigation browsing context flag set, and A is not the one permitted sandboxed navigator of B, then abort these steps negatively.
>-    if (!targetFrame->tree().parent() && m_frame != targetFrame && targetFrame != &m_frame->tree().top() && isSandboxed(SandboxNavigation) && targetFrame->loader().opener() != m_frame) {
>+    if (!targetFrame.tree().parent() && m_frame != &targetFrame && &targetFrame != &m_frame->tree().top() && isSandboxed(SandboxNavigation) && targetFrame.loader().opener() != m_frame) {
>         printNavigationErrorMessage(targetFrame, url(), "The frame attempting navigation is sandboxed, and is not allowed to navigate this popup."_s);
>         return false;
>     }
>@@ -3401,7 +3416,7 @@ bool Document::canNavigate(Frame* targetFrame)
>     //
>     // See http://www.adambarth.com/papers/2008/barth-jackson-mitchell.pdf for
>     // historical information about this security check.
>-    if (canAccessAncestor(securityOrigin(), targetFrame))
>+    if (canAccessAncestor(securityOrigin(), &targetFrame))
>         return true;
> 
>     // Top-level frames are easier to navigate than other frames because they
>@@ -3415,11 +3430,11 @@ bool Document::canNavigate(Frame* targetFrame)
>     // some way related to the frame being navigate (e.g., by the "opener"
>     // and/or "parent" relation). Requiring some sort of relation prevents a
>     // document from navigating arbitrary, unrelated top-level frames.
>-    if (!targetFrame->tree().parent()) {
>-        if (targetFrame == m_frame->loader().opener())
>+    if (!targetFrame.tree().parent()) {
>+        if (&targetFrame == m_frame->loader().opener())
>             return true;
> 
>-        if (canAccessAncestor(securityOrigin(), targetFrame->loader().opener()))
>+        if (canAccessAncestor(securityOrigin(), targetFrame.loader().opener()))
>             return true;
>     }
> 
>@@ -3427,6 +3442,32 @@ bool Document::canNavigate(Frame* targetFrame)
>     return false;
> }
> 
>+// Prevent cross-site top-level redirects from third-party iframes unless the user has ever interacted with the frame.
>+bool Document::isNavigationBlockedByThirdPartyIFrameRedirectBlocking(Frame& targetFrame, const URL& destinationURL)
>+{
>+    if (!settings().thirdPartyIframeRedirectBlockingEnabled())
>+        return false;
>+
>+    // Only prevent top frame navigations by subframes.
>+    if (m_frame == &targetFrame || &targetFrame != &m_frame->tree().top())
>+        return false;
>+
>+    // Only prevent navigations by subframes that the user has not interacted with.
>+    if (m_frame->hasHadUserInteraction())
>+        return false;
>+
>+    // Only prevent navigations by third-party iframes.
>+    if (canAccessAncestor(securityOrigin(), &targetFrame))
>+        return false;
>+
>+    // Only prevent cross-site navigations.
>+    auto* targetDocument = targetFrame.document();
>+    if (targetDocument && (targetDocument->securityOrigin().canAccess(SecurityOrigin::create(destinationURL)) || registrableDomainsAreEqual(targetDocument->url(), destinationURL)))
>+        return false;
>+
>+    return true;
>+}
>+
> void Document::didRemoveAllPendingStylesheet()
> {
>     if (auto* parser = scriptableDocumentParser())
>diff --git a/Source/WebCore/dom/Document.h b/Source/WebCore/dom/Document.h
>index 8025ef23e6b..77f2dbd68f5 100644
>--- a/Source/WebCore/dom/Document.h
>+++ b/Source/WebCore/dom/Document.h
>@@ -706,7 +706,7 @@ public:
> #endif
>     SocketProvider* socketProvider() final;
> 
>-    bool canNavigate(Frame* targetFrame);
>+    bool canNavigate(Frame* targetFrame, const URL& destinationURL = URL());
> 
>     bool usesStyleBasedEditability() const;
>     void setHasElementUsingStyleBasedEditability();
>@@ -1644,6 +1644,9 @@ private:
>     void checkViewportDependentPictures();
>     void checkAppearanceDependentPictures();
> 
>+    bool canNavigateInternal(Frame& targetFrame);
>+    bool isNavigationBlockedByThirdPartyIFrameRedirectBlocking(Frame& targetFrame, const URL& destinationURL);
>+
> #if ENABLE(INTERSECTION_OBSERVER)
>     void notifyIntersectionObserversTimerFired();
> #endif
>diff --git a/Source/WebCore/dom/UserGestureIndicator.cpp b/Source/WebCore/dom/UserGestureIndicator.cpp
>index d1685b9ae5f..f691d386d6b 100644
>--- a/Source/WebCore/dom/UserGestureIndicator.cpp
>+++ b/Source/WebCore/dom/UserGestureIndicator.cpp
>@@ -27,6 +27,7 @@
> #include "UserGestureIndicator.h"
> 
> #include "Document.h"
>+#include "Frame.h"
> #include "ResourceLoadObserver.h"
> #include <wtf/MainThread.h>
> #include <wtf/NeverDestroyed.h>
>@@ -59,6 +60,12 @@ UserGestureIndicator::UserGestureIndicator(Optional<ProcessingUserGestureState>
>         if (processInteractionStyle == ProcessInteractionStyle::Immediate)
>             ResourceLoadObserver::shared().logUserInteractionWithReducedTimeResolution(document->topDocument());
>         document->topDocument().setUserDidInteractWithPage(true);
>+        if (auto* frame = document->frame()) {
>+            if (!frame->hasHadUserInteraction()) {
>+                for (; frame; frame = frame->tree().parent())
>+                    frame->setHasHadUserInteraction();
>+            }
>+        }
>     }
> }
> 
>diff --git a/Source/WebCore/page/DOMWindow.cpp b/Source/WebCore/page/DOMWindow.cpp
>index 1759defc070..2c766f95854 100644
>--- a/Source/WebCore/page/DOMWindow.cpp
>+++ b/Source/WebCore/page/DOMWindow.cpp
>@@ -2103,7 +2103,7 @@ void DOMWindow::finishedLoading()
>     }
> }
> 
>-void DOMWindow::setLocation(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& urlString, SetLocationLocking locking)
>+void DOMWindow::setLocation(DOMWindow& activeWindow, const URL& completedURL, SetLocationLocking locking)
> {
>     if (!isCurrentlyDisplayedInFrame())
>         return;
>@@ -2113,15 +2113,7 @@ void DOMWindow::setLocation(DOMWindow& activeWindow, DOMWindow& firstWindow, con
>         return;
> 
>     auto* frame = this->frame();
>-    if (!activeDocument->canNavigate(frame))
>-        return;
>-
>-    Frame* firstFrame = firstWindow.frame();
>-    if (!firstFrame)
>-        return;
>-
>-    URL completedURL = firstFrame->document()->completeURL(urlString);
>-    if (completedURL.isNull())
>+    if (!activeDocument->canNavigate(frame, completedURL))
>         return;
> 
>     if (isInsecureScriptAccess(activeWindow, completedURL))
>diff --git a/Source/WebCore/page/DOMWindow.h b/Source/WebCore/page/DOMWindow.h
>index effd3f7ecae..8a9c80a0b1e 100644
>--- a/Source/WebCore/page/DOMWindow.h
>+++ b/Source/WebCore/page/DOMWindow.h
>@@ -145,7 +145,7 @@ public:
>     Navigator& clientInformation() { return navigator(); }
> 
>     Location& location();
>-    void setLocation(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& location, SetLocationLocking = LockHistoryBasedOnGestureState);
>+    void setLocation(DOMWindow& activeWindow, const URL& completedURL, SetLocationLocking = LockHistoryBasedOnGestureState);
> 
>     DOMSelection* getSelection();
> 
>diff --git a/Source/WebCore/page/Frame.h b/Source/WebCore/page/Frame.h
>index 9faf2437f57..8ad13d6e1cc 100644
>--- a/Source/WebCore/page/Frame.h
>+++ b/Source/WebCore/page/Frame.h
>@@ -172,6 +172,9 @@ public:
> 
>     bool documentIsBeingReplaced() const { return m_documentIsBeingReplaced; }
> 
>+    bool hasHadUserInteraction() const { return m_hasHadUserInteraction; }
>+    void setHasHadUserInteraction() { m_hasHadUserInteraction = true; }
>+
> // ======== All public functions below this point are candidates to move out of Frame into another class. ========
> 
>     WEBCORE_EXPORT void injectUserScripts(UserScriptInjectionTime);
>@@ -348,6 +351,7 @@ private:
>     bool m_documentIsBeingReplaced { false };
>     unsigned m_navigationDisableCount { 0 };
>     unsigned m_selfOnlyRefCount { 0 };
>+    bool m_hasHadUserInteraction { false };
> 
> protected:
>     UniqueRef<EventHandler> m_eventHandler;
>diff --git a/Source/WebCore/page/Location.cpp b/Source/WebCore/page/Location.cpp
>index 1640cfc123b..9691f59cdbd 100644
>--- a/Source/WebCore/page/Location.cpp
>+++ b/Source/WebCore/page/Location.cpp
>@@ -225,15 +225,25 @@ ExceptionOr<void> Location::assign(DOMWindow& activeWindow, DOMWindow& firstWind
>     return setLocation(activeWindow, firstWindow, url);
> }
> 
>-void Location::replace(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& url)
>+void Location::replace(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& urlString)
> {
>     auto* frame = this->frame();
>     if (!frame)
>         return;
>     ASSERT(frame->document());
>     ASSERT(frame->document()->domWindow());
>+
>+    Frame* firstFrame = firstWindow.frame();
>+    if (!firstFrame || !firstFrame->document())
>+        return;
>+
>+    URL completedURL = firstFrame->document()->completeURL(urlString);
>+    // FIXME: The specification says to throw a SyntaxError if the URL is not valid.
>+    if (completedURL.isNull())
>+        return;
>+
>     // We call DOMWindow::setLocation directly here because replace() always operates on the current frame.
>-    frame->document()->domWindow()->setLocation(activeWindow, firstWindow, url, LockHistoryAndBackForwardList);
>+    frame->document()->domWindow()->setLocation(activeWindow, completedURL, LockHistoryAndBackForwardList);
> }
> 
> void Location::reload(DOMWindow& activeWindow)
>@@ -264,15 +274,26 @@ void Location::reload(DOMWindow& activeWindow)
>     frame->navigationScheduler().scheduleRefresh(activeDocument);
> }
> 
>-ExceptionOr<void> Location::setLocation(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& url)
>+ExceptionOr<void> Location::setLocation(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& urlString)
> {
>     auto* frame = this->frame();
>     ASSERT(frame);
>-    if (!activeWindow.document()->canNavigate(frame))
>+
>+    Frame* firstFrame = firstWindow.frame();
>+    if (!firstFrame || !firstFrame->document())
>+        return { };
>+
>+    URL completedURL = firstFrame->document()->completeURL(urlString);
>+    // FIXME: The specification says to throw a SyntaxError if the URL is not valid.
>+    if (completedURL.isNull())
>+        return { };
>+
>+    if (!activeWindow.document()->canNavigate(frame, completedURL))
>         return Exception { SecurityError };
>+
>     ASSERT(frame->document());
>     ASSERT(frame->document()->domWindow());
>-    frame->document()->domWindow()->setLocation(activeWindow, firstWindow, url);
>+    frame->document()->domWindow()->setLocation(activeWindow, completedURL);
>     return { };
> }
> 
>diff --git a/Source/WebCore/page/Settings.yaml b/Source/WebCore/page/Settings.yaml
>index cb29eac3eff..2b187e13593 100644
>--- a/Source/WebCore/page/Settings.yaml
>+++ b/Source/WebCore/page/Settings.yaml
>@@ -332,6 +332,9 @@ requestAnimationFrameEnabled:
> HTTPSUpgradeEnabled:
>   initial: false
> 
>+thirdPartyIframeRedirectBlockingEnabled:
>+  initial: true
>+
> cookieEnabled:
>   initial: true
> mediaEnabled:
>diff --git a/Source/WebKit/Shared/WebPreferences.yaml b/Source/WebKit/Shared/WebPreferences.yaml
>index 20614ae3687..03a23594ac2 100644
>--- a/Source/WebKit/Shared/WebPreferences.yaml
>+++ b/Source/WebKit/Shared/WebPreferences.yaml
>@@ -42,6 +42,13 @@ HTTPSUpgradeEnabled:
>    humanReadableDescription: "Automatic HTTPS upgrade for known supported sites"
>    category: experimental
> 
>+ThirdPartyIframeRedirectBlockingEnabled:
>+   type: bool
>+   defaultValue: true
>+   humanReadableName: "Block top-level redirects by third-party iframes"
>+   humanReadableDescription: "Block top-level redirects by third-party iframes"
>+   category: experimental
>+
> JavaEnabled:
>   type: bool
>   defaultValue: false

Actions: View | Formatted Diff | Diff

Attachments on bug 193076: 358349 | 358516 | 358523 | 358531 | 358559 | 358579 | 358599