PatchGap
Chrome · Extensions

CVE-2026-87613

Logic Error in Extensions
Medium chrome Logic Error 📄 Reporter bug report 🔧 Commit mapped

Overview

Medium
Severity
—
CVSS
No
Exploited ITW
Fixed
Fix Status
ImpactIncorrect reference resolution in Extensions
DescriptionIncorrect reference resolution in Extensions
ComponentExtensions
Bug ClassLogic Error
Tracker501889544
Fix commit04081f5e8d44 (chromium/src) +329/-200
CISA KEVNot listed
CreditedGoogle
Disclosed2026-09-08

Changed Functions

FunctionChangeNotes
PostOnBackendSequenceAndWait
chrome/browser/extensions/api/storage/settings_sync_unittest.cc
 modified

Files Changed

  • chrome/browser/extensions/api/storage/settings_sync_unittest.cc

Patch

view commit
From 04081f5e8d444664f7152f4a3447b7542fc5db61 Mon Sep 17 00:00:00 2001
From: Marc Treib <[email protected]>
Date: Wed, 29 Jul 2026 02:40:55 -0700
Subject: [PATCH] Extension settings sync: Reject entries with malformed extension IDs

These shouldn't occur in practice, but filtering them out prevents any
potential issues further down the line.

Fixed: 501889544
Change-Id: Idd918b4e86b75895310b422aee8d2c1f6a6a6964
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8157404
Reviewed-by: Devlin Cronin <[email protected]>
Commit-Queue: Marc Treib <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1670086}
---

diff --git a/chrome/browser/extensions/api/storage/settings_sync_unittest.cc b/chrome/browser/extensions/api/storage/settings_sync_unittest.cc
index eaeaab9..700b27d 100644
--- a/chrome/browser/extensions/api/storage/settings_sync_unittest.cc
+++ b/chrome/browser/extensions/api/storage/settings_sync_unittest.cc
@@ -50,6 +50,13 @@
 // To save typing ValueStore::DEFAULTS everywhere.
 const ValueStore::WriteOptions DEFAULTS = ValueStore::DEFAULTS;
 
+// Valid extension IDs for use as test data.
+constexpr char kId1[] = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
+constexpr char kId2[] = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb";
+constexpr char kId3[] = "cccccccccccccccccccccccccccccccc";
+constexpr char kId4[] = "dddddddddddddddddddddddddddddddd";
+constexpr char kId5[] = "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee";
+
 // More saving typing. Maps extension IDs to a list of sync changes for that
 // extension.
 using SettingSyncDataMultimap =
@@ -313,7 +320,7 @@
   });
 
   // Have one extension created before sync is set up, the other created after.
-  AddExtensionAndGetStorage("s1", type);
+  AddExtensionAndGetStorage(kId1, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     EXPECT_EQ(0u, GetAllSyncData(data_type).size());
@@ -322,7 +329,7 @@
         data_type, syncer::SyncDataList(), std::move(sync_processor_wrapper_));
   });
 
