Medium chrome Logic Error 📄 Reporter bug report 🔧 Commit mapped

Overview

Medium
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
ImpactImproper input validation in Sync
DescriptionImproper input validation in Sync
ComponentSync
Bug ClassLogic Error
Tracker513737209
Fix commitdcebabc243ac (chromium/src) +45/-18
CISA KEVNot listed
CreditedGoogle
Disclosed2026-08-25

Changed Functions

FunctionChangeNotes
for
chrome/browser/sync/test/integration/migration_test.cc
modified
IN_PROC_BROWSER_TEST_P
chrome/browser/sync/test/integration/migration_test.cc
modified
TEST
components/sync/engine/syncer_proto_util_unittest.cc
modified

Files Changed

  • chrome/browser/sync/test/integration/migration_test.cc
  • components/sync/engine/syncer_proto_util.cc
  • components/sync/engine/syncer_proto_util_unittest.cc
From dcebabc243ac3fc05519fe8d52fea65ac2490c3b Mon Sep 17 00:00:00 2001
From: Mikel Astiz <[email protected]>
Date: Wed, 22 Jul 2026 09:29:42 -0700
Subject: [PATCH] [sync] Disallow MIGRATION_DONE for NIGORI

Supporting this properly for NIGORI without questionable transitions is
complex, so this patch simply excludes NIGORI from the datatypes that
can react to MIGRATION_DONE.

Change-Id: Ic4f48412d90abf4d97e2d6c7cd1fea40289d0c41
Fixed: 513737209
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8126459
Reviewed-by: Marc Treib <[email protected]>
Commit-Queue: Mikel Astiz <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1666394}
---

diff --git a/chrome/browser/sync/test/integration/migration_test.cc b/chrome/browser/sync/test/integration/migration_test.cc
index 84f431a..e672598 100644
--- a/chrome/browser/sync/test/integration/migration_test.cc
+++ b/chrome/browser/sync/test/integration/migration_test.cc
@@ -212,7 +212,7 @@
 
   enum TriggerMethod { MODIFY_PREF, MODIFY_BOOKMARK, TRIGGER_REFRESH };
 
