From e2b48fec7f31f474704b4b610db64dc65df4c620 Mon Sep 17 00:00:00 2001
From: Alex Moshchuk <alexmos@chromium.org>
Date: Fri, 24 Jul 2026 12:17:31 -0700
Subject: [PATCH] Validate DidCommit URL for error pages that bypass commit checks

When ShouldBypassSecurityChecksForErrorPage() returns true for an
error-page commit (e.g. a CSP-blocked subframe navigation that stays in
the parent's process, or a main-frame error page in the dedicated
error-page process), ValidateDidCommitParams() skips
ValidateURLAndOrigin() because the committed URL legitimately won't
match the process lock. However, the renderer-supplied URL must still
match the URL the browser asked the renderer to commit, since it is
stored in last_committed_url_ and the FrameNavigationEntry and later
used for reload / session history.

Add a check that params->url matches NavigationRequest::GetURL() in
this case, and terminate the renderer with a new
RFH_ERROR_PAGE_URL_MISMATCH bad-message reason on mismatch.

Bug: 511736672
Change-Id: Ib8f27d8ec22c1ccab606318c68c50a871074d688
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8139503
Reviewed-by: Charlie Reis <creis@chromium.org>
Auto-Submit: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1668062}
---

diff --git a/content/browser/bad_message.h b/content/browser/bad_message.h
index 4de389b6..78a3a648 100644
--- a/content/browser/bad_message.h
+++ b/content/browser/bad_message.h
@@ -386,6 +386,7 @@
   RFH_CREATE_NEW_WINDOW_INVALID_PIP_OPTIONS = 358,
   RFPH_POST_MESSAGE_INVALID_DELEGATED_CAPABILITY = 359,
   RFHI_WEBMCP_OPAQUE_TARGET_ORIGIN = 360,
+  RFH_ERROR_PAGE_URL_MISMATCH = 361,
 
   // Please add new elements here. The naming convention is abbreviated class
   // name (e.g. RenderFrameHost becomes RFH) plus a unique description of the
diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc
index afb90f7f..16bff683 100644
--- a/content/browser/renderer_host/render_frame_host_impl.cc
+++ b/content/browser/renderer_host/render_frame_host_impl.cc
@@ -15599,18 +15599,30 @@
     return false;
   }
 
-  // Error pages must commit in a opaque origin. Terminate the renderer
-  // process if this is violated.
-  if (bypass_checks_for_error_page && !params->origin.opaque()) {
-    DEBUG_ALIAS_FOR_ORIGIN(origin_debug_alias, params->origin);
-    bad_message::ReceivedBadMessage(
-        process, bad_message::RFH_ERROR_PROCESS_NON_UNIQUE_ORIGIN_COMMIT);
-    return false;
-  }
+  if (bypass_checks_for_error_page) {
+    // Error pages must commit in a opaque origin. Terminate the renderer
+    // process if this is violated.
+    if (!params->origin.opaque()) {
+      DEBUG_ALIAS_FOR_ORIGIN(origin_debug_alias, params->origin);
+      bad_message::ReceivedBadMessage(
+          process, bad_message::RFH_ERROR_PROCESS_NON_UNIQUE_ORIGIN_COMMIT);
+      return false;
+    }
 
-  if (!bypass_checks_for_error_page &&
-      !ValidateURLAndOrigin(params->url, params->origin,
-                            is_same_document_navigation, navigation_request)) {
+    // Error pages may legitimately commit a URL that doesn't match the process
+    // lock, so the CanCommitOriginAndUrl checks below are skipped. However, the
+    // committed URL must still match the URL that the browser asked the
+    // renderer to commit, since otherwise the renderer could place an arbitrary
+    // URL into session history and `last_committed_url_`.
+    if (navigation_request && !is_same_document_navigation &&
+        params->url != navigation_request->GetURL()) {
+      bad_message::ReceivedBadMessage(process,
+                                      bad_message::RFH_ERROR_PAGE_URL_MISMATCH);
+      return false;
+    }
+  } else if (!ValidateURLAndOrigin(params->url, params->origin,
+                                   is_same_document_navigation,
+                                   navigation_request)) {
     return false;
   }
 
diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 36f23d8..0738c98 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -35,6 +35,7 @@
 #include "content/browser/dom_storage/session_storage_namespace_impl.h"
 #include "content/browser/fenced_frame/fenced_frame.h"
 #include "content/browser/renderer_host/file_utilities_host_impl.h"
+#include "content/browser/renderer_host/navigation_request.h"
 #include "content/browser/renderer_host/navigator.h"
 #include "content/browser/renderer_host/render_frame_host_impl.h"
 #include "content/browser/renderer_host/render_frame_proxy_host.h"
@@ -4926,4 +4927,86 @@
             kill_waiter.Wait());
 }
 
