CVE-2026-79107
Overview
Changed Functions
| Function | Change | Notes |
|---|---|---|
forcomponents/saved_tab_groups/internal/shared_tab_group_data_sync_bridge.cc |
modified |
Files Changed
chrome/browser/sync/test/integration/single_client_shared_tab_group_data_sync_test.cccomponents/saved_tab_groups/internal/shared_tab_group_data_sync_bridge.cccomponents/saved_tab_groups/internal/shared_tab_group_data_sync_bridge.h
Patch
From 4cc022983800a4043f2c8d438dd09d12a7facc92 Mon Sep 17 00:00:00 2001 From: Rushan Suleymanov <[email protected]> Date: Wed, 22 Jul 2026 05:19:38 -0700 Subject: [PATCH] Prevent cross-collaboration deletions in shared tab groups In SharedTabGroupDataSyncBridge::DeleteDataFromLocalStorage, verify that the collaboration ID of the incoming deletion entity matches the collaboration ID of the target group, tab, or missing tab group. If the collaboration IDs do not match, the deletion is ignored to prevent cross-collaboration deletion of data across access boundaries. Bug: 511822878 Change-Id: I6c0140356babb0bfcb527e3024c7194320e973e8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8130940 Reviewed-by: David Pennington <[email protected]> Commit-Queue: Rushan Suleymanov <[email protected]> Cr-Commit-Position: refs/heads/main@{#1666194} --- diff --git a/chrome/browser/sync/test/integration/single_client_shared_tab_group_data_sync_test.cc b/chrome/browser/sync/test/integration/single_client_shared_tab_group_data_sync_test.cc index b187c22a..8dcd1d47 100644 --- a/chrome/browser/sync/test/integration/single_client_shared_tab_group_data_sync_test.cc +++ b/chrome/browser/sync/test/integration/single_client_shared_tab_group_data_sync_test.cc @@ -780,6 +780,79 @@ EXPECT_TRUE(SharedTabGroupDataErrorChecker(GetSyncService(0)).Wait()); } +IN_PROC_BROWSER_TEST_P(SingleClientSharedTabGroupDataSyncTest, + ShouldIgnoreCrossCollaborationGroupDeletion) { + ASSERT_TRUE(SetupSync()); + + const base::Uuid kGroupGuid = base::Uuid::GenerateRandomV4(); + const syncer::CollaborationId kCollaborationId1("collaboration_1"); + const syncer::CollaborationId kCollaborationId2("collaboration_2"); + RegisterCollaboration(kCollaborationId1); + RegisterCollaboration(kCollaborationId2); + + const sync_pb::SharedTabGroupDataSpecifics shared_group_specifics = + MakeSharedTabGroupSpecifics( + kGroupGuid, + /*originating_saved_group_guid=*/base::Uuid::GenerateRandomV4(), + "title", sync_pb::SharedTabGroup_Color_CYAN); + AddSpecificsToFakeServer(shared_group_specifics, kCollaborationId1); + + const sync_pb::SharedTabGroupDataSpecifics shared_tab_specifics = + MakeSharedTabGroupTabSpecifics(base::Uuid::GenerateRandomV4(), kGroupGuid, + "tab 1", GURL("http://google.com/1")); + AddSpecificsToFakeServer(shared_tab_specifics, kCollaborationId1); + + ASSERT_TRUE(SavedTabOrGroupExistsChecker(GetTabGroupSyncService(), kGroupGuid) + .Wait()); + ASSERT_THAT(GetAllTabGroups(), SizeIs(1)); + + // Simulate an injected tombstone for the group from a different + // collaboration. + InjectTombstoneToFakeServer(shared_group_specifics, kCollaborationId2); + + ASSERT_TRUE(AwaitQuiescence()); + + // Group and tab should remain intact. + ASSERT_THAT(GetAllTabGroups(), SizeIs(1)); + EXPECT_THAT(GetAllTabGroups().front().saved_tabs(), SizeIs(1)); +} + +IN_PROC_BROWSER_TEST_P(SingleClientSharedTabGroupDataSyncTest, + ShouldIgnoreCrossCollaborationTabDeletion) { + ASSERT_TRUE(SetupSync()); + + const base::Uuid kGroupGuid = base::Uuid::GenerateRandomV4(); + const syncer::CollaborationId kCollaborationId1("collaboration_1"); + const syncer::CollaborationId kCollaborationId2("collaboration_2"); + RegisterCollaboration(kCollaborationId1); + RegisterCollaboration(kCollaborationId2); + + const sync_pb::SharedTabGroupDataSpecifics shared_group_specifics = + MakeSharedTabGroupSpecifics( + kGroupGuid, + /*originating_saved_group_guid=*/base::Uuid::GenerateRandomV4(), + "title", sync_pb::SharedTabGroup_Color_CYAN); + AddSpecificsToFakeServer(shared_group_specifics, kCollaborationId1); + + const sync_pb::SharedTabGroupDataSpecifics shared_tab_specifics = + MakeSharedTabGroupTabSpecifics(base::Uuid::GenerateRandomV4(), kGroupGuid, + "tab 1", GURL("http://google.com/1")); + AddSpecificsToFakeServer(shared_tab_specifics, kCollaborationId1); + + ASSERT_TRUE(SavedTabOrGroupExistsChecker(GetTabGroupSyncService(), kGroupGuid) + .Wait()); + ASSERT_THAT(GetAllTabGroups(), SizeIs(1)); + + // Simulate an injected tombstone for the tab from a different collaboration. + InjectTombstoneToFakeServer(shared_tab_specifics, kCollaborationId2); + + ASSERT_TRUE(AwaitQuiescence()); + + // Group and tab should remain intact. + ASSERT_THAT(GetAllTabGroups(), SizeIs(1)); + EXPECT_THAT(GetAllTabGroups().front().saved_tabs(), SizeIs(1)); +} + // Android doesn't support PRE_ tests. #if !BUILDFLAG(IS_ANDROID) IN_PROC_BROWSER_TEST_P(SingleClientSharedTabGroupDataSyncTest, diff --git a/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge.cc b/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge.cc index 7ecdde05e..5f15f4e 100644 --- a/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge.cc +++ b/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge.cc @@ -644,14 +644,15 @@ // Process group and tab deletions first. for (const std::unique_ptr<syncer::EntityChange>& change : delete_changes) { - GaiaId last_updated_by; - if (change->data().collaboration_metadata) { - last_updated_by = - change->data().collaboration_metadata->last_updated_by(); + if (!change->data().collaboration_metadata.has_value()) { + // This should never happen. Skip deletion in this case for safety. + continue; } - DeleteDataFromLocalStorage(change->storage_key(), - std::move(last_updated_by), - *ongoing_write_batch_); + GaiaId last_updated_by = + change->data().collaboration_metadata->last_updated_by(); + DeleteDataFromLocalStorage( + change->storage_key(), *change->data().collaboration_metadata, + std::move(last_updated_by), *ongoing_write_batch_); } // Sort tab updates and creations in the reversed order. This is required to @@ -1361,10 +1362,9 @@ void SharedTabGroupDataSyncBridge::DeleteDataFromLocalStorage( const std::string& storage_key, + const syncer::CollaborationMetadata& collaboration_metadata, GaiaId removed_by, syncer::DataTypeStore::WriteBatch& write_batch) { - write_batch.DeleteData(storage_key); - base::Uuid guid = base::Uuid::ParseLowercase(storage_key); if (!guid.is_valid()) { return; @@ -1372,7 +1372,13 @@ // Check if the model contains the group guid. If so, remove that group and // all of its tabs. - if (model_wrapper_->GetGroup(guid)) { + if (const SavedTabGroup* group = model_wrapper_->GetGroup(guid)) { + if (group->collaboration_id() != + collaboration_metadata.collaboration_id()) { + DVLOG(1) << "Ignoring deletion of group from a different collaboration"; + return; + } + write_batch.DeleteData(storage_key); std::erase(tab_groups_waiting_for_commit_, guid); model_wrapper_->RemoveGroup(guid); return; @@ -1380,9 +1386,31 @@ if (const SavedTabGroup* group_containing_tab = model_wrapper_->GetGroupContainingTab(guid)) { + if (group_containing_tab->collaboration_id() != + collaboration_metadata.collaboration_id()) { + DVLOG(1) << "Ignoring deletion of tab from a different collaboration"; + return; + } + write_batch.DeleteData(storage_key); model_wrapper_->RemoveTabFromGroup(group_containing_tab->saved_guid(), guid, std::move(removed_by)); + return; } + + auto it = tabs_missing_groups_.find(guid); + if (it != tabs_missing_groups_.end()) { + if (it->second.collaboration_metadata.collaboration_id() != + collaboration_metadata.collaboration_id()) { + DVLOG(1) << "Ignoring deletion of tab missing group from a different " + "collaboration"; + return; + } + write_batch.DeleteData(storage_key); + tabs_missing_groups_.erase(it); + return; + } + + write_batch.DeleteData(storage_key); } void SharedTabGroupDataSyncBridge::SendToSync( diff --git a/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge.h b/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge.h index 0eb8bfd..e93b031 100644 --- a/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge.h +++ b/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge.h @@ -149,6 +149,7 @@ // tabs will be removed in addition to the group. void DeleteDataFromLocalStorage( const std::string& storage_key, + const syncer::CollaborationMetadata& collaboration_metadata, GaiaId removed_by,
Regression Test / PoC
diff --git a/chrome/browser/sync/test/integration/single_client_shared_tab_group_data_sync_test.cc b/chrome/browser/sync/test/integration/single_client_shared_tab_group_data_sync_test.cc
index b187c22a..8dcd1d47 100644
--- a/chrome/browser/sync/test/integration/single_client_shared_tab_group_data_sync_test.cc
+++ b/chrome/browser/sync/test/integration/single_client_shared_tab_group_data_sync_test.cc
@@ -780,6 +780,79 @@
EXPECT_TRUE(SharedTabGroupDataErrorChecker(GetSyncService(0)).Wait());
}
+IN_PROC_BROWSER_TEST_P(SingleClientSharedTabGroupDataSyncTest,
+ ShouldIgnoreCrossCollaborationGroupDeletion) {
+ ASSERT_TRUE(SetupSync());
+
+ const base::Uuid kGroupGuid = base::Uuid::GenerateRandomV4();
+ const syncer::CollaborationId kCollaborationId1("collaboration_1");
+ const syncer::CollaborationId kCollaborationId2("collaboration_2");
+ RegisterCollaboration(kCollaborationId1);
+ RegisterCollaboration(kCollaborationId2);
+
+ const sync_pb::SharedTabGroupDataSpecifics shared_group_specifics =
+ MakeSharedTabGroupSpecifics(
+ kGroupGuid,
+ /*originating_saved_group_guid=*/base::Uuid::GenerateRandomV4(),
+ "title", sync_pb::SharedTabGroup_Color_CYAN);
+ AddSpecificsToFakeServer(shared_group_specifics, kCollaborationId1);
+
+ const sync_pb::SharedTabGroupDataSpecifics shared_tab_specifics =
+ MakeSharedTabGroupTabSpecifics(base::Uuid::GenerateRandomV4(), kGroupGuid,
+ "tab 1", GURL("http://google.com/1"));
+ AddSpecificsToFakeServer(shared_tab_specifics, kCollaborationId1);
+
+ ASSERT_TRUE(SavedTabOrGroupExistsChecker(GetTabGroupSyncService(), kGroupGuid)
+ .Wait());
+ ASSERT_THAT(GetAllTabGroups(), SizeIs(1));
+
+ // Simulate an injected tombstone for the group from a different
+ // collaboration.
+ InjectTombstoneToFakeServer(shared_group_specifics, kCollaborationId2);
+
+ ASSERT_TRUE(AwaitQuiescence());
+
+ // Group and tab should remain intact.
+ ASSERT_THAT(GetAllTabGroups(), SizeIs(1));
+ EXPECT_THAT(GetAllTabGroups().front().saved_tabs(), SizeIs(1));
+}
+
+IN_PROC_BROWSER_TEST_P(SingleClientSharedTabGroupDataSyncTest,
+ ShouldIgnoreCrossCollaborationTabDeletion) {
+ ASSERT_TRUE(SetupSync());
+
+ const base::Uuid kGroupGuid = base::Uuid::GenerateRandomV4();
+ const syncer::CollaborationId kCollaborationId1("collaboration_1");
+ const syncer::CollaborationId kCollaborationId2("collaboration_2");
+ RegisterCollaboration(kCollaborationId1);
+ RegisterCollaboration(kCollaborationId2);
+
+ const sync_pb::SharedTabGroupDataSpecifics shared_group_specifics =
+ MakeSharedTabGroupSpecifics(
+ kGroupGuid,
+ /*originating_saved_group_guid=*/base::Uuid::GenerateRandomV4(),
+ "title", sync_pb::SharedTabGroup_Color_CYAN);
+ AddSpecificsToFakeServer(shared_group_specifics, kCollaborationId1);
+
+ const sync_pb::SharedTabGroupDataSpecifics shared_tab_specifics =
+ MakeSharedTabGroupTabSpecifics(base::Uuid::GenerateRandomV4(), kGroupGuid,
+ "tab 1", GURL("http://google.com/1"));
+ AddSpecificsToFakeServer(shared_tab_specifics, kCollaborationId1);
+
+ ASSERT_TRUE(SavedTabOrGroupExistsChecker(GetTabGroupSyncService(), kGroupGuid)
+ .Wait());
+ ASSERT_THAT(GetAllTabGroups(), SizeIs(1));
+
+ // Simulate an injected tombstone for the tab from a different collaboration.
+ InjectTombstoneToFakeServer(shared_tab_specifics, kCollaborationId2);
+
+ ASSERT_TRUE(AwaitQuiescence());
+
+ // Group and tab should remain intact.
+ ASSERT_THAT(GetAllTabGroups(), SizeIs(1));
+ EXPECT_THAT(GetAllTabGroups().front().saved_tabs(), SizeIs(1));
+}
+
// Android doesn't support PRE_ tests.
#if !BUILDFLAG(IS_ANDROID)
IN_PROC_BROWSER_TEST_P(SingleClientSharedTabGroupDataSyncTest,
diff --git a/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge_unittest.cc b/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge_unittest.cc
index 76542232..8737352d 100644
--- a/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge_unittest.cc
+++ b/components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge_unittest.cc
@@ -374,8 +374,12 @@
}
std::unique_ptr<syncer::EntityChange> CreateDeleteEntityChange(
- const std::string& storage_key) {
- return syncer::EntityChange::CreateDelete(storage_key, syncer::EntityData());
+ const std::string& storage_key,
+ const CollaborationId& collaboration_id) {
+ return syncer::EntityChange::CreateDelete(
+ storage_key, CreateEntityData(sync_pb::SharedTabGroupDataSpecifics(),
+ collaboration_id, kDefaultGaiaId,
+ /*updated_by=*/kDefaultGaiaId));
}
std::vector<syncer::EntityData> ExtractEntityDataFromBatch(
@@ -900,8 +904,9 @@
.SetCollaborationId(CollaborationId("collaboration 2")));
ASSERT_EQ(model()->Count(), 2);
- ApplySingleEntityChange(CreateDeleteEntityChange(
- group_to_delete.saved_guid().AsLowercaseString()));
+ ApplySingleEntityChange(
+ CreateDeleteEntityChange(group_to_delete.saved_guid().AsLowercaseString(),
+ CollaborationId("collaboration")));
EXPECT_THAT(
model()->saved_tab_groups(),
@@ -927,7 +932,8 @@
ASSERT_THAT(model()->saved_tab_groups().front().saved_tabs(), SizeIs(2));
ApplySingleEntityChange(CreateDeleteEntityChange(
- tab_to_delete.saved_tab_guid().AsLowercaseString()));
+ tab_to_delete.saved_tab_guid().AsLowercaseString(),
+ CollaborationId("collaboration")));
ASSERT_EQ(model()->Count(), 1);
EXPECT_THAT(
@@ -1857,8 +1863,8 @@
syncer::EntityChangeList change_list;
change_list.push_back(
CreateUpdateEntityChange(tab_1_specifics, kCollaborationId));
- change_list.push_back(
- CreateDeleteEntityChange(StorageKeyForTab(group.saved_tabs()[0])));
+ change_list.push_back(CreateDeleteEntityChange(
+ StorageKeyForTab(group.saved_tabs()[0]), kCollaborationId));
bridge()->ApplyIncrementalSyncChanges(bridge()->CreateMetadataChangeList(),
std::move(change_list));
@@ -1927,8 +1933,8 @@
EXPECT_CALL(mock_processor(), Put).Times(0);
syncer::EntityChangeList change_list;
- change_list.push_back(
- CreateDeleteEntityChange(StorageKeyForTab(group.saved_tabs()[0])));
+ change_list.push_back(CreateDeleteEntityChange(
+ StorageKeyForTab(group.saved_tabs()[0]), kCollaborationId));
bridge()->ApplyIncrementalSyncChanges(bridge()->CreateMetadataChangeList(),
std::move(change_list));
@@ -2265,7 +2271,8 @@
MakeTabSpecifics("Tab 2", GURL("http://google.com/2"), group.saved_guid(),
GenerateRandomUniquePosition());
syncer::EntityChangeList change_list;
- change_list.push_back(CreateDeleteEntityChange(StorageKeyForTab(tab)));
+ change_list.push_back(
+ CreateDeleteEntityChange(StorageKeyForTab(tab), kCollaborationId));
change_list.push_back(
CreateUpdateEntityChange(new_tab_specifics, kCollaborationId));
bridge()->ApplyIncrementalSyncChanges(bridge()->CreateMetadataChangeList(),
@@ -2451,6 +2458,130 @@
}
TEST_F(SharedTabGroupDataSyncBridgeTest,
+ ShouldIgnoreCrossCollaborationGroupDeletion) {
+ const CollaborationId kCollaborationId1("collaboration 1");
+ const CollaborationId kCollaborationId2("collaboration 2");
+ ASSERT_TRUE(InitializeBridgeAndModel());
+
+ // Add a group and a tab in collaboration 1.
+ sync_pb::SharedTabGroupDataSpecifics group_specifics =
+ MakeTabGroupSpecifics("title", sync_pb::SharedTabGroup::BLUE);
+ const base::Uuid group_id =
+ base::Uuid::ParseLowercase(group_specifics.guid());
+ sync_pb::SharedTabGroupDataSpecifics tab_specifics =
+ MakeTabSpecifics("tab title 1", GURL("https://google.com/1"), group_id,
+ GenerateRandomUniquePosition());
+
+ syncer::EntityChangeList change_list;
+ change_list.push_back(
+ CreateAddEntityChange(group_specifics, kCollaborationId1));
+ change_list.push_back(
+ CreateAddEntityChange(tab_specifics, kCollaborationId1));
+ bridge()->ApplyIncrementalSyncChanges(bridge()->CreateMetadataChangeList(),
+ std::move(change_list));
+
+ ASSERT_THAT(model()->saved_tab_groups(), SizeIs(1));
+ ASSERT_THAT(model()->saved_tab_groups().front().saved_tabs(), SizeIs(1));
+
+ // Try to delete the group from collaboration 2 (cross-collaboration
+ // deletion).
+ syncer::EntityChangeList delete_group_list;
+ delete_group_list.push_back(syncer::EntityChange::CreateDelete(
+ group_specifics.guid(),
+ CreateEntityData(group_specifics, kCollaborationId2, kDefaultGaiaId,
+ /*updated_by=*/kDefaultGaiaId)));
+ EXPECT_FALSE(
+ bridge()
+ ->ApplyIncrementalSyncChanges(bridge()->CreateMetadataChangeList(),
+ std::move(delete_group_list))
+ .has_value());
+
+ // Group and tab should remain intact.
+ ASSERT_THAT(model()->saved_tab_groups(), SizeIs(1));
+ ASSERT_THAT(model()->saved_tab_groups().front().saved_tabs(), SizeIs(1));
+}
+
+TEST_F(SharedTabGroupDataSyncBridgeTest,
+ ShouldIgnoreCrossCollaborationTabDeletion) {
+ const CollaborationId kCollaborationId1("collaboration 1");
+ const CollaborationId kCollaborationId2("collaboration 2");
+ ASSERT_TRUE(InitializeBridgeAndModel());
+
+ // Add a group and a tab in collaboration 1.
+ sync_pb::SharedTabGroupDataSpecifics group_specifics =
+ MakeTabGroupSpecifics("title", sync_pb::SharedTabGroup::BLUE);
+ const base::Uuid group_id =
+ base::Uuid::ParseLowercase(group_specifics.guid());
+ sync_pb::SharedTabGroupDataSpecifics tab_specifics =
+ MakeTabSpecifics("tab title 1", GURL("https://google.com/1"), group_id,
+ GenerateRandomUniquePosition());
+
+ syncer::EntityChangeList change_list;
+ change_list.push_back(
+ CreateAddEntityChange(group_specifics, kCollaborationId1));
+ change_list.push_back(
+ CreateAddEntityChange(tab_specifics, kCollaborationId1));
+ bridge()->ApplyIncrementalSyncChanges(bridge()->CreateMetadataChangeList(),
+ std::move(change_list));
+
+ ASSERT_THAT(model()->saved_tab_groups(), SizeIs(1));
+ ASSERT_THAT(model()->saved_tab_groups().front().saved_tabs(), SizeIs(1));
+
+ // Try to delete the tab from collaboration 2 (cross-collaboration deletion).
+ syncer::EntityChangeList delete_tab_list;
+ delete_tab_list.push_back(syncer::EntityChange::CreateDelete(
+ tab_specifics.guid(),
+ CreateEntityData(tab_specifics, kCollaborationId2, kDefaultGaiaId,
+ /*updated_by=*/kDefaultGaiaId)));
+ EXPECT_FALSE(
+ bridge()
+ ->ApplyIncrementalSyncChanges(bridge()->CreateMetadataChangeList(),
+ std::move(delete_tab_list))
+ .has_value());
+
+ // Group and tab should still remain intact.
+ ASSERT_THAT(model()->saved_tab_groups(), SizeIs(1));
+ ASSERT_THAT(model()->saved_tab_groups().front().saved_tabs(), SizeIs(1));
+}
+
+TEST_F(SharedTabGroupDataSyncBridgeTest,
+ ShouldIgnoreCrossCollaborationTabMissingGroupDeletion) {
+ const CollaborationId kCollaborationId1("collaboration 1");
+ const CollaborationId kCollaborationId2("collaboration 2");
+ const base::Uuid kMissingGroupGuid = base::Uuid::GenerateRandomV4();
+ ASSERT_TRUE(InitializeBridgeAndModel());
+
+ // Add a tab missing group in collaboration 1.
+ sync_pb::SharedTabGroupDataSpecifics tab_specifics =
+ MakeTabSpecifics("tab title", GURL("http://google.com/1"),
+ kMissingGroupGuid, GenerateRandomUniquePosition());
+
+ ApplySingleEntityChange(
+ CreateAddEntityChange(tab_specifics, kCollaborationId1));
+
+ // Verify that the tab missing group is present.
+ ASSERT_THAT(ExtractEntityDataFromBatch(bridge()->GetAllDataForDebugging()),
+ SizeIs(1));
+
+ // Try to delete the tab missing group from collaboration 2
+ // (cross-collaboration deletion).
+ syncer::EntityChangeList delete_tab_list;
+ delete_tab_list.push_back(syncer::EntityChange::CreateDelete(
+ tab_specifics.guid(),
+ CreateEntityData(tab_specifics, kCollaborationId2, kDefaultGaiaId,
+ /*updated_by=*/kDefaultGaiaId)));
+ EXPECT_FALSE(
+ bridge()
+ ->ApplyIncrementalSyncChanges(bridge()->CreateMetadataChangeList(),
+ std::move(delete_tab_list))
+ .has_value());
+
+ // The tab missing group should still remain intact.
+ EXPECT_THAT(ExtractEntityDataFromBatch(bridge()->GetAllDataForDebugging()),
+ SizeIs(1));
+}
+
+TEST_F(SharedTabGroupDataSyncBridgeTest,
ShouldResolveTabsMissingGroupsOnRemoteUpdate) {
const CollaborationId kCollaborationId("collaboration");
const base::Uuid kMissingGroupGuid = base::Uuid::GenerateRandomV4();
Original Bug Report
Cross-collaboration shared tab group deletion via storage key collision
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. Please see https://chromium.googlesource.com/chromium/src/+/main/docs/security/ai-generated-security-bugs-faq.md for more information.
Overview: A potential logic flaw in SharedTabGroupDataSyncBridge allows an attacker in one collaboration to delete a victim’s shared tab group in a different collaboration. This occurs because the local StorageKey namespace uses only the group GUID, bypassing a production-disabled DCHECK during sync, leading to unauthorized data overwrite and deletion.
Affected files:
components/saved_tab_groups/internal/shared_tab_group_data_sync_bridge.cccomponents/saved_tab_groups/internal/sync_bridge_tab_group_model_wrapper.cccomponents/sync/model/processor_entity_tracker.cc
Estimated timestamp from git blame: 2025-06-02
Summary
SharedTabGroupDataSyncBridge maps remote sync entities to local storage. It correctly generates a globally unique ClientTag for each entity using guid + '|' + collaboration_id. However, the local database StorageKey uses only the guid. Because the StorageKey lacks the collaboration ID, an attacker can create a local storage key collision by injecting an entity into an unrelated collaboration using the GUID of a victim’s group.
Potential Attack Scenario
Based on source code analysis, the following sequence of events outlines how an attacker could exploit this vulnerability. Note that these are potential steps, as our analysis environment cannot execute live Proof of Concepts.
Prerequisites:
- The victim is a member of Collaboration 1 (C1) and Collaboration 2 (C2).
- The attacker is a member of Collaboration 1 (C1).
- The attacker knows the 128-bit UUID (
G2) of a shared tab group belonging to the victim in C2 (e.g., from prior membership or a sharing link).
Step-by-Step Exploitation:
- Injection: The attacker commits a crafted
SHARED_TAB_GROUP_DATAtab entity to C1 using the victim’s group GUID (G2). To bypass collaboration ID checks later, the attacker sets the tab’s parent group GUID to a dummy, non-existent value. DCHECKBypass and Collision: The victim receives the sync update. The sync processor extracts theStorageKey(G2). InProcessorEntityTracker::AddInternal, a check verifies that the storage key is unique (DCHECK(storage_key_to_tag_hash_.find(storage_key) == storage_key_to_tag_hash_.end())). Because this is aDCHECK, it compiles out in production, allowing the localstorage_key_to_tag_hash_mapping forG2to be silently overwritten, pointing to the attacker’s C1 entity.- Database Overwrite: The bridge processes the tab update (
ApplyRemoteTabUpdate). Because the dummy parent group is missing, the code falls into a branch that defers processing and explicitly bypasses collaboration ID verification. It callsStoreSharedTab, which writes the attacker’s tab data directly into theDataTypeStoreusing theG2storage key, silently overwriting the victim’s stored database record for the C2 group. - Cross-Collaboration Deletion: The attacker tombstones (deletes) the crafted C1 entity. The victim receives the deletion action for the
G2storage key.DeleteDataFromLocalStoragedeletes the database record and blindly removes the live C2 group from the in-memorySavedTabGroupModelwithout verifying the collaboration ID.
Impact
- Denial of Service / Data Loss: An attacker can remotely close all tabs associated with a victim’s shared tab group and irreversibly delete the group from their profile.
- Information Disclosure (Potential): If the victim edits their C2 group after Step 3 but before Step 4,
ClientTagBasedDataTypeProcessor::Putmay use the corrupted tracking map to sync the victim’s private C2 changes back to the attacker’s C1 collaboration.
Proposed Fix
- Unique Storage Keys: Update
SharedTabGroupDataSyncBridge::GetStorageKeyto include the collaboration ID (e.g.,guid + '|' + collaboration_id) to ensure the flat local database namespace is unique across collaborations. - Harden Collision Checks: Promote the
DCHECKinProcessorEntityTracker::AddInternal(and related methods) to aCHECKto prevent silent state corruption in production builds. - Strict Verification: Ensure that
ApplyRemoteTabUpdatevalidates thecollaboration_ideven when the parent group is missing, before persisting data to theDataTypeStore.
Evaluated with Chrome root at commit: eca8648a4e1cdfdda68c495a6003059fed641955
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.