Skip to content

Commit

Permalink
Revert (266074@main): Arbitrary cookie access via NetworkConnectionTo…
Browse files Browse the repository at this point in the history
…WebProcess::cookiesForDOM

https://bugs.webkit.org/show_bug.cgi?id=259806
rdar://113364311

Reviewed by J Pascoe.

Causes multiple web process crashes.

* Source/WebCore/loader/PingLoader.cpp:
(WebCore::PingLoader::sendViolationReport):
* Source/WebCore/loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::requestResource):
* Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:
(WebKit::NetworkConnectionToWebProcess::createSocketChannel):
(WebKit::NetworkConnectionToWebProcess::scheduleResourceLoad):
(WebKit::NetworkConnectionToWebProcess::cookiesForDOM):
(WebKit::NetworkConnectionToWebProcess::setCookiesFromDOM):
(WebKit::NetworkConnectionToWebProcess::cookieRequestHeaderFieldValue):
(WebKit::NetworkConnectionToWebProcess::getRawCookies):
(WebKit::NetworkConnectionToWebProcess::cookiesForDOMAsync):
(WebKit::NetworkConnectionToWebProcess::setCookieFromDOMAsync):
(WebKit::NetworkConnectionToWebProcess::domCookiesForHost):
* Source/WebKit/NetworkProcess/NetworkProcess.cpp:
(WebKit::NetworkProcess::allowsFirstPartyForCookies):
* Source/WebKit/NetworkProcess/NetworkProcess.h:
* Source/WebKit/Shared/AuxiliaryProcess.cpp:
(WebKit::AuxiliaryProcess::allowsFirstPartyForCookies):
* Source/WebKit/Shared/AuxiliaryProcess.h:
* Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:
(WebKit::WebLoaderStrategy::scheduleLoadFromNetworkProcess):
* Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:
(WebKit::PDFPlugin::getResourceBytesAtPosition):
* Source/WebKit/WebProcess/Plugins/PluginView.cpp:
(WebKit::PluginView::Stream::start):
* Source/WebKit/WebProcess/WebProcess.cpp:
(WebKit::WebProcess::allowsFirstPartyForCookies):

Canonical link: https://commits.webkit.org/266561@main
  • Loading branch information
charliewolfe committed Aug 4, 2023
1 parent e6264ae commit 274a670
Show file tree
Hide file tree
Showing 11 changed files with 45 additions and 29 deletions.
7 changes: 4 additions & 3 deletions Source/WebCore/loader/PingLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,11 @@ void PingLoader::sendViolationReport(LocalFrame& frame, const URL& reportURL, Re
break;
}

if (document.firstPartyForCookies().isNull() || !document.securityOrigin().isSameSchemeHostPort(SecurityOrigin::create(reportURL).get()))
bool removeCookies = true;
if (document.securityOrigin().isSameSchemeHostPort(SecurityOrigin::create(reportURL).get()))
removeCookies = false;
if (removeCookies)
request.setAllowCookies(false);
else
request.setFirstPartyForCookies(document.firstPartyForCookies());

HTTPHeaderMap originalRequestHeader = request.httpHeaderFields();

Expand Down
4 changes: 1 addition & 3 deletions Source/WebCore/loader/cache/CachedResourceLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1110,10 +1110,8 @@ ResourceErrorOr<CachedResourceHandle<CachedResource>> CachedResourceLoader::requ

// See if we can use an existing resource from the cache.
CachedResourceHandle<CachedResource> resource;
if (auto* document = this->document()) {
if (auto* document = this->document())
request.setDomainForCachePartition(*document);
request.resourceRequest().setFirstPartyForCookies(document->firstPartyForCookies());
}

