From 1917c17d0ad2fdc3e994a9aefd254b8db7bbe457 Mon Sep 17 00:00:00 2001
From: Takashi Toyoshima <[email protected]>
Date: Wed, 22 Apr 2026 19:42:38 -0700
Subject: [PATCH] OOR-CORS: Enforce extra forbidden Sec- header checks

This CL adds a new function ContainsForbiddenSecurityHeader in
header_util to check for unauthorized Sec- headers from renderer.

This function is used in both CorsURLLoaderFactory::IsValidRequest and
CorsURLLoader::FollowRedirect to prevent renderer from injecting or
modifying these headers, while permitting Client Hints and Sec-Purpose.

Change-Id: Ia2923ccbddddba2657d72916a03e657c1785cd85
Bug: 494800494
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7760510
Reviewed-by: Kenichi Ishibashi <[email protected]>
Reviewed-by: Yoshisato Yanagisawa <[email protected]>
Commit-Queue: Takashi Toyoshima <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1619285}
---

diff --git a/services/network/cors/cors_url_loader.cc b/services/network/cors/cors_url_loader.cc
index 3014a28f..6c236903 100644
--- a/services/network/cors/cors_url_loader.cc
+++ b/services/network/cors/cors_url_loader.cc
@@ -456,8 +456,17 @@
     return;
   }
 
+  net::HttpRequestHeaders mutable_modified_headers = modified_headers;
+  if (!process_id_.is_browser() &&
+      ContainsForbiddenSecurityHeader(mutable_modified_headers)) {
+    mojo::ReportBadMessage(
+        "CorsURLLoader: Forbidden Sec- header from renderer in FollowRedirect");
+    HandleComplete(URLLoaderCompletionStatus(net::ERR_INVALID_ARGUMENT));
+    return;
+  }
+
   // Does not allow modifying headers that are stored in `cors_exempt_headers`.