+namespace {
+
+// Helper class to wait for ReadyToCommitNavigation for the next navigation that
+// results in an error page, and then to wait for a renderer kill for the
+// process that will commit that error page.
+class ErrorPageKillWaiter : public WebContentsObserver {
+ public:
+  explicit ErrorPageKillWaiter(WebContents* web_contents)
+      : WebContentsObserver(web_contents) {}
+
+  // WebContentsObserver:
+  void ReadyToCommitNavigation(NavigationHandle* navigation_handle) override {
+    if (NavigationRequest::From(navigation_handle)->DidEncounterError()) {
+      kill_waiter_ = std::make_unique<RenderProcessHostBadIpcMessageWaiter>(
+          navigation_handle->GetRenderFrameHost()->GetProcess());
+      run_loop_.Quit();
+    }
+  }
+
+  void DidFinishNavigation(NavigationHandle* navigation_handle) override {
+    if (!kill_waiter_) {
+      run_loop_.Quit();
+    }
+  }
+
+  std::optional<bad_message::BadMessageReason> Wait() {
+    run_loop_.Run();
+    if (!kill_waiter_) {
+      return std::nullopt;
+    }
+    return kill_waiter_->Wait();
+  }
+
+ private:
+  std::unique_ptr<RenderProcessHostBadIpcMessageWaiter> kill_waiter_;
+  base::RunLoop run_loop_;
+};
+
+}  // namespace
+
+// Tests that a compromised renderer cannot lie about the URL when committing a
+// CSP-blocked subframe error page. Such error pages are allowed to commit a URL
+// that doesn't match the process lock, but the URL must still match the URL
+// that the browser asked the renderer to commit.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       BlockedSubframeErrorPageDidCommitInvalidURL) {
+  GURL main_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  GURL blocked_url(embedded_test_server()->GetURL("b.com", "/title1.html"));
+  GURL spoofed_url(embedded_test_server()->GetURL("c.com", "/title2.html"));
+
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+  RenderFrameHostImpl* main_frame = static_cast<RenderFrameHostImpl*>(
+      shell()->web_contents()->GetPrimaryMainFrame());
+
+  // Add a CSP that blocks all subframe navigations, so the navigation below
+  // commits 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);"));
+
+  // Simulate a compromised renderer that lies about the committed URL.
+  DidCommitUrlReplacer url_replacer(shell()->web_contents(), spoofed_url);
+
+  // Set up an observer to capture the error page process right before it
+  // commits. Note that we can't assume that it will be the same as the main
+  // frame's process due to subframe error page isolation.
+  ErrorPageKillWaiter kill_waiter(shell()->web_contents());
+
+  // Create an iframe that will be blocked by CSP and commit an error page. The
+  // browser process should detect the URL mismatch and terminate the renderer.
+  // We use ExecuteScriptAsync because the renderer might be terminated before
+  // ExecJs returns.
+  ExecuteScriptAsync(main_frame,
+                     JsReplace("var f = document.createElement('iframe');"
+                               "f.src = $1;"
+                               "document.body.appendChild(f);",
+                               blocked_url));
+  EXPECT_EQ(bad_message::RFH_ERROR_PAGE_URL_MISMATCH, kill_waiter.Wait());
+}
+
 }  // namespace content
diff --git a/tools/metrics/histograms/metadata/stability/enums.xml b/tools/metrics/histograms/metadata/stability/enums.xml
index 89212410..81f29cf 100644
--- a/tools/metrics/histograms/metadata/stability/enums.xml
+++ b/tools/metrics/histograms/metadata/stability/enums.xml
@@ -535,6 +535,7 @@
   <int value="358" label="RFH_CREATE_NEW_WINDOW_INVALID_PIP_OPTIONS"/>
   <int value="359" label="RFPH_POST_MESSAGE_INVALID_DELEGATED_CAPABILITY"/>
   <int value="360" label="RFHI_WEBMCP_OPAQUE_TARGET_ORIGIN"/>