-  syncer::DataTypeSet GetPreferredDataTypes() {
+  syncer::DataTypeSet GetPreferredDataTypesEligibleForMigration() {
     // SyncServiceImpl must already have been created before we can call
     // GetPreferredDataTypes().
     DCHECK(GetSyncService(0));
@@ -246,15 +246,21 @@
     // Doesn't make sense to migrate commit only types.
     preferred_data_types.RemoveAll(syncer::CommitOnlyTypes());
 
+    if (!UseGcDirective()) {
+      // NIGORI migration is disallowed for MIGRATION_DONE response.
+      preferred_data_types.Remove(syncer::NIGORI);
+    }
+
     return preferred_data_types;
   }
 
   // Returns a MigrationList with every enabled data type in its own
   // set.
-  MigrationList GetPreferredDataTypesList() {
+  MigrationList GetEligibleDataTypeMigrationList() {
     MigrationList migration_list;
-    const syncer::DataTypeSet preferred_data_types = GetPreferredDataTypes();
-    for (syncer::DataType type : preferred_data_types) {
+    const syncer::DataTypeSet types =
+        GetPreferredDataTypesEligibleForMigration();
+    for (syncer::DataType type : types) {
       migration_list.push_back(MakeSet(type));
     }
     return migration_list;
@@ -407,11 +413,17 @@
 // Two data types with one being nigori.
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest, PrefsNigoriIndividiaully) {
+  if (!UseGcDirective()) {
+    GTEST_SKIP() << "NIGORI migration is disallowed for MIGRATION_DONE.";
+  }
   RunSingleClientMigrationTest(MakeList(syncer::PREFERENCES, syncer::NIGORI),
                                TRIGGER_REFRESH);
 }
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest, PrefsNigoriBoth) {
+  if (!UseGcDirective()) {
+    GTEST_SKIP() << "NIGORI migration is disallowed for MIGRATION_DONE.";
+  }
   RunSingleClientMigrationTest(
       MakeList(MakeSet(syncer::PREFERENCES, syncer::NIGORI)), MODIFY_PREF);
 }
@@ -419,40 +431,49 @@
 // The whole shebang -- all data types.
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest, AllTypesIndividually) {
   ASSERT_TRUE(SetupClients());
-  RunSingleClientMigrationTest(GetPreferredDataTypesList(), MODIFY_BOOKMARK);
+  RunSingleClientMigrationTest(GetEligibleDataTypeMigrationList(),
+                               MODIFY_BOOKMARK);
 }
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest,
                        AllTypesIndividuallyTriggerRefresh) {
   ASSERT_TRUE(SetupClients());
-  RunSingleClientMigrationTest(GetPreferredDataTypesList(), TRIGGER_REFRESH);
+  RunSingleClientMigrationTest(GetEligibleDataTypeMigrationList(),
+                               TRIGGER_REFRESH);
 }
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest, AllTypesAtOnce) {
   ASSERT_TRUE(SetupClients());
-  RunSingleClientMigrationTest(MakeList(GetPreferredDataTypes()), MODIFY_PREF);
+  RunSingleClientMigrationTest(
+      MakeList(GetPreferredDataTypesEligibleForMigration()), MODIFY_PREF);
 }
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest,
                        AllTypesAtOnceTriggerRefresh) {
   ASSERT_TRUE(SetupClients());
-  RunSingleClientMigrationTest(MakeList(GetPreferredDataTypes()),
-                               TRIGGER_REFRESH);
+  RunSingleClientMigrationTest(
+      MakeList(GetPreferredDataTypesEligibleForMigration()), TRIGGER_REFRESH);
 }
 
 // All data types plus nigori.
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest,
                        AllTypesWithNigoriIndividually) {
+  if (!UseGcDirective()) {
+    GTEST_SKIP() << "NIGORI migration is disallowed for MIGRATION_DONE.";
+  }
   ASSERT_TRUE(SetupClients());
-  MigrationList migration_list = GetPreferredDataTypesList();
+  MigrationList migration_list = GetEligibleDataTypeMigrationList();
   migration_list.push_front(MakeSet(syncer::NIGORI));
   RunSingleClientMigrationTest(migration_list, MODIFY_BOOKMARK);
 }
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest, AllTypesWithNigoriAtOnce) {
+  if (!UseGcDirective()) {
+    GTEST_SKIP() << "NIGORI migration is disallowed for MIGRATION_DONE.";
+  }
   ASSERT_TRUE(SetupClients());
-  syncer::DataTypeSet all_types = GetPreferredDataTypes();
+  syncer::DataTypeSet all_types = GetPreferredDataTypesEligibleForMigration();
   all_types.Put(syncer::NIGORI);
   RunSingleClientMigrationTest(MakeList(all_types), MODIFY_PREF);
 }
