Protect NetworkProcess::m_swServers from bad session IDs
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 28 Jun 2019 21:53:42 +0000 (21:53 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 28 Jun 2019 21:53:42 +0000 (21:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199298
<rdar://problem/51859081>

Reviewed by Chris Dumez.

Protect NetworkProcess from receiving bad session IDs in service worker code path by checking for session ID validity whenever interacting with the map.
One of the check is done in WebProcess in which case, if the session ID is bad, the SW connection to NetworkProcess will not be made.
For bad session IDs, this will in that case trigger timing out of service worker operations.

For get/clear data, exit early in case of bad session ID.

Made some refactoring to remove swOriginStoreForSession method.
In the one call site where it is used, the store should already be created so we reuse existingSWOriginStoreForSession.

Added a bunch of additional ASSERTs.

* NetworkProcess/NetworkProcess.cpp:
(WebKit::NetworkProcess::destroySession):
(WebKit::NetworkProcess::fetchWebsiteData):
(WebKit::NetworkProcess::deleteWebsiteData):
(WebKit::NetworkProcess::deleteWebsiteDataForRegistrableDomains):
(WebKit::NetworkProcess::registrableDomainsWithWebsiteData):
(WebKit::NetworkProcess::actualPrepareToSuspend):
(WebKit::NetworkProcess::swServerForSession):
(WebKit::NetworkProcess::existingSWOriginStoreForSession const):
(WebKit::NetworkProcess::registerSWServerConnection):
* NetworkProcess/NetworkProcess.h:
* WebProcess/Network/NetworkProcessConnection.cpp:
(WebKit::NetworkProcessConnection::initializeSWClientConnection):
* WebProcess/Storage/WebSWClientConnection.cpp:
(WebKit::WebSWClientConnection::WebSWClientConnection):
(WebKit::WebSWClientConnection::initializeConnectionIfNeeded):
(WebKit::WebSWClientConnection::ensureConnectionAndSend):

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

Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/NetworkProcess.cpp
Source/WebKit/NetworkProcess/NetworkProcess.h
Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp
Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp

index ea1a468..5fa74c4 100644 (file)
@@ -1,3 +1,40 @@
+2019-06-28  Youenn Fablet  <youenn@apple.com>
+
+        Protect NetworkProcess::m_swServers from bad session IDs
+        https://bugs.webkit.org/show_bug.cgi?id=199298
+        <rdar://problem/51859081>
+
+        Reviewed by Chris Dumez.
+
+        Protect NetworkProcess from receiving bad session IDs in service worker code path by checking for session ID validity whenever interacting with the map.
+        One of the check is done in WebProcess in which case, if the session ID is bad, the SW connection to NetworkProcess will not be made.
+        For bad session IDs, this will in that case trigger timing out of service worker operations.
+
+        For get/clear data, exit early in case of bad session ID.
+
+        Made some refactoring to remove swOriginStoreForSession method.
+        In the one call site where it is used, the store should already be created so we reuse existingSWOriginStoreForSession.
+
+        Added a bunch of additional ASSERTs.
+
+        * NetworkProcess/NetworkProcess.cpp:
+        (WebKit::NetworkProcess::destroySession):
+        (WebKit::NetworkProcess::fetchWebsiteData):
+        (WebKit::NetworkProcess::deleteWebsiteData):
+        (WebKit::NetworkProcess::deleteWebsiteDataForRegistrableDomains):
+        (WebKit::NetworkProcess::registrableDomainsWithWebsiteData):
+        (WebKit::NetworkProcess::actualPrepareToSuspend):
+        (WebKit::NetworkProcess::swServerForSession):
+        (WebKit::NetworkProcess::existingSWOriginStoreForSession const):
+        (WebKit::NetworkProcess::registerSWServerConnection):
+        * NetworkProcess/NetworkProcess.h:
+        * WebProcess/Network/NetworkProcessConnection.cpp:
+        (WebKit::NetworkProcessConnection::initializeSWClientConnection):
+        * WebProcess/Storage/WebSWClientConnection.cpp:
+        (WebKit::WebSWClientConnection::WebSWClientConnection):
+        (WebKit::WebSWClientConnection::initializeConnectionIfNeeded):
+        (WebKit::WebSWClientConnection::ensureConnectionAndSend):
+
 2019-06-28  Timothy Hatcher  <timothy@apple.com>
 
         Rename effectiveAppearanceIsInactive and useInactiveAppearance to better match UIUserInterfaceLevel.
index c3bca81..3ae2c9e 100644 (file)
@@ -577,6 +577,10 @@ void NetworkProcess::setSession(const PAL::SessionID& sessionID, Ref<NetworkSess
 
 void NetworkProcess::destroySession(const PAL::SessionID& sessionID)
 {
+    ASSERT(sessionID.isValid());
+    if (!sessionID.isValid())
+        return;
+
     ASSERT(sessionID != PAL::SessionID::defaultSessionID());
 
     if (auto session = m_networkSessions.take(sessionID))
@@ -1250,6 +1254,10 @@ static void fetchDiskCacheEntries(NetworkCache::Cache* cache, PAL::SessionID ses
 
 void NetworkProcess::fetchWebsiteData(PAL::SessionID sessionID, OptionSet<WebsiteDataType> websiteDataTypes, OptionSet<WebsiteDataFetchOption> fetchOptions, uint64_t callbackID)
 {
+    ASSERT(sessionID.isValid());
+    if (!sessionID.isValid())
+        return;
+
     struct CallbackAggregator final : public ThreadSafeRefCounted<CallbackAggregator> {
         explicit CallbackAggregator(Function<void (WebsiteData)>&& completionHandler)
             : m_completionHandler(WTFMove(completionHandler))
@@ -1344,6 +1352,10 @@ void NetworkProcess::fetchWebsiteData(PAL::SessionID sessionID, OptionSet<Websit
 
 void NetworkProcess::deleteWebsiteData(PAL::SessionID sessionID, OptionSet<WebsiteDataType> websiteDataTypes, WallTime modifiedSince, uint64_t callbackID)
 {
+    ASSERT(sessionID.isValid());
+    if (!sessionID.isValid())
+        return;
+
 #if PLATFORM(COCOA)
     if (websiteDataTypes.contains(WebsiteDataType::HSTSCache)) {
         if (auto* networkStorageSession = storageSession(sessionID))
@@ -1551,6 +1563,10 @@ static Vector<WebCore::SecurityOriginData> filterForRegistrableDomains(const Has
 
 void NetworkProcess::deleteWebsiteDataForRegistrableDomains(PAL::SessionID sessionID, OptionSet<WebsiteDataType> websiteDataTypes, HashMap<RegistrableDomain, WebsiteDataToRemove>&& domains, bool shouldNotifyPage, CompletionHandler<void(const HashSet<RegistrableDomain>&)>&& completionHandler)
 {
+    ASSERT(sessionID.isValid());
+    if (!sessionID.isValid())
+        return;
+
     OptionSet<WebsiteDataFetchOption> fetchOptions = WebsiteDataFetchOption::DoNotCreateProcesses;
 
     struct CallbackAggregator final : public ThreadSafeRefCounted<CallbackAggregator> {
@@ -1749,6 +1765,10 @@ void NetworkProcess::deleteCookiesForTesting(PAL::SessionID sessionID, Registrab
 
 void NetworkProcess::registrableDomainsWithWebsiteData(PAL::SessionID sessionID, OptionSet<WebsiteDataType> websiteDataTypes, bool shouldNotifyPage, CompletionHandler<void(HashSet<RegistrableDomain>&&)>&& completionHandler)
 {
+    ASSERT(sessionID.isValid());
+    if (!sessionID.isValid())
+        return;
+
     OptionSet<WebsiteDataFetchOption> fetchOptions = WebsiteDataFetchOption::DoNotCreateProcesses;
     
     struct CallbackAggregator final : public ThreadSafeRefCounted<CallbackAggregator> {
@@ -2035,8 +2055,10 @@ void NetworkProcess::actualPrepareToSuspend(ShouldAcknowledgeWhenReadyToSuspend
         connection->cleanupForSuspension([delayedTaskCounter] { });
 
 #if ENABLE(SERVICE_WORKER)
-    for (auto& server : m_swServers.values())
+    for (auto& server : m_swServers.values()) {
+        ASSERT(m_swServers.get(server->sessionID()) == server.get());
         server->startSuspension([delayedTaskCounter] { });
+    }
 #endif
 
     for (auto& session : m_networkSessions)
@@ -2367,7 +2389,7 @@ bool NetworkProcess::needsServerToContextConnectionForRegistrableDomain(const Re
 SWServer& NetworkProcess::swServerForSession(PAL::SessionID sessionID)
 {
     ASSERT(sessionID.isValid());
-    
+
     auto result = m_swServers.ensure(sessionID, [&] {
         auto path = m_swDatabasePaths.get(sessionID);
         // There should already be a registered path for this PAL::SessionID.
@@ -2383,13 +2405,12 @@ SWServer& NetworkProcess::swServerForSession(PAL::SessionID sessionID)
     return *result.iterator->value;
 }
 
-WebSWOriginStore& NetworkProcess::swOriginStoreForSession(PAL::SessionID sessionID)
-{
-    return static_cast<WebSWOriginStore&>(swServerForSession(sessionID).originStore());
-}
-
 WebSWOriginStore* NetworkProcess::existingSWOriginStoreForSession(PAL::SessionID sessionID) const
 {
+    ASSERT(sessionID.isValid());
+    if (!sessionID.isValid())
+        return nullptr;
+
     auto* swServer = m_swServers.get(sessionID);
     if (!swServer)
         return nullptr;
@@ -2430,7 +2451,10 @@ void NetworkProcess::registerSWServerConnection(WebSWServerConnection& connectio
     ASSERT(parentProcessHasServiceWorkerEntitlement());
     ASSERT(!m_swServerConnections.contains(connection.identifier()));
     m_swServerConnections.add(connection.identifier(), &connection);
-    swOriginStoreForSession(connection.sessionID()).registerSWServerConnection(connection);
+    auto* store = existingSWOriginStoreForSession(connection.sessionID());
+    ASSERT(store);
+    if (store)
+        store->registerSWServerConnection(connection);
 }
 
 void NetworkProcess::unregisterSWServerConnection(WebSWServerConnection& connection)
index 0e1dabd..4f09984 100644 (file)
@@ -463,7 +463,6 @@ private:
     
     void disableServiceWorkerProcessTerminationDelay();
     
-    WebSWOriginStore& swOriginStoreForSession(PAL::SessionID);
     WebSWOriginStore* existingSWOriginStoreForSession(PAL::SessionID) const;
     bool needsServerToContextConnectionForRegistrableDomain(const WebCore::RegistrableDomain&) const;
 
index 90b4b54..d9adcc2 100644 (file)
@@ -279,6 +279,7 @@ void NetworkProcessConnection::removeSWClientConnection(WebSWClientConnection& c
 
 SWServerConnectionIdentifier NetworkProcessConnection::initializeSWClientConnection(WebSWClientConnection& connection)
 {
+    ASSERT(connection.sessionID().isValid());
     SWServerConnectionIdentifier identifier;
     bool result = m_connection->sendSync(Messages::NetworkConnectionToWebProcess::EstablishSWServerConnection(connection.sessionID()), Messages::NetworkConnectionToWebProcess::EstablishSWServerConnection::Reply(identifier), 0);
     ASSERT_UNUSED(result, result);
index 5790545..2b5ed70 100644 (file)
@@ -56,7 +56,6 @@ WebSWClientConnection::WebSWClientConnection(SessionID sessionID)
     : m_sessionID(sessionID)
     , m_swOriginTable(makeUniqueRef<WebSWOriginTable>())
 {
-    ASSERT(sessionID.isValid());
     initializeConnectionIfNeeded();
 }
 
@@ -68,6 +67,10 @@ WebSWClientConnection::~WebSWClientConnection()
 
 void WebSWClientConnection::initializeConnectionIfNeeded()
 {
+    ASSERT(m_sessionID.isValid());
+    if (!m_sessionID.isValid())
+        return;
+
     if (m_connection)
         return;
 
@@ -83,7 +86,8 @@ template<typename U>
 void WebSWClientConnection::ensureConnectionAndSend(const U& message)
 {
     initializeConnectionIfNeeded();
-    send(message);
+    if (m_connection)
+        send(message);
 }
 
 void WebSWClientConnection::scheduleJobInServer(const ServiceWorkerJobData& jobData)