-  AddExtensionAndGetStorage("s2", type);
+  AddExtensionAndGetStorage(kId2, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     EXPECT_EQ(0u, GetAllSyncData(data_type).size());
@@ -342,8 +349,8 @@
   base::Value value2(base::Value::Type::LIST);
   value2.GetList().Append("barValue");
 
-  ValueStore* storage1 = AddExtensionAndGetStorage("s1", type);
-  ValueStore* storage2 = AddExtensionAndGetStorage("s2", type);
+  ValueStore* storage1 = AddExtensionAndGetStorage(kId1, type);
+  ValueStore* storage2 = AddExtensionAndGetStorage(kId2, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     storage1->Set(DEFAULTS, "foo", value1);
@@ -351,16 +358,16 @@
 
     SettingSyncDataMultimap all_sync_data = GetAllSyncData(data_type);
     EXPECT_EQ(2u, all_sync_data.size());
-    EXPECT_EQ(1u, all_sync_data["s1"]->size());
-    EXPECT_PRED_FORMAT2(ValuesEq, &value1, &(*all_sync_data["s1"])[0]->value());
-    EXPECT_EQ(1u, all_sync_data["s2"]->size());
-    EXPECT_PRED_FORMAT2(ValuesEq, &value2, &(*all_sync_data["s2"])[0]->value());
+    EXPECT_EQ(1u, all_sync_data[kId1]->size());
+    EXPECT_PRED_FORMAT2(ValuesEq, &value1, &(*all_sync_data[kId1])[0]->value());
+    EXPECT_EQ(1u, all_sync_data[kId2]->size());
+    EXPECT_PRED_FORMAT2(ValuesEq, &value2, &(*all_sync_data[kId2])[0]->value());
 
     syncer::SyncDataList sync_data;
     sync_data.push_back(
-        settings_sync_util::CreateData("s1", "foo", value1, data_type));
+        settings_sync_util::CreateData(kId1, "foo", value1, data_type));
     sync_data.push_back(
-        settings_sync_util::CreateData("s2", "bar", value2, data_type));
+        settings_sync_util::CreateData(kId2, "bar", value2, data_type));
 
     GetSyncableService(data_type)->MergeDataAndStartSyncing(
         data_type, sync_data, std::move(sync_processor_wrapper_));
@@ -375,7 +382,7 @@
 
     storage1->Set(DEFAULTS, "foo", value2);
     EXPECT_EQ(1u, sync_processor_->changes().size());
-    SettingSyncData* change = sync_processor_->GetOnlyChange("s1", "foo");
+    SettingSyncData* change = sync_processor_->GetOnlyChange(kId1, "foo");
     EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, change->change_type());
     EXPECT_EQ(value2, change->value());
 
@@ -391,8 +398,8 @@
   base::Value value2(base::Value::Type::LIST);
   value2.GetList().Append("barValue");
 
-  ValueStore* storage1 = AddExtensionAndGetStorage("s1", type);
-  ValueStore* storage2 = AddExtensionAndGetStorage("s2", type);
+  ValueStore* storage1 = AddExtensionAndGetStorage(kId1, type);
+  ValueStore* storage2 = AddExtensionAndGetStorage(kId2, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     storage1->Set(DEFAULTS, "foo", value1);
@@ -403,10 +410,10 @@
 
     // All settings should have been pushed to sync.
     EXPECT_EQ(2u, sync_processor_->changes().size());
-    SettingSyncData* change = sync_processor_->GetOnlyChange("s1", "foo");
+    SettingSyncData* change = sync_processor_->GetOnlyChange(kId1, "foo");
     EXPECT_EQ(syncer::SyncChange::ACTION_ADD, change->change_type());
     EXPECT_EQ(value1, change->value());
-    change = sync_processor_->GetOnlyChange("s2", "bar");
+    change = sync_processor_->GetOnlyChange(kId2, "bar");
     EXPECT_EQ(syncer::SyncChange::ACTION_ADD, change->change_type());
     EXPECT_EQ(value2, change->value());
 
@@ -427,23 +434,23 @@
   base::DictValue expected1, expected2;
 
   // Pre-populate one of the storage areas.
-  ValueStore* storage1 = AddExtensionAndGetStorage("s1", type);
+  ValueStore* storage1 = AddExtensionAndGetStorage(kId1, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     storage1->Set(DEFAULTS, "overwriteMe", value1);
 
     syncer::SyncDataList sync_data;
     sync_data.push_back(
-        settings_sync_util::CreateData("s1", "foo", value1, data_type));
+        settings_sync_util::CreateData(kId1, "foo", value1, data_type));
     sync_data.push_back(
-        settings_sync_util::CreateData("s2", "bar", value2, data_type));
+        settings_sync_util::CreateData(kId2, "bar", value2, data_type));
     GetSyncableService(data_type)->MergeDataAndStartSyncing(
         data_type, sync_data, std::move(sync_processor_wrapper_));
     expected1.Set("foo", value1.Clone());
     expected2.Set("bar", value2.Clone());
   });
 
