From fdab935fad75a812d0018c29f1a7161f5f36e9b2 Mon Sep 17 00:00:00 2001
From: Emily Stark <estark@google.com>
Date: Wed, 18 Mar 2026 18:54:50 -0700
Subject: [PATCH] Dump without crashing on headers from renderer-initiated navigations

This CL adds validation to headers provided by renderer-initiated
navigations via OpenURL() or BeginNavigation(). It's not clear exactly
what headers a compromised renderer should be able to set (see
https://chromium.googlesource.com/chromium/src/+/HEAD/docs/security/compromised-renderers.md#HTTP-request-headers),
so, via code inspection and CQ, I constructed an allowlist of
non-security-sensitive headers that I determined renderers do set in
practice, and added DumpWithoutCrashing() calls to validate whether this
allowlist is correct/compatible. After evaluating crash data, we'll
convert this check to kill renderers that provide invalid headers. We'll
subsequently have to evaluate additions to the allowlist on a
case-by-case basis because I don't think there is any canonically
correct set of headers that a renderer should be allowed to set.

The `Origin` header is a bit of a special case. The logic for setting
`Origin` is very complicated; sometimes it is set in Blink, sometimes it
is set or overwritten in the browser process, and in different locations
depending on the type of request. For navigation requests,
NavigationRequest was overwriting with the correct value for
non-GET/-HEAD requests, but preserving the value provided by the
renderer on GET/HEAD requests. Based on the existing code comments, it
seems that the intention is to not send an Origin header on GET/HEAD
navigation requests, so I've modified NavigationRequest to
DumpWithoutCrashing to see if we can enforce that, and also
DumpWithoutCrashing if an incorrect Origin value is provided on other
requests. As a possible future cleanup, it might be better if Blink
never set the `Origin` header on navigation requests and always left it
to the browser process to set, so that `Origin` could be removed from
the allowlist of headers that renderers are allowed to provide (see
https://crbug.com/491783215).

Bug: 487795397
Change-Id: I5c19fcff4f0bf08c272f8728d576c3ff8765cbe3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7617635
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Emily Stark <estark@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: mmenke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1601694}
---

diff --git a/content/browser/preloading/prerender/prerender_host_registry_unittest.cc b/content/browser/preloading/prerender/prerender_host_registry_unittest.cc
index ee80b59..7b8a7601 100644
--- a/content/browser/preloading/prerender/prerender_host_registry_unittest.cc
+++ b/content/browser/preloading/prerender/prerender_host_registry_unittest.cc
@@ -1152,7 +1152,7 @@
 TEST_F(PrerenderHostRegistryTest, PurposeHeaderIsIgnoredForParamMatching) {
   EXPECT_TRUE(CheckIsActivatedForParams(
       base::BindLambdaForTesting([](NavigationSimulatorImpl* navigation) {
-        navigation->set_request_headers("Purpose: Test");
+        navigation->set_request_headers("Sec-Purpose: Test");
       })));
   ExpectUniqueSampleOfActivationNavigationParamsMatch(
       PrerenderHost::ActivationNavigationParamsMatch::kOk);
diff --git a/content/browser/renderer_host/ipc_utils.cc b/content/browser/renderer_host/ipc_utils.cc
index c9ca97a..3178822 100644
--- a/content/browser/renderer_host/ipc_utils.cc
+++ b/content/browser/renderer_host/ipc_utils.cc
@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/debug/crash_logging.h"
+#include "base/debug/dump_without_crashing.h"
 #include "base/strings/to_string.h"
 #include "content/browser/bad_message.h"
 #include "content/browser/blob_storage/chrome_blob_storage_context.h"
@@ -23,6 +24,7 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/url_constants.h"
 #include "mojo/public/cpp/system/message_pipe.h"
+#include "net/http/http_request_headers.h"
 #include "third_party/blink/public/mojom/navigation/navigation_params.mojom.h"
 #include "ui/base/window_open_disposition.h"
 
@@ -261,6 +263,10 @@
     return false;
   }
 
+  if (!VerifyNavigationHeaders(process, params->extra_headers)) {
+    return false;
+  }
+
   if (params->initiator_base_url) {
     // `initiator_base_url` should only be defined for about:blank and
     // about:srcdoc navigations, and should never be an empty GURL (if it is not
@@ -456,4 +462,35 @@
   return true;
 }
 
+bool VerifyNavigationHeaders(RenderProcessHost* process,
+                             const std::string& headers) {
+  net::HttpRequestHeaders parsed_headers;
+  parsed_headers.AddHeadersFromString(headers);
+  for (net::HttpRequestHeaders::Iterator header(parsed_headers);
+       header.GetNext();) {
+    // Headers should be strictly allowlisted because there can be security
+    // consequences if a compromised renderer can set arbitrary headers (e.g.,
+    // for CSRF prevention).
+    //
+    // This list allowlists `Origin`, but the value of the `Origin` header is
+    // further validated in NavigationRequest::AddAdditionalRequestHeaders.
+    if (header.name() != net::HttpRequestHeaders::kUpgradeInsecureRequests &&
+        header.name() != net::HttpRequestHeaders::kOrigin &&
+        header.name() != net::HttpRequestHeaders::kContentType &&
+        header.name() != net::HttpRequestHeaders::kUserAgent &&
+        header.name() != net::HttpRequestHeaders::kSecPurpose &&
+        header.name() != net::HttpRequestHeaders::kDNT) {
+      // TODO(https://crbug.com/40093290): Once we have enough data, this should
+      // be a `bad_message::ReceivedBadMessage` and return `false`.
+      SCOPED_CRASH_KEY_STRING64("Bug487795397", "invalid_header",
+                                header.name());
+      if (base::FeatureList::IsEnabled(
+              features::kDumpOnInvalidNavigationHeaders)) {
+        base::debug::DumpWithoutCrashing();
+      }
+    }
+  }
+  return true;
+}
+
 }  // namespace content
