From 470a5614ecfbdd85e5b0bb97719ffafa85410872 Mon Sep 17 00:00:00 2001
From: Alex Moshchuk <[email protected]>
Date: Fri, 01 May 2026 20:31:33 -0700
Subject: [PATCH] Don't use precursors for error page origins for kCurrentProcess.

Currently, when a subframe navigation fails due to deterministic
failures (e.g., CSP or BLOCKED_BY_CLIENT), the resulting error page is
committed in the current process (`ErrorPageProcess::kCurrentProcess`)
to avoid spawning a new process for a potentially privileged
destination.

Previously, this error page was given an opaque origin derived from
the destination URL. If a compromised renderer intentionally triggered
a CSP failure against a cross-site victim URL, it could force an error
page with the victim's precursor to commit within the attacker's
process. This is risky, and among other problems, it allowed the
compromised renderer to inject a sandboxed srcdoc iframe into the
error page, which would inherit the victim precursor and could be
incorrectly granted a dedicated SiteInstance/process belonging to the
victim.

This CL fixes this by forcing error pages that stay in the current
process to use opaque unique origins with no precursor. Note that
this only affects subframe error pages, since main frame error pages
have error page isolation which avoids these problems.

Change-Id: Ib43c88233b36cd0ba84dff993134e2fcaa52ba13
Bug: 502348223, 487300831
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7794327
Commit-Queue: Alex Moshchuk <[email protected]>
Reviewed-by: Charlie Reis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1624238}
---

diff --git a/content/browser/renderer_host/navigation_request.cc b/content/browser/renderer_host/navigation_request.cc
index 6cdaa2bd..9ec42c6d 100644
--- a/content/browser/renderer_host/navigation_request.cc
+++ b/content/browser/renderer_host/navigation_request.cc
@@ -5527,9 +5527,6 @@
   }
 
   if (state_ < NavigationRequest::CANCELING) {
-    CHECK(browser_initiated_error_navigation_type_ !=
-          BrowserInitiatedErrorNavigationType::kNone);
-
     if (browser_initiated_error_navigation_type_ ==
         BrowserInitiatedErrorNavigationType::kPostCommit) {
       // Post-commit error page normally goes through the "non-error page"
@@ -5537,9 +5534,8 @@
       return ErrorPageProcess::kPostCommitErrorPage;
     }
 
-    // Otherwise, this is a normal browser-initiated error navigation, which
-    // should fall out of this block and use existing process selection
-    // behavior.
+    // Otherwise, this is a normal error navigation, which should fall out of
+    // this block and use existing process selection behavior.
   }
 
   // By policy we can isolate all error pages from both the current and
@@ -6525,6 +6521,13 @@
           previous_origin.GetTupleOrPrecursorTupleIfOpaque();
   if (!is_error_page_with_same_precursor) {
     commit_params_->force_new_document_sequence_number = true;
+  } else {
+    // We only preserve the document sequence number for temporary errors that
+    // could later be reloaded and succeed, which don't stay in the current
+    // process. Fatal errors routed to kCurrentProcess have a pure opaque origin
+    // and will not share the precursor, so they will always force a new
+    // document sequence number.
+    CHECK_NE(ComputeErrorPageProcess(), ErrorPageProcess::kCurrentProcess);
   }
 
   PopulateDocumentTokenForCrossDocumentNavigation();