-  ValueStore* storage2 = AddExtensionAndGetStorage("s2", type);
+  ValueStore* storage2 = AddExtensionAndGetStorage(kId2, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     // All changes should be local, so no sync changes.
@@ -466,8 +473,8 @@
   value2.GetList().Append("barValue");
 
   // Make storage1 initialised from local data, storage2 initialised from sync.
-  ValueStore* storage1 = AddExtensionAndGetStorage("s1", type);
-  ValueStore* storage2 = AddExtensionAndGetStorage("s2", type);
+  ValueStore* storage1 = AddExtensionAndGetStorage(kId1, type);
+  ValueStore* storage2 = AddExtensionAndGetStorage(kId2, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     // Maintain dictionaries mirrored to the expected values of the settings in
@@ -479,7 +486,7 @@
 
     syncer::SyncDataList sync_data;
     sync_data.push_back(
-        settings_sync_util::CreateData("s2", "bar", value2, data_type));
+        settings_sync_util::CreateData(kId2, "bar", value2, data_type));
 
     GetSyncableService(data_type)->MergeDataAndStartSyncing(
         data_type, sync_data, std::move(sync_processor_wrapper_));
@@ -488,9 +495,9 @@
     // Make sync add some settings.
     syncer::SyncChangeList change_list;
     change_list.push_back(
-        settings_sync_util::CreateAdd("s1", "bar", value2, data_type));
+        settings_sync_util::CreateAdd(kId1, "bar", value2, data_type));
     change_list.push_back(
-        settings_sync_util::CreateAdd("s2", "foo", value1, data_type));
+        settings_sync_util::CreateAdd(kId2, "foo", value1, data_type));
     GetSyncableService(data_type)->ProcessSyncChanges(FROM_HERE, change_list);
     expected1.Set("bar", value2.Clone());
     expected2.Set("foo", value1.Clone());
@@ -502,9 +509,9 @@
     // initial setting.
     change_list.clear();
     change_list.push_back(
-        settings_sync_util::CreateUpdate("s1", "bar", value2, data_type));
+        settings_sync_util::CreateUpdate(kId1, "bar", value2, data_type));
     change_list.push_back(
-        settings_sync_util::CreateUpdate("s2", "bar", value1, data_type));
+        settings_sync_util::CreateUpdate(kId2, "bar", value1, data_type));
     GetSyncableService(data_type)->ProcessSyncChanges(FROM_HERE, change_list);
     expected1.Set("bar", value2.Clone());
     expected2.Set("bar", value1.Clone());
@@ -516,9 +523,9 @@
     // the new setting.
     change_list.clear();
     change_list.push_back(
-        settings_sync_util::CreateDelete("s1", "foo", data_type));
+        settings_sync_util::CreateDelete(kId1, "foo", data_type));
     change_list.push_back(
-        settings_sync_util::CreateDelete("s2", "foo", data_type));
+        settings_sync_util::CreateDelete(kId2, "foo", data_type));
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/chrome/browser/extensions/api/storage/settings_sync_unittest.cc b/chrome/browser/extensions/api/storage/settings_sync_unittest.cc
index eaeaab9..700b27d 100644
--- a/chrome/browser/extensions/api/storage/settings_sync_unittest.cc
+++ b/chrome/browser/extensions/api/storage/settings_sync_unittest.cc
@@ -50,6 +50,13 @@
 // To save typing ValueStore::DEFAULTS everywhere.
 const ValueStore::WriteOptions DEFAULTS = ValueStore::DEFAULTS;
 
+// Valid extension IDs for use as test data.
+constexpr char kId1[] = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
+constexpr char kId2[] = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb";
+constexpr char kId3[] = "cccccccccccccccccccccccccccccccc";
+constexpr char kId4[] = "dddddddddddddddddddddddddddddddd";
+constexpr char kId5[] = "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee";
+
 // More saving typing. Maps extension IDs to a list of sync changes for that
 // extension.
 using SettingSyncDataMultimap =
@@ -313,7 +320,7 @@
   });
 
   // Have one extension created before sync is set up, the other created after.
