Changeset 249649 in webkit

Timestamp:

Sep 9, 2019 10:44:33 AM (5 years ago)

Author:

pvollan@apple.com

Message:

[macOS] Pid is sometimes invalid when creating sandbox extensions by pid.
​https://bugs.webkit.org/show_bug.cgi?id=201543
<rdar://problem/54733465>

Reviewed by Brent Fulgham.

There is a race condition when starting a load of a local file, where the WebContent process has not finished
launching yet, and its pid is not available. When we try to create a sandbox extension by using the pid of the
WebContent process, it is not available in the cases where the WebContent process has just launched and has not
finished launching yet. This patch creates a new dummy Web page message, 'LoadRequestWaitingForPID', which will
be sent instead of a normal 'LoadRequest' message, and only when the WebContent process has not finished
launching. When the WebContent process has finished launching, and we are about to actually send the pending
messages, we can detect that a 'LoadRequestWaitingForPID' has been appended for sending, and replace it with a
normal 'LoadReqest' message where we have created the sandbox extension issue with a valid pid. The message
'LoadRequestWaitingForPID' is never intended to reach the WebContent process, it is just there to replace with
a normal 'LoadRequest' message with a new sandbox extension. In the implementation of the message handler on
the WebContent process side, we assert that the method is never called. This patch makes sure the ordering of
the Web page messages are the same, even when we modify the message.

• UIProcess/AuxiliaryProcessProxy.cpp:

(WebKit::AuxiliaryProcessProxy::didFinishLaunching):

• UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::maybeInitializeSandboxExtensionHandle):
(WebKit::WebPageProxy::loadRequestWithNavigationShared):
(WebKit::WebPageProxy::loadFile):

• WebProcess/WebPage/WebPage.cpp:

(WebKit::WebPage::fileLoadRequest):

• WebProcess/WebPage/WebPage.h:
• WebProcess/WebPage/WebPage.messages.in:

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/AuxiliaryProcessProxy.cpp (modified) (2 diffs)
• UIProcess/WebPageProxy.cpp (modified) (5 diffs)
• WebProcess/WebPage/WebPage.cpp (modified) (1 diff)
• WebProcess/WebPage/WebPage.h (modified) (1 diff)
• WebProcess/WebPage/WebPage.messages.in (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-09-09  Per Arne Vollan  <pvollan@apple.com>
+
+        [macOS] Pid is sometimes invalid when creating sandbox extensions by pid.
+        https://bugs.webkit.org/show_bug.cgi?id=201543
+        <rdar://problem/54733465>
+
+        Reviewed by Brent Fulgham.
+
+        There is a race condition when starting a load of a local file, where the WebContent process has not finished
+        launching yet, and its pid is not available. When we try to create a sandbox extension by using the pid of the
+        WebContent process, it is not available in the cases where the WebContent process has just launched and has not
+        finished launching yet. This patch creates a new dummy Web page message, 'LoadRequestWaitingForPID', which will
+        be sent instead of a normal 'LoadRequest' message, and only when the WebContent process has not finished
+        launching. When the WebContent process has finished launching, and we are about to actually send the pending
+        messages, we can detect that a 'LoadRequestWaitingForPID' has been appended for sending, and replace it with a
+        normal 'LoadReqest' message where we have created the sandbox extension issue with a valid pid. The message
+        'LoadRequestWaitingForPID' is never intended to reach the WebContent process, it is just there to replace with
+        a normal 'LoadRequest' message with a new sandbox extension. In the implementation of the message handler on
+        the WebContent process side, we assert that the method is never called. This patch makes sure the ordering of
+        the Web page messages are the same, even when we modify the message.
+
+        * UIProcess/AuxiliaryProcessProxy.cpp:
+        (WebKit::AuxiliaryProcessProxy::didFinishLaunching):
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::maybeInitializeSandboxExtensionHandle):
+        (WebKit::WebPageProxy::loadRequestWithNavigationShared):
+        (WebKit::WebPageProxy::loadFile):
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::fileLoadRequest):
+        * WebProcess/WebPage/WebPage.h:
+        * WebProcess/WebPage/WebPage.messages.in:
+
 2019-09-09  Youenn Fablet  <youenn@apple.com>
 

