CVE-2026-87613

Medium watch CVSS 9 chrome Logic Error 📄 Reporter bug report 🔧 Commit mapped

Overview

Medium

Severity

CVSS

Exploited ITW

Fixed

Fix Status

ImpactIncorrect reference resolution in Extensions

DescriptionIncorrect reference resolution in Extensions

ComponentExtensions

Bug ClassLogic Error

Action tierwatch — logic/policy bug — security-relevant, no memory-unsafety
Computed by PatchGap's deterministic exploitability ruler from the fixing commit. patch-now = RCE-class & attacker-reachable in a default config, or exploited in the wild; watch = real security signal below that bar; noise = no security relevance.

Tracker501889544

Fix commit04081f5e8d44 (chromium/src) +329/-200

CWECWE-706

CVSS vectorCVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:C/C:H/I:H/A:H

CISA KEVNot listed

CreditedGoogle

Reported2026-04-12 (per vendor advisory)

Fix landed2026-07-29 (public git)

Disclosed2026-09-08

Patch gap41 days — fix public in source before the advisory

Background

Extension ID: a 32-character string (lowercase a–p) that uniquely identifies a Chrome extension and is used as a lookup key for its per-extension settings storage.
`chrome.storage.sync`: the extension settings storage area whose contents are propagated across a user’s devices through Chrome Sync, so its records originate off-device.
`crx_file::id_util::IdIsValid`: the canonical validator that returns true only for a well-formed extension ID (correct length and character set).
`SettingSyncData`: a wrapper around one inbound sync record that exposes its extension_id() and key(), used to group incoming settings by owning extension.

Root Cause Analysis

Both code paths that ingest extension-settings sync records — the initial merge over initial_sync_data and the incremental apply over sync_changes — took the untrusted extension_id() from each SettingSyncData and immediately used it as the key into grouped_sync_data (grouped_sync_data[data.extension_id()]) without ever checking that the string was a valid extension ID. Because sync records arrive from the sync server / another synced device rather than from local extension registration, a malformed ID string could be carried straight into the grouping map and then resolved downstream against the settings-storage layer as if it named a real extension. The violated invariant is that every extension-settings key handled by the backend must be a syntactically valid extension ID; the removed code enforced no such precondition.

The fix adds an early guard in each loop that calls crx_file::id_util::IdIsValid(data.extension_id()) and, on failure, logs at DVLOG(1) and continues past the record before the grouped_sync_data[...] reference is ever created, so malformed IDs can no longer reach the reference-resolution step. The change is purely a tightening: it introduces a filter where none previously existed.

Key insight

The single mistake was trusting the extension_id() field of a sync-originated record as a valid reference key without validation; the fix resolves it by validating the ID with crx_file::id_util::IdIsValid and skipping any record that fails, before the ID is used to index the grouping map.

Attack Path

Inject a malformed sync record An attacker who can supply extension-settings sync data (via a compromised/malicious sync stream or a controlled synced device) crafts a record whose extension_id() is not a well-formed extension ID.
Record reaches the backend loop During MergeDataAndStartSyncing (initial data) or ProcessSyncChanges (incremental changes), the record is wrapped in a SettingSyncData and the loop reads its extension_id() with no validity check (pre-patch).
Malformed ID used as a reference key The unvalidated ID is used directly in grouped_sync_data[data.extension_id()], so it is resolved against the settings-storage layer as though it named a legitimate extension.
Incorrect reference resolution downstream The malformed key propagates into per-extension storage handling, producing the “incorrect reference resolution” condition the fix guards against; the specific downstream misbehavior is not shown in the diff (inferred).

Impact Assessment

An attacker able to control extension-settings sync data could introduce records keyed by malformed extension IDs that the browser process would resolve as extension references, leading to incorrect reference resolution in the extensions settings backend (inferred as to concrete downstream effect). The precondition is the ability to feed sync data to the victim (control over sync content or a synced device), and the affected context is the browser process’s extension storage subsystem. The vendor advisory rates this medium severity and does not report in-the-wild exploitation; NVD scores it CVSS 9.0. The advisory provides no exploited-in-the-wild status or severity, so none is asserted here.

Changed Functions

Function	Change	Notes
`for` chrome/browser/extensions/api/storage/sync_storage_backend.cc	modified
`if` chrome/browser/extensions/api/storage/sync_storage_backend.cc	modified
`PostOnBackendSequenceAndWait` chrome/browser/extensions/api/storage/settings_sync_unittest.cc	modified

Files Changed

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

Audit Directions

Trusting sync-supplied identifiers
Audit every consumer of SettingSyncData::extension_id() and other sync-record identifier fields for a missing crx_file::id_util::IdIsValid (or equivalent) check before the value is used as a map key, path component, or lookup reference.
Map-index reference resolution
Look for map[untrusted_key] patterns in the extensions and sync layers where indexing implicitly creates or resolves an entry, since an unvalidated key silently manufactures a reference to a nonexistent entity.
Validation asymmetry between locally-registered and remotely-sourced IDs
Compare paths that obtain extension IDs from local registration (already validated) against those that receive IDs from sync, mojo, or IPC, and ensure the remote paths apply the same ID-validity precondition.

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/sync_storage_backend.cc b/chrome/browser/extensions/api/storage/sync_storage_backend.cc
index 98e2663..f034bef 100644
--- a/chrome/browser/extensions/api/storage/sync_storage_backend.cc
+++ b/chrome/browser/extensions/api/storage/sync_storage_backend.cc
@@ -12,6 +12,7 @@
 #include "chrome/browser/extensions/api/storage/settings_sync_processor.h"
 #include "chrome/browser/extensions/api/storage/settings_sync_util.h"
 #include "chrome/browser/extensions/api/storage/syncable_settings_storage.h"