-  AddExtensionAndGetStorage("s1", type);
+  AddExtensionAndGetStorage(kId1, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     EXPECT_EQ(0u, GetAllSyncData(data_type).size());
@@ -322,7 +329,7 @@
         data_type, syncer::SyncDataList(), std::move(sync_processor_wrapper_));
   });
 
-  AddExtensionAndGetStorage("s2", type);
+  AddExtensionAndGetStorage(kId2, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     EXPECT_EQ(0u, GetAllSyncData(data_type).size());
@@ -342,8 +349,8 @@
   base::Value value2(base::Value::Type::LIST);
   value2.GetList().Append("barValue");
 
-  ValueStore* storage1 = AddExtensionAndGetStorage("s1", type);
-  ValueStore* storage2 = AddExtensionAndGetStorage("s2", type);
+  ValueStore* storage1 = AddExtensionAndGetStorage(kId1, type);
+  ValueStore* storage2 = AddExtensionAndGetStorage(kId2, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     storage1->Set(DEFAULTS, "foo", value1);
@@ -351,16 +358,16 @@
 
     SettingSyncDataMultimap all_sync_data = GetAllSyncData(data_type);
     EXPECT_EQ(2u, all_sync_data.size());
-    EXPECT_EQ(1u, all_sync_data["s1"]->size());
-    EXPECT_PRED_FORMAT2(ValuesEq, &value1, &(*all_sync_data["s1"])[0]->value());
-    EXPECT_EQ(1u, all_sync_data["s2"]->size());
-    EXPECT_PRED_FORMAT2(ValuesEq, &value2, &(*all_sync_data["s2"])[0]->value());
+    EXPECT_EQ(1u, all_sync_data[kId1]->size());
+    EXPECT_PRED_FORMAT2(ValuesEq, &value1, &(*all_sync_data[kId1])[0]->value());
+    EXPECT_EQ(1u, all_sync_data[kId2]->size());
+    EXPECT_PRED_FORMAT2(ValuesEq, &value2, &(*all_sync_data[kId2])[0]->value());
 
     syncer::SyncDataList sync_data;
     sync_data.push_back(
-        settings_sync_util::CreateData("s1", "foo", value1, data_type));
+        settings_sync_util::CreateData(kId1, "foo", value1, data_type));
     sync_data.push_back(
-        settings_sync_util::CreateData("s2", "bar", value2, data_type));
+        settings_sync_util::CreateData(kId2, "bar", value2, data_type));
 
     GetSyncableService(data_type)->MergeDataAndStartSyncing(
         data_type, sync_data, std::move(sync_processor_wrapper_));
@@ -375,7 +382,7 @@
 
     storage1->Set(DEFAULTS, "foo", value2);
     EXPECT_EQ(1u, sync_processor_->changes().size());
-    SettingSyncData* change = sync_processor_->GetOnlyChange("s1", "foo");
+    SettingSyncData* change = sync_processor_->GetOnlyChange(kId1, "foo");
     EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, change->change_type());
     EXPECT_EQ(value2, change->value());
 
@@ -391,8 +398,8 @@
   base::Value value2(base::Value::Type::LIST);
   value2.GetList().Append("barValue");
 