-  for (const auto& header : modified_headers.GetHeaderVector()) {
+  for (const auto& header : mutable_modified_headers.GetHeaderVector()) {
     if (request_.cors_exempt_headers.HasHeader(header.key)) {
       LOG(WARNING) << "A client is trying to modify header value for '"
                    << header.key << "', but it is not permitted.";
@@ -480,9 +489,10 @@
     request_.headers.RemoveHeader(name);
     request_.cors_exempt_headers.RemoveHeader(name);
   }
-  request_.headers.MergeFrom(modified_headers);
 
-  if (GetSecSharedStorageWritableHeader(modified_headers)) {
+  request_.headers.MergeFrom(mutable_modified_headers);
+
+  if (GetSecSharedStorageWritableHeader(mutable_modified_headers)) {
     request_.shared_storage_writable_eligible = true;
   } else if (std::ranges::contains(removed_headers,
                                    kSecSharedStorageWritableHeader)) {
diff --git a/services/network/cors/cors_url_loader_factory.cc b/services/network/cors/cors_url_loader_factory.cc
index bf02d66..b83f8b4 100644
--- a/services/network/cors/cors_url_loader_factory.cc
+++ b/services/network/cors/cors_url_loader_factory.cc
@@ -583,7 +583,7 @@
 }
 
 bool CorsURLLoaderFactory::IsValidRequest(
-    const ResourceRequest& request,
+    ResourceRequest& request,
     uint32_t options,
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
   if (request.url.SchemeIs(url::kDataScheme)) {
@@ -863,6 +863,13 @@
     return false;
   }
 
+  if (!process_id_.is_browser() &&
+      ContainsForbiddenSecurityHeader(request.headers)) {
+    mojo::ReportBadMessage(
+        "CorsURLLoaderFactory: Forbidden Sec- header from renderer");
+    return false;
+  }
+
   if (!AreRequestHeadersSafe(request.headers) ||
       !AreRequestHeadersSafe(request.cors_exempt_headers)) {
     return false;
diff --git a/services/network/cors/cors_url_loader_factory.h b/services/network/cors/cors_url_loader_factory.h
index fb44c6f4..565a58aa 100644
--- a/services/network/cors/cors_url_loader_factory.h
+++ b/services/network/cors/cors_url_loader_factory.h
@@ -159,7 +159,7 @@
   void DestroyCorsURLLoader(CorsURLLoader* loader);
 
   bool IsValidRequest(
-      const ResourceRequest& request,
+      ResourceRequest& request,
       uint32_t options,
       const net::MutableNetworkTrafficAnnotationTag& traffic_annotation);
 
diff --git a/services/network/public/cpp/features.cc b/services/network/public/cpp/features.cc
index 97cdcac..c4458bf6 100644
--- a/services/network/public/cpp/features.cc
+++ b/services/network/public/cpp/features.cc
@@ -652,4 +652,7 @@
                    "initial_doh_probe_timeout",
                    base::Seconds(5));
 
+BASE_FEATURE(kRestrictForbiddenSecurityHeaders,
+             base::FEATURE_ENABLED_BY_DEFAULT);
+
 }  // namespace network::features
diff --git a/services/network/public/cpp/features.h b/services/network/public/cpp/features.h
index 55aacccf..3d11d098 100644
--- a/services/network/public/cpp/features.h
+++ b/services/network/public/cpp/features.h
@@ -445,6 +445,9 @@
 COMPONENT_EXPORT(NETWORK_CPP_FLAGS_AND_SWITCHES)
 BASE_DECLARE_FEATURE_PARAM(base::TimeDelta, kDelayInitialDohProbeTimeoutParam);
 
+COMPONENT_EXPORT(NETWORK_CPP_FLAGS_AND_SWITCHES)
+BASE_DECLARE_FEATURE(kRestrictForbiddenSecurityHeaders);
+
 }  // namespace network::features
 
 #endif  // SERVICES_NETWORK_PUBLIC_CPP_FEATURES_H_
diff --git a/services/network/public/cpp/header_util.cc b/services/network/public/cpp/header_util.cc
index 18ced593..ca0abc8 100644
--- a/services/network/public/cpp/header_util.cc
+++ b/services/network/public/cpp/header_util.cc
@@ -4,16 +4,20 @@
 
 #include "services/network/public/cpp/header_util.h"
 
+#include <map>
 #include <string>
 #include <vector>
 
 #include "base/containers/fixed_flat_map.h"
+#include "base/feature_list.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "net/base/mime_sniffer.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
+#include "services/network/public/cpp/features.h"
 #include "services/network/public/mojom/url_response_head.mojom.h"
 #include "url/gurl.h"
 
@@ -105,6 +109,94 @@
   return true;
 }
 
+bool ContainsForbiddenSecurityHeader(net::HttpRequestHeaders& headers) {
+  static const bool enabled =
+      base::FeatureList::IsEnabled(features::kRestrictForbiddenSecurityHeaders);
+  if (!enabled) {
+    return false;
+  }
+
+  std::map<std::string, std::string> headers_to_truncate;
+
+  auto sanitize_and_check_security_header = [&](std::string_view name,
+                                                std::string_view value) {
+    // Client Hints are harmless and set by the renderer.
+    if (base::StartsWith(name, "Sec-CH-",
+                         base::CompareCase::INSENSITIVE_ASCII)) {
+      size_t size = value.size();
+      base::UmaHistogramCounts10000("NetworkService.SecCHHeaderSize", size);
+      if (size > 1024) {
+        headers_to_truncate[std::string(name)] =
+            std::string(value.substr(0, 1024));
+      }
+      return true;
+    }
+    // Sec-Purpose is used for prefetch hints and contains short strings.
+    if (base::EqualsCaseInsensitiveASCII(name, "Sec-Purpose")) {
+      return value.size() < 256;
+    }
+    // Browsing Topics API headers contain structured interest tokens.
+    if (base::EqualsCaseInsensitiveASCII(name, "Sec-Browsing-Topics")) {
+      return value.size() < 1024;
+    }
+    // Shared Storage and FLEDGE fetch headers use structured boolean "?1".
+    if (base::EqualsCaseInsensitiveASCII(name, "Sec-Shared-Storage-Writable") ||
+        base::EqualsCaseInsensitiveASCII(name, "Sec-Ad-Auction-Fetch")) {
+      return value == "?1";
+    }
+    // Shared Storage data origin headers contain origin URLs.
+    if (base::EqualsCaseInsensitiveASCII(name,
+                                         "Sec-Shared-Storage-Data-Origin")) {
+      return value.size() <= 267;
+    }
+    // Speculation Rules headers contain comma-separated tokens.
+    if (base::EqualsCaseInsensitiveASCII(name, "Sec-Speculation-Tags")) {
+      size_t size = value.size();
+      base::UmaHistogramCounts10000(
+          "NetworkService.SecSpeculationTagsHeaderSize", size);
+      if (size > 2048) {
+        std::string_view truncated_value = value.substr(0, 2048);
+        size_t last_comma = truncated_value.rfind(',');

diff --git a/services/network/public/cpp/header_util_unittest.cc b/services/network/public/cpp/header_util_unittest.cc
index d876da32..fc2feddf 100644
--- a/services/network/public/cpp/header_util_unittest.cc
+++ b/services/network/public/cpp/header_util_unittest.cc
@@ -8,6 +8,7 @@
 #include "base/strings/stringprintf.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_response_headers.h"
+#include "services/network/public/cpp/features.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace network {
@@ -143,4 +144,174 @@
   }
 }
 
+TEST(HeaderUtilTest, ContainsForbiddenSecurityHeader) {
+  net::HttpRequestHeaders headers;
+
+  // Normal case
+  headers.SetHeader("Sec-CH-UA", "Normal Value");
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  std::string value;
+  auto value_opt = headers.GetHeader("Sec-CH-UA");
+  ASSERT_TRUE(value_opt.has_value());
+  value = *value_opt;
+  EXPECT_EQ(value, "Normal Value");
+
+  // Truncation case
+  std::string long_value(2000, 'a');
+  headers.SetHeader("Sec-CH-UA-Long", long_value);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  value_opt = headers.GetHeader("Sec-CH-UA-Long");
+  ASSERT_TRUE(value_opt.has_value());
+  value = *value_opt;
+  EXPECT_EQ(value.length(), 1024u);
+  EXPECT_EQ(value, long_value.substr(0, 1024));
+
+  // Boundary cases for Sec-CH-
+  std::string value_1023(1023, 'a');
+  headers.SetHeader("Sec-CH-UA-1023", value_1023);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  EXPECT_EQ(*headers.GetHeader("Sec-CH-UA-1023"), value_1023);
+
+  std::string value_1024(1024, 'a');
+  headers.SetHeader("Sec-CH-UA-1024", value_1024);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  EXPECT_EQ(*headers.GetHeader("Sec-CH-UA-1024"), value_1024);
+
+  std::string value_1025(1025, 'a');
+  headers.SetHeader("Sec-CH-UA-1025", value_1025);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  EXPECT_EQ(headers.GetHeader("Sec-CH-UA-1025")->length(), 1024u);
+
+  // Non-Sec-CH- header should not be truncated even if long
+  headers.SetHeader("X-Custom-Header", long_value);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  value_opt = headers.GetHeader("X-Custom-Header");
+  ASSERT_TRUE(value_opt.has_value());
+  value = *value_opt;
+  EXPECT_EQ(value.length(), 2000u);
+
+  // Sec-Shared-Storage-Data-Origin boundary cases
+  net::HttpRequestHeaders origin_headers;
+  std::string origin_267(267, 'a');
+  origin_headers.SetHeader("Sec-Shared-Storage-Data-Origin", origin_267);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(origin_headers));
+
+  std::string origin_268(268, 'a');
+  origin_headers.SetHeader("Sec-Shared-Storage-Data-Origin", origin_268);
+  EXPECT_TRUE(ContainsForbiddenSecurityHeader(origin_headers));
+
+  // Forbidden Sec- header
+  headers.SetHeader("Sec-Invalid", "value");
+  EXPECT_TRUE(ContainsForbiddenSecurityHeader(headers));
+}
+
+TEST(HeaderUtilTest,
+     ContainsForbiddenSecurityHeader_SecSpeculationTags_Truncation) {
+  net::HttpRequestHeaders headers;
+
+  // Normal case (< 2048)
+  std::string normal_value = "tag1,tag2";
+  headers.SetHeader("Sec-Speculation-Tags", normal_value);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  auto value_opt = headers.GetHeader("Sec-Speculation-Tags");
+  ASSERT_TRUE(value_opt.has_value());
+  EXPECT_EQ(*value_opt, normal_value);
+
+  // Truncation case with comma
+  std::string long_value = std::string(2000, 'a') + "," + std::string(100, 'b');
+  // total size = 2000 + 1 + 100 = 2101 > 2048
+  headers.SetHeader("Sec-Speculation-Tags", long_value);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  value_opt = headers.GetHeader("Sec-Speculation-Tags");
+  ASSERT_TRUE(value_opt.has_value());
+  std::string value = *value_opt;
+  EXPECT_LE(value.length(), 2048u);
+  // It should be truncated at the last comma before 2048.
+  // 2000 'a's + 1 comma = 2001 bytes. The next is 'b'.
+  // So it should truncate at the comma.
+  EXPECT_EQ(value, std::string(2000, 'a'));
+
+  // Truncation case without comma (fallback to 2048)
+  std::string very_long_tag(2500, 'c');
+  headers.SetHeader("Sec-Speculation-Tags", very_long_tag);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  value_opt = headers.GetHeader("Sec-Speculation-Tags");
+  ASSERT_TRUE(value_opt.has_value());
+  value = *value_opt;
+  EXPECT_EQ(value.length(), 2048u);
+  EXPECT_EQ(value, very_long_tag.substr(0, 2048));
+
+  // Boundary cases for Sec-Speculation-Tags
+  std::string value_2047(2047, 'a');
+  headers.SetHeader("Sec-Speculation-Tags", value_2047);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  EXPECT_EQ(*headers.GetHeader("Sec-Speculation-Tags"), value_2047);
+
+  std::string value_2048(2048, 'a');
+  headers.SetHeader("Sec-Speculation-Tags", value_2048);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  EXPECT_EQ(*headers.GetHeader("Sec-Speculation-Tags"), value_2048);
+
+  std::string value_2049(2049, 'a');
+  headers.SetHeader("Sec-Speculation-Tags", value_2049);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  EXPECT_EQ(headers.GetHeader("Sec-Speculation-Tags")->length(), 2048u);
+
+  // Boundary cases with comma
+  std::string comma_at_2047 = std::string(2047, 'a') + ",b";
+  headers.SetHeader("Sec-Speculation-Tags", comma_at_2047);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  EXPECT_EQ(*headers.GetHeader("Sec-Speculation-Tags"), std::string(2047, 'a'));
+
+  std::string comma_at_2048 = std::string(2048, 'a') + ",b";
+  headers.SetHeader("Sec-Speculation-Tags", comma_at_2048);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  EXPECT_EQ(*headers.GetHeader("Sec-Speculation-Tags"), std::string(2048, 'a'));
+}
+
+TEST(HeaderUtilTest, ContainsForbiddenSecurityHeader_SecAdAuction_Truncation) {
+  net::HttpRequestHeaders headers;
+
+  // Normal case (< 2048)
+  std::string normal_value = "auction-signal-data";
+  headers.SetHeader("Sec-Ad-Auction-Signals", normal_value);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  auto value_opt = headers.GetHeader("Sec-Ad-Auction-Signals");
+  ASSERT_TRUE(value_opt.has_value());
+  EXPECT_EQ(*value_opt, normal_value);
+
+  // Truncation case
+  std::string long_value(2500, 'a');
+  headers.SetHeader("Sec-Ad-Auction-Signals", long_value);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  value_opt = headers.GetHeader("Sec-Ad-Auction-Signals");
+  ASSERT_TRUE(value_opt.has_value());
+  std::string value = *value_opt;
+  EXPECT_EQ(value.length(), 2048u);
+  EXPECT_EQ(value, long_value.substr(0, 2048));
+
+  // Boundary cases for Sec-Ad-Auction-Signals
+  std::string value_2047(2047, 'a');
+  headers.SetHeader("Sec-Ad-Auction-Signals", value_2047);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  EXPECT_EQ(*headers.GetHeader("Sec-Ad-Auction-Signals"), value_2047);
+
+  std::string value_2048(2048, 'a');
+  headers.SetHeader("Sec-Ad-Auction-Signals", value_2048);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  EXPECT_EQ(*headers.GetHeader("Sec-Ad-Auction-Signals"), value_2048);
+
+  std::string value_2049(2049, 'a');
+  headers.SetHeader("Sec-Ad-Auction-Signals", value_2049);
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+  EXPECT_EQ(headers.GetHeader("Sec-Ad-Auction-Signals")->length(), 2048u);
+
+  // Sec-Ad-Auction-Fetch should still be strictly checked
+  headers.SetHeader("Sec-Ad-Auction-Fetch", "?1");
+  EXPECT_FALSE(ContainsForbiddenSecurityHeader(headers));
+
+  headers.SetHeader("Sec-Ad-Auction-Fetch", "?0");
+  EXPECT_TRUE(ContainsForbiddenSecurityHeader(headers));
+}
+
 }  // namespace network