Private groups: fix TLV truncation, roster downgrade, removal notice, block, media, signable bytes

Addresses the Codex review and adversarial-review findings on #1383:

- TLV encoding now throws GroupTLVError.valueTooLong instead of clamping to
  65535 and truncating, so an oversize group message fails to seal and
  surfaces send_failed rather than shipping ciphertext recipients drop.
- Roster nicknames truncate on a Character boundary (never mid-scalar), so a
  multi-byte nickname can no longer make the whole signed roster undecodable.
- Invites now bump the epoch (rotate the key) like removals, giving every
  roster change a strictly-increasing epoch so out-of-order invite states no
  longer last-writer-wins a just-added member back out.
- Removing a member now sends them a creator-signed roster-without-them under
  a throwaway all-zero key (never the rotated key), so their client
  deactivates the group and surfaces "removed" instead of going silently dark.
- /block is enforced in the group receive path: a blocked member's messages
  are dropped from display and notifications, consistent with every other
  inbound path.
- Media affordances are disabled in group chats (both computed sites) so the
  composer can't strand a media placeholder that never sends; media-in-groups
  is a documented v2 item.
- Creator signature now covers the group name and the sender signature covers
  the epoch (wire-format-affecting; needs Android parity before ship).
- Explicit isGroup guard in markPrivateMessagesAsRead so read/delivered
  receipts can never leak into group conversations under a future refactor.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
