Low chrome Logic Error 🔧 Commit mapped

Overview

Low
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
ImpactInsufficient validation of untrusted input in Network
DescriptionInsufficient validation of untrusted input in Network
ComponentNetwork
Bug ClassLogic Error
Tracker514058566
Fix commit100d829635f5 (chromium/src) +80/-25
CISA KEVNot listed
CreditedGoogle
Disclosed2026-06-30

Changed Functions

FunctionChangeNotes
if
services/network/restricted_cookie_manager.cc
modified
TEST_P
services/network/restricted_cookie_manager_unittest.cc
modified

Files Changed

  • services/network/restricted_cookie_manager.cc
  • services/network/restricted_cookie_manager_unittest.cc
From 100d829635f5ede56c8c777d8b41320b5c738d15 Mon Sep 17 00:00:00 2001
From: Chris Fredrickson <[email protected]>
Date: Fri, 22 May 2026 07:41:52 -0700
Subject: [PATCH] Add missing ValidateAccessToCookiesAt call in SetCookieFromString

Fixed: 514058566
Change-Id: I18c9cef8c1d9c2322d375d2aa8384c962400eb1e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7868213
Auto-Submit: Chris Fredrickson <[email protected]>
Reviewed-by: mmenke <[email protected]>
Commit-Queue: Chris Fredrickson <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1634942}
---

diff --git a/services/network/restricted_cookie_manager.cc b/services/network/restricted_cookie_manager.cc
index cc4607a..164f12c7 100644
--- a/services/network/restricted_cookie_manager.cc
+++ b/services/network/restricted_cookie_manager.cc
@@ -956,6 +956,9 @@
     const std::string& cookie) {
   TRACE_EVENT("net", "RestrictedCookieManager::SetCookieFromString");
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Note: `ValidateAccessToCookiesAt` is checked in `SetCanonicalCookie` below
+  // in the happy path, and in the `if (!parsed_cookie)` block in this function
+  // for the unhappy path.
   base::ElapsedTimer timer;
 
   // The cookie is about to be set. Proactively increment the version so it's
@@ -970,6 +973,9 @@
           url, cookie, base::Time::Now(), /*server_time=*/std::nullopt,
           cookie_partition_key_, net::CookieSourceType::kScript, &status);
   if (!parsed_cookie) {
+    if (!ValidateAccessToCookiesAt(url, site_for_cookies, top_frame_origin)) {
+      return;
+    }
     if (cookie_observer_) {
       std::vector<network::mojom::CookieOrLineWithAccessResultPtr>
           result_with_access_result;
diff --git a/services/network/restricted_cookie_manager_unittest.cc b/services/network/restricted_cookie_manager_unittest.cc
index 4467964c..bbc844e4 100644
--- a/services/network/restricted_cookie_manager_unittest.cc
+++ b/services/network/restricted_cookie_manager_unittest.cc
@@ -129,6 +129,8 @@
 
   std::vector<CookieOp>& recorded_activity() { return recorded_activity_; }
 
+  void Flush() { receivers_.FlushForTesting(); }
+
   mojo::PendingRemote<mojom::CookieAccessObserver> GetRemote() {
     mojo::PendingRemote<mojom::CookieAccessObserver> remote;
     receivers_.Add(this, remote.InitWithNewPipeAndPassReceiver());
@@ -419,7 +421,8 @@
     received_bad_message_ = true;
   }
 
-  std::vector<RecordingCookieObserver::CookieOp>& recorded_activity() {
+  std::vector<RecordingCookieObserver::CookieOp>& RecordedActivity() {
+    recording_client_.Flush();
     return recording_client_.recorded_activity();
   }
 
@@ -788,7 +791,7 @@
               testing::SizeIs(3));
   WaitForCallback();
   EXPECT_THAT(
-      recorded_activity(),
+      RecordedActivity(),
       UnorderedElementsAre(
           MatchesCookieOp(
               mojom::CookieAccessDetails::Type::kRead, kDefaultUrlWithPath,
@@ -805,7 +808,7 @@
               kDefaultSiteForCookies,
               CookieOrLine("cookie3=value3", mojom::CookieOrLine::Tag::kCookie),
               net::IsInclude())));
-  recorded_activity().clear();
+  RecordedActivity().clear();
 
   // Modify one of the cookies and do another get.
   SetSessionCookie("cookie2", "updated-value2", "example.com", "/");
@@ -816,7 +819,7 @@
   WaitForCallback();
 
   // There should only be an access notification for the modified cookie.
-  EXPECT_THAT(recorded_activity(),
+  EXPECT_THAT(RecordedActivity(),
               UnorderedElementsAre(MatchesCookieOp(
                   mojom::CookieAccessDetails::Type::kRead, kDefaultUrlWithPath,
                   kDefaultSiteForCookies,
@@ -841,8 +844,8 @@
                     net::StorageAccessApiStatus::kNone, GetAllCookiesOptions()),
                 testing::SizeIs(101));
     WaitForCallback();
-    EXPECT_THAT(recorded_activity(), testing::SizeIs(101));
-    recorded_activity().clear();
+    EXPECT_THAT(RecordedActivity(), testing::SizeIs(101));
+    RecordedActivity().clear();
   }
 }
 
@@ -891,7 +894,7 @@
                               net::StorageAccessApiStatus::kNone,
                               std::move(options), /*is_ad_tagged=*/true);
   WaitForCallback();
-  EXPECT_THAT(recorded_activity().back().is_ad_tagged, true);
+  EXPECT_THAT(RecordedActivity().back().is_ad_tagged, true);
 
   SetSessionCookie("cookie-name", "cookie-value", "example.com", "/");
   options = mojom::CookieManagerGetOptions::New();
@@ -903,7 +906,7 @@
                               std::move(options), /*is_ad_tagged=*/false);
   WaitForCallback();
 
