Prevent CanCommitURL renderer kills for document.open on error pages.
Failed navigations commit an error page with the failed URL from the
browser process's perspective, but the renderer process uses
kUnreachableWebDataURL instead. This chrome-error://chromewebdata URL
can end up being inherited if document.open() is later used. This does
not normally occur on error pages, but it is possible in Android WebView
apps, leading to CanCommitURL failures because the chrome-error: scheme
has not been granted to the process.
This CL avoids a renderer kill in this scenario by granting access to
the kUnreachableWebDataURL when a process is legitimately committing a
failed navigation.
This CL also avoids checking CanAccessMaybeOpaqueOrigin for the
kUnreachableWebDataURL, since that function cannot compute the expected
process lock when error page isolation is not in effect. The rest of the
checks (including the CPSP::State::CanCommitURL check) still apply.
Bug: 326250356
Change-Id: I2ac56c14ce4dda242d0dd37e8087cd934228d9d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5601582
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1310986}
diff --git a/content/browser/child_process_security_policy_impl.cc b/content/browser/child_process_security_policy_impl.cc
index c9fe8fa..26fbdc7e 100644
--- a/content/browser/child_process_security_policy_impl.cc
+++ b/content/browser/child_process_security_policy_impl.cc
@@ -1389,7 +1389,15 @@
// With site isolation, a URL from a site may only be committed in a process
// dedicated to that site. This check will ensure that |url| can't commit if
// the process is locked to a different site.
- if (!CanAccessMaybeOpaqueOrigin(child_id, url,
+ //
+ // We skip this check specifically for the error page URL,
+ // chrome-error://chromewebdata, because it can commit in any process (due to
+ // a lack of subframe error page isolation) and because it is difficult to
+ // compute its expected process lock. We still verify in the
+ // state->CanCommitURL call below that the process has actually been granted
+ // access to this URL, rather than just returning true for it.
+ if (url != GURL(kUnreachableWebDataURL) &&
+ !CanAccessMaybeOpaqueOrigin(child_id, url,
false /* url_is_precursor_of_opaque_origin */,
AccessType::kCanCommitNewOrigin)) {
LogCanCommitUrlFailureReason("cannot_access_origin");
diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc
index 4b3fafe..3de2b28 100644
--- a/content/browser/renderer_host/render_frame_host_impl.cc
+++ b/content/browser/renderer_host/render_frame_host_impl.cc
@@ -11901,17 +11901,30 @@
void RenderFrameHostImpl::UpdatePermissionsForNavigation(
NavigationRequest* request) {
+ ChildProcessSecurityPolicyImpl::GetInstance()->GrantCommitURL(
+ GetProcess()->GetID(), request->common_params().url);
+ if (request->IsLoadDataWithBaseURL()) {
+ // When there's a base URL specified for the data URL, we also need to
+ // grant access to the base URL. This allows file: and other unexpected
+ // schemes to be accepted at commit time and during CORS checks (e.g., for
+ // font requests).
ChildProcessSecurityPolicyImpl::GetInstance()->GrantCommitURL(
- GetProcess()->GetID(), request->common_params().url);
- if (request->IsLoadDataWithBaseURL()) {
- // When there's a base URL specified for the data URL, we also need to
- // grant access to the base URL. This allows file: and other unexpected
- // schemes to be accepted at commit time and during CORS checks (e.g., for
- // font requests).
- ChildProcessSecurityPolicyImpl::GetInstance()->GrantCommitURL(
- GetProcess()->GetID(),
- request->common_params().base_url_for_data_url);
- }
+ GetProcess()->GetID(), request->common_params().base_url_for_data_url);
+ }
+
+ if (request->DidEncounterError()) {
+ // Failed navigations will end up using chrome-error://chromewebdata as the
+ // URL in RenderFrameImpl::CommitFailedNavigation. This does not immediately
+ // get sent back at DidCommit time, but it can be inherited as the URL via
+ // document.open, so we must grant access to that URL as well in case
+ // embedders (e.g., Android WebView apps) call document.open on an error
+ // page. See https://crbug.com/326250356#comment36.
+ // TODO(crbug.com/40150370): The browser process should tell the renderer
+ // process to use kUnreachableWebDataURL, rather than having the renderer
+ // process make the change independently.
+ ChildProcessSecurityPolicyImpl::GetInstance()->GrantCommitURL(
+ GetProcess()->GetID(), GURL(kUnreachableWebDataURL));
+ }
// We may be returning to an existing NavigationEntry that had been granted
// file access. If this is a different process, we will need to grant the
diff --git a/content/browser/renderer_host/render_frame_host_impl_browsertest.cc b/content/browser/renderer_host/render_frame_host_impl_browsertest.cc
index 9c8b8807..c4a08a09 100644
--- a/content/browser/renderer_host/render_frame_host_impl_browsertest.cc
+++ b/content/browser/renderer_host/render_frame_host_impl_browsertest.cc
@@ -4656,6 +4656,86 @@
EXPECT_TRUE(root_frame_host()->has_committed_any_navigation_);
}
+// Ensure that calling document.open in an error page does not cause a renderer
+// kill when it inherits the unreachable error URL.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
+ DocumentOpenInErrorPage) {
+ GURL error_url(embedded_test_server()->GetURL("error.com", "/empty.html"));
+ std::unique_ptr<URLLoaderInterceptor> url_interceptor =
+ URLLoaderInterceptor::SetupRequestFailForURL(error_url,
+ net::ERR_DNS_TIMED_OUT);
+ EXPECT_FALSE(NavigateToURL(shell(), error_url));
+
+ // Calling document.open should not cause CanCommitURL to fail, even though
+ // the error page URL is inherited.
+ // See https://crbug.com/326250356#comment36.
+ EXPECT_TRUE(ExecJs(shell(), "document.open();"));
+ EXPECT_EQ(GURL(kUnreachableWebDataURL),
+ root_frame_host()->last_document_url_in_renderer());
+
+ // Ensure the renderer process has not crashed.
+ ASSERT_TRUE(ExecJs(shell(), "true"));
+ ASSERT_TRUE(root_frame_host()->IsRenderFrameLive());
+}
+
+// Similar to DocumentOpenInErrorPage, but without error page isolation in the
+// main frame, which changes ProcessLock expectations and can thus fail in
+// additional ways.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
+ DocumentOpenInErrorPageWithoutErrorPageIsolation) {
+ // Disable error page isolation in main frames, similar to Android WebView.
+ class NoErrorPageIsolationContentBrowserClient
+ : public ContentBrowserTestContentBrowserClient {
+ public:
+ bool ShouldIsolateErrorPage(bool in_main_frame) override { return false; }
+ } no_error_isolation_client;
+
+ GURL error_url(embedded_test_server()->GetURL("error.com", "/empty.html"));
+ std::unique_ptr<URLLoaderInterceptor> url_interceptor =
+ URLLoaderInterceptor::SetupRequestFailForURL(error_url,
+ net::ERR_DNS_TIMED_OUT);
+ EXPECT_FALSE(NavigateToURL(shell(), error_url));
+
+ // Calling document.open should not cause CanCommitURL to fail, even though
+ // the error page URL is inherited.
+ // See https://crbug.com/326250356#comment36.
+ EXPECT_TRUE(ExecJs(shell(), "document.open();"));
+ EXPECT_EQ(GURL(kUnreachableWebDataURL),
+ root_frame_host()->last_document_url_in_renderer());
+
+ // Ensure the renderer process has not crashed.
+ ASSERT_TRUE(ExecJs(shell(), "true"));
+ ASSERT_TRUE(root_frame_host()->IsRenderFrameLive());
+}
+
+// Similar to DocumentOpenInErrorPage, but when loading the error page in a
+// subframe, which lacks error page isolation.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
+ DocumentOpenInErrorPageSubframe) {
+ GURL main_frame_url(
+ embedded_test_server()->GetURL("/frame_tree/page_with_one_frame.html"));
+ ASSERT_TRUE(NavigateToURL(shell(), main_frame_url));
+
+ GURL error_url(embedded_test_server()->GetURL("error.com", "/empty.html"));
+ std::unique_ptr<URLLoaderInterceptor> url_interceptor =
+ URLLoaderInterceptor::SetupRequestFailForURL(error_url,
+ net::ERR_DNS_TIMED_OUT);
+ EXPECT_TRUE(NavigateIframeToURL(web_contents(), "child0", error_url));
+ RenderFrameHostImpl* subframe =
+ static_cast<RenderFrameHostImpl*>(ChildFrameAt(root_frame_host(), 0));
+
+ // Calling document.open should not cause CanCommitURL to fail, even though
+ // the error page URL is inherited.
+ // See https://crbug.com/326250356#comment36.
+ EXPECT_TRUE(ExecJs(subframe, "document.open();"));
+ EXPECT_EQ(GURL(kUnreachableWebDataURL),
+ subframe->last_document_url_in_renderer());
+
+ // Ensure the renderer process has not crashed.
+ ASSERT_TRUE(ExecJs(subframe, "true"));
+ ASSERT_TRUE(subframe->IsRenderFrameLive());
+}
+
// Test the LifecycleStateImpl when a renderer crashes during navigation.
// When navigating after a crash, the new RenderFrameHost should
// become active immediately, prior to the navigation committing. This is