@@ -11739,10 +11742,16 @@
 url::Origin NavigationRequest::GetOriginForURLLoaderFactoryUnchecked() {
   if (DidEncounterError()) {
     // Error pages commit in an opaque origin in the renderer process. If this
-    // NavigationRequest resulted in committing an error page, return an
-    // opaque origin that has precursor information consistent with the URL
-    // being requested.  Note: this is intentionally done first; cases like
-    // errors in srcdoc frames need not inherit the parent's origin for errors.
+    // NavigationRequest resulted in committing an error page, return an opaque
+    // origin. We usually derive the precursor for that opaque origin from the
+    // destination URL, with one exception: if the error page commits in the
+    // current process (e.g., for unrecoverable errors in subframes), we leave
+    // the precursor empty. This prevents compromised renderers from gaining
+    // access to opaque origins with precursors that aren't normally allowed in
+    // the process (crbug.com/502348223).
+    if (ComputeErrorPageProcess() == ErrorPageProcess::kCurrentProcess) {
+      return url::Origin();
+    }
     return url::Origin::Create(common_params().url).DeriveNewOpaqueOrigin();
   }
 
diff --git a/content/browser/renderer_host/render_frame_host_manager_browsertest.cc b/content/browser/renderer_host/render_frame_host_manager_browsertest.cc
index 7fbe6debe..672b606 100644
--- a/content/browser/renderer_host/render_frame_host_manager_browsertest.cc
+++ b/content/browser/renderer_host/render_frame_host_manager_browsertest.cc
@@ -4834,8 +4834,16 @@
     EXPECT_EQ(4, nav_controller.GetEntryCount());
     EXPECT_EQ(test_url, child1->current_frame_host()->GetLastCommittedURL());
 
-    // Error pages should commit in an opaque origin.
-    EXPECT_TRUE(IsOriginOpaqueAndCompatibleWithURL(child1, test_url));
+    // Error pages should commit in an opaque origin. This particular error
+    // stays in the current process, so when B2 navigates B1 to C, it stays in
+    // B's process. The origin's precursor should be empty, and more
+    // specifically, it should not be C, to guard against compromised renderers
+    // gaining access to cross-site precursors (crbug.com/502348223).
+    const url::Origin& child1_origin =
+        child1->current_frame_host()->GetLastCommittedOrigin();
+    EXPECT_TRUE(child1_origin.opaque());
+    EXPECT_TRUE(
+        child1_origin.GetTupleOrPrecursorTupleIfOpaque().GetURL().is_empty());
 
     // net::ERR_BLOCKED_BY_CLIENT errors in subframes should commit in the
     // the correct process based on whether isolation is enabled or not.
diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index d4f420a..56b28eb 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -4048,4 +4048,80 @@
   EXPECT_FALSE(subframe->IsRenderFrameLive());
 }
 
+// Tests that a compromised renderer cannot exploit a CSP-blocked subframe error
+// page to place a srcdoc frame into a sandboxed SiteInstance for a site that it
+// doesn't have access to. This verifies the fix for
+// https://crbug.com/502348223.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       ErrorPagePrecursorDoesNotLeakToSandboxedSrcdoc) {
+  GURL attacker_url(
+      embedded_test_server()->GetURL("attacker.test", "/title1.html"));
+  GURL victim_url(
+      embedded_test_server()->GetURL("victim.test", "/title1.html"));
+
+  EXPECT_TRUE(NavigateToURL(shell(), attacker_url));
+  RenderFrameHostImpl* main_frame = static_cast<RenderFrameHostImpl*>(
+      shell()->web_contents()->GetPrimaryMainFrame());
+
+  // Set CSP to block iframes, so we get an error page.
+  EXPECT_TRUE(ExecJs(main_frame,
+                     "var meta = document.createElement('meta');"
+                     "meta.httpEquiv = 'Content-Security-Policy';"
+                     "meta.content = \"frame-src 'none'\";"
+                     "document.head.appendChild(meta);"));
+
+  // Create an iframe to victim.test. It will be blocked and commit an error
+  // page.
+  TestNavigationObserver nav_observer(shell()->web_contents());
+  EXPECT_TRUE(
+      ExecJs(main_frame, JsReplace("var f = document.createElement('iframe');"
+                                   "f.src = $1;"
+                                   "document.body.appendChild(f);",
+                                   victim_url)));
+  nav_observer.Wait();
+
+  EXPECT_FALSE(nav_observer.last_navigation_succeeded());
+  EXPECT_EQ(net::ERR_BLOCKED_BY_CSP, nav_observer.last_net_error_code());
+
+  RenderFrameHostImpl* error_frame =
+      main_frame->child_at(0)->current_frame_host();
+  EXPECT_TRUE(error_frame->IsErrorDocument());
+  ASSERT_EQ(error_frame->GetProcess(), main_frame->GetProcess());
+
+  // Simulate a compromised renderer by injecting a sandboxed srcdoc inside the
+  // error page. Once we have error page isolation for subframes, attacker.test
+  // won't be able to do this step.
+  TestNavigationObserver srcdoc_observer(shell()->web_contents());
+  EXPECT_TRUE(ExecJs(error_frame,
+                     "var f = document.createElement('iframe');"
+                     "f.sandbox = 'allow-scripts';"
+                     "f.srcdoc = 'foo';"
+                     "document.body.appendChild(f);"));
+  srcdoc_observer.Wait();
+
+  RenderFrameHostImpl* srcdoc_frame =
+      error_frame->child_at(0)->current_frame_host();
+
+  // With the fix, the error page's opaque origin has no precursor. Check that
+  // the sandboxed srcdoc's SiteInstance was not derived from victim.test.
+  EXPECT_TRUE(srcdoc_frame->GetLastCommittedOrigin().opaque());
+  EXPECT_TRUE(srcdoc_frame->GetLastCommittedOrigin()
+                  .GetTupleOrPrecursorTupleIfOpaque()
+                  .GetURL()
+                  .is_empty());
+  SiteInfo site_info = srcdoc_frame->GetSiteInstance()->GetSiteInfo();
+  EXPECT_FALSE(site_info.site_url().DomainIs("victim.test"));
+
+  // OOPSIFs require site isolation, so the srcdoc frame will be in a new
+  // sandboxed process if site isolation is enabled; otherwise, it will go into
+  // the error page's current unsandboxed process.
+  if (AreAllSitesIsolatedForTesting()) {
+    EXPECT_TRUE(site_info.IsSandboxed());
+    EXPECT_NE(srcdoc_frame->GetProcess(), error_frame->GetProcess());
+  } else {
+    EXPECT_FALSE(site_info.IsSandboxed());
+    EXPECT_EQ(srcdoc_frame->GetProcess(), error_frame->GetProcess());
+  }

diff --git a/content/browser/renderer_host/render_frame_host_manager_browsertest.cc b/content/browser/renderer_host/render_frame_host_manager_browsertest.cc
index 7fbe6debe..672b606 100644
--- a/content/browser/renderer_host/render_frame_host_manager_browsertest.cc
+++ b/content/browser/renderer_host/render_frame_host_manager_browsertest.cc
@@ -4834,8 +4834,16 @@
     EXPECT_EQ(4, nav_controller.GetEntryCount());
     EXPECT_EQ(test_url, child1->current_frame_host()->GetLastCommittedURL());
 
-    // Error pages should commit in an opaque origin.
-    EXPECT_TRUE(IsOriginOpaqueAndCompatibleWithURL(child1, test_url));
+    // Error pages should commit in an opaque origin. This particular error
+    // stays in the current process, so when B2 navigates B1 to C, it stays in
+    // B's process. The origin's precursor should be empty, and more
+    // specifically, it should not be C, to guard against compromised renderers
+    // gaining access to cross-site precursors (crbug.com/502348223).
+    const url::Origin& child1_origin =
+        child1->current_frame_host()->GetLastCommittedOrigin();
+    EXPECT_TRUE(child1_origin.opaque());
+    EXPECT_TRUE(
+        child1_origin.GetTupleOrPrecursorTupleIfOpaque().GetURL().is_empty());
 
     // net::ERR_BLOCKED_BY_CLIENT errors in subframes should commit in the
     // the correct process based on whether isolation is enabled or not.
diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index d4f420a..56b28eb 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -4048,4 +4048,80 @@
   EXPECT_FALSE(subframe->IsRenderFrameLive());
 }
 