+#include "components/crx_file/id_util.h"
 #include "components/sync/model/sync_change_processor.h"
 #include "components/sync/protocol/entity_data.h"
 #include "extensions/browser/api/storage/backend_task_runner.h"
@@ -171,6 +172,11 @@
 
   for (const syncer::SyncData& sync_data : initial_sync_data) {
     SettingSyncData data(sync_data);
+    if (!crx_file::id_util::IdIsValid(data.extension_id())) {
+      DVLOG(1) << "Ignoring sync data with invalid extension ID "
+               << data.extension_id();
+      continue;
+    }
     base::DictValue& settings = grouped_sync_data[data.extension_id()];
     DCHECK(!settings.Find(data.key()))
         << "Duplicate settings for " << data.extension_id() << "/"
@@ -222,7 +228,12 @@
   std::map<ExtensionId, SettingSyncDataList*> grouped_sync_data;
 
   for (const syncer::SyncChange& change : sync_changes) {
-    std::unique_ptr<SettingSyncData> data(new SettingSyncData(change));
+    auto data = std::make_unique<SettingSyncData>(change);
+    if (!crx_file::id_util::IdIsValid(data->extension_id())) {
+      DVLOG(1) << "Ignoring sync change with invalid extension ID "
+               << data->extension_id();
+      continue;
+    }
     SettingSyncDataList*& group = grouped_sync_data[data->extension_id()];
     if (!group) {
       group = new SettingSyncDataList();
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());
+              sync_processor_->GetOnlyChange(kId4, "bar")->change_type());
 
     GetSyncableService(data_type)->StopSyncing(data_type);
   });
@@ -667,9 +674,9 @@
 
   // storage1 is an extension, storage2 is an app.
   ValueStore* storage1 =
-      AddExtensionAndGetStorage("s1", Manifest::Type::kExtension);
+      AddExtensionAndGetStorage(kId1, Manifest::Type::kExtension);
   ValueStore* storage2 =
-      AddExtensionAndGetStorage("s2", Manifest::Type::kLegacyPackagedApp);
+      AddExtensionAndGetStorage(kId2, Manifest::Type::kLegacyPackagedApp);
 
   PostOnBackendSequenceAndWait(FROM_HERE, [&, this]() {
     storage1->Set(DEFAULTS, "foo", value1);
@@ -678,20 +685,20 @@
     SettingSyncDataMultimap extension_sync_data =
         GetAllSyncData(syncer::EXTENSION_SETTINGS);
     EXPECT_EQ(1u, extension_sync_data.size());
-    EXPECT_EQ(1u, extension_sync_data["s1"]->size());
+    EXPECT_EQ(1u, extension_sync_data[kId1]->size());
     EXPECT_PRED_FORMAT2(ValuesEq, &value1,
-                        &(*extension_sync_data["s1"])[0]->value());
+                        &(*extension_sync_data[kId1])[0]->value());
 
     SettingSyncDataMultimap app_sync_data =
         GetAllSyncData(syncer::APP_SETTINGS);
     EXPECT_EQ(1u, app_sync_data.size());
-    EXPECT_EQ(1u, app_sync_data["s2"]->size());
-    EXPECT_PRED_FORMAT2(ValuesEq, &value2, &(*app_sync_data["s2"])[0]->value());
+    EXPECT_EQ(1u, app_sync_data[kId2]->size());
+    EXPECT_PRED_FORMAT2(ValuesEq, &value2, &(*app_sync_data[kId2])[0]->value());
 
     // Stop each separately, there should be no changes either time.
     syncer::SyncDataList sync_data;
     sync_data.push_back(settings_sync_util::CreateData(
-        "s1", "foo", value1, syncer::EXTENSION_SETTINGS));
+        kId1, "foo", value1, syncer::EXTENSION_SETTINGS));
 
     GetSyncableService(syncer::EXTENSION_SETTINGS)
         ->MergeDataAndStartSyncing(syncer::EXTENSION_SETTINGS, sync_data,
@@ -701,7 +708,7 @@
     EXPECT_EQ(0u, sync_processor_->changes().size());
 
     sync_data.clear();
-    sync_data.push_back(settings_sync_util::CreateData("s2", "bar", value2,
... (file diff truncated)

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:

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.
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().
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).
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.
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.
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.

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.
The victim’s browser receives the sync update.
The browser extracts the ID without validation and constructs a traversed file path.
The browser opens the target extension’s LevelDB database (if inactive) or creates new LevelDB files at the location.
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

References

Vendor advisory Fix commit Bug tracker NVD

On This Page