From d1af88617bcee9296d37e96533d86055d61de1c9 Mon Sep 17 00:00:00 2001 From: areebahmeddd Date: Fri, 31 Jul 2026 08:40:06 +0530 Subject: [PATCH 1/4] prevent changing creator in groups --- bitchat/Services/Groups/GroupStore.swift | 22 +++++++++- bitchatTests/Services/GroupStoreTests.swift | 47 +++++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/bitchat/Services/Groups/GroupStore.swift b/bitchat/Services/Groups/GroupStore.swift index 3a45dcb7..a8bdca6d 100644 --- a/bitchat/Services/Groups/GroupStore.swift +++ b/bitchat/Services/Groups/GroupStore.swift @@ -68,7 +68,8 @@ final class GroupStore: ObservableObject { } /// Inserts or replaces a group and its current key. Rejects rosters over - /// the hard cap or groups whose creator is missing from the roster. + /// the hard cap, groups whose creator is missing from the roster, and any + /// attempt to change the creator of a group we already hold. @discardableResult func upsert(_ group: BitchatGroup, key: Data) -> Bool { guard group.groupID.count == BitchatGroup.groupIDLength, @@ -76,6 +77,25 @@ final class GroupStore: ObservableObject { !group.members.isEmpty, group.members.count <= BitchatGroup.maxMembers, group.creator != nil else { return false } + // A group keeps the creator it was created with. + // + // `applyGroupState` checks that the sender is the creator the state + // NAMES, that the creator signature verifies, that we are still in the + // roster, and that the epoch does not regress — but nothing compares the + // creator against the group we already hold. Each check passes for an + // attacker who simply names themselves creator of an existing groupID at + // a higher epoch and signs with their own key, and the roster and key are + // then replaced wholesale. Members would seal their next message under a + // key the attacker holds, while the real group silently stops seeing + // them; nothing in the UI changes. + // + // Pinning here rather than in `applyGroupState` because this is the one + // point every write goes through, local and remote alike. + if let existing = groups.first(where: { $0.groupID == group.groupID }), + existing.creatorFingerprint != group.creatorFingerprint { + SecureLogger.warning("Refusing group state that changes the creator of an existing group", category: .security) + return false + } guard keychain.saveIdentityKey(key, forKey: Self.keychainKey(for: group.groupID)) else { SecureLogger.error("Failed to store group key in keychain", category: .security) return false diff --git a/bitchatTests/Services/GroupStoreTests.swift b/bitchatTests/Services/GroupStoreTests.swift index adf04806..a8ca7cce 100644 --- a/bitchatTests/Services/GroupStoreTests.swift +++ b/bitchatTests/Services/GroupStoreTests.swift @@ -81,6 +81,53 @@ struct GroupStoreTests { #expect(store.group(withID: group.groupID)?.members == [creator]) } + // MARK: - Creator pinning + + @Test func upsertRefusesToChangeTheCreatorOfAnExistingGroup() throws { + let store = GroupStore(keychain: MockKeychain(), persistsToDisk: false) + let creator = makeMember(seed: 0xC1, nickname: "creator") + let me = makeMember(seed: 0x0E, nickname: "me") + let attacker = makeMember(seed: 0xEE, nickname: "attacker") + let group = try #require(store.createGroup(named: "trip", creator: creator)) + #expect(store.updateRoster(groupID: group.groupID, members: [creator, me]) != nil) + let realKey = try #require(store.key(forGroupID: group.groupID)) + + // Same groupID, same name, higher epoch, attacker as creator — every + // other check in applyGroupState passes for this. + let hijack = BitchatGroup( + groupID: group.groupID, + name: group.name, + epoch: group.epoch + 1, + members: [attacker, me], + creatorFingerprint: attacker.fingerprint + ) + + #expect(!store.upsert(hijack, key: Data(repeating: 0xAB, count: 32))) + + let stored = try #require(store.group(withID: group.groupID)) + #expect(stored.creatorFingerprint == creator.fingerprint) + #expect(stored.epoch == group.epoch) + #expect(store.key(forGroupID: group.groupID) == realKey) + } + + @Test func upsertStillAcceptsANewEpochFromTheSameCreator() throws { + // The other side of the boundary: pinning the creator must not stop the + // real creator rotating the key, or removing a member breaks. + let store = GroupStore(keychain: MockKeychain(), persistsToDisk: false) + let creator = makeMember(seed: 0xC1, nickname: "creator") + let me = makeMember(seed: 0x0E, nickname: "me") + let group = try #require(store.createGroup(named: "trip", creator: creator)) + + var rotated = group + rotated.epoch = group.epoch + 1 + rotated.members = [creator, me] + let newKey = Data(repeating: 0x5A, count: 32) + + #expect(store.upsert(rotated, key: newKey)) + #expect(store.group(withID: group.groupID)?.epoch == group.epoch + 1) + #expect(store.key(forGroupID: group.groupID) == newKey) + } + // MARK: - Rotation @Test func rotateKeyBumpsEpochAndReplacesKey() throws { From c836b10d68e48abe6e53d724b417b24400ea5b0b Mon Sep 17 00:00:00 2001 From: areebahmeddd Date: Fri, 31 Jul 2026 09:11:00 +0530 Subject: [PATCH 2/4] fix docs --- bitchat/Services/Groups/GroupStore.swift | 19 +++++-------------- bitchatTests/Services/GroupStoreTests.swift | 2 +- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/bitchat/Services/Groups/GroupStore.swift b/bitchat/Services/Groups/GroupStore.swift index a8bdca6d..a9aa7faa 100644 --- a/bitchat/Services/Groups/GroupStore.swift +++ b/bitchat/Services/Groups/GroupStore.swift @@ -77,20 +77,11 @@ final class GroupStore: ObservableObject { !group.members.isEmpty, group.members.count <= BitchatGroup.maxMembers, group.creator != nil else { return false } - // A group keeps the creator it was created with. - // - // `applyGroupState` checks that the sender is the creator the state - // NAMES, that the creator signature verifies, that we are still in the - // roster, and that the epoch does not regress — but nothing compares the - // creator against the group we already hold. Each check passes for an - // attacker who simply names themselves creator of an existing groupID at - // a higher epoch and signs with their own key, and the roster and key are - // then replaced wholesale. Members would seal their next message under a - // key the attacker holds, while the real group silently stops seeing - // them; nothing in the UI changes. - // - // Pinning here rather than in `applyGroupState` because this is the one - // point every write goes through, local and remote alike. + // A group keeps the creator it was created with. `applyGroupState` + // checks the sender is the creator the state NAMES, but never against + // the creator already stored, so a peer naming themselves creator of a + // known groupID at a higher epoch replaces the roster and key wholesale. + // Pinned here, the one point every write goes through. if let existing = groups.first(where: { $0.groupID == group.groupID }), existing.creatorFingerprint != group.creatorFingerprint { SecureLogger.warning("Refusing group state that changes the creator of an existing group", category: .security) diff --git a/bitchatTests/Services/GroupStoreTests.swift b/bitchatTests/Services/GroupStoreTests.swift index a8ca7cce..2f426c49 100644 --- a/bitchatTests/Services/GroupStoreTests.swift +++ b/bitchatTests/Services/GroupStoreTests.swift @@ -92,7 +92,7 @@ struct GroupStoreTests { #expect(store.updateRoster(groupID: group.groupID, members: [creator, me]) != nil) let realKey = try #require(store.key(forGroupID: group.groupID)) - // Same groupID, same name, higher epoch, attacker as creator — every + // Same groupID, same name, higher epoch, attacker as creator. Every // other check in applyGroupState passes for this. let hijack = BitchatGroup( groupID: group.groupID, From cf2a1e77ec644a0033e6f24ced013b3c0df883fa Mon Sep 17 00:00:00 2001 From: areebahmeddd Date: Sun, 2 Aug 2026 00:22:57 +0530 Subject: [PATCH 3/4] cover group removal path --- bitchat/Services/Groups/GroupStore.swift | 15 +++++++++------ bitchat/ViewModels/ChatGroupCoordinator.swift | 12 ++++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/bitchat/Services/Groups/GroupStore.swift b/bitchat/Services/Groups/GroupStore.swift index a9aa7faa..7e570ede 100644 --- a/bitchat/Services/Groups/GroupStore.swift +++ b/bitchat/Services/Groups/GroupStore.swift @@ -77,14 +77,17 @@ final class GroupStore: ObservableObject { !group.members.isEmpty, group.members.count <= BitchatGroup.maxMembers, group.creator != nil else { return false } - // A group keeps the creator it was created with. `applyGroupState` - // checks the sender is the creator the state NAMES, but never against - // the creator already stored, so a peer naming themselves creator of a - // known groupID at a higher epoch replaces the roster and key wholesale. - // Pinned here, the one point every write goes through. + // A group keeps the creator it was created with. A peer naming + // themselves creator of a known groupID at a higher epoch would + // otherwise replace the roster and key wholesale. `applyGroupState` + // pins this before its removal branch; here is the one point every + // write goes through. if let existing = groups.first(where: { $0.groupID == group.groupID }), existing.creatorFingerprint != group.creatorFingerprint { - SecureLogger.warning("Refusing group state that changes the creator of an existing group", category: .security) + SecureLogger.warning( + "Refusing group state: creator \(group.creatorFingerprint.prefix(8))… does not match stored creator \(existing.creatorFingerprint.prefix(8))…", + category: .security + ) return false } guard keychain.saveIdentityKey(key, forKey: Self.keychainKey(for: group.groupID)) else { diff --git a/bitchat/ViewModels/ChatGroupCoordinator.swift b/bitchat/ViewModels/ChatGroupCoordinator.swift index b8a2b2a6..131cbe94 100644 --- a/bitchat/ViewModels/ChatGroupCoordinator.swift +++ b/bitchat/ViewModels/ChatGroupCoordinator.swift @@ -558,6 +558,18 @@ private extension ChatGroupCoordinator { let myFingerprint = context.myNoiseFingerprint() let existing = context.groupStore.group(withID: state.groupID) + // A group keeps the creator it was created with. The checks above only + // prove the sender is the creator the state names, which an attacker + // satisfies by naming themselves. Checked before the removal branch, + // which drops the group without ever reaching `upsert`. + if let existing, existing.creatorFingerprint != state.creatorFingerprint { + SecureLogger.warning( + "Dropping group state claiming creator \(state.creatorFingerprint.prefix(8))… for a group created by \(existing.creatorFingerprint.prefix(8))…", + category: .security + ) + return + } + // A creator-signed roster that no longer includes us is a removal. guard state.members.contains(where: { $0.fingerprint == myFingerprint }) else { if let existing { From 81e8c0a23d62d4ef01b79386a09ec835c8b08e1f Mon Sep 17 00:00:00 2001 From: areebahmeddd Date: Tue, 4 Aug 2026 00:40:55 +0530 Subject: [PATCH 4/4] ensure valid creator and key presence when loading groups from disk --- bitchat/Services/Groups/GroupStore.swift | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/bitchat/Services/Groups/GroupStore.swift b/bitchat/Services/Groups/GroupStore.swift index 7e570ede..0fd59a1c 100644 --- a/bitchat/Services/Groups/GroupStore.swift +++ b/bitchat/Services/Groups/GroupStore.swift @@ -190,8 +190,11 @@ final class GroupStore: ObservableObject { let stored = try? JSONDecoder().decode([BitchatGroup].self, from: data) else { return } - // Only groups whose key survived in the keychain are usable. - groups = stored.filter { key(forGroupID: $0.groupID) != nil } + // Only groups whose key survived in the keychain are usable. Disk bypasses + // `upsert`, so re-check the creator invariant to avoid unmatchable creators. + groups = stored.filter { + !$0.creatorFingerprint.isEmpty && $0.creator != nil && key(forGroupID: $0.groupID) != nil + } } private static func defaultFileURL() -> URL? {