Skip to content

Commit

Permalink
Process Isolation Bypass via navigating to about:* context
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=257348
rdar://109853048

Reviewed by Chris Dumez and J Pascoe.

This patch fixes an issue where we will never swap processes when navigating from an
about:* page. A compromised WebContent process could leverage this behavior to avoid
swapping processes when navigating to a cross-origin domain.

This is fixed by checking the following when navigating from an about:* page.
- The page has not committed a provisional load to a URL that isn't about:*.
- The source process last loaded a domain from the same origin as the domain we are navigating to.

If neither of these two conditions are met, we won’t reuse the process.

* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::didExplicitOpenForFrame):
(WebKit::WebPageProxy::didCommitLoadForFrame):
* Source/WebKit/UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::processForNavigationInternal):
* Source/WebKit/UIProcess/WebProcessProxy.h:
(WebKit::WebProcessProxy::didCommitMeaningfulProvisionalLoad):
(WebKit::WebProcessProxy::hasCommittedAnyMeaningfulProvisionalLoads const):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

Originally-landed-as: 259548.792@safari-7615-branch (f52bc7e). rdar://113174653
Canonical link: https://commits.webkit.org/266648@main
  • Loading branch information
charliewolfe authored and JonWBedard committed Aug 7, 2023
1 parent cfa0760 commit 0d4cfda
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 1 deletion.
4 changes: 4 additions & 0 deletions Source/WebKit/UIProcess/WebPageProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5461,6 +5461,8 @@ void WebPageProxy::didExplicitOpenForFrame(FrameIdentifier frameID, URL&& url, S

m_hasCommittedAnyProvisionalLoads = true;
m_process->didCommitProvisionalLoad();
if (!url.protocolIsAbout())
m_process->didCommitMeaningfulProvisionalLoad();

internals().pageLoadState.commitChanges();
}
Expand Down Expand Up @@ -5702,6 +5704,8 @@ void WebPageProxy::didCommitLoadForFrame(FrameIdentifier frameID, FrameInfoData&

m_hasCommittedAnyProvisionalLoads = true;
m_process->didCommitProvisionalLoad();
if (!request.url().protocolIsAbout())
m_process->didCommitMeaningfulProvisionalLoad();

if (frame->isMainFrame()) {
m_hasUpdatedRenderingAfterDidCommitLoad = false;
Expand Down
2 changes: 1 addition & 1 deletion Source/WebKit/UIProcess/WebProcessPool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2003,7 +2003,7 @@ std::tuple<Ref<WebProcessProxy>, SuspendedPageProxy*, ASCIILiteral> WebProcessPo
if (!m_configuration->processSwapsOnNavigationWithinSameNonHTTPFamilyProtocol() && !sourceURL.protocolIsInHTTPFamily() && sourceURL.protocol() == targetURL.protocol())
return { WTFMove(sourceProcess), nullptr, "Navigation within the same non-HTTP(s) protocol"_s };

if (!sourceURL.isValid() || !targetURL.isValid() || sourceURL.isEmpty() || sourceURL.protocolIsAbout() || targetRegistrableDomain.matches(sourceURL))
if (!sourceURL.isValid() || !targetURL.isValid() || sourceURL.isEmpty() || targetRegistrableDomain.matches(sourceURL) || (sourceURL.protocolIsAbout() && (!sourceProcess->hasCommittedAnyMeaningfulProvisionalLoads() || sourceProcess->registrableDomain().matches(targetURL))) || targetRegistrableDomain.matches(sourceURL))
return { WTFMove(sourceProcess), nullptr, "Navigation is same-site"_s };

auto reason = "Navigation is cross-site"_s;
Expand Down
4 changes: 4 additions & 0 deletions Source/WebKit/UIProcess/WebProcessProxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,9 @@ class WebProcessProxy : public AuxiliaryProcessProxy, private ProcessThrottlerCl
void didCommitProvisionalLoad() { m_hasCommittedAnyProvisionalLoads = true; }
bool hasCommittedAnyProvisionalLoads() const { return m_hasCommittedAnyProvisionalLoads; }

void didCommitMeaningfulProvisionalLoad() { m_hasCommittedAnyMeaningfulProvisionalLoads = true; }
bool hasCommittedAnyMeaningfulProvisionalLoads() const { return m_hasCommittedAnyMeaningfulProvisionalLoads; }

#if PLATFORM(WATCHOS)
void startBackgroundActivityForFullscreenInput();
void endBackgroundActivityForFullscreenInput();
Expand Down Expand Up @@ -702,6 +705,7 @@ class WebProcessProxy : public AuxiliaryProcessProxy, private ProcessThrottlerCl
#endif

bool m_hasCommittedAnyProvisionalLoads { false };
bool m_hasCommittedAnyMeaningfulProvisionalLoads { false }; // True if the process has committed a provisional load to a URL that was not about:*.
bool m_isPrewarmed;
LockdownMode m_lockdownMode { LockdownMode::Disabled };
WebCore::CrossOriginMode m_crossOriginMode { WebCore::CrossOriginMode::Shared };
Expand Down
64 changes: 64 additions & 0 deletions Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
Original file line number Diff line number Diff line change
Expand Up @@ -8748,6 +8748,70 @@ HTTPServer server2({

#endif // PLATFORM(IOS_FAMILY)

TEST(ProcessSwap, NewProcessAfterNavigatingToCrossOriginThroughAboutPage)
{
using namespace TestWebKitAPI;

HTTPServer server({
{ "/source.html"_s, { ""_s } },
{ "/destination.html"_s, { ""_s } },
});

auto webView = adoptNS([[WKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600)]);

[webView synchronouslyLoadRequest:server.request("/source.html"_s)];
[webView synchronouslyLoadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"about:blank"]]];

auto pid1 = [webView _webProcessIdentifier];

[webView synchronouslyLoadRequest:server.requestWithLocalhost("/destination.html"_s)];

auto pid2 = [webView _webProcessIdentifier];
EXPECT_NE(pid1, pid2);
}

TEST(ProcessSwap, ReuseProcessAfterNavigatingToSameOriginThroughAboutPage)
{
using namespace TestWebKitAPI;

HTTPServer server({
{ "/source.html"_s, { ""_s } },
{ "/destination.html"_s, { ""_s } },
});

auto webView = adoptNS([[WKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600)]);

[webView synchronouslyLoadRequest:server.request("/source.html"_s)];
[webView synchronouslyLoadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"about:blank"]]];

auto pid1 = [webView _webProcessIdentifier];

[webView synchronouslyLoadRequest:server.request("/destination.html"_s)];

auto pid2 = [webView _webProcessIdentifier];
EXPECT_EQ(pid1, pid2);
}

TEST(ProcessSwap, ReuseProcessAfterNavigatingFromAboutPage)
{
using namespace TestWebKitAPI;

HTTPServer server({
{ "/destination.html"_s, { ""_s } },
});

auto webView = adoptNS([[WKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600)]);

[webView synchronouslyLoadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"about:blank"]]];

auto pid1 = [webView _webProcessIdentifier];

[webView synchronouslyLoadRequest:server.request("/destination.html"_s)];

auto pid2 = [webView _webProcessIdentifier];
EXPECT_EQ(pid1, pid2);
}

// The WebProcess cache cannot be enabled on devices with too little RAM so we need to disable
// tests relying on it on iOS. The WebProcess cache is disabled by default on iOS anyway.
#if !PLATFORM(IOS_FAMILY)
Expand Down

0 comments on commit 0d4cfda

Please sign in to comment.