+  <int value="361" label="RFH_ERROR_PAGE_URL_MISMATCH"/>
 </enum>
 
 <enum name="BadMessageReasonExtensions">

diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 36f23d8..0738c98 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -35,6 +35,7 @@
 #include "content/browser/dom_storage/session_storage_namespace_impl.h"
 #include "content/browser/fenced_frame/fenced_frame.h"
 #include "content/browser/renderer_host/file_utilities_host_impl.h"
+#include "content/browser/renderer_host/navigation_request.h"
 #include "content/browser/renderer_host/navigator.h"
 #include "content/browser/renderer_host/render_frame_host_impl.h"
 #include "content/browser/renderer_host/render_frame_proxy_host.h"
@@ -4926,4 +4927,86 @@
             kill_waiter.Wait());
 }
 
+namespace {
+
+// Helper class to wait for ReadyToCommitNavigation for the next navigation that
+// results in an error page, and then to wait for a renderer kill for the
+// process that will commit that error page.
+class ErrorPageKillWaiter : public WebContentsObserver {
+ public:
+  explicit ErrorPageKillWaiter(WebContents* web_contents)
+      : WebContentsObserver(web_contents) {}
+
+  // WebContentsObserver:
+  void ReadyToCommitNavigation(NavigationHandle* navigation_handle) override {
+    if (NavigationRequest::From(navigation_handle)->DidEncounterError()) {
+      kill_waiter_ = std::make_unique<RenderProcessHostBadIpcMessageWaiter>(
+          navigation_handle->GetRenderFrameHost()->GetProcess());
+      run_loop_.Quit();
+    }
+  }
+
+  void DidFinishNavigation(NavigationHandle* navigation_handle) override {
+    if (!kill_waiter_) {
+      run_loop_.Quit();
+    }
+  }
+
+  std::optional<bad_message::BadMessageReason> Wait() {
+    run_loop_.Run();
+    if (!kill_waiter_) {
+      return std::nullopt;
+    }
+    return kill_waiter_->Wait();
+  }
+
+ private:
+  std::unique_ptr<RenderProcessHostBadIpcMessageWaiter> kill_waiter_;
+  base::RunLoop run_loop_;
+};
+
+}  // namespace
+
+// Tests that a compromised renderer cannot lie about the URL when committing a
+// CSP-blocked subframe error page. Such error pages are allowed to commit a URL
+// that doesn't match the process lock, but the URL must still match the URL
+// that the browser asked the renderer to commit.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       BlockedSubframeErrorPageDidCommitInvalidURL) {
+  GURL main_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  GURL blocked_url(embedded_test_server()->GetURL("b.com", "/title1.html"));
+  GURL spoofed_url(embedded_test_server()->GetURL("c.com", "/title2.html"));
+
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+  RenderFrameHostImpl* main_frame = static_cast<RenderFrameHostImpl*>(
+      shell()->web_contents()->GetPrimaryMainFrame());
+
+  // Add a CSP that blocks all subframe navigations, so the navigation below
+  // commits 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);"));
+
+  // Simulate a compromised renderer that lies about the committed URL.
+  DidCommitUrlReplacer url_replacer(shell()->web_contents(), spoofed_url);
+
+  // Set up an observer to capture the error page process right before it
+  // commits. Note that we can't assume that it will be the same as the main
+  // frame's process due to subframe error page isolation.
+  ErrorPageKillWaiter kill_waiter(shell()->web_contents());
+
+  // Create an iframe that will be blocked by CSP and commit an error page. The
+  // browser process should detect the URL mismatch and terminate the renderer.
+  // We use ExecuteScriptAsync because the renderer might be terminated before
+  // ExecJs returns.
+  ExecuteScriptAsync(main_frame,
+                     JsReplace("var f = document.createElement('iframe');"
+                               "f.src = $1;"
+                               "document.body.appendChild(f);",
+                               blocked_url));
+  EXPECT_EQ(bad_message::RFH_ERROR_PAGE_URL_MISMATCH, kill_waiter.Wait());
+}
+
 }  // namespace content