-  ValueStore* storage1 = AddExtensionAndGetStorage("s1", type);
-  ValueStore* storage2 = AddExtensionAndGetStorage("s2", type);
+  ValueStore* storage1 = AddExtensionAndGetStorage(kId1, type);
+  ValueStore* storage2 = AddExtensionAndGetStorage(kId2, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     storage1->Set(DEFAULTS, "foo", value1);
@@ -403,10 +410,10 @@
 
     // All settings should have been pushed to sync.
     EXPECT_EQ(2u, sync_processor_->changes().size());
-    SettingSyncData* change = sync_processor_->GetOnlyChange("s1", "foo");
+    SettingSyncData* change = sync_processor_->GetOnlyChange(kId1, "foo");
     EXPECT_EQ(syncer::SyncChange::ACTION_ADD, change->change_type());
     EXPECT_EQ(value1, change->value());
-    change = sync_processor_->GetOnlyChange("s2", "bar");
+    change = sync_processor_->GetOnlyChange(kId2, "bar");
     EXPECT_EQ(syncer::SyncChange::ACTION_ADD, change->change_type());
     EXPECT_EQ(value2, change->value());
 
@@ -427,23 +434,23 @@
   base::DictValue expected1, expected2;
 
   // Pre-populate one of the storage areas.
-  ValueStore* storage1 = AddExtensionAndGetStorage("s1", type);
+  ValueStore* storage1 = AddExtensionAndGetStorage(kId1, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     storage1->Set(DEFAULTS, "overwriteMe", value1);
 
     syncer::SyncDataList sync_data;
     sync_data.push_back(
-        settings_sync_util::CreateData("s1", "foo", value1, data_type));
+        settings_sync_util::CreateData(kId1, "foo", value1, data_type));
     sync_data.push_back(
-        settings_sync_util::CreateData("s2", "bar", value2, data_type));
+        settings_sync_util::CreateData(kId2, "bar", value2, data_type));
     GetSyncableService(data_type)->MergeDataAndStartSyncing(
         data_type, sync_data, std::move(sync_processor_wrapper_));
     expected1.Set("foo", value1.Clone());
     expected2.Set("bar", value2.Clone());
   });
 
-  ValueStore* storage2 = AddExtensionAndGetStorage("s2", type);
+  ValueStore* storage2 = AddExtensionAndGetStorage(kId2, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     // All changes should be local, so no sync changes.
@@ -466,8 +473,8 @@
   value2.GetList().Append("barValue");
 
   // Make storage1 initialised from local data, storage2 initialised from sync.
-  ValueStore* storage1 = AddExtensionAndGetStorage("s1", type);
-  ValueStore* storage2 = AddExtensionAndGetStorage("s2", type);
+  ValueStore* storage1 = AddExtensionAndGetStorage(kId1, type);
+  ValueStore* storage2 = AddExtensionAndGetStorage(kId2, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     // Maintain dictionaries mirrored to the expected values of the settings in
@@ -479,7 +486,7 @@
 
     syncer::SyncDataList sync_data;
     sync_data.push_back(
-        settings_sync_util::CreateData("s2", "bar", value2, data_type));
+        settings_sync_util::CreateData(kId2, "bar", value2, data_type));
 
     GetSyncableService(data_type)->MergeDataAndStartSyncing(
         data_type, sync_data, std::move(sync_processor_wrapper_));
@@ -488,9 +495,9 @@
     // Make sync add some settings.
     syncer::SyncChangeList change_list;
     change_list.push_back(
-        settings_sync_util::CreateAdd("s1", "bar", value2, data_type));
+        settings_sync_util::CreateAdd(kId1, "bar", value2, data_type));
     change_list.push_back(
-        settings_sync_util::CreateAdd("s2", "foo", value1, data_type));
+        settings_sync_util::CreateAdd(kId2, "foo", value1, data_type));
     GetSyncableService(data_type)->ProcessSyncChanges(FROM_HERE, change_list);
     expected1.Set("bar", value2.Clone());
     expected2.Set("foo", value1.Clone());
@@ -502,9 +509,9 @@
     // initial setting.
     change_list.clear();
     change_list.push_back(
-        settings_sync_util::CreateUpdate("s1", "bar", value2, data_type));
+        settings_sync_util::CreateUpdate(kId1, "bar", value2, data_type));
     change_list.push_back(
-        settings_sync_util::CreateUpdate("s2", "bar", value1, data_type));
+        settings_sync_util::CreateUpdate(kId2, "bar", value1, data_type));
     GetSyncableService(data_type)->ProcessSyncChanges(FROM_HERE, change_list);
     expected1.Set("bar", value2.Clone());
     expected2.Set("bar", value1.Clone());