@@ -819,17 +840,19 @@
 // will only tell the client about the migrations one at a time.
 IN_PROC_BROWSER_TEST_P(MigrationTwoClientTest, MigrationHellWithoutNigori) {
   ASSERT_TRUE(SetupClients());
-  MigrationList migration_list = GetPreferredDataTypesList();
+  MigrationList migration_list = GetEligibleDataTypeMigrationList();
   // Let the first nudge be a datatype that's neither prefs nor bookmarks.
   migration_list.push_front(MakeSet(syncer::THEMES));
-  ASSERT_EQ(MakeSet(syncer::NIGORI), migration_list.back());
-  migration_list.pop_back();
+  base::Erase(migration_list, MakeSet(syncer::NIGORI));
   RunTwoClientMigrationTest(migration_list, MODIFY_BOOKMARK);
 }
 
 IN_PROC_BROWSER_TEST_P(MigrationTwoClientTest, MigrationHellWithNigori) {
+  if (!UseGcDirective()) {
+    GTEST_SKIP() << "NIGORI migration is disallowed for MIGRATION_DONE.";
+  }
   ASSERT_TRUE(SetupClients());
-  MigrationList migration_list = GetPreferredDataTypesList();
+  MigrationList migration_list = GetEligibleDataTypeMigrationList();
   // Let the first nudge be a datatype that's neither prefs nor bookmarks.
   migration_list.push_front(MakeSet(syncer::THEMES));
   ASSERT_EQ(MakeSet(syncer::NIGORI), migration_list.back());
diff --git a/components/sync/engine/syncer_proto_util.cc b/components/sync/engine/syncer_proto_util.cc
index 5442290..3fa45a0 100644
--- a/components/sync/engine/syncer_proto_util.cc
+++ b/components/sync/engine/syncer_proto_util.cc
@@ -238,8 +238,10 @@
 }  // namespace
 
 DataTypeSet GetTypesToMigrate(const ClientToServerResponse& response) {
-  return GetDataTypeSetFromSpecificsFieldNumberList(
+  DataTypeSet types = GetDataTypeSetFromSpecificsFieldNumberList(
       response.migrated_data_type_id());
+  types.Remove(NIGORI);
+  return types;
 }
 
 SyncProtocolError ConvertErrorPBToSyncProtocolError(
diff --git a/components/sync/engine/syncer_proto_util_unittest.cc b/components/sync/engine/syncer_proto_util_unittest.cc
index 13be8bf..d9fadd5 100644
--- a/components/sync/engine/syncer_proto_util_unittest.cc
+++ b/components/sync/engine/syncer_proto_util_unittest.cc
@@ -32,14 +32,16 @@
 }  // namespace
 
 // Builds a ClientToServerResponse with some data type ids, including
-// invalid ones.  GetTypesToMigrate() should return only the valid
-// data types.
+// invalid ones and NIGORI. GetTypesToMigrate() should return only the valid
+// data types excluding NIGORI.
 TEST(SyncerProtoUtil, GetTypesToMigrate) {
   sync_pb::ClientToServerResponse response;
   response.add_migrated_data_type_id(
       GetSpecificsFieldNumberFromDataType(BOOKMARKS));
   response.add_migrated_data_type_id(
       GetSpecificsFieldNumberFromDataType(HISTORY_DELETE_DIRECTIVES));
+  response.add_migrated_data_type_id(
+      GetSpecificsFieldNumberFromDataType(NIGORI));
   response.add_migrated_data_type_id(-1);
   EXPECT_EQ(DataTypeSet({BOOKMARKS, HISTORY_DELETE_DIRECTIVES}),
             GetTypesToMigrate(response));
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/chrome/browser/sync/test/integration/migration_test.cc b/chrome/browser/sync/test/integration/migration_test.cc
index 84f431a..e672598 100644
--- a/chrome/browser/sync/test/integration/migration_test.cc
+++ b/chrome/browser/sync/test/integration/migration_test.cc
@@ -212,7 +212,7 @@
 
   enum TriggerMethod { MODIFY_PREF, MODIFY_BOOKMARK, TRIGGER_REFRESH };
 
-  syncer::DataTypeSet GetPreferredDataTypes() {
+  syncer::DataTypeSet GetPreferredDataTypesEligibleForMigration() {
     // SyncServiceImpl must already have been created before we can call
     // GetPreferredDataTypes().
     DCHECK(GetSyncService(0));
@@ -246,15 +246,21 @@
     // Doesn't make sense to migrate commit only types.
     preferred_data_types.RemoveAll(syncer::CommitOnlyTypes());
 
+    if (!UseGcDirective()) {
+      // NIGORI migration is disallowed for MIGRATION_DONE response.
+      preferred_data_types.Remove(syncer::NIGORI);
+    }
+
     return preferred_data_types;
   }
 
   // Returns a MigrationList with every enabled data type in its own
   // set.
-  MigrationList GetPreferredDataTypesList() {
+  MigrationList GetEligibleDataTypeMigrationList() {
     MigrationList migration_list;
-    const syncer::DataTypeSet preferred_data_types = GetPreferredDataTypes();
-    for (syncer::DataType type : preferred_data_types) {
+    const syncer::DataTypeSet types =
+        GetPreferredDataTypesEligibleForMigration();
+    for (syncer::DataType type : types) {
       migration_list.push_back(MakeSet(type));
     }
     return migration_list;
@@ -407,11 +413,17 @@
 // Two data types with one being nigori.
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest, PrefsNigoriIndividiaully) {
+  if (!UseGcDirective()) {
+    GTEST_SKIP() << "NIGORI migration is disallowed for MIGRATION_DONE.";
+  }
   RunSingleClientMigrationTest(MakeList(syncer::PREFERENCES, syncer::NIGORI),
                                TRIGGER_REFRESH);
 }
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest, PrefsNigoriBoth) {
+  if (!UseGcDirective()) {
+    GTEST_SKIP() << "NIGORI migration is disallowed for MIGRATION_DONE.";
+  }
   RunSingleClientMigrationTest(
       MakeList(MakeSet(syncer::PREFERENCES, syncer::NIGORI)), MODIFY_PREF);
 }
@@ -419,40 +431,49 @@
 // The whole shebang -- all data types.
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest, AllTypesIndividually) {
   ASSERT_TRUE(SetupClients());
-  RunSingleClientMigrationTest(GetPreferredDataTypesList(), MODIFY_BOOKMARK);
+  RunSingleClientMigrationTest(GetEligibleDataTypeMigrationList(),
+                               MODIFY_BOOKMARK);
 }
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest,
                        AllTypesIndividuallyTriggerRefresh) {
   ASSERT_TRUE(SetupClients());
-  RunSingleClientMigrationTest(GetPreferredDataTypesList(), TRIGGER_REFRESH);
+  RunSingleClientMigrationTest(GetEligibleDataTypeMigrationList(),
+                               TRIGGER_REFRESH);
 }
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest, AllTypesAtOnce) {
   ASSERT_TRUE(SetupClients());
-  RunSingleClientMigrationTest(MakeList(GetPreferredDataTypes()), MODIFY_PREF);
+  RunSingleClientMigrationTest(
+      MakeList(GetPreferredDataTypesEligibleForMigration()), MODIFY_PREF);
 }
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest,
                        AllTypesAtOnceTriggerRefresh) {
   ASSERT_TRUE(SetupClients());
-  RunSingleClientMigrationTest(MakeList(GetPreferredDataTypes()),
-                               TRIGGER_REFRESH);
+  RunSingleClientMigrationTest(
+      MakeList(GetPreferredDataTypesEligibleForMigration()), TRIGGER_REFRESH);
 }
 
 // All data types plus nigori.
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest,
                        AllTypesWithNigoriIndividually) {
+  if (!UseGcDirective()) {
+    GTEST_SKIP() << "NIGORI migration is disallowed for MIGRATION_DONE.";
+  }
   ASSERT_TRUE(SetupClients());
-  MigrationList migration_list = GetPreferredDataTypesList();
+  MigrationList migration_list = GetEligibleDataTypeMigrationList();
   migration_list.push_front(MakeSet(syncer::NIGORI));
   RunSingleClientMigrationTest(migration_list, MODIFY_BOOKMARK);
 }
 
 IN_PROC_BROWSER_TEST_P(MigrationSingleClientTest, AllTypesWithNigoriAtOnce) {
+  if (!UseGcDirective()) {
+    GTEST_SKIP() << "NIGORI migration is disallowed for MIGRATION_DONE.";
+  }
   ASSERT_TRUE(SetupClients());
-  syncer::DataTypeSet all_types = GetPreferredDataTypes();
+  syncer::DataTypeSet all_types = GetPreferredDataTypesEligibleForMigration();
   all_types.Put(syncer::NIGORI);
   RunSingleClientMigrationTest(MakeList(all_types), MODIFY_PREF);
 }