if (request.allowsCaching())
resource = memoryCache.resourceForRequest(request.resourceRequest(), page.sessionID());
Expand Down
19 changes: 9 additions & 10 deletions Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ void NetworkConnectionToWebProcess::didReceiveInvalidMessage(IPC::Connection&, I

void NetworkConnectionToWebProcess::createSocketChannel(const ResourceRequest& request, const String& protocol, WebSocketIdentifier identifier, WebPageProxyIdentifier webPageProxyID, std::optional<FrameIdentifier> frameID, std::optional<PageIdentifier> pageID, const ClientOrigin& clientOrigin, bool hadMainFrameMainResourcePrivateRelayed, bool allowPrivacyProxy, OptionSet<AdvancedPrivacyProtections> advancedPrivacyProtections, ShouldRelaxThirdPartyCookieBlocking shouldRelaxThirdPartyCookieBlocking, WebCore::StoredCredentialsPolicy storedCredentialsPolicy)
{
NETWORK_PROCESS_MESSAGE_CHECK(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, RegistrableDomain { request.firstPartyForCookies() }));
NETWORK_PROCESS_MESSAGE_CHECK(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, request.firstPartyForCookies()));

ASSERT(!m_networkSocketChannels.contains(identifier));
if (auto channel = NetworkSocketChannel::create(*this, m_sessionID, request, protocol, identifier, webPageProxyID, frameID, pageID, clientOrigin, hadMainFrameMainResourcePrivateRelayed, allowPrivacyProxy, advancedPrivacyProtections, shouldRelaxThirdPartyCookieBlocking, storedCredentialsPolicy))
Expand Down Expand Up @@ -558,8 +558,7 @@ std::unique_ptr<ServiceWorkerFetchTask> NetworkConnectionToWebProcess::createFet

void NetworkConnectionToWebProcess::scheduleResourceLoad(NetworkResourceLoadParameters&& loadParameters, std::optional<NetworkResourceLoadIdentifier> existingLoaderToResume)
{
if (loadParameters.request.allowCookies())
NETWORK_PROCESS_MESSAGE_CHECK(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, RegistrableDomain { loadParameters.request.firstPartyForCookies() }));
NETWORK_PROCESS_MESSAGE_CHECK(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, loadParameters.request.firstPartyForCookies()));

CONNECTION_RELEASE_LOG(Loading, "scheduleResourceLoad: (parentPID=%d, pageProxyID=%" PRIu64 ", webPageID=%" PRIu64 ", frameID=%" PRIu64 ", resourceID=%" PRIu64 ", existingLoaderToResume=%" PRIu64 ")", loadParameters.parentPID, loadParameters.webPageProxyID.toUInt64(), loadParameters.webPageID.toUInt64(), loadParameters.webFrameID.object().toUInt64(), loadParameters.identifier.toUInt64(), valueOrDefault(existingLoaderToResume).toUInt64());

Expand Down Expand Up @@ -782,7 +781,7 @@ void NetworkConnectionToWebProcess::registerURLSchemesAsCORSEnabled(Vector<Strin

void NetworkConnectionToWebProcess::cookiesForDOM(const URL& firstParty, const SameSiteInfo& sameSiteInfo, const URL& url, FrameIdentifier frameID, PageIdentifier pageID, IncludeSecureCookies includeSecureCookies, ApplyTrackingPrevention applyTrackingPrevention, ShouldRelaxThirdPartyCookieBlocking shouldRelaxThirdPartyCookieBlocking, CompletionHandler<void(String cookieString, bool secureCookiesAccessed)>&& completionHandler)
{
NETWORK_PROCESS_MESSAGE_CHECK_COMPLETION(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, RegistrableDomain { firstParty }), completionHandler({ }, false));
NETWORK_PROCESS_MESSAGE_CHECK_COMPLETION(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, firstParty), completionHandler({ }, false));

auto* networkStorageSession = storageSession();
if (!networkStorageSession)
Expand All @@ -799,7 +798,7 @@ void NetworkConnectionToWebProcess::cookiesForDOM(const URL& firstParty, const S

void NetworkConnectionToWebProcess::setCookiesFromDOM(const URL& firstParty, const SameSiteInfo& sameSiteInfo, const URL& url, WebCore::FrameIdentifier frameID, PageIdentifier pageID, ApplyTrackingPrevention applyTrackingPrevention, const String& cookieString, ShouldRelaxThirdPartyCookieBlocking shouldRelaxThirdPartyCookieBlocking)
{
NETWORK_PROCESS_MESSAGE_CHECK(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, RegistrableDomain { firstParty }));
NETWORK_PROCESS_MESSAGE_CHECK(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, firstParty));