trunk/Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp

@@ -28 +28 @@
 
 #include "AuxiliaryProcessMessages.h"
+#include "LoadParameters.h"
+#include "WebPageMessages.h"
 #include <wtf/RunLoop.h>
 
@@ -176 +178 @@
         std::unique_ptr<IPC::Encoder> message = WTFMove(m_pendingMessages[i].first);
         OptionSet<IPC::SendOption> sendOptions = m_pendingMessages[i].second;
+#if HAVE(SANDBOX_ISSUE_MACH_EXTENSION_TO_PROCESS_BY_PID)
+        if (message->messageName() == "LoadRequestWaitingForPID") {
+            auto buffer = message->buffer();
+            auto bufferSize = message->bufferSize();
+            std::unique_ptr<IPC::Decoder> decoder = makeUnique<IPC::Decoder>(buffer, bufferSize, nullptr, Vector<IPC::Attachment> { });
+            LoadParameters loadParameters;
+            String sandboxExtensionPath;
+            if (decoder->decode(loadParameters) && decoder->decode(sandboxExtensionPath)) {
+                SandboxExtension::createHandleForReadByPid(sandboxExtensionPath, processIdentifier(), loadParameters.sandboxExtensionHandle);
+                send(Messages::WebPage::LoadRequest(loadParameters), decoder->destinationID());
+                continue;
+            }
+        }
+#endif
         m_connection->sendMessage(WTFMove(message), sendOptions);
     }

trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -1072 +1072 @@
 #if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID)
         if (SandboxExtension::createHandleForReadByPid(resourceDirectoryURL.fileSystemPath(), process.processIdentifier(), sandboxExtensionHandle)) {
-            m_process->assumeReadAccessToBaseURL(*this, resourceDirectoryURL);
+            process.assumeReadAccessToBaseURL(*this, resourceDirectoryURL);
             return;
         }
-#endif
+#else
         if (SandboxExtension::createHandle(resourceDirectoryURL.fileSystemPath(), SandboxExtension::Type::ReadOnly, sandboxExtensionHandle)) {
-            m_process->assumeReadAccessToBaseURL(*this, resourceDirectoryURL);
+            process.assumeReadAccessToBaseURL(*this, resourceDirectoryURL);
             return;
         }
+#endif
     }
 
@@ -1093 +1094 @@
         return;
     }
-#endif
+#else
     if (SandboxExtension::createHandle("/", SandboxExtension::Type::ReadOnly, sandboxExtensionHandle)) {
         willAcquireUniversalFileReadSandboxExtension(process);
         return;
     }
+#endif
 
 #if PLATFORM(COCOA)
@@ -1107 +1109 @@
     auto baseURL = URL(URL(), url.baseAsString());
     auto basePath = baseURL.fileSystemPath();
-    if (!basePath.isNull() && SandboxExtension::createHandle(basePath, SandboxExtension::Type::ReadOnly, sandboxExtensionHandle))
-        m_process->assumeReadAccessToBaseURL(*this, baseURL);
+    if (basePath.isNull())
+        return;
+#if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID)
+    if (SandboxExtension::createHandleForReadByPid(basePath, process.processIdentifier(), sandboxExtensionHandle))
+        process.assumeReadAccessToBaseURL(*this, baseURL);
+#else
+    if (SandboxExtension::createHandle(basePath, SandboxExtension::Type::ReadOnly, sandboxExtensionHandle))
+        process.assumeReadAccessToBaseURL(*this, baseURL);
+#endif
 }
 
@@ -1166 +1175 @@
     addPlatformLoadParameters(loadParameters);
 