-  EXPECT_THAT(recorded_activity().back().is_ad_tagged, false);
+  EXPECT_THAT(RecordedActivity().back().is_ad_tagged, false);
 }
 
 TEST_P(RestrictedCookieManagerTest, SetCookieAdTagged) {
@@ -914,7 +917,7 @@
       /*is_ad_tagged=*/true,
       /*apply_devtools_overrides=*/false, "cookie-name=cookie-value;path=/");
   WaitForCallback();
-  EXPECT_THAT(recorded_activity().back().is_ad_tagged, true);
+  EXPECT_THAT(RecordedActivity().back().is_ad_tagged, true);
 
   backend()->SetCookieFromString(
       kDefaultUrlWithPath, net::SiteForCookies(), kDefaultOrigin,
@@ -922,7 +925,7 @@
       /*apply_devtools_overrides=*/false, "cookie-name=cookie-value;path=/");
   WaitForCallback();
 
-  EXPECT_THAT(recorded_activity().back().is_ad_tagged, false);
+  EXPECT_THAT(RecordedActivity().back().is_ad_tagged, false);
 }
 
 TEST_P(RestrictedCookieManagerTest, GetAllForUrlPolicy) {
@@ -951,7 +954,7 @@
   WaitForCallback();
 
   if (ThirdPartyCookieDisabledByDevtools()) {
-    EXPECT_THAT(recorded_activity(),
+    EXPECT_THAT(RecordedActivity(),
                 ElementsAre(MatchesCookieOp(
                     mojom::CookieAccessDetails::Type::kRead,
                     "https://example.com/test/", net::SiteForCookies(),
@@ -962,7 +965,7 @@
                              EXCLUDE_USER_PREFERENCES}))));
 
   } else {
-    EXPECT_THAT(recorded_activity(),
+    EXPECT_THAT(RecordedActivity(),
                 ElementsAre(MatchesCookieOp(
                     mojom::CookieAccessDetails::Type::kRead,
                     "https://example.com/test/", net::SiteForCookies(),
@@ -990,11 +993,11 @@
   if (ThirdPartyCookieDisabledByDevtools()) {
     // No additional activity when 3PCs are forced, because the second access
     // was not a change in result.
-    EXPECT_THAT(recorded_activity(), testing::SizeIs(1));
+    EXPECT_THAT(RecordedActivity(), testing::SizeIs(1));
   } else {
     WaitForCallback();
     EXPECT_THAT(
-        recorded_activity(),
+        RecordedActivity(),
         ElementsAre(testing::_,
                     MatchesCookieOp(
                         mojom::CookieAccessDetails::Type::kRead,
@@ -1034,7 +1037,7 @@
     WaitForCallback();
 
     EXPECT_THAT(
-        recorded_activity(),
+        RecordedActivity(),
         ElementsAre(MatchesCookieOp(
             mojom::CookieAccessDetails::Type::kRead, kDefaultUrlWithPath,
             net::SiteForCookies(),
@@ -1058,7 +1061,7 @@
 
     // A second cookie access should not generate a notification.
     EXPECT_THAT(
-        recorded_activity(),
+        RecordedActivity(),
         ElementsAre(MatchesCookieOp(
             mojom::CookieAccessDetails::Type::kRead, kDefaultUrlWithPath,
             net::SiteForCookies(),
@@ -1091,7 +1094,7 @@
 
     // A change in access result (allowed -> blocked) should generate a new
     // notification.
-    EXPECT_EQ(recorded_activity().size(), 2ul);
+    EXPECT_EQ(RecordedActivity().size(), 2ul);
   }
 
   // Allow the cookie access.
@@ -1113,7 +1116,7 @@
     // A change in access result (blocked -> allowed) should generate a new
     // notification.
     EXPECT_THAT(
-        recorded_activity(),
+        RecordedActivity(),
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/services/network/restricted_cookie_manager_unittest.cc b/services/network/restricted_cookie_manager_unittest.cc
index 4467964c..bbc844e4 100644
--- a/services/network/restricted_cookie_manager_unittest.cc
+++ b/services/network/restricted_cookie_manager_unittest.cc
@@ -129,6 +129,8 @@
 
   std::vector<CookieOp>& recorded_activity() { return recorded_activity_; }
 
+  void Flush() { receivers_.FlushForTesting(); }
+
   mojo::PendingRemote<mojom::CookieAccessObserver> GetRemote() {
     mojo::PendingRemote<mojom::CookieAccessObserver> remote;
     receivers_.Add(this, remote.InitWithNewPipeAndPassReceiver());
@@ -419,7 +421,8 @@
     received_bad_message_ = true;
   }
 
-  std::vector<RecordingCookieObserver::CookieOp>& recorded_activity() {
+  std::vector<RecordingCookieObserver::CookieOp>& RecordedActivity() {
+    recording_client_.Flush();
     return recording_client_.recorded_activity();
   }
 
@@ -788,7 +791,7 @@
               testing::SizeIs(3));
   WaitForCallback();
   EXPECT_THAT(
-      recorded_activity(),
+      RecordedActivity(),
       UnorderedElementsAre(
           MatchesCookieOp(
               mojom::CookieAccessDetails::Type::kRead, kDefaultUrlWithPath,
@@ -805,7 +808,7 @@
               kDefaultSiteForCookies,
               CookieOrLine("cookie3=value3", mojom::CookieOrLine::Tag::kCookie),
               net::IsInclude())));
-  recorded_activity().clear();
+  RecordedActivity().clear();
 
   // Modify one of the cookies and do another get.
   SetSessionCookie("cookie2", "updated-value2", "example.com", "/");
@@ -816,7 +819,7 @@
   WaitForCallback();
 
   // There should only be an access notification for the modified cookie.
-  EXPECT_THAT(recorded_activity(),
+  EXPECT_THAT(RecordedActivity(),
               UnorderedElementsAre(MatchesCookieOp(
                   mojom::CookieAccessDetails::Type::kRead, kDefaultUrlWithPath,
                   kDefaultSiteForCookies,
@@ -841,8 +844,8 @@
                     net::StorageAccessApiStatus::kNone, GetAllCookiesOptions()),
                 testing::SizeIs(101));
     WaitForCallback();
-    EXPECT_THAT(recorded_activity(), testing::SizeIs(101));
-    recorded_activity().clear();
+    EXPECT_THAT(RecordedActivity(), testing::SizeIs(101));
+    RecordedActivity().clear();
   }
 }
 
@@ -891,7 +894,7 @@
                               net::StorageAccessApiStatus::kNone,
                               std::move(options), /*is_ad_tagged=*/true);
   WaitForCallback();
-  EXPECT_THAT(recorded_activity().back().is_ad_tagged, true);
+  EXPECT_THAT(RecordedActivity().back().is_ad_tagged, true);
 
   SetSessionCookie("cookie-name", "cookie-value", "example.com", "/");
   options = mojom::CookieManagerGetOptions::New();
@@ -903,7 +906,7 @@
                               std::move(options), /*is_ad_tagged=*/false);
   WaitForCallback();
 
-  EXPECT_THAT(recorded_activity().back().is_ad_tagged, false);
+  EXPECT_THAT(RecordedActivity().back().is_ad_tagged, false);
 }
 
 TEST_P(RestrictedCookieManagerTest, SetCookieAdTagged) {
@@ -914,7 +917,7 @@
       /*is_ad_tagged=*/true,
       /*apply_devtools_overrides=*/false, "cookie-name=cookie-value;path=/");
   WaitForCallback();
-  EXPECT_THAT(recorded_activity().back().is_ad_tagged, true);
+  EXPECT_THAT(RecordedActivity().back().is_ad_tagged, true);
 
   backend()->SetCookieFromString(
       kDefaultUrlWithPath, net::SiteForCookies(), kDefaultOrigin,
@@ -922,7 +925,7 @@
       /*apply_devtools_overrides=*/false, "cookie-name=cookie-value;path=/");
   WaitForCallback();
 
-  EXPECT_THAT(recorded_activity().back().is_ad_tagged, false);
+  EXPECT_THAT(RecordedActivity().back().is_ad_tagged, false);
 }
 
 TEST_P(RestrictedCookieManagerTest, GetAllForUrlPolicy) {
@@ -951,7 +954,7 @@
   WaitForCallback();
 
   if (ThirdPartyCookieDisabledByDevtools()) {
-    EXPECT_THAT(recorded_activity(),
+    EXPECT_THAT(RecordedActivity(),
                 ElementsAre(MatchesCookieOp(
                     mojom::CookieAccessDetails::Type::kRead,
                     "https://example.com/test/", net::SiteForCookies(),
@@ -962,7 +965,7 @@
                              EXCLUDE_USER_PREFERENCES}))));
 
   } else {
-    EXPECT_THAT(recorded_activity(),
+    EXPECT_THAT(RecordedActivity(),
                 ElementsAre(MatchesCookieOp(
                     mojom::CookieAccessDetails::Type::kRead,
                     "https://example.com/test/", net::SiteForCookies(),
@@ -990,11 +993,11 @@
   if (ThirdPartyCookieDisabledByDevtools()) {
     // No additional activity when 3PCs are forced, because the second access
     // was not a change in result.
-    EXPECT_THAT(recorded_activity(), testing::SizeIs(1));
+    EXPECT_THAT(RecordedActivity(), testing::SizeIs(1));
   } else {
     WaitForCallback();
     EXPECT_THAT(
-        recorded_activity(),
+        RecordedActivity(),
         ElementsAre(testing::_,
                     MatchesCookieOp(
                         mojom::CookieAccessDetails::Type::kRead,
@@ -1034,7 +1037,7 @@
     WaitForCallback();
 
     EXPECT_THAT(
-        recorded_activity(),
+        RecordedActivity(),
         ElementsAre(MatchesCookieOp(
             mojom::CookieAccessDetails::Type::kRead, kDefaultUrlWithPath,
             net::SiteForCookies(),
@@ -1058,7 +1061,7 @@
 
     // A second cookie access should not generate a notification.
     EXPECT_THAT(
-        recorded_activity(),
+        RecordedActivity(),
         ElementsAre(MatchesCookieOp(
             mojom::CookieAccessDetails::Type::kRead, kDefaultUrlWithPath,
             net::SiteForCookies(),
@@ -1091,7 +1094,7 @@
 
     // A change in access result (allowed -> blocked) should generate a new
     // notification.
-    EXPECT_EQ(recorded_activity().size(), 2ul);
+    EXPECT_EQ(RecordedActivity().size(), 2ul);
   }
 
   // Allow the cookie access.
@@ -1113,7 +1116,7 @@
     // A change in access result (blocked -> allowed) should generate a new
     // notification.
     EXPECT_THAT(
-        recorded_activity(),
+        RecordedActivity(),
         Contains(MatchesCookieOp(
             mojom::CookieAccessDetails::Type::kRead, kDefaultUrlWithPath,
             net::SiteForCookies(),
@@ -1154,7 +1157,7 @@
 
   WaitForCallback();
 
-  EXPECT_THAT(recorded_activity(),
+  EXPECT_THAT(RecordedActivity(),
               ElementsAre(MatchesCookieOp(
                   mojom::CookieAccessDetails::Type::kRead,
                   "https://example.com/test/", net::SiteForCookies(),
@@ -1361,6 +1364,52 @@
   ASSERT_TRUE(received_bad_message());
 }
 
+TEST_P(RestrictedCookieManagerTest,
+       SetCookieFromStringInvalidCookieAndWrongOrigin) {
+  ExpectBadMessage();
+  // Control characters are disallowed in cookie names, so
+  // `CanonicalCookie::Create` fails in the backend. Verify that no cookie
+  // access is recorded in that case, since the given URL should not have access
+  // to cookies anyway.
+  backend()->SetCookieFromString(
+      kOtherUrlWithPath, kDefaultSiteForCookies, kDefaultOrigin,
+      net::StorageAccessApiStatus::kNone,
+      /*is_ad_tagged=*/false,
+      /*apply_devtools_overrides=*/false, "bad\x01name=value");
+  service_remote_.FlushForTesting();
+  EXPECT_TRUE(received_bad_message());
+  EXPECT_THAT(RecordedActivity(), IsEmpty());
+}
+
+TEST_P(RestrictedCookieManagerTest,
+       SetCookieFromStringInvalidCookieAndWrongOriginNoObserver) {
+  mojo::Remote<mojom::RestrictedCookieManager> local_service_remote;
+
+  auto local_service = std::make_unique<RestrictedCookieManager>(
+      RestrictedCookieManagerRole(), &cookie_monster_, cookie_settings_,
+      kDefaultOrigin, isolation_info_,
+      /*cookies_setting_overrides=*/CookieSettingOverrides(),
+      /*devtools_cookies_setting_overrides=*/
+      DevtoolsCookieSettingOverrides(),
+      mojo::PendingRemote<mojom::CookieAccessObserver>(),
+      ComputeFirstPartySetMetadataSync(kDefaultOrigin, &cookie_monster_,
+                                       isolation_info_));
+
+  mojo::Receiver<mojom::RestrictedCookieManager> local_receiver(
+      local_service.get(), local_service_remote.BindNewPipeAndPassReceiver());
+
+  ExpectBadMessage();
+  // The control character causes this cookie to be rejected.
+  local_service_remote->SetCookieFromString(
+      kOtherUrlWithPath, kDefaultSiteForCookies, kDefaultOrigin,
+      net::StorageAccessApiStatus::kNone,
+      /*is_ad_tagged=*/false,
+      /*apply_devtools_overrides=*/false, "bad\x01name=value");
+
+  local_service_remote.FlushForTesting();
+  EXPECT_TRUE(received_bad_message());
+}
+
 TEST_P(RestrictedCookieManagerTest, SetCanonicalCookiePolicy) {
   service_->OverrideIsolationInfoForTesting(kOtherIsolationInfo);
   {
@@ -1382,7 +1431,7 @@
 
   WaitForCallback();
   if (ThirdPartyCookieDisabledByDevtools()) {
-    EXPECT_THAT(recorded_activity(),
+    EXPECT_THAT(RecordedActivity(),
                 Contains(MatchesCookieOp(
                     mojom::CookieAccessDetails::Type::kChange,
                     "https://example.com/", net::SiteForCookies(),
@@ -1391,7 +1440,7 @@
                         {net::CookieInclusionStatus::ExclusionReason::
                              EXCLUDE_USER_PREFERENCES}))));
   } else {
-    EXPECT_THAT(recorded_activity(),
+    EXPECT_THAT(RecordedActivity(),
                 ElementsAre(MatchesCookieOp(
                     mojom::CookieAccessDetails::Type::kChange,
                     "https://example.com/", net::SiteForCookies(),
@@ -1415,7 +1464,7 @@
   }
 
   WaitForCallback();
-  EXPECT_THAT(recorded_activity(),
+  EXPECT_THAT(RecordedActivity(),
               Contains(MatchesCookieOp(
                   mojom::CookieAccessDetails::Type::kChange,
                   "https://example.com/", net::SiteForCookies(),
@@ -1443,11 +1492,11 @@
   if (ThirdPartyCookieDisabledByDevtools()) {
     // If third party cookies were force disabled, the cookie would never have
     // been set and thus no operation added to read it
-    EXPECT_THAT(recorded_activity(), testing::SizeIs(2));
+    EXPECT_THAT(RecordedActivity(), testing::SizeIs(2));
 
   } else {
     WaitForCallback();
-    EXPECT_THAT(recorded_activity(),
+    EXPECT_THAT(RecordedActivity(),
                 Contains(MatchesCookieOp(
                     mojom::CookieAccessDetails::Type::kRead,
                     "https://example.com/test/",
@@ -1472,7 +1521,7 @@
 
   WaitForCallback();
 
-  EXPECT_THAT(recorded_activity(),
+  EXPECT_THAT(RecordedActivity(),
               ElementsAre(MatchesCookieOp(
                   mojom::CookieAccessDetails::Type::kChange,
                   "https://example.com/", net::SiteForCookies(),
Loading diff…

Original Bug Report

The reporter's bug is still restricted on the tracker. Chrome de-restricts security bugs ~30–90 days after the fix ships; a later run will backfill it here.