Attachment 356848 Details for Bug 191982 – Patch

[patch] Patch

bug-191982-20181207155440.patch (text/plain), 14.46 KB, created by Chris Dumez on 2018-12-07 15:54:41 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-12-07 15:54:41 PST

Size: 14.46 KB

patch

obsolete

>Subversion Revision: 238911
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 6131f6b7c5e2407aec5aa05e6447c5276a210f49..f53eb6be47865eb6c7b79420c1b20d1662df49ff 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,39 @@
>+2018-12-07  Chris Dumez  <cdumez@apple.com>
>+
>+        Restore MESSAGE_CHECK_URL() security check on sourceURL in didPerformClientRedirect()
>+        https://bugs.webkit.org/show_bug.cgi?id=191982
>+        <rdar://problem/46258054>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Have the WebPageProxy remember the local paths it previously visited so that the
>+        MESSAGE_CHECK_URL() checks still work when process-swap on navigation is enabled.
>+
>+        Add back MESSAGE_CHECK_URL() on sourceURL in didPerformClientRedirect().
>+
>+        * UIProcess/Cocoa/WebPageProxyCocoa.mm:
>+        (WebKit::WebPageProxy::createSandboxExtensionsIfNeeded):
>+        * UIProcess/RemoteWebInspectorProxy.cpp:
>+        (WebKit::RemoteWebInspectorProxy::createFrontendPageAndWindow):
>+        * UIProcess/WebInspectorProxy.cpp:
>+        (WebKit::WebInspectorProxy::createFrontendPage):
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::loadRequestWithNavigation):
>+        (WebKit::WebPageProxy::loadFile):
>+        (WebKit::WebPageProxy::loadDataWithNavigation):
>+        (WebKit::WebPageProxy::loadAlternateHTML):
>+        (WebKit::WebPageProxy::reload):
>+        (WebKit::WebPageProxy::didPerformClientRedirect):
>+        (WebKit::WebPageProxy::backForwardGoToItem):
>+        (WebKit::WebPageProxy::checkURLReceivedFromCurrentOrPreviousWebProcess):
>+        (WebKit::WebPageProxy::addPreviouslyVisitedPath):
>+        (WebKit::WebPageProxy::willAcquireUniversalFileReadSandboxExtension):
>+        * UIProcess/WebPageProxy.h:
>+        * UIProcess/WebProcessProxy.cpp:
>+        (WebKit::WebProcessProxy::assumeReadAccessToBaseURL):
>+        * UIProcess/WebProcessProxy.h:
>+        * UIProcess/mac/WebPageProxyMac.mm:
>+
> 2018-12-06  Chris Dumez  <cdumez@apple.com>
> 
>         PSON logic gets confused by concurrent decidePolicyForNavigationAction requests
>diff --git a/Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm b/Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm
>index 65aecd92192fe6661f1baa42d340786ce4d1d532..f387b452ccfa4cf33c8d763ef90ecaae6c9561f5 100644
>--- a/Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm
>+++ b/Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm
>@@ -121,7 +121,7 @@ void WebPageProxy::createSandboxExtensionsIfNeeded(const Vector<String>& files,
>         BOOL isDirectory;
>         if ([[NSFileManager defaultManager] fileExistsAtPath:files[0] isDirectory:&isDirectory] && !isDirectory) {
>             SandboxExtension::createHandle("/", SandboxExtension::Type::ReadOnly, fileReadHandle);
>-            process().willAcquireUniversalFileReadSandboxExtension();
>+            willAcquireUniversalFileReadSandboxExtension();
>         }
>     }
> 
>diff --git a/Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp b/Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp
>index 927da1b639e211c1906c65e12530ce47c1715b06..3c9da53d85bc9ac2b235113c806917727910231a 100644
>--- a/Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp
>+++ b/Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp
>@@ -148,7 +148,7 @@ void RemoteWebInspectorProxy::createFrontendPageAndWindow()
>     trackInspectorPage(m_inspectorPage);
> 
>     m_inspectorPage->process().addMessageReceiver(Messages::RemoteWebInspectorProxy::messageReceiverName(), m_inspectorPage->pageID(), *this);
>-    m_inspectorPage->process().assumeReadAccessToBaseURL(WebInspectorProxy::inspectorBaseURL());
>+    m_inspectorPage->process().assumeReadAccessToBaseURL(*m_inspectorPage, WebInspectorProxy::inspectorBaseURL());
> }
> 
> void RemoteWebInspectorProxy::closeFrontendPageAndWindow()
>diff --git a/Source/WebKit/UIProcess/WebInspectorProxy.cpp b/Source/WebKit/UIProcess/WebInspectorProxy.cpp
>index bec502bd58bc7204cbb21ced6b940c8aec12be36..308c504c128cb05a16a12a4897b47c586c48453e 100644
>--- a/Source/WebKit/UIProcess/WebInspectorProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebInspectorProxy.cpp
>@@ -385,7 +385,7 @@ void WebInspectorProxy::createFrontendPage()
>     trackInspectorPage(m_inspectorPage);
> 
>     m_inspectorPage->process().addMessageReceiver(Messages::WebInspectorProxy::messageReceiverName(), m_inspectedPage->pageID(), *this);
>-    m_inspectorPage->process().assumeReadAccessToBaseURL(WebInspectorProxy::inspectorBaseURL());
>+    m_inspectorPage->process().assumeReadAccessToBaseURL(*m_inspectorPage, WebInspectorProxy::inspectorBaseURL());
> }
> 
> void WebInspectorProxy::openLocalInspectorFrontend(bool canAttach, bool underTest)
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index ccfa86947fe01e1ef0c3ddc0ad986ee0b84a4fb9..f84e9cdc8e075e83ccfd0cb2d611f31b32ea244c 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -240,7 +240,7 @@
> #define MERGE_WHEEL_EVENTS 1
> 
> #define MESSAGE_CHECK(assertion) MESSAGE_CHECK_BASE(assertion, m_process->connection())
>-#define MESSAGE_CHECK_URL(url) MESSAGE_CHECK_BASE(m_process->checkURLReceivedFromWebProcess(url), m_process->connection())
>+#define MESSAGE_CHECK_URL(url) MESSAGE_CHECK_BASE(checkURLReceivedFromCurrentOrPreviousWebProcess(url), m_process->connection())
> 
> #define RELEASE_LOG_IF_ALLOWED(channel, ...) RELEASE_LOG_IF(isAlwaysOnLoggingAllowed(), channel, __VA_ARGS__)
> 
>@@ -1065,7 +1065,7 @@ void WebPageProxy::loadRequestWithNavigation(API::Navigation& navigation, Resour
>     loadParameters.clientRedirectSourceForHistory = navigation.clientRedirectSourceForHistory();
>     bool createdExtension = maybeInitializeSandboxExtensionHandle(url, loadParameters.sandboxExtensionHandle);
>     if (createdExtension)
>-        m_process->willAcquireUniversalFileReadSandboxExtension();
>+        willAcquireUniversalFileReadSandboxExtension();
>     addPlatformLoadParameters(loadParameters);
> 
>     m_process->send(Messages::WebPage::LoadRequest(loadParameters), m_pageID);
>@@ -1109,7 +1109,7 @@ RefPtr<API::Navigation> WebPageProxy::loadFile(const String& fileURLString, cons
>     SandboxExtension::createHandle(resourceDirectoryPath, SandboxExtension::Type::ReadOnly, loadParameters.sandboxExtensionHandle);
>     addPlatformLoadParameters(loadParameters);
> 
>-    m_process->assumeReadAccessToBaseURL(resourceDirectoryURL);
>+    m_process->assumeReadAccessToBaseURL(*this, resourceDirectoryURL);
>     m_process->send(Messages::WebPage::LoadRequest(loadParameters), m_pageID);
>     m_process->responsivenessTimer().start();
> 
>@@ -1146,7 +1146,7 @@ void WebPageProxy::loadDataWithNavigation(API::Navigation& navigation, const IPC
>     loadParameters.userData = UserData(process().transformObjectsToHandles(userData).get());
>     addPlatformLoadParameters(loadParameters);
> 
>-    m_process->assumeReadAccessToBaseURL(baseURL);
>+    m_process->assumeReadAccessToBaseURL(*this, baseURL);
>     m_process->send(Messages::WebPage::LoadData(loadParameters), m_pageID);
>     m_process->responsivenessTimer().start();
> }
>@@ -1184,8 +1184,8 @@ void WebPageProxy::loadAlternateHTML(const IPC::DataReference& htmlData, const S
>     loadParameters.userData = UserData(process().transformObjectsToHandles(userData).get());
>     addPlatformLoadParameters(loadParameters);
> 
>-    m_process->assumeReadAccessToBaseURL(baseURL);
>-    m_process->assumeReadAccessToBaseURL(unreachableURL);
>+    m_process->assumeReadAccessToBaseURL(*this, baseURL);
>+    m_process->assumeReadAccessToBaseURL(*this, unreachableURL);
>     m_process->send(Messages::WebPage::LoadAlternateHTML(loadParameters), m_pageID);
>     m_process->responsivenessTimer().start();
> }
>@@ -1249,7 +1249,7 @@ RefPtr<API::Navigation> WebPageProxy::reload(OptionSet<WebCore::ReloadOption> op
>         // We may not have an extension yet if back/forward list was reinstated after a WebProcess crash or a browser relaunch
>         bool createdExtension = maybeInitializeSandboxExtensionHandle(URL(URL(), url), sandboxExtensionHandle);
>         if (createdExtension)
>-            m_process->willAcquireUniversalFileReadSandboxExtension();
>+            willAcquireUniversalFileReadSandboxExtension();
>     }
> 
>     if (!isValid())
>@@ -4481,6 +4481,7 @@ void WebPageProxy::didPerformClientRedirect(const String& sourceURLString, const
>     WebFrameProxy* frame = m_process->webFrame(frameID);
>     MESSAGE_CHECK(frame);
>     MESSAGE_CHECK(frame->page() == this);
>+    MESSAGE_CHECK_URL(sourceURLString);
>     MESSAGE_CHECK_URL(destinationURLString);
> 
>     if (frame->isMainFrame()) {
>@@ -5165,7 +5166,7 @@ void WebPageProxy::backForwardGoToItem(const BackForwardItemIdentifier& itemID,
> 
>     bool createdExtension = maybeInitializeSandboxExtensionHandle(URL(URL(), item->url()), sandboxExtensionHandle);
>     if (createdExtension)
>-        m_process->willAcquireUniversalFileReadSandboxExtension();
>+        willAcquireUniversalFileReadSandboxExtension();
>     m_backForwardList->goToItem(*item);
> }
> 
>@@ -8293,6 +8294,42 @@ void WebPageProxy::updateCurrentModifierState()
> #endif
> }
> 
>+bool WebPageProxy::checkURLReceivedFromCurrentOrPreviousWebProcess(const String& urlString)
>+{
>+    return checkURLReceivedFromCurrentOrPreviousWebProcess(URL(URL(), urlString));
>+}
>+
>+bool WebPageProxy::checkURLReceivedFromCurrentOrPreviousWebProcess(const URL& url)
>+{
>+    if (!url.isLocalFile())
>+        return true;
>+
>+    if (m_mayHaveUniversalFileReadSandboxExtension)
>+        return true;
>+
>+    String path = url.fileSystemPath();
>+    auto startsWithURLPath = [&path](const String& visitedPath) {
>+        return path.startsWith(visitedPath);
>+    };
>+
>+    auto localPathsEnd = m_previouslyVisitedPaths.end();
>+    if (std::find_if(m_previouslyVisitedPaths.begin(), localPathsEnd, startsWithURLPath) != localPathsEnd)
>+        return true;
>+
>+    return m_process->checkURLReceivedFromWebProcess(url);
>+}
>+
>+void WebPageProxy::addPreviouslyVisitedPath(const String& path)
>+{
>+    m_previouslyVisitedPaths.add(path);
>+}
>+
>+void WebPageProxy::willAcquireUniversalFileReadSandboxExtension()
>+{
>+    m_mayHaveUniversalFileReadSandboxExtension = true;
>+    process().willAcquireUniversalFileReadSandboxExtension();
>+}
>+
> #if ENABLE(DATA_DETECTION)
> 
> void WebPageProxy::detectDataInAllFrames(WebCore::DataDetectorTypes types, CompletionHandler<void(const DataDetectionResult&)>&& completionHandler)
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.h b/Source/WebKit/UIProcess/WebPageProxy.h
>index 5ed6070b9b32f88c2f49bce4f3c139fc556e4562..46ec516b706158fbc2ecc520f9468ec9a2ada29e 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.h
>+++ b/Source/WebKit/UIProcess/WebPageProxy.h
>@@ -361,6 +361,8 @@ public:
>     WebsiteDataStore& websiteDataStore() { return m_websiteDataStore; }
>     void changeWebsiteDataStore(WebsiteDataStore&);
> 
>+    void addPreviouslyVisitedPath(const String&);
>+
> #if ENABLE(DATA_DETECTION)
>     NSArray *dataDetectionResults() { return m_dataDetectionResults.get(); }
>     void detectDataInAllFrames(WebCore::DataDetectorTypes, CompletionHandler<void(const DataDetectionResult&)>&&);
>@@ -1844,6 +1846,10 @@ private:
>     void stopURLSchemeTask(uint64_t handlerIdentifier, uint64_t taskIdentifier);
>     void loadSynchronousURLSchemeTask(URLSchemeTaskParameters&&, Messages::WebPageProxy::LoadSynchronousURLSchemeTask::DelayedReply&&);
> 
>+    bool checkURLReceivedFromCurrentOrPreviousWebProcess(const String&);
>+    bool checkURLReceivedFromCurrentOrPreviousWebProcess(const URL&);
>+    void willAcquireUniversalFileReadSandboxExtension();
>+
>     void handleAutoFillButtonClick(const UserData&);
> 
>     void didResignInputElementStrongPasswordAppearance(const UserData&);
>@@ -2285,11 +2291,13 @@ private:
>     std::optional<SpellDocumentTag> m_spellDocumentTag;
> 
>     std::optional<MonotonicTime> m_pageLoadStart;
>+    HashSet<String> m_previouslyVisitedPaths;
> 
>     RunLoop::Timer<WebPageProxy> m_resetRecentCrashCountTimer;
>     unsigned m_recentCrashCount { 0 };
> 
>     bool m_needsFontAttributes { false };
>+    bool m_mayHaveUniversalFileReadSandboxExtension { false };
> 
> #if HAVE(PENCILKIT)
>     std::unique_ptr<EditableImageController> m_editableImageController;
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.cpp b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>index 3e5328114c3145200d6ebf57e8a3297cf44b2ed1..dd567f8b73cc064f36cac62d6d47ade3c93684c1 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>@@ -497,7 +497,7 @@ void WebProcessProxy::didDestroyWebUserContentControllerProxy(WebUserContentCont
>     m_webUserContentControllerProxies.remove(&proxy);
> }
> 
>-void WebProcessProxy::assumeReadAccessToBaseURL(const String& urlString)
>+void WebProcessProxy::assumeReadAccessToBaseURL(WebPageProxy& page, const String& urlString)
> {
>     URL url(URL(), urlString);
>     if (!url.isLocalFile())
>@@ -513,6 +513,7 @@ void WebProcessProxy::assumeReadAccessToBaseURL(const String& urlString)
>     // Client loads an alternate string. This doesn't grant universal file read, but the web process is assumed
>     // to have read access to this directory already.
>     m_localPathsWithAssumedReadAccess.add(path);
>+    page.addPreviouslyVisitedPath(path);
> }
> 
> bool WebProcessProxy::hasAssumedReadAccessToURL(const URL& url) const
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.h b/Source/WebKit/UIProcess/WebProcessProxy.h
>index 6f79a832642161e0c410c80501dadf469e405866..2361ae773a77efecbfa5649aee762e596e7bda97 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.h
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.h
>@@ -153,7 +153,7 @@ public:
>     void updateTextCheckerState();
> 
>     void willAcquireUniversalFileReadSandboxExtension() { m_mayHaveUniversalFileReadSandboxExtension = true; }
>-    void assumeReadAccessToBaseURL(const String&);
>+    void assumeReadAccessToBaseURL(WebPageProxy&, const String&);
>     bool hasAssumedReadAccessToURL(const URL&) const;
> 
>     bool checkURLReceivedFromWebProcess(const String&);
>diff --git a/Source/WebKit/UIProcess/mac/WebPageProxyMac.mm b/Source/WebKit/UIProcess/mac/WebPageProxyMac.mm
>index e2ab29c2b5c6717584637b3b7aecc208b7636864..d1ae09ab3a6aa2255ab2a9e84aa22a626da34ba9 100644
>--- a/Source/WebKit/UIProcess/mac/WebPageProxyMac.mm
>+++ b/Source/WebKit/UIProcess/mac/WebPageProxyMac.mm
>@@ -65,7 +65,7 @@
> #import <wtf/text/StringConcatenate.h>
> 
> #define MESSAGE_CHECK(assertion) MESSAGE_CHECK_BASE(assertion, process().connection())
>-#define MESSAGE_CHECK_URL(url) MESSAGE_CHECK_BASE(m_process->checkURLReceivedFromWebProcess(url), m_process->connection())
>+#define MESSAGE_CHECK_URL(url) MESSAGE_CHECK_BASE(checkURLReceivedFromCurrentOrPreviousWebProcess(url), m_process->connection())
> 
> using namespace WebCore;
>

Actions: View | Formatted Diff | Diff

Attachments on bug 191982: 355694 | 355736 | 355743 | 356848