auto* networkStorageSession = storageSession();
if (!networkStorageSession)
Expand All @@ -815,7 +814,7 @@ void NetworkConnectionToWebProcess::setCookiesFromDOM(const URL& firstParty, con

void NetworkConnectionToWebProcess::cookieRequestHeaderFieldValue(const URL& firstParty, const SameSiteInfo& sameSiteInfo, const URL& url, std::optional<FrameIdentifier> frameID, std::optional<PageIdentifier> pageID, IncludeSecureCookies includeSecureCookies, ApplyTrackingPrevention applyTrackingPrevention, ShouldRelaxThirdPartyCookieBlocking shouldRelaxThirdPartyCookieBlocking, CompletionHandler<void(String, bool)>&& completionHandler)
{
NETWORK_PROCESS_MESSAGE_CHECK_COMPLETION(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, RegistrableDomain { firstParty }), completionHandler({ }, false));
NETWORK_PROCESS_MESSAGE_CHECK_COMPLETION(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, firstParty), completionHandler({ }, false));

auto* networkStorageSession = storageSession();
if (!networkStorageSession)
Expand All @@ -826,7 +825,7 @@ void NetworkConnectionToWebProcess::cookieRequestHeaderFieldValue(const URL& fir

void NetworkConnectionToWebProcess::getRawCookies(const URL& firstParty, const SameSiteInfo& sameSiteInfo, const URL& url, std::optional<FrameIdentifier> frameID, std::optional<PageIdentifier> pageID, ApplyTrackingPrevention applyTrackingPrevention, ShouldRelaxThirdPartyCookieBlocking shouldRelaxThirdPartyCookieBlocking, CompletionHandler<void(Vector<WebCore::Cookie>&&)>&& completionHandler)
{
NETWORK_PROCESS_MESSAGE_CHECK_COMPLETION(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, RegistrableDomain { firstParty }), completionHandler({ }));
NETWORK_PROCESS_MESSAGE_CHECK_COMPLETION(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, firstParty), completionHandler({ }));

auto* networkStorageSession = storageSession();
if (!networkStorageSession)
Expand Down Expand Up @@ -855,7 +854,7 @@ void NetworkConnectionToWebProcess::deleteCookie(const URL& url, const String& c

void NetworkConnectionToWebProcess::cookiesForDOMAsync(const URL& firstParty, const SameSiteInfo& sameSiteInfo, const URL& url, FrameIdentifier frameID, PageIdentifier pageID, IncludeSecureCookies includeSecureCookies, ApplyTrackingPrevention applyTrackingPrevention, ShouldRelaxThirdPartyCookieBlocking shouldRelaxThirdPartyCookieBlocking, WebCore::CookieStoreGetOptions&& options, CompletionHandler<void(std::optional<Vector<WebCore::Cookie>>&&)>&& completionHandler)
{
NETWORK_PROCESS_MESSAGE_CHECK_COMPLETION(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, RegistrableDomain { firstParty }), completionHandler(std::nullopt));
NETWORK_PROCESS_MESSAGE_CHECK_COMPLETION(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, firstParty), completionHandler(std::nullopt));

auto* networkStorageSession = storageSession();
if (!networkStorageSession)
Expand All @@ -872,7 +871,7 @@ void NetworkConnectionToWebProcess::cookiesForDOMAsync(const URL& firstParty, co

void NetworkConnectionToWebProcess::setCookieFromDOMAsync(const URL& firstParty, const SameSiteInfo& sameSiteInfo, const URL& url, WebCore::FrameIdentifier frameID, PageIdentifier pageID, ApplyTrackingPrevention applyTrackingPrevention, ShouldRelaxThirdPartyCookieBlocking shouldRelaxThirdPartyCookieBlocking, WebCore::Cookie&& cookie, CompletionHandler<void(bool)>&& completionHandler)
{
NETWORK_PROCESS_MESSAGE_CHECK(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, RegistrableDomain { firstParty }));
NETWORK_PROCESS_MESSAGE_CHECK(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, firstParty));