jack
2026-07-06 21:17:15 +02:00
co-authored by Claude Fable 5
parent 7ad19ab4c3
commit ccccb2dd8c
6 changed files with 191 additions and 35 deletions
+3 -1
View File
@@ -193,7 +193,9 @@ final class ConversationUIModel: ObservableObject {
private func refreshComputedState() {
if let selectedPeerID = privateConversationModel.selectedPeerID {
canSendMediaInCurrentContext = !(selectedPeerID.isGeoDM || selectedPeerID.isGeoChat)
// Media transfer is not wired for groups in v1; keep it off so the
// composer can't strand a media placeholder that never sends.
canSendMediaInCurrentContext = !(selectedPeerID.isGeoDM || selectedPeerID.isGeoChat || selectedPeerID.isGroup)
return
}
+67 -30
View File
@@ -63,13 +63,24 @@ struct BitchatGroup: Codable, Equatable {
// MARK: - TLV helpers
enum GroupTLVError: Error, Equatable {
/// A TLV value exceeded the 16-bit length field. Encoding fails instead
/// of silently truncating (which would ship a value the receiver drops).
case valueTooLong
}
private enum GroupTLV {
static func put(_ type: UInt8, _ value: Data, into out: inout Data) {
/// Appends a (type, 16-bit length, value) triple. Throws rather than
/// truncating when `value` does not fit the 16-bit length field, so an
/// oversize field surfaces a send failure instead of a silently truncated
/// blob the recipient rejects during decrypt/verify.
static func put(_ type: UInt8, _ value: Data, into out: inout Data) throws {
guard value.count <= Int(UInt16.max) else { throw GroupTLVError.valueTooLong }
out.append(type)
let length = UInt16(clamping: value.count)
let length = UInt16(value.count)
out.append(UInt8((length >> 8) & 0xFF))
out.append(UInt8(length & 0xFF))
out.append(value.prefix(Int(length)))
out.append(value)
}
/// Iterates (type, value) pairs; returns nil on malformed framing.
@@ -131,7 +142,10 @@ enum GroupRosterCoding {
member.signingKey.count == signingKeyLength else { return nil }
out.append(fingerprintData)
out.append(member.signingKey)
let nickname = Data(member.nickname.utf8).prefix(maxNicknameBytes)
// Truncate on a Character boundary so the byte prefix is always
// valid UTF-8; a raw byte-prefix could split a multi-byte scalar
// and make the whole signed roster undecodable on the recipient.
let nickname = truncatedNicknameBytes(member.nickname)
out.append(UInt8(nickname.count))
out.append(nickname)
}
@@ -160,6 +174,16 @@ enum GroupRosterCoding {
guard offset == data.endIndex else { return nil }
return members
}
/// UTF-8 bytes of `nickname` trimmed to at most `maxNicknameBytes`,
/// dropping whole Characters so the result is never split mid-scalar.
private static func truncatedNicknameBytes(_ nickname: String) -> Data {
var candidate = nickname
while Data(candidate.utf8).count > maxNicknameBytes {
candidate.removeLast()
}
return Data(candidate.utf8)
}
}
// MARK: - Group state payload (groupInvite / groupKeyUpdate over Noise)
@@ -192,14 +216,17 @@ struct GroupStatePayload: Equatable {
static let signingDomain = Data("bitchat-group-v1".utf8)
/// The bytes the creator signs. Binding the key and roster by hash keeps
/// the signed content fixed-size.
static func signingContent(groupID: Data, epoch: UInt32, key: Data, rosterBlob: Data) -> Data {
/// The bytes the creator signs. Binding the key, roster, and name by hash
/// keeps the signed content fixed-size. The name is covered so a relay
/// that caches/replays a signed state (e.g. store-and-forward) cannot swap
/// the display name while keeping a valid creator signature.
static func signingContent(groupID: Data, epoch: UInt32, key: Data, rosterBlob: Data, name: String) -> Data {
var content = signingDomain
content.append(groupID)
content.append(GroupTLV.epochData(epoch))
content.append(key.sha256Hash())
content.append(rosterBlob.sha256Hash())
content.append(Data(name.utf8).sha256Hash())
return content
}
@@ -211,7 +238,7 @@ struct GroupStatePayload: Equatable {
sign: (Data) -> Data?
) -> GroupStatePayload? {
guard let rosterBlob = GroupRosterCoding.encode(group.members) else { return nil }
let content = signingContent(groupID: group.groupID, epoch: group.epoch, key: key, rosterBlob: rosterBlob)
let content = signingContent(groupID: group.groupID, epoch: group.epoch, key: key, rosterBlob: rosterBlob, name: group.name)
guard let signature = sign(content) else { return nil }
return GroupStatePayload(
groupID: group.groupID,
@@ -229,13 +256,17 @@ struct GroupStatePayload: Equatable {
let fingerprintData = Data(hexString: creatorFingerprint),
fingerprintData.count == 32 else { return nil }
var out = Data()
GroupTLV.put(FieldType.groupID.rawValue, groupID, into: &out)
GroupTLV.put(FieldType.name.rawValue, Data(name.utf8), into: &out)
GroupTLV.put(FieldType.key.rawValue, key, into: &out)
GroupTLV.put(FieldType.epoch.rawValue, GroupTLV.epochData(epoch), into: &out)
GroupTLV.put(FieldType.roster.rawValue, rosterBlob, into: &out)
GroupTLV.put(FieldType.creatorFingerprint.rawValue, fingerprintData, into: &out)
GroupTLV.put(FieldType.signature.rawValue, signature, into: &out)
do {
try GroupTLV.put(FieldType.groupID.rawValue, groupID, into: &out)
try GroupTLV.put(FieldType.name.rawValue, Data(name.utf8), into: &out)
try GroupTLV.put(FieldType.key.rawValue, key, into: &out)
try GroupTLV.put(FieldType.epoch.rawValue, GroupTLV.epochData(epoch), into: &out)
try GroupTLV.put(FieldType.roster.rawValue, rosterBlob, into: &out)
try GroupTLV.put(FieldType.creatorFingerprint.rawValue, fingerprintData, into: &out)
try GroupTLV.put(FieldType.signature.rawValue, signature, into: &out)
} catch {
return nil
}
return out
}
@@ -292,7 +323,7 @@ struct GroupStatePayload: Equatable {
guard members.count <= BitchatGroup.maxMembers,
let creator = members.first(where: { $0.fingerprint == creatorFingerprint }),
let rosterBlob = GroupRosterCoding.encode(members) else { return false }
let content = GroupStatePayload.signingContent(groupID: groupID, epoch: epoch, key: key, rosterBlob: rosterBlob)
let content = GroupStatePayload.signingContent(groupID: groupID, epoch: epoch, key: key, rosterBlob: rosterBlob, name: name)
return GroupCrypto.verify(signature: signature, for: content, publicKey: creator.signingKey)
}
@@ -326,12 +357,12 @@ struct GroupMessageEnvelope: Equatable {
case ciphertext = 0x04
}
func encode() -> Data {
func encode() throws -> Data {
var out = Data()
GroupTLV.put(FieldType.groupID.rawValue, groupID, into: &out)
GroupTLV.put(FieldType.epoch.rawValue, GroupTLV.epochData(epoch), into: &out)
GroupTLV.put(FieldType.nonce.rawValue, nonce, into: &out)
GroupTLV.put(FieldType.ciphertext.rawValue, ciphertext, into: &out)
try GroupTLV.put(FieldType.groupID.rawValue, groupID, into: &out)
try GroupTLV.put(FieldType.epoch.rawValue, GroupTLV.epochData(epoch), into: &out)
try GroupTLV.put(FieldType.nonce.rawValue, nonce, into: &out)
try GroupTLV.put(FieldType.ciphertext.rawValue, ciphertext, into: &out)
return out
}
@@ -392,10 +423,14 @@ enum GroupCrypto {
case signature = 0x06
}
/// Bytes the sender signs: domain | groupID | messageID | timestamp | content.
static func messageSigningContent(groupID: Data, messageID: String, timestampMs: UInt64, content: String) -> Data {
/// Bytes the sender signs: domain | groupID | epoch | messageID | timestamp | content.
/// Covering the epoch stops a current member from re-sealing another
/// member's decrypted inner bytes under a later epoch key (the signature
/// would no longer verify at the new epoch).
static func messageSigningContent(groupID: Data, epoch: UInt32, messageID: String, timestampMs: UInt64, content: String) -> Data {
var data = messageSigningDomain
data.append(groupID)
data.append(GroupTLV.epochData(epoch))
data.append(Data(messageID.utf8))
data.append(GroupTLV.timestampData(timestampMs))
data.append(Data(content.utf8))
@@ -424,6 +459,7 @@ enum GroupCrypto {
) throws -> Data {
let signingContent = messageSigningContent(
groupID: groupID,
epoch: epoch,
messageID: messageID,
timestampMs: timestampMs,
content: content
@@ -433,12 +469,12 @@ enum GroupCrypto {
}
var inner = Data()
GroupTLV.put(InnerField.messageID.rawValue, Data(messageID.utf8), into: &inner)
GroupTLV.put(InnerField.senderSigningKey.rawValue, senderSigningKey, into: &inner)
GroupTLV.put(InnerField.senderNickname.rawValue, Data(senderNickname.utf8), into: &inner)
GroupTLV.put(InnerField.timestamp.rawValue, GroupTLV.timestampData(timestampMs), into: &inner)
GroupTLV.put(InnerField.content.rawValue, Data(content.utf8), into: &inner)
GroupTLV.put(InnerField.signature.rawValue, signature, into: &inner)
try GroupTLV.put(InnerField.messageID.rawValue, Data(messageID.utf8), into: &inner)
try GroupTLV.put(InnerField.senderSigningKey.rawValue, senderSigningKey, into: &inner)
try GroupTLV.put(InnerField.senderNickname.rawValue, Data(senderNickname.utf8), into: &inner)
try GroupTLV.put(InnerField.timestamp.rawValue, GroupTLV.timestampData(timestampMs), into: &inner)
try GroupTLV.put(InnerField.content.rawValue, Data(content.utf8), into: &inner)
try GroupTLV.put(InnerField.signature.rawValue, signature, into: &inner)
do {
let symmetricKey = SymmetricKey(data: key)
@@ -453,7 +489,7 @@ enum GroupCrypto {
nonce: Data(sealed.nonce),
ciphertext: ciphertext
)
return envelope.encode()
return try envelope.encode()
} catch {
throw GroupCryptoError.sealFailed
}
@@ -513,6 +549,7 @@ enum GroupCrypto {
let signingContent = messageSigningContent(
groupID: envelope.groupID,
epoch: envelope.epoch,
messageID: messageID,
timestampMs: timestampMs,
content: content
+38 -2
View File
@@ -35,6 +35,8 @@ protocol ChatGroupContext: AnyObject {
func cryptoIdentity(for peerID: PeerID) -> (fingerprint: String, signingKey: Data)?
/// The connected short peer ID whose fingerprint matches, if any.
func connectedPeerID(forFingerprint fingerprint: String) -> PeerID?
/// Whether the user has blocked the identity with this Noise fingerprint.
func isFingerprintBlocked(_ fingerprint: String) -> Bool
// MARK: Transport
func sendGroupInvitePayload(_ payload: Data, to peerID: PeerID)
@@ -99,6 +101,10 @@ extension ChatViewModel: ChatGroupContext {
return meshService.isPeerConnected(shortID) ? shortID : nil
}
func isFingerprintBlocked(_ fingerprint: String) -> Bool {
identityManager.isBlocked(fingerprint: fingerprint)
}
func sendGroupInvitePayload(_ payload: Data, to peerID: PeerID) {
meshService.sendGroupInvite(payload, to: peerID)
}
@@ -230,9 +236,12 @@ final class ChatGroupCoordinator {
signingKey: identity.signingKey,
nickname: context.peerNickname(for: peerID) ?? nickname
)
// Rotate the key (epoch + 1) on every roster change, not just removals.
// A monotonically increasing epoch per roster gives the receiver a
// strict ordering: two out-of-order invite states can no longer share
// an epoch and last-writer-wins a just-added member back out.
let members = group.members + [newMember]
guard let updated = context.groupStore.updateRoster(groupID: group.groupID, members: members),
let key = context.groupStore.key(forGroupID: group.groupID),
guard let (updated, key) = context.groupStore.rotateKey(groupID: group.groupID, members: members),
let payload = signedStatePayload(for: updated, key: key) else {
return .error(message: String(localized: "system.group.invite_failed", comment: "Error when building or signing a group invite fails"))
}
@@ -280,6 +289,7 @@ final class ChatGroupCoordinator {
}
distributeState(payload, group: rotated, excluding: [], type: .keyUpdate)
notifyRemovedMember(member, rotated: rotated)
return .success(message: String(
format: String(localized: "system.group.removed_member", comment: "System message after removing a member and rotating the key; placeholder is the nickname"),
@@ -402,6 +412,12 @@ final class ChatGroupCoordinator {
}
// Our own broadcast echoed back via relay or sync replay.
guard plaintext.senderSigningKey != context.mySigningPublicKey() else { return }
// Honor /block inside groups too: drop display + notification for a
// blocked member, consistent with every other inbound path.
guard !context.isFingerprintBlocked(member.fingerprint) else {
SecureLogger.debug("Dropping group message from blocked member", category: .security)
return
}
let groupPeerID = group.peerID
// Trust the authenticated inner timestamp (clamped so a future-dated
@@ -493,6 +509,26 @@ private extension ChatGroupCoordinator {
}
}
/// Tells a just-removed member they're out so their client can deactivate
/// the group instead of silently going dark (dropping every message under
/// the epoch it no longer has the key for). The notice is a creator-signed
/// state whose roster excludes the removee their `applyGroupState`
/// removal branch fires on the missing-self roster and surfaces the
/// "removed from group" system message.
///
/// It carries a throwaway all-zero key, never the rotated key, so the
/// removee cannot decrypt post-removal traffic. State is sent 1:1 over
/// authenticated Noise, so no remaining member ever receives this blob
/// (and even if one did, its own missing-self check would not match).
/// If the removee is offline the notice can't be delivered same v1
/// limitation as any other missed key update, documented in the PR.
func notifyRemovedMember(_ removed: GroupMember, rotated: BitchatGroup) {
guard let peerID = context.connectedPeerID(forFingerprint: removed.fingerprint) else { return }
let throwawayKey = Data(count: BitchatGroup.keyLength)
guard let payload = signedStatePayload(for: rotated, key: throwawayKey) else { return }
context.sendGroupKeyUpdatePayload(payload, to: peerID)
}
func applyGroupState(from peerID: PeerID, payload: Data, isInvite: Bool) {
guard let state = GroupStatePayload.decode(payload) else {
SecureLogger.warning("Malformed group state payload from \(peerID.id.prefix(8))", category: .security)
@@ -185,6 +185,13 @@ final class ChatLifecycleCoordinator {
func markPrivateMessagesAsRead(from peerID: PeerID) {
context.markChatAsRead(from: peerID)
// Group chats are keyed under a virtual group_ peerID; no member IS the
// conversation peer, so the receipt loops below (which gate on
// senderPeerID == peerID) must never emit a read/delivered receipt for
// one. This guard makes that explicit so a future refactor of the
// receipt matching can't silently start leaking receipts into groups.
guard !peerID.isGroup else { return }
if peerID.isGeoDM,
let recipientHex = context.nostrKeyMapping[peerID],
case .location(let channel) = context.activeChannel,
+3 -1
View File
@@ -102,7 +102,9 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, TransportEventDele
@MainActor
var canSendMediaInCurrentContext: Bool {
if let peer = selectedPrivateChatPeer {
return !(peer.isGeoDM || peer.isGeoChat)
// Media transfer is not wired for groups in v1 (sendFilePrivate
// rejects the virtual group_ recipient), so keep the affordance off.
return !(peer.isGeoDM || peer.isGeoChat || peer.isGroup)
}
switch activeChannel {
case .mesh: return true
+73 -1
View File
@@ -142,7 +142,7 @@ struct GroupProtocolTests {
Issue.record("roster should encode")
return
}
let content = GroupStatePayload.signingContent(groupID: groupID, epoch: 1, key: key, rosterBlob: rosterBlob)
let content = GroupStatePayload.signingContent(groupID: groupID, epoch: 1, key: key, rosterBlob: rosterBlob, name: group.name)
let payload = GroupStatePayload(
groupID: groupID,
name: group.name,
@@ -299,4 +299,76 @@ struct GroupProtocolTests {
#expect(GroupMessageEnvelope.decode(Data([0x01, 0x00])) == nil)
#expect(GroupStatePayload.decode(Data([0xFF, 0x00, 0x01])) == nil)
}
// MARK: - Oversize / UTF-8 safety (Codex findings)
@Test func oversizeMessageContentFailsToSealInsteadOfTruncating() {
// A content whose UTF-8 exceeds the 16-bit TLV length must fail to
// seal (surfacing send_failed) rather than silently truncate into a
// ciphertext recipients would drop.
let oversize = String(repeating: "a", count: 70_000)
#expect(throws: (any Error).self) {
_ = try GroupCrypto.sealMessage(
content: oversize,
messageID: "big",
senderNickname: "alice",
senderSigningKey: member.member.signingKey,
timestampMs: 1,
groupID: groupID,
epoch: 1,
key: key,
sign: member.sign
)
}
}
@Test func multiByteNicknameTruncatesOnScalarBoundary() throws {
// 40 euro signs = 120 UTF-8 bytes; a raw 64-byte prefix would split
// the 21st scalar and make the roster undecodable. Truncation must
// land on a Character boundary so the blob round-trips.
let euros = String(repeating: "", count: 40)
let wide = GroupMember(
fingerprint: creator.fingerprint,
signingKey: creator.member.signingKey,
nickname: euros
)
let blob = try #require(GroupRosterCoding.encode([wide]))
let decoded = try #require(GroupRosterCoding.decode(blob))
#expect(decoded.count == 1)
#expect(Data(decoded[0].nickname.utf8).count <= 64)
#expect(decoded[0].nickname.allSatisfy { $0 == "" })
#expect(!decoded[0].nickname.isEmpty)
}
// MARK: - Signable-bytes forward-proofing
@Test func creatorSignatureCoversName() throws {
let group = makeGroup()
let payload = try #require(GroupStatePayload.makeSigned(group: group, key: key, sign: creator.sign))
#expect(payload.verifyCreatorSignature())
// Swapping only the display name must invalidate the creator signature.
let renamed = GroupStatePayload(
groupID: payload.groupID,
name: "totally different name",
key: payload.key,
epoch: payload.epoch,
members: payload.members,
creatorFingerprint: payload.creatorFingerprint,
signature: payload.signature
)
#expect(!renamed.verifyCreatorSignature())
}
@Test func messageSignatureCoversEpoch() {
// The signed bytes differ by epoch, so a signature captured at one
// epoch cannot verify when re-sealed under a later epoch key.
let atEpoch1 = GroupCrypto.messageSigningContent(
groupID: groupID, epoch: 1, messageID: "m", timestampMs: 1, content: "x"
)
let atEpoch2 = GroupCrypto.messageSigningContent(
groupID: groupID, epoch: 2, messageID: "m", timestampMs: 1, content: "x"
)
#expect(atEpoch1 != atEpoch2)
}
}