[webkit-changes] [WebKit/WebKit] e3a441: Regression: Third-party service worker processes g...

[Chris Dumez]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: e3a44192dffa8a8c75699347efd3df4ac4defbd0
      https://github.com/WebKit/WebKit/commit/e3a44192dffa8a8c75699347efd3df4ac4defbd0
  Author: Chris Dumez <cdumez at apple.com>
  Date:   2024-03-26 (Tue, 26 Mar 2024)

  Changed paths:
    M Source/WebCore/workers/service/server/SWServer.cpp
    M Source/WebCore/workers/service/server/SWServerToContextConnection.cpp
    M Source/WebCore/workers/service/server/SWServerWorker.cpp
    M Source/WebCore/workers/service/server/SWServerWorker.h
    M Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp
    M Source/WebKit/NetworkProcess/SharedWorker/WebSharedWorker.cpp
    M Source/WebKit/NetworkProcess/SharedWorker/WebSharedWorker.h
    M Source/WebKit/NetworkProcess/SharedWorker/WebSharedWorkerServer.cpp
    M Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm

  Log Message:
  -----------
  Regression: Third-party service worker processes get killed when trying to do loads
https://bugs.webkit.org/show_bug.cgi?id=267298
rdar://120587179

Reviewed by Youenn Fablet.

We recently added origin IPC message checks to make sure that WebProcesses can
only access cookies from specific origins. Due to a bug in our service worker
code, the network process would allow the wrong origin in the case of
third-party service workers (it would allow the client origin instead of the
top origin). As a result, the network process would kill the third-party
service worker process as soon as it would try to attempt a load. This was
occurring on https://boingboing.net/2024/01/06/stanley-water-bottle-madness-grips-america.html
for example, with the tiktok.com service worker.

When the network process needs to launch a service worker process, it sends an
EstablishRemoteWorkerContextConnectionToNetworkProcess IPC to the UIProcess,
with a given registrable domain. The UIProcess would use this registrable
domain to select a suitable WebProcess or launch one. When launching a new
process, the UIProcess would send an IPC to the network process telling it this
new WebProcess is allowed to access cookies under the given registrable domain.

Both from the process selection point of view and from the network process
cookie access point of view, we expect this registrable domain to be the top
origin's registrable domain. As a result, in the example above, the third-party
tiktok.com service worker under boingboing.net, would be expected to use a
"boingboing.net" WebProcess and only have access to cookies under the
"boingboing.net" first party.

However, our service worker logic was using the registrable domain of the
service worker script URL instead. As a result, we would select a "tiktok.com"
WebProcess for the service worker process, which was wrong from a site
isolation perspective (since top-level tiktok.com would share the same process
as tiktok.com under boingboing.net). Also, the network process would allow
access to top-level tiktok.com cookies if the WebProcess requested them, which
was wrong too.

Later on, the service worker would try to do a load. The network request would
request use "boingboing.net" as firstParty for cookies, which is correct.
However, the network process would reject such load, since the process is only
allowed to use "tiktok.com" as first party for cookies. It would then kill the
service worker process for good measure since it would assume it is compromised.

To address the issue, we now properly use the registrable domain of the top
level origin when sending the EstablishRemoteWorkerContextConnectionToNetworkProcess
IPC for and service worker connection selection in general. I updated the shared
worker code as well to maintain consistency.

Note that in order to write an API test for this, I had to restore the service
worker from disk first. When the service worker is newly registered by JS, we
would first tell the network process to allow the wrong client origin. However,
a later IPC to tell the network process to also allow the top level origin. As
a result, newly registered third-party service workers would not get terminated
which is why our tests were passing. Those service worker origins were allowed
access to cookies they shouldn't have access to though so there was a security
issue still for them.
When restoring the service worker from disk though, we'd only send a single IPC
to the network process telling it to allow the original of the service worker
script URL. This allowed me to write a test and explains the service worker
processes terminations on boingboing.net.

* Source/WebCore/workers/service/server/SWServer.cpp:
(WebCore::SWServer::addRegistrationFromStore):
(WebCore::SWServer::scheduleJob):
(WebCore::SWServer::tryInstallContextData):
(WebCore::SWServer::runServiceWorkerIfNecessary):
(WebCore::SWServer::markAllWorkersForRegistrableDomainAsTerminated):
(WebCore::SWServer::removeContextConnectionIfPossible):
(WebCore::SWServer::fireFunctionalEvent):
* Source/WebCore/workers/service/server/SWServerToContextConnection.cpp:
(WebCore::SWServerToContextConnection::terminateWhenPossible):
* Source/WebCore/workers/service/server/SWServerWorker.cpp:
(WebCore::m_lastNavigationWasAppInitiated):
(WebCore::SWServerWorker::contextConnection):
(WebCore::SWServerWorker::terminationIfPossibleTimerFired):
* Source/WebCore/workers/service/server/SWServerWorker.h:
(WebCore::SWServerWorker::topRegistrableDomain const):
(WebCore::SWServerWorker::registrableDomain const): Deleted.
* Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:
(WebKit::WebSWServerConnection::startFetch):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:
(-[SWMessageHandlerForRestoreFromDiskTest resetExpectedMessage:]):

Originally-landed-as: 272448.87 at safari-7618-branch (b70a943d3745). rdar://124556696
Canonical link: https://commits.webkit.org/276704@main



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications