From 3ac9952760d91169308e05d855a87fc5a2c4733a Mon Sep 17 00:00:00 2001
From: Alex Moshchuk <alexmos@chromium.org>
Date: Mon, 20 Jul 2026 14:37:39 -0700
Subject: [PATCH] Remove error page opaque origin exemption from VerifyInitiatorOrigin

VerifyInitiatorOrigin() currently early-returns true for any opaque
initiator_origin when the navigating frame is an error document,
regardless of the precursor. Since subframe error pages are not
process-isolated, this allows the embedding renderer to send
BeginNavigation/OpenURL on behalf of such a frame with an opaque
initiator carrying an arbitrary precursor, which is then trusted by
downstream consumers via GetTupleOrPrecursorTupleIfOpaque().

This CL removes this exemption because it is actually no longer
needed. This is because ChildProcessSecurityPolicyImpl::HostsOrigin()
has now been switched to rely on committed origin tracking. Any error
page that could legitimately initiate a navigation has already
committed in this process, so its (opaque) origin has been recorded by
ChildProcessSecurityPolicyImpl::AddCommittedOrigin and the
HostsOrigin() check below will accept it, even if the precursor
doesn't match the process lock. So, cases like error page reloads
should continue to work normally without this exemption.

This is already covered in tests for both main frames (with
ErrorPageNavigationReload) and subframes (with
ErrorPageNavigationReload_InSubframe_BlockedByClient). It matters both
with and without error page isolation (e.g., in the main frame case,
the initiator origin's precursor corresponds to the unreachable URL's
origin, but the process lock is chrome-error://chromewebdata), though
the subframe case without error page isolation has more security
consequences, since a dedicated error page process is much harder to
compromise. A new test is added to ensure a compromised renderer can't
spoof the initiator origin for the subframe error case.

Note also that subframe error pages that stay in process now commit
with an empty precursor after r1624238, which is another reason why
they no longer need this exemption.

Bug: 516398679
Change-Id: I83c231ab7c57a9e0b10dad74324296db2a06e42e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8126215
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1664952}
---
diff --git a/content/browser/renderer_host/ipc_utils.cc b/content/browser/renderer_host/ipc_utils.cc
index 9c5c718..088e6b31 100644
--- a/content/browser/renderer_host/ipc_utils.cc
+++ b/content/browser/renderer_host/ipc_utils.cc
@@ -80,41 +80,22 @@
     const RenderFrameHostImpl* current_rfh = nullptr,
     GURL* navigation_url = nullptr,
     std::optional<blink::LocalFrameToken>* initiator_frame_token = nullptr) {
-  // TODO(crbug.com/40109437): Ideally, origin verification should be performed
-  // even if `initiator_origin` is opaque, to ensure that the precursor origin
-  // matches the process lock. However, there are a couple of cases where this
-  // doesn't yet work, which are documented and skipped below.
-  if (initiator_origin.opaque()) {
-    // Reloads initiated from error pages may currently lead to a precursor
-    // mismatch, since the error page loads with an opaque origin with the
-    // original URL's origin as its precursor, which may not match the error
-    // page's process lock. This is seen in the following
-    // RenderFrameHostManagerTest tests:
-    // 1. ErrorPageNavigationReload:
-    //    - renderer origin lock = chrome-error://chromewebdata/
-    //    - precursor of initiator origin = http://127.0.0.1:.../
-    // 2. ErrorPageNavigationReload_InSubframe_BlockedByClient
-    //    - renderer origin lock = http://b.com:.../
-    //    - precursor of initiator origin = http://c.com:.../
-    if (current_rfh && current_rfh->IsErrorDocument()) {
-      return true;
-    }
-
-    // Navigations in subframes of MHTML documents may have precursor origins
-    // that do not match the process lock of the MHTML document. This is seen
-    // in NavigationMhtmlBrowserTest.DataIframe, where:
-    //   - renderer origin lock = { file:/// sandboxed }
-    //   - precursor of initiator origin = http://8.8.8.8/
-    // In the past, this case used to be special-cased here, but this is no
-    // longer needed now that ChildProcessSecurityPolicy's enforcements have
-    // been switched to use committed origin tracking. Any frame in the MHTML
-    // page that could legitimately initiate such a navigation has already
-    // committed in this process, so its (opaque) origin has been recorded by
-    // ChildProcessSecurityPolicyImpl::AddCommittedOrigin and the HostsOrigin()
-    // check below will accept it. There is therefore no need to skip the check
-    // for MHTML subframes, and doing so would allow the renderer to claim an
-    // opaque initiator with an arbitrary precursor.
-  }
+  // Important Note about opaque origins: these checks used to be skipped for
+  // opaque origins in two tricky cases, error pages and MHTML subframes. These
+  // exemptions are no longer needed now that ChildProcessSecurityPolicy's
+  // enforcements have been switched to use committed origin tracking. Any error
+  // page or MHTML subframe that could legitimately initiate a navigation has
+  // already committed in this process, so its (opaque) origin has been recorded
+  // by ChildProcessSecurityPolicyImpl::AddCommittedOrigin and the HostsOrigin()
+  // check below will accept it, even if the precursor doesn't match the process
+  // lock. This is covered in tests such as ErrorPageNavigationReload,
+  // ErrorPageNavigationReload_InSubframe_BlockedByClient, and
+  // NavigationMhtmlBrowserTest.DataIframe.
+  //
+  // Warning: avoid skipping this check for future cases, as doing so carries
+  // security consequences, allowing the renderer to claim an opaque initiator
+  // with an arbitrary precursor. See crbug.com/516398679 and
+  // crbug.com/517606780.
 
   auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
   // TODO(crbug.com/379869738): Remove GetUnsafeValue.
diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 1cafdb7..5f9b27f22 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -2376,6 +2376,72 @@
   EXPECT_EQ(bad_message::INVALID_INITIATOR_ORIGIN, kill_waiter.Wait());
 }
 
+// Similar to the test above, but ensure that opaque initiator origins are
+// validated even when the frame sending BeginNavigation is currently showing an
+// error page. Some subframe error pages are not currently process-isolated, so
+// a renderer for the embedding site can send IPCs on their behalf and must not
+// be allowed to claim an arbitrary precursor.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       InvalidBeginNavigationOpaqueInitiatorFromErrorPage) {
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+
+  // Prepare to intercept BeginNavigation mojo IPC. This has to be done before
+  // the test creates the RenderFrameHostImpl that is the target of the IPC.
+  url::Origin injected_origin =
+      url::Origin::Create(GURL("http://evil.com")).DeriveNewOpaqueOrigin();
+  BeginNavigationInitiatorReplacer injector(web_contents, injected_origin);
+
+  // Explicitly isolating a.com helps ensure that this test is applicable on
+  // platforms without site-per-process.
+  IsolateOrigin("a.com");
+
+  // Navigate to a test page that will be locked to a.com.
+  GURL main_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(web_contents, main_url));
+  RenderFrameHostImpl* main_frame =
+      web_contents->GetPrimaryFrameTree().root()->current_frame_host();
+
+  // Set CSP to block iframes, so the subframe navigation below results in an
+  // error page that stays in the current (a.com) process.
+  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 a subframe that fails to load and commits an error page.
+  GURL blocked_url(embedded_test_server()->GetURL("a.com", "/title2.html"));
+  TestNavigationObserver error_observer(web_contents);
+  EXPECT_TRUE(
+      ExecJs(main_frame, JsReplace("var f = document.createElement('iframe');"
+                                   "f.src = $1;"
+                                   "document.body.appendChild(f);",
+                                   blocked_url)));
+  error_observer.Wait();
+  EXPECT_FALSE(error_observer.last_navigation_succeeded());
+
+  RenderFrameHostImpl* error_frame =
+      main_frame->child_at(0)->current_frame_host();
+  ASSERT_TRUE(error_frame->IsErrorDocument());
+
+  // Start monitoring for renderer kills.
+  RenderProcessHostBadIpcMessageWaiter kill_waiter(error_frame->GetProcess());
+
+  // Have the error page subframe navigate and lie that the initiator origin is
+  // an opaque origin with the precursor of evil.com. Note that an absolute URL
+  // is required here, since relative URLs in error documents resolve against
+  // chrome-error://chromewebdata/ and would be blocked in the renderer.
+  injector.Activate();
+  // Don't expect a response for the script, as the process may be killed
+  // before the script sends its completion message.
+  ExecuteScriptAsync(error_frame,
+                     JsReplace("window.location = $1;", blocked_url));
+
+  // Verify that the renderer was terminated.
+  EXPECT_EQ(bad_message::INVALID_INITIATOR_ORIGIN, kill_waiter.Wait());
+}
+
 IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
                        MissingBeginNavigationInitiator) {
   // Prepare to intercept BeginNavigation mojo IPC.  This has to be done before

diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 1cafdb7..5f9b27f22 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -2376,6 +2376,72 @@
   EXPECT_EQ(bad_message::INVALID_INITIATOR_ORIGIN, kill_waiter.Wait());
 }
 
+// Similar to the test above, but ensure that opaque initiator origins are
+// validated even when the frame sending BeginNavigation is currently showing an
+// error page. Some subframe error pages are not currently process-isolated, so
+// a renderer for the embedding site can send IPCs on their behalf and must not
+// be allowed to claim an arbitrary precursor.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       InvalidBeginNavigationOpaqueInitiatorFromErrorPage) {
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+
+  // Prepare to intercept BeginNavigation mojo IPC. This has to be done before
+  // the test creates the RenderFrameHostImpl that is the target of the IPC.
+  url::Origin injected_origin =
+      url::Origin::Create(GURL("http://evil.com")).DeriveNewOpaqueOrigin();
+  BeginNavigationInitiatorReplacer injector(web_contents, injected_origin);
+
+  // Explicitly isolating a.com helps ensure that this test is applicable on
+  // platforms without site-per-process.
+  IsolateOrigin("a.com");
+
+  // Navigate to a test page that will be locked to a.com.
+  GURL main_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(web_contents, main_url));
+  RenderFrameHostImpl* main_frame =
+      web_contents->GetPrimaryFrameTree().root()->current_frame_host();
+
+  // Set CSP to block iframes, so the subframe navigation below results in an
+  // error page that stays in the current (a.com) process.
+  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 a subframe that fails to load and commits an error page.
+  GURL blocked_url(embedded_test_server()->GetURL("a.com", "/title2.html"));
+  TestNavigationObserver error_observer(web_contents);
+  EXPECT_TRUE(
+      ExecJs(main_frame, JsReplace("var f = document.createElement('iframe');"
+                                   "f.src = $1;"
+                                   "document.body.appendChild(f);",
+                                   blocked_url)));
+  error_observer.Wait();
+  EXPECT_FALSE(error_observer.last_navigation_succeeded());
+
+  RenderFrameHostImpl* error_frame =
+      main_frame->child_at(0)->current_frame_host();
+  ASSERT_TRUE(error_frame->IsErrorDocument());
+
+  // Start monitoring for renderer kills.
+  RenderProcessHostBadIpcMessageWaiter kill_waiter(error_frame->GetProcess());
+
+  // Have the error page subframe navigate and lie that the initiator origin is
+  // an opaque origin with the precursor of evil.com. Note that an absolute URL
+  // is required here, since relative URLs in error documents resolve against
+  // chrome-error://chromewebdata/ and would be blocked in the renderer.
+  injector.Activate();
+  // Don't expect a response for the script, as the process may be killed
+  // before the script sends its completion message.
+  ExecuteScriptAsync(error_frame,
+                     JsReplace("window.location = $1;", blocked_url));
+
+  // Verify that the renderer was terminated.
+  EXPECT_EQ(bad_message::INVALID_INITIATOR_ORIGIN, kill_waiter.Wait());
+}
+
 IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
                        MissingBeginNavigationInitiator) {
   // Prepare to intercept BeginNavigation mojo IPC.  This has to be done before