@@ -516,9 +523,9 @@
     // the new setting.
     change_list.clear();
     change_list.push_back(
-        settings_sync_util::CreateDelete("s1", "foo", data_type));
+        settings_sync_util::CreateDelete(kId1, "foo", data_type));
     change_list.push_back(
-        settings_sync_util::CreateDelete("s2", "foo", data_type));
+        settings_sync_util::CreateDelete(kId2, "foo", data_type));
     GetSyncableService(data_type)->ProcessSyncChanges(FROM_HERE, change_list);
     expected1.Remove("foo");
     expected2.Remove("foo");
@@ -540,10 +547,10 @@
 
   // Make storage1/2 initialised from local data, storage3/4 initialised from
   // sync.
-  ValueStore* storage1 = AddExtensionAndGetStorage("s1", type);
-  ValueStore* storage2 = AddExtensionAndGetStorage("s2", type);
-  ValueStore* storage3 = AddExtensionAndGetStorage("s3", type);
-  ValueStore* storage4 = AddExtensionAndGetStorage("s4", type);
+  ValueStore* storage1 = AddExtensionAndGetStorage(kId1, type);
+  ValueStore* storage2 = AddExtensionAndGetStorage(kId2, type);
+  ValueStore* storage3 = AddExtensionAndGetStorage(kId3, type);
+  ValueStore* storage4 = AddExtensionAndGetStorage(kId4, type);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     storage1->Set(DEFAULTS, "foo", value1);
@@ -551,9 +558,9 @@
 
     syncer::SyncDataList sync_data;
     sync_data.push_back(
-        settings_sync_util::CreateData("s3", "bar", value2, data_type));
+        settings_sync_util::CreateData(kId3, "bar", value2, data_type));
     sync_data.push_back(
-        settings_sync_util::CreateData("s4", "bar", value2, data_type));
+        settings_sync_util::CreateData(kId4, "bar", value2, data_type));
 
     GetSyncableService(data_type)->MergeDataAndStartSyncing(
         data_type, sync_data, std::move(sync_processor_wrapper_));
@@ -564,16 +571,16 @@
     storage3->Set(DEFAULTS, "foo", value1);
     storage4->Set(DEFAULTS, "foo", value1);
 
-    SettingSyncData* change = sync_processor_->GetOnlyChange("s1", "bar");
+    SettingSyncData* change = sync_processor_->GetOnlyChange(kId1, "bar");
     EXPECT_EQ(syncer::SyncChange::ACTION_ADD, change->change_type());
     EXPECT_EQ(value2, change->value());
-    sync_processor_->GetOnlyChange("s2", "bar");
+    sync_processor_->GetOnlyChange(kId2, "bar");
     EXPECT_EQ(syncer::SyncChange::ACTION_ADD, change->change_type());
     EXPECT_EQ(value2, change->value());
-    change = sync_processor_->GetOnlyChange("s3", "foo");
+    change = sync_processor_->GetOnlyChange(kId3, "foo");
     EXPECT_EQ(syncer::SyncChange::ACTION_ADD, change->change_type());
     EXPECT_EQ(value1, change->value());
-    change = sync_processor_->GetOnlyChange("s4", "foo");
+    change = sync_processor_->GetOnlyChange(kId4, "foo");
     EXPECT_EQ(syncer::SyncChange::ACTION_ADD, change->change_type());
     EXPECT_EQ(value1, change->value());
 
@@ -586,16 +593,16 @@
     storage3->Set(DEFAULTS, "bar", value1);
     storage4->Set(DEFAULTS, "foo", value2);
 
-    change = sync_processor_->GetOnlyChange("s1", "bar");
+    change = sync_processor_->GetOnlyChange(kId1, "bar");
     EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, change->change_type());
     EXPECT_EQ(value1, change->value());
-    change = sync_processor_->GetOnlyChange("s2", "foo");
+    change = sync_processor_->GetOnlyChange(kId2, "foo");
     EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, change->change_type());
     EXPECT_EQ(value2, change->value());
-    change = sync_processor_->GetOnlyChange("s3", "bar");
+    change = sync_processor_->GetOnlyChange(kId3, "bar");
     EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, change->change_type());
     EXPECT_EQ(value1, change->value());