+// Tests that a compromised renderer cannot exploit a CSP-blocked subframe error
+// page to place a srcdoc frame into a sandboxed SiteInstance for a site that it
+// doesn't have access to. This verifies the fix for
+// https://crbug.com/502348223.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       ErrorPagePrecursorDoesNotLeakToSandboxedSrcdoc) {
+  GURL attacker_url(
+      embedded_test_server()->GetURL("attacker.test", "/title1.html"));
+  GURL victim_url(
+      embedded_test_server()->GetURL("victim.test", "/title1.html"));
+
+  EXPECT_TRUE(NavigateToURL(shell(), attacker_url));
+  RenderFrameHostImpl* main_frame = static_cast<RenderFrameHostImpl*>(
+      shell()->web_contents()->GetPrimaryMainFrame());
+
+  // Set CSP to block iframes, so we get an error page.
+  EXPECT_TRUE(ExecJs(main_frame,
+                     "var meta = document.createElement('meta');"
+                     "meta.httpEquiv = 'Content-Security-Policy';"
+                     "meta.content = \"frame-src 'none'\";"
+                     "document.head.appendChild(meta);"));
+
+  // Create an iframe to victim.test. It will be blocked and commit an error
+  // page.
+  TestNavigationObserver nav_observer(shell()->web_contents());
+  EXPECT_TRUE(
+      ExecJs(main_frame, JsReplace("var f = document.createElement('iframe');"
+                                   "f.src = $1;"
+                                   "document.body.appendChild(f);",
+                                   victim_url)));
+  nav_observer.Wait();
+
+  EXPECT_FALSE(nav_observer.last_navigation_succeeded());
+  EXPECT_EQ(net::ERR_BLOCKED_BY_CSP, nav_observer.last_net_error_code());
+
+  RenderFrameHostImpl* error_frame =
+      main_frame->child_at(0)->current_frame_host();
+  EXPECT_TRUE(error_frame->IsErrorDocument());
+  ASSERT_EQ(error_frame->GetProcess(), main_frame->GetProcess());
+
+  // Simulate a compromised renderer by injecting a sandboxed srcdoc inside the
+  // error page. Once we have error page isolation for subframes, attacker.test
+  // won't be able to do this step.
+  TestNavigationObserver srcdoc_observer(shell()->web_contents());
+  EXPECT_TRUE(ExecJs(error_frame,
+                     "var f = document.createElement('iframe');"
+                     "f.sandbox = 'allow-scripts';"
+                     "f.srcdoc = 'foo';"
+                     "document.body.appendChild(f);"));
+  srcdoc_observer.Wait();
+
+  RenderFrameHostImpl* srcdoc_frame =
+      error_frame->child_at(0)->current_frame_host();
+
+  // With the fix, the error page's opaque origin has no precursor. Check that
+  // the sandboxed srcdoc's SiteInstance was not derived from victim.test.
+  EXPECT_TRUE(srcdoc_frame->GetLastCommittedOrigin().opaque());
+  EXPECT_TRUE(srcdoc_frame->GetLastCommittedOrigin()
+                  .GetTupleOrPrecursorTupleIfOpaque()
+                  .GetURL()
+                  .is_empty());
+  SiteInfo site_info = srcdoc_frame->GetSiteInstance()->GetSiteInfo();
+  EXPECT_FALSE(site_info.site_url().DomainIs("victim.test"));
+
+  // OOPSIFs require site isolation, so the srcdoc frame will be in a new
+  // sandboxed process if site isolation is enabled; otherwise, it will go into
+  // the error page's current unsandboxed process.
+  if (AreAllSitesIsolatedForTesting()) {
+    EXPECT_TRUE(site_info.IsSandboxed());
+    EXPECT_NE(srcdoc_frame->GetProcess(), error_frame->GetProcess());
+  } else {
+    EXPECT_FALSE(site_info.IsSandboxed());
+    EXPECT_EQ(srcdoc_frame->GetProcess(), error_frame->GetProcess());
+  }
+}
+
 }  // namespace content