@@ -819,17 +840,19 @@
 // will only tell the client about the migrations one at a time.
 IN_PROC_BROWSER_TEST_P(MigrationTwoClientTest, MigrationHellWithoutNigori) {
   ASSERT_TRUE(SetupClients());
-  MigrationList migration_list = GetPreferredDataTypesList();
+  MigrationList migration_list = GetEligibleDataTypeMigrationList();
   // Let the first nudge be a datatype that's neither prefs nor bookmarks.
   migration_list.push_front(MakeSet(syncer::THEMES));
-  ASSERT_EQ(MakeSet(syncer::NIGORI), migration_list.back());
-  migration_list.pop_back();
+  base::Erase(migration_list, MakeSet(syncer::NIGORI));
   RunTwoClientMigrationTest(migration_list, MODIFY_BOOKMARK);
 }
 
 IN_PROC_BROWSER_TEST_P(MigrationTwoClientTest, MigrationHellWithNigori) {
+  if (!UseGcDirective()) {
+    GTEST_SKIP() << "NIGORI migration is disallowed for MIGRATION_DONE.";
+  }
   ASSERT_TRUE(SetupClients());
-  MigrationList migration_list = GetPreferredDataTypesList();
+  MigrationList migration_list = GetEligibleDataTypeMigrationList();
   // Let the first nudge be a datatype that's neither prefs nor bookmarks.
   migration_list.push_front(MakeSet(syncer::THEMES));
   ASSERT_EQ(MakeSet(syncer::NIGORI), migration_list.back());
diff --git a/components/sync/engine/syncer_proto_util_unittest.cc b/components/sync/engine/syncer_proto_util_unittest.cc
index 13be8bf..d9fadd5 100644
--- a/components/sync/engine/syncer_proto_util_unittest.cc
+++ b/components/sync/engine/syncer_proto_util_unittest.cc
@@ -32,14 +32,16 @@
 }  // namespace
 
 // Builds a ClientToServerResponse with some data type ids, including