-    change = sync_processor_->GetOnlyChange("s4", "foo");
+    change = sync_processor_->GetOnlyChange(kId4, "foo");
     EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, change->change_type());
     EXPECT_EQ(value2, change->value());
 
@@ -609,13 +616,13 @@
     storage4->Remove("bar");
 
     EXPECT_EQ(syncer::SyncChange::ACTION_DELETE,
-              sync_processor_->GetOnlyChange("s1", "foo")->change_type());
+              sync_processor_->GetOnlyChange(kId1, "foo")->change_type());
     EXPECT_EQ(syncer::SyncChange::ACTION_DELETE,
-              sync_processor_->GetOnlyChange("s2", "bar")->change_type());
+              sync_processor_->GetOnlyChange(kId2, "bar")->change_type());
     EXPECT_EQ(syncer::SyncChange::ACTION_DELETE,
-              sync_processor_->GetOnlyChange("s3", "foo")->change_type());
+              sync_processor_->GetOnlyChange(kId3, "foo")->change_type());
     EXPECT_EQ(syncer::SyncChange::ACTION_DELETE,
-              sync_processor_->GetOnlyChange("s4", "bar")->change_type());
+              sync_processor_->GetOnlyChange(kId4, "bar")->change_type());
 
     // Remove some nonexistent settings.
     sync_processor_->ClearChanges();
@@ -640,21 +647,21 @@
     storage4->Clear();
 
     EXPECT_EQ(syncer::SyncChange::ACTION_DELETE,
-              sync_processor_->GetOnlyChange("s1", "foo")->change_type());
+              sync_processor_->GetOnlyChange(kId1, "foo")->change_type());
     EXPECT_EQ(syncer::SyncChange::ACTION_DELETE,
-              sync_processor_->GetOnlyChange("s1", "bar")->change_type());
+              sync_processor_->GetOnlyChange(kId1, "bar")->change_type());
     EXPECT_EQ(syncer::SyncChange::ACTION_DELETE,
-              sync_processor_->GetOnlyChange("s2", "foo")->change_type());
+              sync_processor_->GetOnlyChange(kId2, "foo")->change_type());
     EXPECT_EQ(syncer::SyncChange::ACTION_DELETE,
-              sync_processor_->GetOnlyChange("s2", "bar")->change_type());
+              sync_processor_->GetOnlyChange(kId2, "bar")->change_type());
     EXPECT_EQ(syncer::SyncChange::ACTION_DELETE,
-              sync_processor_->GetOnlyChange("s3", "foo")->change_type());
+              sync_processor_->GetOnlyChange(kId3, "foo")->change_type());
     EXPECT_EQ(syncer::SyncChange::ACTION_DELETE,
-              sync_processor_->GetOnlyChange("s3", "bar")->change_type());
+              sync_processor_->GetOnlyChange(kId3, "bar")->change_type());
     EXPECT_EQ(syncer::SyncChange::ACTION_DELETE,
-              sync_processor_->GetOnlyChange("s4", "foo")->change_type());
+              sync_processor_->GetOnlyChange(kId4, "foo")->change_type());
     EXPECT_EQ(syncer::SyncChange::ACTION_DELETE,
-              sync_processor_->GetOnlyChange("s4", "bar")->change_type());
... (truncated)
Loading diff…

Original Bug Report

reported by [email protected]

Potential Path Traversal in Sync Storage via Unvalidated extension_id

Project Fortify, an experimental security project, has identified the following potential security issue. If you’re a feature owner CC-ed on this bug, please do your best to review these reports without the Chrome Security team.

Overview: An unvalidated extension_id provided via Chrome Sync is used as a filesystem path component. By providing a malicious ID containing traversal segments, an attacker can cause the browser to create or overwrite LevelDB files outside the intended storage directory, potentially injecting data into other extensions.

