Attachment 342476 Details for Bug 186546 – Patch

[patch] Patch

bug-186546-20180611161944.patch (text/plain), 14.86 KB, created by Chris Dumez on 2018-06-11 16:19:45 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-11 16:19:45 PDT

Size: 14.86 KB

patch

obsolete

>Subversion Revision: 232730
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index d8ff6a3d010b9bbf291d9f6785a4c6d9ad60eb6e..2853ffb601e8658c240f69708d9a1cbbdeb021a4 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,21 @@
>+2018-06-11  Chris Dumez  <cdumez@apple.com>
>+
>+        http/tests/security/xss-DENIED-script-inject-into-inactive-window2.html times out with PSON enabled
>+        https://bugs.webkit.org/show_bug.cgi?id=186546
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a hasOpenedFrames flag to NavigationAction, which we'll use in the UIProcess when deciding
>+        to process swap on navigation or not.
>+
>+        Test: http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html
>+
>+        * loader/FrameLoader.cpp:
>+        (WebCore::FrameLoader::loadURL):
>+        * loader/NavigationAction.h:
>+        (WebCore::NavigationAction::hasOpenedFrames const):
>+        (WebCore::NavigationAction::setHasOpenedFrames):
>+
> 2018-06-11  Chris Dumez  <cdumez@apple.com>
> 
>         http/tests/security/cors-post-redirect-307.html fails with PSON enabled
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 9409aebcd213dc08bae5cd0937dba63768d7215d..d820e7caacf36d8b72d23ca369e8c81ab5230cc5 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,28 @@
>+2018-06-11  Chris Dumez  <cdumez@apple.com>
>+
>+        http/tests/security/xss-DENIED-script-inject-into-inactive-window2.html times out with PSON enabled
>+        https://bugs.webkit.org/show_bug.cgi?id=186546
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Disable process swap on navigation in frames that have opened other frames via
>+        window.open(). These new windows may have a WindowProxy to their opener, and it
>+        would therefore be unsafe to process swap at this point.
>+
>+        * Shared/NavigationActionData.cpp:
>+        (WebKit::NavigationActionData::encode const):
>+        (WebKit::NavigationActionData::decode):
>+        * Shared/NavigationActionData.h:
>+        * UIProcess/API/APINavigation.h:
>+        (API::Navigation::setHasOpenedFrames):
>+        (API::Navigation::hasOpenedFrames const):
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
>+        * UIProcess/WebProcessPool.cpp:
>+        (WebKit::WebProcessPool::processForNavigationInternal):
>+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
>+        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
>+
> 2018-06-11  Chris Dumez  <cdumez@apple.com>
> 
>         http/tests/security/cors-post-redirect-307.html fails with PSON enabled
>diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
>index fea4ecfde6bc24972ac5aaff1c198b598a9f70dd..04d436e29825c6257a1f47611e314041419320ad 100644
>--- a/Source/WebCore/loader/FrameLoader.cpp
>+++ b/Source/WebCore/loader/FrameLoader.cpp
>@@ -1350,6 +1350,7 @@ void FrameLoader::loadURL(FrameLoadRequest&& frameLoadRequest, const String& ref
> 
>     NavigationAction action { frameLoadRequest.requester(), request, frameLoadRequest.initiatedByMainFrame(), newLoadType, isFormSubmission, event, frameLoadRequest.shouldOpenExternalURLsPolicy(), frameLoadRequest.downloadAttribute() };
>     action.setIsCrossOriginWindowOpenNavigation(frameLoadRequest.isCrossOriginWindowOpenNavigation());
>+    action.setHasOpenedFrames(!m_openedFrames.isEmpty());
>     if (auto* opener = this->opener()) {
>         auto pageID = opener->loader().client().pageID();
>         auto frameID = opener->loader().client().frameID();
>diff --git a/Source/WebCore/loader/NavigationAction.h b/Source/WebCore/loader/NavigationAction.h
>index 7ec2da30de2c2719f7231aef38e3c428d6435bbc..740d51394eb78b9d1375421df3f0cb41f306b12a 100644
>--- a/Source/WebCore/loader/NavigationAction.h
>+++ b/Source/WebCore/loader/NavigationAction.h
>@@ -123,6 +123,9 @@ public:
>     void setOpener(std::optional<PageIDAndFrameIDPair>&& opener) { m_opener = WTFMove(opener); }
>     const std::optional<PageIDAndFrameIDPair>& opener() const { return m_opener; }
> 
>+    bool hasOpenedFrames() const { return m_hasOpenedFrames; }
>+    void setHasOpenedFrames(bool value) { m_hasOpenedFrames = value; }
>+
>     void setTargetBackForwardItem(HistoryItem&);
>     const std::optional<BackForwardItemIdentifier>& targetBackForwardItemIdentifier() const { return m_targetBackForwardItemIdentifier; }
> 
>@@ -140,6 +143,7 @@ private:
>     AtomicString m_downloadAttribute;
>     bool m_treatAsSameOriginNavigation;
>     bool m_isCrossOriginWindowOpenNavigation { false };
>+    bool m_hasOpenedFrames { false };
>     std::optional<PageIDAndFrameIDPair> m_opener;
>     std::optional<BackForwardItemIdentifier> m_targetBackForwardItemIdentifier;
> };
>diff --git a/Source/WebKit/Shared/NavigationActionData.cpp b/Source/WebKit/Shared/NavigationActionData.cpp
>index e61b2acaa1f70e7d05bca7fb4990aed2950714d6..7bdab841331ec2c79a20b934914208715e36bdd2 100644
>--- a/Source/WebKit/Shared/NavigationActionData.cpp
>+++ b/Source/WebKit/Shared/NavigationActionData.cpp
>@@ -49,6 +49,7 @@ void NavigationActionData::encode(IPC::Encoder& encoder) const
>     encoder << isRedirect;
>     encoder << treatAsSameOriginNavigation;
>     encoder << isCrossOriginWindowOpenNavigation;
>+    encoder << hasOpenedFrames;
>     encoder << opener;
>     encoder << targetBackForwardItemIdentifier;
> }
>@@ -109,6 +110,11 @@ std::optional<NavigationActionData> NavigationActionData::decode(IPC::Decoder& d
>     if (!isCrossOriginWindowOpenNavigation)
>         return std::nullopt;
> 
>+    std::optional<bool> hasOpenedFrames;
>+    decoder >> hasOpenedFrames;
>+    if (!hasOpenedFrames)
>+        return std::nullopt;
>+
>     std::optional<std::optional<std::pair<uint64_t, uint64_t>>> opener;
>     decoder >> opener;
>     if (!opener)
>@@ -121,7 +127,7 @@ std::optional<NavigationActionData> NavigationActionData::decode(IPC::Decoder& d
>         
>     return {{ WTFMove(navigationType), WTFMove(modifiers), WTFMove(mouseButton), WTFMove(syntheticClickType), WTFMove(*userGestureTokenIdentifier),
>         WTFMove(*canHandleRequest), WTFMove(shouldOpenExternalURLsPolicy), WTFMove(*downloadAttribute), WTFMove(clickLocationInRootViewCoordinates),
>-        WTFMove(*isRedirect), *treatAsSameOriginNavigation, *isCrossOriginWindowOpenNavigation, WTFMove(*opener), WTFMove(*targetBackForwardItemIdentifier) }};
>+        WTFMove(*isRedirect), *treatAsSameOriginNavigation, *isCrossOriginWindowOpenNavigation, *hasOpenedFrames, WTFMove(*opener), WTFMove(*targetBackForwardItemIdentifier) }};
> }
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/Shared/NavigationActionData.h b/Source/WebKit/Shared/NavigationActionData.h
>index bf73982dd710b1507663ada305057318883fe492..09db575641f6bc2636b76bc28c75090e3401a046 100644
>--- a/Source/WebKit/Shared/NavigationActionData.h
>+++ b/Source/WebKit/Shared/NavigationActionData.h
>@@ -53,6 +53,7 @@ struct NavigationActionData {
>     bool isRedirect { false };
>     bool treatAsSameOriginNavigation { false };
>     bool isCrossOriginWindowOpenNavigation { false };
>+    bool hasOpenedFrames { false };
>     std::optional<std::pair<uint64_t, uint64_t>> opener;
>     std::optional<WebCore::BackForwardItemIdentifier> targetBackForwardItemIdentifier;
> };
>diff --git a/Source/WebKit/UIProcess/API/APINavigation.h b/Source/WebKit/UIProcess/API/APINavigation.h
>index 9e5917710e58c384cb57aeaf26f21ada119363cd..0f95fc5f9b21d25e610fa80f23cc0405d5c53f32 100644
>--- a/Source/WebKit/UIProcess/API/APINavigation.h
>+++ b/Source/WebKit/UIProcess/API/APINavigation.h
>@@ -91,6 +91,9 @@ public:
>     void setIsCrossOriginWindowOpenNavigation(bool value) { m_isCrossOriginWindowOpenNavigation = value; }
>     bool isCrossOriginWindowOpenNavigation() const { return m_isCrossOriginWindowOpenNavigation; }
> 
>+    void setHasOpenedFrames(bool value) { m_hasOpenedFrames = value; }
>+    bool hasOpenedFrames() const { return m_hasOpenedFrames; }
>+
>     void setOpener(const std::optional<std::pair<uint64_t, uint64_t>>& opener) { m_opener = opener; }
>     const std::optional<std::pair<uint64_t, uint64_t>>& opener() const { return m_opener; }
> 
>@@ -117,6 +120,7 @@ private:
>     std::optional<WebCore::FrameLoadType> m_backForwardFrameLoadType;
>     bool m_treatAsSameOriginNavigation { false };
>     bool m_isCrossOriginWindowOpenNavigation { false };
>+    bool m_hasOpenedFrames { false };
>     std::optional<std::pair<uint64_t, uint64_t>> m_opener;
> };
> 
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index c1c961d82256a9b049516aeba8b59fad78b2f09d..8c1d35fac0855787a5668b794cf16f5c2aea6745 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -4025,6 +4025,7 @@ void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, const Secur
>     navigation->setCurrentRequestIsRedirect(navigationActionData.isRedirect);
>     navigation->setTreatAsSameOriginNavigation(navigationActionData.treatAsSameOriginNavigation);
>     navigation->setIsCrossOriginWindowOpenNavigation(navigationActionData.isCrossOriginWindowOpenNavigation);
>+    navigation->setHasOpenedFrames(navigationActionData.hasOpenedFrames);
>     navigation->setOpener(navigationActionData.opener);
> 
>     auto listener = makeRef(frame->setUpPolicyListenerProxy(listenerID, PolicyListenerType::NavigationAction));
>diff --git a/Source/WebKit/UIProcess/WebProcessPool.cpp b/Source/WebKit/UIProcess/WebProcessPool.cpp
>index de709cdfbb765ce51bda109df16abc3141783c9d..4c0f00e0fc7d1f73e845de7be5765cec835f3d78 100644
>--- a/Source/WebKit/UIProcess/WebProcessPool.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessPool.cpp
>@@ -2137,6 +2137,10 @@ Ref<WebProcessProxy> WebProcessPool::processForNavigationInternal(WebPageProxy&
>     if (navigation.opener())
>         return page.process();
> 
>+    // FIXME: We should support process swap when a window has opened other windows via window.open.
>+    if (navigation.hasOpenedFrames())
>+        return page.process();
>+
>     if (auto* backForwardListItem = navigation.targetItem()) {
>         if (auto* suspendedPage = backForwardListItem->suspendedPage()) {
>             ASSERT(suspendedPage->process());
>diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
>index e9fc74da4ebec46856176eb071a75de6eb577cf3..019d8be590a5a62efe4ebe0dd36ef7edfcbc26cb 100644
>--- a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
>+++ b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
>@@ -867,6 +867,7 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const Navigat
>     navigationActionData.isRedirect = didReceiveRedirectResponse;
>     navigationActionData.treatAsSameOriginNavigation = navigationAction.treatAsSameOriginNavigation();
>     navigationActionData.isCrossOriginWindowOpenNavigation = navigationAction.isCrossOriginWindowOpenNavigation();
>+    navigationActionData.hasOpenedFrames = navigationAction.hasOpenedFrames();
>     navigationActionData.opener = navigationAction.opener();
>     navigationActionData.targetBackForwardItemIdentifier = navigationAction.targetBackForwardItemIdentifier();
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index cac2459c71239a577d64b6f8fc187a4ef89a71ed..93cb298e7d5a5f2c6b0d637c2ca58e06265e6605 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2018-06-11  Chris Dumez  <cdumez@apple.com>
>+
>+        http/tests/security/xss-DENIED-script-inject-into-inactive-window2.html times out with PSON enabled
>+        https://bugs.webkit.org/show_bug.cgi?id=186546
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add layout test coverage.
>+
>+        * http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson-expected.txt: Added.
>+        * http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html: Added.
>+
> 2018-06-11  Chris Dumez  <cdumez@apple.com>
> 
>         http/tests/security/cors-post-redirect-307.html fails with PSON enabled
>diff --git a/LayoutTests/http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson-expected.txt b/LayoutTests/http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..4cc180b115761567dcc7a2af6ecea783797417d0
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson-expected.txt
>@@ -0,0 +1 @@
>+This page doesn't do anything special.
>diff --git a/LayoutTests/http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html b/LayoutTests/http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..6a59c2c706ce887f804d8521596c2132e9f0a034
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html
>@@ -0,0 +1,60 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<script>
>+if (window.testRunner) {
>+    testRunner.dumpAsText();
>+    testRunner.setCanOpenWindows();
>+    testRunner.setPopupBlockingEnabled(false);
>+    testRunner.setCloseRemainingWindowsWhenComplete(true);
>+    testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
>+    testRunner.waitUntilDone();
>+}
>+</script>
>+</head>
>+<body>
>+<p id="description">This tests that a &lt;script&gt; added to an inactive document window does not execute. The test FAILED if you see &quot;XSS&quot; on this page. Popup blocking must be disabled to run this test by hand.</p>
>+<script>
>+// Idea: Open a new window and have it navigate its opener to the victim site while holding a reference to the opener document.
>+var openerDocument;
>+var intervalId;
>+if (document.location.search.indexOf("?actually-attack") !== -1) {
>+    document.getElementById("description").textContent = 'Check the original window. The test FAILED if you see "XSS" on the page. Otherwise, it PASSED.';
>+
>+    // Case: New window
>+    openerDocument = window.opener.document;
>+
>+    // Navigate same frame to victim.
>+    var link = openerDocument.createElement("a");
>+    link.target = "_self";
>+    link.href = "http://localhost:8000/security/resources/innocent-victim.html";
>+    link.click();
>+
>+    intervalId = window.setInterval(checkDidLoadVictim, 100);
>+} else {
>+    // Case: Initial load
>+    var link = document.createElement("a");
>+    link.target = "_blank";
>+    link.href = "?actually-attack";
>+    link.click(); // Open a new window.
>+}
>+
>+function checkDidLoadVictim()
>+{
>+    try {
>+        openerDocument.location.href
>+    } catch (e) {
>+        // Victim loaded.
>+        window.clearInterval(intervalId);
>+
>+        // Run code in victim.
>+        var scriptToRunInVictim = openerDocument.createElement("script");
>+        scriptToRunInVictim.textContent = "document.writeln('XSS')";
>+        openerDocument.body.appendChild(scriptToRunInVictim);
>+        if (window.testRunner)
>+            window.setTimeout(function () { window.testRunner.notifyDone(); }, 500);
>+    }
>+}
>+</script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 186546: 342476 | 342493