[14] SWServer dereferences unchecked HashMap end iterator

Severity: Low | Component: WebCore Service Worker Server | 3bce213

Rated Low because the diff fixes a HashMap end-iterator dereference reachable only when paired client maps desynchronise across a race the commit message states has no known reproducer; impact is bounded to a crash in the service-worker bookkeeping path, with no evidence of attacker-controlled read/write.

A crash occurs in topLevelServiceWorkerClientFromPageIdentifier() when the Service Worker maps m_clientIdentifiersPerOrigin and m_clientsById become out-of-sync. The function iterates over client identifiers from m_clientIdentifiersPerOrigin and calls m_clientsById.find() for each one, but never checks whether the result equals m_clientsById.end() before dereferencing.

Source/WebCore/workers/service/server/SWServer.cpp

 auto clientIterator = m_clientsById.find(clientIdentifier);
-ASSERT(clientIterator != m_clientsById.end());
+if (clientIterator == m_clientsById.end()) [[unlikely]] {
+    ASSERT_NOT_REACHED();
+    return std::nullopt;
+}
 return clientIterator->value;
...
 for (auto clientIdentifier : iterator->value.identifiers) {
     auto clientIterator = m_clientsById.find(clientIdentifier);
+    if (clientIterator == m_clientsById.end()) {
+        ASSERT_NOT_REACHED();
+        continue;
+    }
     if (clientIterator->value->frameType == ServiceWorkerClientFrameType::TopLevel && clientIterator->value->pageIdentifier == pageIdentifier)
         return clientIterator->value;
 }

Patch Details

In serviceWorkerClientWithOriginByID(), the previous ASSERT(clientIterator != m_clientsById.end()) is replaced with a real end-iterator check that returns std::nullopt in release builds. In topLevelServiceWorkerClientFromPageIdentifier(), the loop that iterates over iterator->value.identifiers and calls m_clientsById.find(clientIdentifier) previously dereferenced unconditionally; an end-iterator check with ASSERT_NOT_REACHED() and continue is added so missing entries are skipped.

Dereferencing a HashMap iterator without an end-check when paired-map invariants can desynchronize across asynchronous lifecycle paths.

Background

SWServer is the cross-process owner of service-worker state for a session. m_clientIdentifiersPerOrigin is a HashMap<ClientOrigin, ...> whose value type carries an identifiers set listing every ScriptExecutionContextIdentifier that has registered as a client of that origin. m_clientsById is a separate HashMap<ScriptExecutionContextIdentifier, RefPtr<ServiceWorkerClientData>> keyed by the same identifier. The intended invariant is that every identifier appearing in any per-origin identifiers set has a corresponding entry in m_clientsById. HashMap::find() returns an iterator equal to end() when the key is not present, and dereferencing that end iterator is undefined behaviour. ASSERT(...) in WebCore is compiled out in release builds, so an ASSERT-only guard provides no runtime protection.

Analysis

topLevelServiceWorkerClientFromPageIdentifier() obtained a client identifier from the per-origin set in m_clientIdentifiersPerOrigin and then performed m_clientsById.find(clientIdentifier) without checking whether the returned iterator equals m_clientsById.end(). The function then dereferenced clientIterator->value. The two maps are populated and torn down through separate code paths — the commit message describes this as a race that can leave them out-of-sync.

serviceWorkerClientWithOriginByID() had the same shape but with only an ASSERT — fine in debug builds, removed in release, so release builds dereferenced the end iterator unconditionally.

This vulnerability weakened availability of the network process that hosts SWServer. The pre-fix code reaches a HashMap end-iterator dereference whenever the two client maps desynchronise; the reachable consequence is a crash in the service-worker bookkeeping path, with no evidence in the diff of attacker-controlled memory contents being written or read. Two parallel HashMaps keyed off the same identifier, maintained by separate code paths with an implicit invariant enforced only by ASSERT, is a recurring WebKit pattern — the commit message itself notes that clientIsAppInitiatedForRegistrableDomain() already had the defensive end-check; the bug is that sibling methods diverged from it.

Audit directions

Paired HashMaps with an implicit "every key in map A appears in map B" invariant, guarded only by ASSERT. Audit Source/WebCore/workers/service/server/SWServer.cpp and related SWServer* files for other .find() sites on m_clientsById, m_clientIdentifiersPerOrigin, m_registrations, and m_runningOrTerminatingWorkers that dereference the iterator without an end-check.
ASSERT(iterator != container.end()) immediately followed by iterator->value. Grep Source/WebCore/ and Source/WebKit/NetworkProcess/ServiceWorker/ for the regex ASSERT\(.*!= .*\.end\(\)\); followed within five lines by a dereference of the same iterator. Each hit is a candidate for the same defensive-rewrite.
Registration/unregistration code paths maintaining m_clientIdentifiersPerOrigin and m_clientsById. Search for m_clientsById.add, m_clientsById.remove, m_clientIdentifiersPerOrigin.add, .identifiers.add, .identifiers.remove) to determine which sequence drops one map's entry without the other. Verify whether either map can be mutated re-entrantly from IPC delivery while the other is being iterated.
clientIterator->value->frameType chains where clientIterator->value is a RefPtr/std::unique_ptr that could be null even when the iterator is valid. Check whether m_clientsById ever stores a null value transiently during construction or teardown of a ServiceWorkerClientData.