diff --git a/content/browser/renderer_host/ipc_utils.h b/content/browser/renderer_host/ipc_utils.h
index 70bb639..b649a90 100644
--- a/content/browser/renderer_host/ipc_utils.h
+++ b/content/browser/renderer_host/ipc_utils.h
@@ -87,6 +87,16 @@
     const std::optional<blink::LocalFrameToken>& initiator_frame_token,
     int initiator_process_id);
 
+// Verifies that |headers| are valid for a navigation request initiated by
+// |process|. For now, this always returns true, indicating that the |headers|
+// are valid. TODO(https://crbug.com/487795397): Later, after evaluating debug
+// data, this will be converted to terminate |process| and return false if
+// |headers| are invalid.
+//
+// This function has to be called on the UI thread.
+bool VerifyNavigationHeaders(RenderProcessHost* process,
+                             const std::string& headers);
+
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_RENDERER_HOST_IPC_UTILS_H_
diff --git a/content/browser/renderer_host/navigation_request.cc b/content/browser/renderer_host/navigation_request.cc
index f1c701ad..67a0d7ce 100644
--- a/content/browser/renderer_host/navigation_request.cc
+++ b/content/browser/renderer_host/navigation_request.cc
@@ -400,12 +400,47 @@
   }
 
   // Next, set the HTTP Origin if needed.