auto* networkStorageSession = storageSession();
if (!networkStorageSession)
Expand All @@ -892,7 +891,7 @@ void NetworkConnectionToWebProcess::domCookiesForHost(const URL& url, bool subsc
{
auto host = url.host().toString();
NETWORK_PROCESS_MESSAGE_CHECK_COMPLETION(HashSet<String>::isValidValue(host), completionHandler({ }));
NETWORK_PROCESS_MESSAGE_CHECK_COMPLETION(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, RegistrableDomain { url }), completionHandler({ }));
NETWORK_PROCESS_MESSAGE_CHECK_COMPLETION(m_networkProcess->allowsFirstPartyForCookies(m_webProcessIdentifier, url), completionHandler({ }));

auto* networkStorageSession = storageSession();
if (!networkStorageSession)
Expand Down
14 changes: 8 additions & 6 deletions Source/WebKit/NetworkProcess/NetworkProcess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -426,14 +426,16 @@ void NetworkProcess::webProcessWillLoadWebArchive(WebCore::ProcessIdentifier pro
}).iterator->value.first = LoadedWebArchive::Yes;
}

bool NetworkProcess::allowsFirstPartyForCookies(WebCore::ProcessIdentifier processIdentifier, const RegistrableDomain& firstPartyDomain)
bool NetworkProcess::allowsFirstPartyForCookies(WebCore::ProcessIdentifier processIdentifier, const URL& firstParty)
{
#if PLATFORM(GTK)
// FIXME: This shouldn't be needed but is hit for some web socket tests on GTK.
if (firstPartyDomain.isEmpty())
return true;
#endif
return AuxiliaryProcess::allowsFirstPartyForCookies(firstParty, [&] {
RegistrableDomain firstPartyDomain(firstParty);
return allowsFirstPartyForCookies(processIdentifier, firstPartyDomain);
});
}