-// invalid ones.  GetTypesToMigrate() should return only the valid
-// data types.
+// invalid ones and NIGORI. GetTypesToMigrate() should return only the valid
+// data types excluding NIGORI.
 TEST(SyncerProtoUtil, GetTypesToMigrate) {
   sync_pb::ClientToServerResponse response;
   response.add_migrated_data_type_id(
       GetSpecificsFieldNumberFromDataType(BOOKMARKS));
   response.add_migrated_data_type_id(
       GetSpecificsFieldNumberFromDataType(HISTORY_DELETE_DIRECTIVES));
+  response.add_migrated_data_type_id(
+      GetSpecificsFieldNumberFromDataType(NIGORI));
   response.add_migrated_data_type_id(-1);
   EXPECT_EQ(DataTypeSet({BOOKMARKS, HISTORY_DELETE_DIRECTIVES}),
             GetTypesToMigrate(response));
Loading diff…

Original Bug Report

reported by [email protected]

Potential silent sync encryption downgrade via NIGORI migration handling (via MIGRATION_DONE)

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. Please see https://chromium.googlesource.com/chromium/src/+/main/docs/security/ai-generated-security-bugs-faq.md for more information.

Overview: A logic flaw in Chrome Sync allows a malicious server or MITM to silently downgrade a user’s encryption from a custom passphrase to server-known keys. By triggering a migration for the NIGORI data type, an attacker can force a local state wipe that erases custom passphrase protection, allowing a subsequent transition to keystore encryption without user notification.

Affected files:

  • components/sync/service/data_type_manager_impl.cc
  • components/sync/engine/syncer_proto_util.cc
  • components/sync/nigori/nigori_sync_bridge_impl.cc
  • components/sync/service/backend_migrator.cc
  • components/sync/service/glue/sync_engine_backend.cc

Estimated timestamp from git blame: Unknown (Google3 checkout)

Summary

A logic vulnerability in Chrome Sync’s migration handling could allow an attacker who controls the sync server response (or a compromised network process) to silently strip a user’s end-to-end encryption (CUSTOM_PASSPHRASE) and downgrade it to a server-known KEYSTORE_PASSPHRASE. This effectively allows the server to decrypt sensitive user data synced after the downgrade, such as passwords and Wi-Fi credentials.

Technical Details

The vulnerability exists because the sync client’s migration logic does not adequately protect the NIGORI control type (which manages encryption keys) from being targeted for migration by the server.

Root Cause

  1. Migration Trigger: When a sync server returns error_code = MIGRATION_DONE with migrated_data_type_id containing the field number for NigoriSpecifics (47745), the client processes this as a request to migrate the NIGORI data type.
    • Reference: components/sync/engine/syncer_proto_util.cc:311 calls GetTypesToMigrate(response).
  2. State Purge: The BackendMigrator initiates a purge of the type to be migrated. For NIGORI, this causes DataTypeManagerImpl to call configurer_->ClearNigoriDataForMigration().
    • Reference: components/sync/service/data_type_manager_impl.cc:550.
  3. Local Wipe: This call reaches NigoriSyncBridgeImpl::ApplyDisableSyncChanges(), which wipes the local cryptographer and resets the passphrase_type to UNKNOWN. This effectively erases the client’s knowledge that a custom passphrase was ever in use.
    • Reference: components/sync/nigori/nigori_sync_bridge_impl.cc:930-945.
  4. Silent Re-encryption: After the purge, BackendMigrator re-enables NIGORI for reconfiguration. During the subsequent sync cycle, the client downloads the state from the server. Because the local passphrase_type is now UNKNOWN, the bridge accepts a transition to KEYSTORE_PASSPHRASE provided by the server, as transitions from UNKNOWN to any valid type are permitted.
    • Reference: components/sync/nigori/nigori_sync_bridge_impl.cc:187-193 in IsValidPassphraseTransition.

Potential Attack Scenario

An attacker controlling the sync response (compromised server or MITM) could follow these suggested steps:

  1. Serve a ClientToServerResponse with error_code = MIGRATION_DONE and migrated_data_type_id = [47745].
  2. The client will silently wipe its local encryption state and custom passphrase keys.
  3. The client re-enables sync for the NIGORI type.
  4. In the next sync update, serve NigoriSpecifics with passphrase_type = KEYSTORE_PASSPHRASE and new keys known to the server.
  5. The client accepts this transition silently. All future sensitive data (passwords, etc.) will be encrypted with the server’s keys.

Impact

This allows a compromised backend or network process to escalate its influence to the browser process and decrypt user credentials. The attack is entirely silent; no engine reset or UI prompts are visible to the user.

Suggested Fix

The sync client should explicitly prevent the NIGORI data type from being targeted for migration by the server. In components/sync/engine/syncer_proto_util.cc, GetTypesToMigrate should filter out control types like NIGORI. Additionally, NigoriSyncBridgeImpl should be hardened to prevent silent transitions away from CUSTOM_PASSPHRASE even if the local state has been cleared for migration, perhaps by persisting a ‘was custom passphrase’ bit across migration purges.

Evaluated with Chrome root at commit: 1a8d40fc44df2088d5945c0bf53584038aa1614a


Results so far have been promising, but there can be wrong deductions. Feel free to adjust as follows:

  • If you are familiar with the severity guidelines, you may adjust the severity.
  • If this is a false positive, and there’s no work to be done, please close as WAI.
  • If there is work to do here but not a vulnerability, please change the issue type to Task/Bug/FR.

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