+  std::optional<std::string> existing_origin =
+      headers->GetHeader(net::HttpRequestHeaders::kOrigin);
   if (NeedsHTTPOrigin(headers, method)) {
+    // TODO(https://crbug.com/491783215): investigate whether it is possible to
+    // set Origin headers (at least on navigation requests) exclusively in the
+    // browser process and kill any renderer that provides Origin itself.
     url::Origin origin_header_value = initiator_origin.value_or(url::Origin());
     origin_header_value = Referrer::SanitizeOriginForRequest(
         url, origin_header_value, referrer->policy);
-    headers->SetHeader(net::HttpRequestHeaders::kOrigin,
-                       origin_header_value.Serialize());
+    std::string serialized_origin = origin_header_value.Serialize();
+    if (existing_origin && existing_origin != serialized_origin) {
+      if (base::FeatureList::IsEnabled(features::kDumpOnOriginHeaderMismatch)) {
+        // TODO(https://crbug.com/487795397): this should
+        // be a `bad_message::ReceivedBadMessage` and return `false` once
+        // DumpWithoutCrashing data is evaluated.
+        SCOPED_CRASH_KEY_STRING64("Bug487795397", "invalid_header",
+                                  net::HttpRequestHeaders::kOrigin);
+        SCOPED_CRASH_KEY_STRING64("Bug487795397", "existing_origin",
+                                  existing_origin.value());
+        SCOPED_CRASH_KEY_STRING64("Bug487795397", "serialized_origin",
+                                  serialized_origin);
+        SCOPED_CRASH_KEY_BOOL("Bug487795397", "needs_origin_header", true);
+        base::debug::DumpWithoutCrashing();
+      }
+    }
+    headers->SetHeader(net::HttpRequestHeaders::kOrigin, serialized_origin);
+  } else {
+    if (existing_origin) {
+      if (base::FeatureList::IsEnabled(
+              features::kDumpOnUnexpectedOriginHeader)) {
+        // TODO(https://crbug.com/40093290): this should
+        // be a `bad_message::ReceivedBadMessage` and return `false` once
+        // DumpWithoutCrashing() data is evaluated.
+        SCOPED_CRASH_KEY_STRING64("Bug487795397", "invalid_header",
+                                  net::HttpRequestHeaders::kOrigin);
+        SCOPED_CRASH_KEY_STRING64("Bug487795397", "existing_origin",
+                                  existing_origin.value());
+        SCOPED_CRASH_KEY_BOOL("Bug487795397", "needs_origin_header", false);
+        base::debug::DumpWithoutCrashing();
+      }
+    }
   }
 
   if (base::FeatureList::IsEnabled(features::kDocumentPolicyNegotiation)) {

diff --git a/content/browser/preloading/prerender/prerender_host_registry_unittest.cc b/content/browser/preloading/prerender/prerender_host_registry_unittest.cc
index ee80b59..7b8a7601 100644
--- a/content/browser/preloading/prerender/prerender_host_registry_unittest.cc
+++ b/content/browser/preloading/prerender/prerender_host_registry_unittest.cc
@@ -1152,7 +1152,7 @@
 TEST_F(PrerenderHostRegistryTest, PurposeHeaderIsIgnoredForParamMatching) {
   EXPECT_TRUE(CheckIsActivatedForParams(
       base::BindLambdaForTesting([](NavigationSimulatorImpl* navigation) {
-        navigation->set_request_headers("Purpose: Test");
+        navigation->set_request_headers("Sec-Purpose: Test");
       })));
   ExpectUniqueSampleOfActivationNavigationParamsMatch(
       PrerenderHost::ActivationNavigationParamsMatch::kOk);
diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 2ea9a8e..37e2fb8 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -8,6 +8,7 @@
 #include <tuple>
 
 #include "base/command_line.h"
+#include "base/debug/dump_without_crashing.h"
 #include "base/feature_list.h"
 #include "base/files/file_util.h"
 #include "base/functional/bind.h"
@@ -15,6 +16,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/run_loop.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -276,7 +278,12 @@
 // doesn't perform any dangerous operations in such cases.
 class SecurityExploitBrowserTest : public ContentBrowserTest {
  public:
-  SecurityExploitBrowserTest() {}
+  SecurityExploitBrowserTest() {
+    feature_list_.InitWithFeatures({features::kDumpOnInvalidNavigationHeaders,
+                                    features::kDumpOnUnexpectedOriginHeader,
+                                    features::kDumpOnOriginHeaderMismatch},
+                                   {});
+  }
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     // EmbeddedTestServer::InitializeAndListen() initializes its |base_url_|
@@ -311,6 +318,9 @@
     IsolateOriginsForTesting(embedded_test_server(), shell()->web_contents(),
                              {hostname});
   }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
 };
 
 void SecurityExploitBrowserTest::TestFileChooserWithPath(
@@ -2778,4 +2788,213 @@
   EXPECT_FALSE(main_frame->IsRenderFrameLive());
 }
 
+namespace {
+class ScopedDumpWithoutCrashingCallback {
+ public:
+  explicit ScopedDumpWithoutCrashingCallback(base::RepeatingClosure callback) {
+    DCHECK(!callback_);
+    callback_ = std::move(callback);
+    base::debug::SetDumpWithoutCrashingFunction(&DumpWithoutCrashingHandler);
+  }
+
+  ~ScopedDumpWithoutCrashingCallback() {
+    base::debug::SetDumpWithoutCrashingFunction(nullptr);
+    callback_.Reset();
+  }
+
+ private:
+  static void DumpWithoutCrashingHandler() {
+    if (callback_) {
+      callback_.Run();
+    }
+  }
+
+  static base::RepeatingClosure callback_;
+};
+
+base::RepeatingClosure ScopedDumpWithoutCrashingCallback::callback_;
+
+class NavigationHeaderInterceptor : public FrameHostInterceptor {
+ public:
+  explicit NavigationHeaderInterceptor(WebContents* web_contents)
+      : FrameHostInterceptor(web_contents) {}
+
+  NavigationHeaderInterceptor(const NavigationHeaderInterceptor&) = delete;
+  NavigationHeaderInterceptor& operator=(const NavigationHeaderInterceptor&) =
+      delete;
+
+  void set_headers_to_inject(const std::string& headers) {
+    headers_to_inject_ = headers;
+  }
+
+  void Activate() { is_activated_ = true; }
+
+  bool WillDispatchBeginNavigation(
+      RenderFrameHost* render_frame_host,
+      blink::mojom::CommonNavigationParamsPtr* common_params,
+      blink::mojom::BeginNavigationParamsPtr* begin_params,
+      mojo::PendingRemote<blink::mojom::BlobURLToken>* blob_url_token,
+      mojo::PendingAssociatedRemote<mojom::NavigationClient>* navigation_client)
+      override {
+    if (is_activated_ && headers_to_inject_.has_value()) {
+      (*begin_params)->headers = headers_to_inject_.value();
+      is_activated_ = false;
+    }
+
+    return true;
+  }
+
+ private:
+  std::optional<std::string> headers_to_inject_;
+  bool is_activated_ = false;
+};
+}  // namespace
+
+// Tests that DumpWithoutCrashing() is called if a renderer process provides
+// arbitrary headers in a navigation request.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       ForbiddenHeaderInBeginNavigation) {
+  GURL start_url(embedded_test_server()->GetURL("a.test", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), start_url));
+
+  NavigationHeaderInterceptor interceptor(shell()->web_contents());
+  interceptor.set_headers_to_inject("Cookie: secret=123");
+  interceptor.Activate();
+
+  RenderFrameHost* rfhi = shell()->web_contents()->GetPrimaryMainFrame();
+  base::RunLoop run_loop;
+  ScopedDumpWithoutCrashingCallback dump_callback(run_loop.QuitClosure());
+
+  ExecuteScriptAsync(rfhi, "location = '/title2.html';");
+  run_loop.Run();
+}
+
+// Tests that DumpWithoutCrashing() is called if a renderer process provides
+// arbitrary headers in an OpenURL request.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest, ForbiddenHeaderInOpenURL) {
+  GURL start_url(embedded_test_server()->GetURL("a.test", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), start_url));
+
+  RenderFrameHostImpl* rfhi = static_cast<RenderFrameHostImpl*>(
+      shell()->web_contents()->GetPrimaryMainFrame());
+
+  auto params = CreateOpenURLParams(
+      embedded_test_server()->GetURL("a.test", "/title2.html"));
+  params->extra_headers = "Cookie: secret=123";
+
+  base::RunLoop run_loop;
+  ScopedDumpWithoutCrashingCallback dump_callback(run_loop.QuitClosure());
+  static_cast<mojom::FrameHost*>(rfhi)->OpenURL(std::move(params));
+  run_loop.Run();
+}
+
+// Tests that if a renderer provides an Origin header on an OpenURL request,
+// DumpWithoutCrashing() is called.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       ForbiddenOriginHeaderInOpenURL) {
+  GURL start_url(embedded_test_server()->GetURL("a.test", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), start_url));
+
+  RenderFrameHostImpl* rfhi = static_cast<RenderFrameHostImpl*>(
+      shell()->web_contents()->GetPrimaryMainFrame());
+
+  auto params = CreateOpenURLParams(
+      embedded_test_server()->GetURL("a.test", "/echoheader?origin"));
+  params->extra_headers = "Origin: https://b.test";
+
+  base::RunLoop run_loop;
+  ScopedDumpWithoutCrashingCallback dump_callback(run_loop.QuitClosure());
+  TestNavigationObserver nav_observer(shell()->web_contents());
+  static_cast<mojom::FrameHost*>(rfhi)->OpenURL(std::move(params));
+  nav_observer.Wait();
+  run_loop.Run();
+  // TODO(https://crbug.com/487795397): after DumpWithoutCrashing data is
+  // evaluated, the renderer should be killed if it provides an incorrect Origin
+  // header on an OpenURL request.
+  EXPECT_EQ("https://b.test",
+            EvalJs(shell(), "document.body.innerText").ExtractString());
+}
+
+// Tests that if a renderer provides an Origin header on a GET request,
+// DumpWithoutCrashing() is called.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       OriginHeaderGETMismatchesInitiator) {
+  GURL start_url(embedded_test_server()->GetURL("a.test", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), start_url));
+
+  NavigationHeaderInterceptor interceptor(shell()->web_contents());
+  interceptor.set_headers_to_inject("Origin: https://b.test");
+  interceptor.Activate();
+
+  RenderFrameHost* rfhi = shell()->web_contents()->GetPrimaryMainFrame();
+  base::RunLoop run_loop;
+  ScopedDumpWithoutCrashingCallback dump_callback(run_loop.QuitClosure());
+  TestNavigationObserver nav_observer(shell()->web_contents());
+  ExecuteScriptAsync(rfhi, "location = '/echoheader?origin';");
+  nav_observer.Wait();
+  run_loop.Run();
+  // TODO(https://crbug.com/487795397): after DumpWithoutCrashing data is
+  // evaluated, the renderer should be killed if it provides an incorrect Origin
+  // header on a GET request.
+  EXPECT_EQ("https://b.test",
+            EvalJs(shell(), "document.body.innerText").ExtractString());
+}
+
+// Tests that if a renderer provides an Origin header on a cross-origin GET
+// request, DumpWithoutCrashing() is called.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       OriginHeaderCrossOriginGETMismatchesInitiator) {
+  GURL start_url(embedded_test_server()->GetURL("a.test", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), start_url));
+
+  NavigationHeaderInterceptor interceptor(shell()->web_contents());
+  interceptor.set_headers_to_inject("Origin: https://b.test");
+  interceptor.Activate();
+
+  RenderFrameHost* rfhi = shell()->web_contents()->GetPrimaryMainFrame();
+  base::RunLoop run_loop;
+  ScopedDumpWithoutCrashingCallback dump_callback(run_loop.QuitClosure());
+  TestNavigationObserver nav_observer(shell()->web_contents());
+  ExecuteScriptAsync(rfhi, "location = '" +
+                               embedded_test_server()
+                                   ->GetURL("c.test", "/echoheader?origin")
+                                   .spec() +
+                               "'");
+  nav_observer.Wait();
+  run_loop.Run();
+  // TODO(https://crbug.com/487795397): after DumpWithoutCrashing data is
+  // evaluated, the renderer should be killed if it provides an incorrect Origin
+  // header on an OpenURL request.
+  EXPECT_EQ("https://b.test",
+            EvalJs(shell(), "document.body.innerText").ExtractString());
+}
+
+// Tests that a renderer can provide an Origin header on a POST request but
+// DumpWithoutCrashing() is called if the origin was incorrect.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       OriginHeaderPOSTMismatchesInitiator) {
+  GURL start_url(embedded_test_server()->GetURL("a.test", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), start_url));
+
+  NavigationHeaderInterceptor interceptor(shell()->web_contents());
+  interceptor.set_headers_to_inject("Origin: https://b.test");
+  interceptor.Activate();
+
+  RenderFrameHost* rfhi = shell()->web_contents()->GetPrimaryMainFrame();
+  base::RunLoop run_loop;
+  ScopedDumpWithoutCrashingCallback dump_callback(run_loop.QuitClosure());
+  TestNavigationObserver nav_observer(shell()->web_contents());
+  ExecuteScriptAsync(
+      rfhi,
+      "var f = document.createElement('form'); f.action='/echoheader?origin'; "
+      "f.method='POST'; document.body.appendChild(f); f.submit();");
+  nav_observer.Wait();
+  run_loop.Run();
+  // TODO(https://crbug.com/487795397): after DumpWithoutCrashing data is
+  // evaluated, the renderer should be killed if it provides an incorrect Origin
+  // header on a POST request.
+  EXPECT_EQ(url::Origin::Create(start_url).Serialize(),
+            EvalJs(shell(), "document.body.innerText").ExtractString());
+}
+
 }  // namespace content