Restore MESSAGE_CHECK_URL() security check on sourceURL in didPerformClientRedirect()
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Dec 2018 16:01:00 +0000 (16:01 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Dec 2018 16:01:00 +0000 (16:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=191982
<rdar://problem/46258054>

Reviewed by Alex Christensen.

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:

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm
Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp
Source/WebKit/UIProcess/WebInspectorProxy.cpp
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebPageProxy.h
Source/WebKit/UIProcess/WebProcessProxy.cpp
Source/WebKit/UIProcess/WebProcessProxy.h
Source/WebKit/UIProcess/mac/WebPageProxyMac.mm

index 3260d9a..4a78961 100644 (file)
@@ -1,3 +1,39 @@
+2018-12-12  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 Alex Christensen.
+
+        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-11-30  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [WPE] Add API to notify about frame displayed view backend callback
index 308d009..a50b5ef 100644 (file)
@@ -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();
         }
     }
 
index 927da1b..3c9da53 100644 (file)
@@ -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()
index bec502b..308c504 100644 (file)
@@ -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)
index 15186de..c54cde0 100644 (file)
 #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__)
 
@@ -1066,7 +1066,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);
@@ -1110,7 +1110,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();
 
@@ -1147,7 +1147,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();
 }
@@ -1185,8 +1185,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();
 }
@@ -1250,7 +1250,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())
@@ -4498,6 +4498,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()) {
@@ -5182,7 +5183,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);
 }
 
@@ -8312,6 +8313,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)
index e033ddb..a32e831 100644 (file)
@@ -362,6 +362,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&)>&&);
@@ -1849,6 +1851,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&);
@@ -2292,11 +2298,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;
index 3e53281..dd567f8 100644 (file)
@@ -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
index 6f79a83..2361ae7 100644 (file)
@@ -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&);
index e2ab29c..d1ae09a 100644 (file)
@@ -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;