Affected files:

  • chrome/browser/extensions/api/storage/setting_sync_data.cc
  • chrome/browser/extensions/api/storage/sync_storage_backend.cc
  • extensions/browser/api/storage/value_store_util.cc
  • components/value_store/value_store_factory_impl.cc
  • components/value_store/lazy_leveldb.cc
  • components/sync/model/syncable_service_based_bridge.cc
  • chrome/browser/extensions/api/storage/syncable_settings_storage.cc

Estimated timestamp from git blame: 2022-11-18

Summary

The Chrome Extension Settings Sync implementation contains a potential path traversal vulnerability. An unvalidated extension_id provided by the Sync server is used to construct the directory path for LevelDB storage. By providing a malicious extension_id containing path traversal segments (e.g., ../../), an attacker can cause the browser process to create or open LevelDB files at arbitrary locations relative to the user’s profile directory. This can be leveraged to inject malicious data into the local storage of other installed extensions.

Technical Details

When the browser receives synchronized settings data (for the EXTENSION_SETTINGS or APP_SETTINGS data types), the sequence of events is as follows:

  1. Sync Update Received: The SyncableServiceBasedBridge receives the sync entities. Its IsEntityDataValid method unconditionally returns true for these data types, relying on the underlying SyncableService for validation.
  2. Unvalidated Extraction: SyncStorageBackend::ProcessSyncChanges iterates through the changes and creates SettingSyncData objects. In SettingSyncData::ExtractSyncData (chrome/browser/extensions/api/storage/setting_sync_data.cc:61), the extension_id is extracted directly from the protobuf: extension_id_ = extension_specifics.extension_id();. Crucially, this ID is never validated using crx_file::id_util::IdIsValid().
  3. Path Construction: SyncStorageBackend calls GetOrCreateStorageWithSyncData, passing this unvalidated extension_id. This calls down to value_store_util::GetValueStoreDir (extensions/browser/api/storage/value_store_util.cc:41), which appends the ID to a base directory using base::FilePath::AppendASCII(id).
  4. Traversal Permitted: base::FilePath::AppendASCII does not sanitize or reject .. segments unless the string starts with an absolute path separator. Thus, a payload like ../../Local Extension Settings/<target_extension_id> is successfully appended.
  5. Database Opening: The traversed path is passed to LazyLevelDb::EnsureDbIsOpen, which eventually calls OS-level APIs (via leveldb_env::OpenDB and base::CreateDirectoryAndGetError). The OS resolves the traversal components, causing LevelDB to open or create files at the targeted location.
  6. Data Injection: Once opened, the sync logic writes the attacker-controlled JSON payload into the targeted LevelDB database.

Note: While active extension databases are protected by same-process file locks, an attacker can target the storage of inactive extensions or create arbitrary LevelDB files in other writable locations.

Potential Reproduction Steps

Please note: These are theoretical steps as our tooling agent cannot execute code to verify the exploit.

  1. From an attacker-controlled sync client (signed into the victim’s account), commit an EXTENSION_SETTINGS entity with a malicious extension_id containing traversal segments, e.g., ../../Local Extension Settings/<target_extension_id>. Include a payload of key/value pairs to inject.
  2. The victim’s browser receives the sync update.
  3. The browser extracts the ID without validation and constructs a traversed file path.
  4. The browser opens the target extension’s LevelDB database (if inactive) or creates new LevelDB files at the location.
  5. The attacker-controlled payload is written into the targeted LevelDB database.

Suggested Fix

Add validation to SettingSyncData::ExtractSyncData in chrome/browser/extensions/api/storage/setting_sync_data.cc. Before assigning extension_id_, verify it using crx_file::id_util::IdIsValid(extension_specifics.extension_id()). If the ID is invalid, the sync data should be rejected and ignored, similar to how it is handled in ExtensionSyncData::PopulateFromExtensionSpecifics.

Evaluated with Chrome root at commit: 096fc8fdbfacf2546485756d03f160a3d04fcc9b

Results so far have been promising, but there can be wrong deductions. If this proves to be a false positive, please close as WAI; data from false positives will be used to improve accuracy over time. And please feel free to reach out to me directly if you have concerns or feedback on the project.

View on issue tracker
On This Page