+#if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID)
+    if (processIdentifier() || !url.isLocalFile())
+        process->send(Messages::WebPage::LoadRequest(loadParameters), webPageID);
+    else {
+        String sandboxExtensionPath;
+        if (!m_pageLoadState.resourceDirectoryURL().isEmpty()) {
+            sandboxExtensionPath = m_pageLoadState.resourceDirectoryURL().fileSystemPath();
+            process->assumeReadAccessToBaseURL(*this, m_pageLoadState.resourceDirectoryURL());
+        } else {
+            sandboxExtensionPath = "/";
+            willAcquireUniversalFileReadSandboxExtension(process);
+        }
+        process->send(Messages::WebPage::LoadRequestWaitingForPID(loadParameters, sandboxExtensionPath), webPageID);
+    }
+#else
     process->send(Messages::WebPage::LoadRequest(loadParameters), webPageID);
+#endif
     process->responsivenessTimer().start();
 }
@@ -1213 +1238 @@
     loadParameters.userData = UserData(process().transformObjectsToHandles(userData).get());
 #if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID)
-    if (!SandboxExtension::createHandleForReadByPid(resourceDirectoryPath, processIdentifier(), loadParameters.sandboxExtensionHandle))
-#endif
+    SandboxExtension::createHandleForReadByPid(resourceDirectoryPath, processIdentifier(), loadParameters.sandboxExtensionHandle);
+#else
     SandboxExtension::createHandle(resourceDirectoryPath, SandboxExtension::Type::ReadOnly, loadParameters.sandboxExtensionHandle);
+#endif
     addPlatformLoadParameters(loadParameters);
 
     m_process->assumeReadAccessToBaseURL(*this, resourceDirectoryURL);
+#if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID)
+    if (processIdentifier())
+        m_process->send(Messages::WebPage::LoadRequest(loadParameters), m_webPageID);
+    else
+        m_process->send(Messages::WebPage::LoadRequestWaitingForPID(loadParameters, resourceDirectoryPath), m_webPageID);
+#else
     m_process->send(Messages::WebPage::LoadRequest(loadParameters), m_webPageID);
+#endif
     m_process->responsivenessTimer().start();
 

trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp

@@ -1554 +1554 @@
 }
 
+// LoadRequestWaitingForPID should never be sent to the WebProcess. It must always be converted to a LoadRequest message.
+NO_RETURN void WebPage::loadRequestWaitingForPID(LoadParameters&&, const String&)
+{
+    RELEASE_ASSERT_NOT_REACHED();
+}
+
 void WebPage::loadDataImpl(uint64_t navigationID, bool shouldTreatAsContinuingLoad, Optional<WebsitePoliciesData>&& websitePolicies, Ref<SharedBuffer>&& sharedBuffer, const String& MIMEType, const String& encodingName, const URL& baseURL, const URL& unreachableURL, const UserData& userData, ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy)
 {

trunk/Source/WebKit/WebProcess/WebPage/WebPage.h

@@ -1318 +1318 @@
     void platformDidReceiveLoadParameters(const LoadParameters&);
     void loadRequest(LoadParameters&&);
+    void loadRequestWaitingForPID(LoadParameters&&, const String&);
     void loadData(LoadParameters&&);
     void loadAlternateHTML(LoadParameters&&);

trunk/Source/WebKit/WebProcess/WebPage/WebPage.messages.in

@@ -166 +166 @@
     LoadDataInFrame(IPC::DataReference data, String MIMEType, String encodingName, URL baseURL, WebCore::FrameIdentifier frameID)
     LoadRequest(struct WebKit::LoadParameters loadParameters)
+    LoadRequestWaitingForPID(struct WebKit::LoadParameters loadParameters, String sandboxExtensionPath)
     LoadData(struct WebKit::LoadParameters loadParameters)
     LoadAlternateHTML(struct WebKit::LoadParameters loadParameters)