bool NetworkProcess::allowsFirstPartyForCookies(WebCore::ProcessIdentifier processIdentifier, const RegistrableDomain& firstPartyDomain)
{
if (!decltype(m_allowedFirstPartiesForCookies)::isValidKey(processIdentifier)) {
ASSERT_NOT_REACHED();
return false;
Expand Down
1 change: 1 addition & 0 deletions Source/WebKit/NetworkProcess/NetworkProcess.h
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ class NetworkProcess : public AuxiliaryProcess, private DownloadManager::Client,
void deleteWebsiteDataForOrigin(PAL::SessionID, OptionSet<WebsiteDataType>, const WebCore::ClientOrigin&, CompletionHandler<void()>&&);
void deleteWebsiteDataForOrigins(PAL::SessionID, OptionSet<WebsiteDataType>, const Vector<WebCore::SecurityOriginData>& origins, const Vector<String>& cookieHostNames, const Vector<String>& HSTSCacheHostnames, const Vector<RegistrableDomain>&, CompletionHandler<void()>&&);

bool allowsFirstPartyForCookies(WebCore::ProcessIdentifier, const URL&);
bool allowsFirstPartyForCookies(WebCore::ProcessIdentifier, const RegistrableDomain&);
void addAllowedFirstPartyForCookies(WebCore::ProcessIdentifier, WebCore::RegistrableDomain&&, LoadedWebArchive, CompletionHandler<void()>&&);
void webProcessWillLoadWebArchive(WebCore::ProcessIdentifier);
Expand Down
17 changes: 17 additions & 0 deletions Source/WebKit/Shared/AuxiliaryProcess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,25 @@ void AuxiliaryProcess::didReceiveMemoryPressureEvent(bool isCritical)

#endif // !PLATFORM(COCOA)

bool AuxiliaryProcess::allowsFirstPartyForCookies(const URL& firstParty, Function<bool()>&& domainCheck)
{
// FIXME: This should probably not be necessary. If about:blank is the first party for cookies,
// we should set it to be the inherited origin then remove this exception.
if (firstParty.isAboutBlank())
return true;

if (firstParty.isNull())
return true; // FIXME: This shouldn't be allowed.

return domainCheck();
}

bool AuxiliaryProcess::allowsFirstPartyForCookies(const WebCore::RegistrableDomain& firstPartyDomain, HashSet<WebCore::RegistrableDomain>& allowedFirstPartiesForCookies)
{
// FIXME: This shouldn't be needed but it is hit sometimes at least with PDFs.
if (firstPartyDomain.isEmpty())
return true;

if (!std::remove_reference_t<decltype(allowedFirstPartiesForCookies)>::isValidValue(firstPartyDomain)) {
ASSERT_NOT_REACHED();
return false;
Expand Down
1 change: 1 addition & 0 deletions Source/WebKit/Shared/AuxiliaryProcess.h
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ class AuxiliaryProcess : public IPC::Connection::Client, public IPC::MessageSend
// IPC::Connection::Client.
void didClose(IPC::Connection&) override;

bool allowsFirstPartyForCookies(const URL&, Function<bool()>&&);
bool allowsFirstPartyForCookies(const WebCore::RegistrableDomain&, HashSet<WebCore::RegistrableDomain>&);

private:
Expand Down
2 changes: 1 addition & 1 deletion Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,7 @@ void WebLoaderStrategy::scheduleLoadFromNetworkProcess(ResourceLoader& resourceL
existingNetworkResourceLoadIdentifierToResume = std::exchange(m_existingNetworkResourceLoadIdentifierToResume, std::nullopt);
WEBLOADERSTRATEGY_RELEASE_LOG("scheduleLoad: Resource is being scheduled with the NetworkProcess (priority=%d, existingNetworkResourceLoadIdentifierToResume=%" PRIu64 ")", static_cast<int>(resourceLoader.request().priority()), valueOrDefault(existingNetworkResourceLoadIdentifierToResume).toUInt64());

if (frame && !frame->settings().siteIsolationEnabled() && loadParameters.request.allowCookies() && !WebProcess::singleton().allowsFirstPartyForCookies(loadParameters.request.firstPartyForCookies()))
if (frame && !frame->settings().siteIsolationEnabled() && !WebProcess::singleton().allowsFirstPartyForCookies(loadParameters.request.firstPartyForCookies()))
RELEASE_LOG_FAULT(IPC, "scheduleLoad: Process will terminate due to failed allowsFirstPartyForCookies check");

if (WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::ScheduleResourceLoad(loadParameters, existingNetworkResourceLoadIdentifierToResume), 0) != IPC::Error::NoError) {
Expand Down
2 changes: 0 additions & 2 deletions Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1031,8 +1031,6 @@ static void dataProviderReleaseInfoCallback(void* info)
resourceRequest.setURL(m_view->mainResourceURL());
resourceRequest.setHTTPHeaderField(HTTPHeaderName::Range, makeString("bytes="_s, position, "-"_s, position + count - 1));
resourceRequest.setCachePolicy(ResourceRequestCachePolicy::DoNotUseAnyCache);
if (auto* document = coreFrame->document())
resourceRequest.setFirstPartyForCookies(document->topDocument().url());

#if !LOG_DISABLED
pdfLog(makeString("Scheduling a stream loader for request ", identifier, " (", count, " bytes at ", position, ")"));
Expand Down
3 changes: 0 additions & 3 deletions Source/WebKit/WebProcess/Plugins/PluginView.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,6 @@ void PluginView::Stream::start()
auto* frame = m_pluginView->frame();
ASSERT(frame);

if (auto* document = frame->document())
m_request.setFirstPartyForCookies(document->topDocument().url());

WebProcess::singleton().webLoaderStrategy().schedulePluginStreamLoad(*frame, *this, ResourceRequest {m_request}, [this, protectedThis = Ref { *this }](RefPtr<NetscapePlugInStreamLoader>&& loader) {
m_loader = WTFMove(loader);
});
Expand Down
4 changes: 3 additions & 1 deletion Source/WebKit/WebProcess/WebProcess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2351,7 +2351,9 @@ void WebProcess::addAllowedFirstPartyForCookies(WebCore::RegistrableDomain&& fir

bool WebProcess::allowsFirstPartyForCookies(const URL& firstParty)
{
return AuxiliaryProcess::allowsFirstPartyForCookies(WebCore::RegistrableDomain { firstParty }, m_allowedFirstPartiesForCookies);
return AuxiliaryProcess::allowsFirstPartyForCookies(firstParty, [&] {
return AuxiliaryProcess::allowsFirstPartyForCookies(WebCore::RegistrableDomain { firstParty }, m_allowedFirstPartiesForCookies);
});
}

} // namespace WebKit
Expand Down

0 comments on commit 274a670

Please sign in to comment.