permissionlesstech/bitchat
1
0
Fork 0
mirror of https://github.com/permissionlesstech/bitchat.git synced 2026-07-24 22:45:19 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix security audit findings: 3 critical, 7 high

Browse Source 
A broad audit surfaced ten critical/high issues across the crypto,
transport, identity, and panic-wipe layers. This fixes all ten.

Critical:
- Nostr DMs were unauthenticated. The NIP-17 seal was signed with a
  throwaway ephemeral key and the receiver never verified it, so anyone
  who knows a recipient's npub could forge messages (and delivery/read
  receipts) into an existing trusted conversation. The seal is now
  signed with the sender's real identity key, and the receiver verifies
  the seal signature and that seal.pubkey == rumor.pubkey.
  NOTE: this is a breaking wire-protocol change (see PR).
- Public BLE messages trusted registry membership instead of the packet
  signature. Since senderID is attacker-controlled, any verified peer
  could be impersonated in public chat. A valid signature from the
  claimed sender is now required before any registry identity is used.
- Unverified announces still persisted the announced identity, letting a
  replayed noisePublicKey overwrite a victim's stored signing key and
  nickname. persistIdentity is now gated on verification.

High:
- Noise decrypt trapped on a 16-19 byte ciphertext (negative prefix
  length after nonce extraction) — a remote crash. Now validated.
- Identity-cache debounce save used Timer.scheduledTimer on a GCD queue
  with no run loop, so it never fired; block/verify/favorite changes
  only persisted on explicit forceSave. Replaced with a
  DispatchSourceTimer on the queue; forceSave is now serialized.
- Identity-cache key load couldn't tell "missing" from a transient
  keychain failure and would regenerate (deleting) the key, orphaning
  the cache. Now uses getIdentityKeyWithResult and falls back to a
  session-only ephemeral key without clobbering the persisted key/cache.
- BLE receive-dedup key lacked a payload digest, so post-handshake
  flushes (queued msgs + delivery/read acks in the same ms) were dropped
  as duplicates. Digest added, matching the ingress registry.
- Maintenance timer was created only in init and never recreated after a
  panic stop/start, silently degrading the mesh until app restart. Now
  recreated in startServices.
- Panic wipe left persisted location state (selected channel, teleport
  set, bookmarks) and cached per-geohash Nostr private keys behind. Both
  are now cleared.
- Panic spawned an orphan NostrRelayManager instead of reusing .shared,
  splitting relay state from every other component. Now reuses .shared.

Tests updated to assert the fixed behavior (announce no longer persists
unverified identities; public messages require a signature; receive
dedup ID includes the payload digest).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
jack
2026-06-12 14:07:28 +02:00
co-authored by Claude Fable 5
parent fdf28aa5bb
commit ca63893197
13 changed files with 223 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files
bitchat/Identity/SecureIdentityStateManager.swift
+71 -21
View File
@@ -151,39 +151,70 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol {
// Thread safety
private let queue = DispatchQueue(label: "bitchat.identity.state", attributes: .concurrent)
// Debouncing for keychain saves
private var saveTimer: Timer?
// Debouncing for keychain saves.
// A DispatchSourceTimer on `queue` is used rather than Timer.scheduledTimer:
// saves are scheduled from inside `queue.async(flags: .barrier)` blocks that
// run on GCD worker threads, which have no active run loop, so a
// Timer.scheduledTimer there would never fire.
private var saveTimer: DispatchSourceTimer?
private let saveDebounceInterval: TimeInterval = 2.0 // Save at most once every 2 seconds
private var pendingSave = false
// Encryption key
private let encryptionKey: SymmetricKey
/// True when `encryptionKey` is a throwaway generated this session because the
/// persisted key could not be read (device locked / access denied). In that
/// state we must NOT persist (it would overwrite the real cache with data the
/// next launch can't decrypt) and must NOT delete the existing cache.
private let encryptionKeyIsEphemeral: Bool
init(_ keychain: KeychainManagerProtocol) {
self.keychain = keychain
// Generate or retrieve encryption key from keychain
// Retrieve (or, only on genuine first run, generate) the cache
// encryption key. We MUST distinguish "key doesn't exist yet" from a
// transient failure (device locked / access denied): the legacy
// getIdentityKey(forKey:) collapses both to nil, and generating+saving a
// new key deletes the existing one first — permanently orphaning the
// encrypted cache on a launch that merely couldn't read the key.
let loadedKey: SymmetricKey
let keyIsEphemeral: Bool
// Try to load from keychain
if let keyData = keychain.getIdentityKey(forKey: encryptionKeyName) {
switch keychain.getIdentityKeyWithResult(forKey: encryptionKeyName) {
case .success(let keyData):
loadedKey = SymmetricKey(data: keyData)
keyIsEphemeral = false
SecureLogger.logKeyOperation(.load, keyType: "identity cache encryption key", success: true)
}
// Generate new key if needed
else {
loadedKey = SymmetricKey(size: .bits256)
let keyData = loadedKey.withUnsafeBytes { Data($0) }
// Save to keychain
case .itemNotFound:
// Genuine first run: generate and persist a new key.
let newKey = SymmetricKey(size: .bits256)
let keyData = newKey.withUnsafeBytes { Data($0) }
let saved = keychain.saveIdentityKey(keyData, forKey: encryptionKeyName)
loadedKey = newKey
// If even the save failed, treat the key as ephemeral so we don't
// later try to persist a cache the next launch can't read.
keyIsEphemeral = !saved
SecureLogger.logKeyOperation(.generate, keyType: "identity cache encryption key", success: saved)
case .deviceLocked, .authenticationFailed, .accessDenied, .otherError:
// Transient/critical read failure. Do NOT overwrite the persisted
// key. Use a session-only ephemeral key; the real key and cache are
// left intact for a healthy launch.
SecureLogger.warning("Identity cache key unavailable; using ephemeral key for this session (not persisting)", category: .security)
loadedKey = SymmetricKey(size: .bits256)
keyIsEphemeral = true
}
self.encryptionKey = loadedKey
self.encryptionKeyIsEphemeral = keyIsEphemeral
// Load identity cache on init
// Only read the persisted cache when we hold the real key; with an
// ephemeral key the decrypt would fail and discard the real cache.
if !keyIsEphemeral {
loadIdentityCache()
}
}
deinit {
forceSave()
@@ -211,23 +242,38 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol {
}
}
/// Schedules a debounced save. Always invoked on `queue` under a barrier, so
/// the timer/pendingSave bookkeeping is serialized with all other state.
private func saveIdentityCache() {
// Mark that we need to save
pendingSave = true
// Cancel any existing timer
saveTimer?.invalidate()
// Schedule a new save after the debounce interval
saveTimer = Timer.scheduledTimer(withTimeInterval: saveDebounceInterval, repeats: false) { [weak self] _ in
self?.performSave()
// Cancel any pending timer and schedule a fresh one on `queue`.
saveTimer?.cancel()
let timer = DispatchSource.makeTimerSource(queue: queue)
timer.schedule(deadline: .now() + saveDebounceInterval)
timer.setEventHandler { [weak self] in
guard let self else { return }
// Hop to a barrier so performSave reads/writes state exclusively.
self.queue.async(flags: .barrier) { self.performSave() }
}
saveTimer = timer
timer.resume()
}
/// Writes the cache to the keychain. Must run on `queue` with exclusive
/// (barrier) access.
private func performSave() {
guard pendingSave else { return }
pendingSave = false
// Never persist under an ephemeral key — it would overwrite the real
// cache with data the next launch cannot decrypt.
guard !encryptionKeyIsEphemeral else {
SecureLogger.debug("Skipping identity cache save (ephemeral key this session)", category: .security)
return
}
do {
let data = try JSONEncoder().encode(cache)
let sealedBox = try AES.GCM.seal(data, using: encryptionKey)
@@ -240,10 +286,14 @@ final class SecureIdentityStateManager: SecureIdentityStateManagerProtocol {
}
}
// Force immediate save (for app termination)
// Force immediate save (for app termination). Runs synchronously on `queue`
// so the write completes before the caller proceeds (e.g. app exit).
func forceSave() {
saveTimer?.invalidate()
performSave()
queue.sync(flags: .barrier) {
self.saveTimer?.cancel()
self.saveTimer = nil
self.performSave()
}
}
// MARK: - Social Identity Management
bitchat/Noise/NoiseProtocol.swift
+7
View File
@@ -322,6 +322,13 @@ final class NoiseCipherState {
throw NoiseError.replayDetected
}
// The 4-byte nonce prefix has been stripped, so the remaining bytes
// must still hold at least the 16-byte Poly1305 tag. The up-front
// `ciphertext.count >= 16` guard is not sufficient here (it counts
// the nonce), and `prefix(count - 16)` would trap on a short payload.
guard actualCiphertext.count >= 16 else {
throw NoiseError.invalidCiphertext
}
// Split ciphertext and tag
encryptedData = actualCiphertext.prefix(actualCiphertext.count - 16)
tag = actualCiphertext.suffix(16)
bitchat/Nostr/NostrIdentityBridge.swift
+7
View File
@@ -82,6 +82,13 @@ final class NostrIdentityBridge {
}
deviceSeedCache = nil
// Also drop the in-memory derived per-geohash identities. These hold the
// actual secp256k1 private keys; if left cached, post-panic geohash
// messages would still be signed with pre-panic keys (linkable across the
// wipe) until the app is force-quit.
cacheLock.lock()
derivedIdentityCache.removeAll()
cacheLock.unlock()
}
// MARK: - Per-Geohash Identities (Location Channels)
bitchat/Nostr/NostrProtocol.swift
+30 -13
View File
@@ -39,22 +39,23 @@ struct NostrProtocol {
content: content
)
// 2. Create ephemeral key for this message
let ephemeralKey = try P256K.Schnorr.PrivateKey()
// Created ephemeral key for seal
// 3. Seal the rumor (encrypt to recipient)
// 2. Seal the rumor (encrypt to recipient) and sign it with the SENDER'S
// real identity key. NIP-17 requires the seal be signed by the sender
// so the recipient can authenticate who sent the message; signing with
// a throwaway key leaves DMs forgeable/impersonatable.
let senderKey = try senderIdentity.schnorrSigningKey()
let sealedEvent = try createSeal(
rumor: rumor,
recipientPubkey: recipientPubkey,
senderKey: ephemeralKey
senderKey: senderKey
)
// 4. Gift wrap the sealed event (encrypt to recipient again)
// 3. Gift wrap the sealed event with a throwaway ephemeral key (the wrap
// layer hides the sender's identity from relays; createGiftWrap mints
// its own ephemeral key internally).
let giftWrap = try createGiftWrap(
seal: sealedEvent,
recipientPubkey: recipientPubkey,
senderKey: ephemeralKey
recipientPubkey: recipientPubkey
)
// Created gift wrap
@@ -84,7 +85,15 @@ struct NostrProtocol {
throw error
}
// 2. Open the seal
// 2. Authenticate the seal. The seal MUST be signed by the sender's real
// identity key (NIP-17); without this check a DM is forgeable by anyone
// who knows the recipient's npub. Verify the seal's own signature.
guard seal.isValidSignature() else {
SecureLogger.error("❌ Rejecting DM: seal signature is missing or invalid", category: .session)
throw NostrError.invalidEvent
}
// 3. Open the seal
let rumor: NostrEvent
do {
rumor = try openSeal(
@@ -97,7 +106,16 @@ struct NostrProtocol {
throw error
}
return (content: rumor.content, senderPubkey: rumor.pubkey, timestamp: rumor.created_at)
// 4. The sender claimed inside the rumor must match the key that actually
// signed the seal, otherwise the sender field is unauthenticated and
// spoofable.
guard seal.pubkey == rumor.pubkey else {
SecureLogger.error("❌ Rejecting DM: rumor pubkey does not match seal signer", category: .session)
throw NostrError.invalidEvent
}
// Return the seal signer's pubkey as the authenticated sender.
return (content: rumor.content, senderPubkey: seal.pubkey, timestamp: rumor.created_at)
}
/// Create a geohash-scoped ephemeral public message (kind 20000)
@@ -195,8 +213,7 @@ struct NostrProtocol {
private static func createGiftWrap(
seal: NostrEvent,
recipientPubkey: String,
senderKey: P256K.Schnorr.PrivateKey // This is the ephemeral key used for the seal
recipientPubkey: String
) throws -> NostrEvent {
let sealJSON = try seal.jsonString()
bitchat/Services/BLE/BLEAnnounceHandler.swift
+6 -1
View File
@@ -168,8 +168,13 @@ final class BLEAnnounceHandler {
env.updateTopology(peerID, neighbors)
}
// Persist cryptographic identity and signing key for robust offline verification
// Persist cryptographic identity and signing key for robust offline
// verification — only for verified announces. Persisting unverified
// announces would let an attacker who replays a victim's noisePublicKey
// overwrite the victim's stored signing key/nickname (identity poisoning).
if verifiedAnnounce {
env.persistIdentity(announcement)
}
let announceBackID = "announce-back-\(peerID)"
let shouldSendBack = !env.dedupContains(announceBackID)
bitchat/Services/BLE/BLEPublicMessageHandler.swift
+17 -2
View File
@@ -68,14 +68,29 @@ final class BLEPublicMessageHandler {
// Snapshot peers to avoid concurrent mutation while iterating during nickname collision checks.
let peersSnapshot = env.peersSnapshot()
// Public messages are always signed by their sender. `senderID` is
// attacker-controlled, so registry membership alone is NOT proof of
// identity — a peer in the registry as "verified" could be impersonated
// by anyone spoofing their senderID. Require a valid packet signature
// from the claimed sender (our own echoes are exempt; they are matched
// by self-broadcast tracking below).
let isSelf = peerID == env.localPeerID()
let signedDisplayName = isSelf ? nil : env.signedSenderDisplayName(packet, peerID)
guard isSelf || signedDisplayName != nil else {
SecureLogger.warning("🚫 Dropping public message with missing/invalid signature for claimed sender \(peerID.id.prefix(8))…", category: .security)
return
}
// Authenticity is established; prefer the registry's collision-resolved
// display name, then the signature-derived name.
guard let senderNickname = BLEPeerSenderDisplayName.resolveKnownPeer(
peerID: peerID,
localPeerID: env.localPeerID(),
localNickname: env.localNickname(),
peers: peersSnapshot,
allowConnectedUnverified: false
) ?? env.signedSenderDisplayName(packet, peerID) else {
SecureLogger.warning("🚫 Dropping public message from unverified or unknown peer \(peerID.id.prefix(8))…", category: .security)
) ?? signedDisplayName else {
SecureLogger.warning("🚫 Dropping public message from unknown peer \(peerID.id.prefix(8))…", category: .security)
return
}
bitchat/Services/BLE/BLEReceivePipeline.swift
+7 -1
View File
@@ -12,7 +12,13 @@ struct BLEReceivedPacketContext: Equatable {
struct BLEReceivePipeline {
static func context(for packet: BitchatPacket, localPeerID: PeerID) -> BLEReceivedPacketContext {
let senderID = PeerID(hexData: packet.senderID)
let messageID = "\(senderID)-\(packet.timestamp)-\(packet.type)"
// Include a payload digest so that distinct packets sharing the same
// sender/timestamp(ms)/type are not collapsed as duplicates. The
// post-handshake flush sends queued messages, delivery and read receipts
// back-to-back within a single millisecond; without the digest every
// packet after the first would be silently dropped.
let digestPrefix = packet.payload.sha256Hash().prefix(4).hexEncodedString()
let messageID = "\(senderID)-\(packet.timestamp)-\(packet.type)-\(digestPrefix)"
let messageType = MessageType(rawValue: packet.type)
let allowSelfSyncReplay = packet.ttl == 0 && senderID == localPeerID
let shouldDeduplicate = messageType != .fragment && !allowSelfSyncReplay
bitchat/Services/BLE/BLEService.swift
+23 -9
View File
@@ -234,15 +234,7 @@ final class BLEService: NSObject {
}
// Single maintenance timer for all periodic tasks (dispatch-based for determinism)
let timer = DispatchSource.makeTimerSource(queue: bleQueue)
timer.schedule(deadline: .now() + TransportConfig.bleMaintenanceInterval,
repeating: TransportConfig.bleMaintenanceInterval,
leeway: .seconds(TransportConfig.bleMaintenanceLeewaySeconds))
timer.setEventHandler { [weak self] in
self?.performMaintenance()
}
timer.resume()
maintenanceTimer = timer
startMaintenanceTimer()
// Publish initial empty state
requestPeerDataPublish()
@@ -435,7 +427,29 @@ final class BLEService: NSObject {
// MARK: Lifecycle
/// Creates and starts the periodic maintenance timer if it is not already
/// running. Idempotent so it can be called from both `init` and
/// `startServices()` — the latter matters after a panic reset, where
/// `stopServices()` cancels and nils the timer.
private func startMaintenanceTimer() {
guard maintenanceTimer == nil else { return }
let timer = DispatchSource.makeTimerSource(queue: bleQueue)
timer.schedule(deadline: .now() + TransportConfig.bleMaintenanceInterval,
repeating: TransportConfig.bleMaintenanceInterval,
leeway: .seconds(TransportConfig.bleMaintenanceLeewaySeconds))
timer.setEventHandler { [weak self] in
self?.performMaintenance()
}
timer.resume()
maintenanceTimer = timer
}
func startServices() {
// Restart the maintenance timer if a prior stopServices() cancelled it
// (e.g. the panic flow), otherwise periodic announces, peer reconciliation
// and cache cleanup would never resume until app restart.
startMaintenanceTimer()
// Start BLE services if not already running
if centralManager?.state == .poweredOn {
centralManager?.scanForPeripherals(
bitchat/Services/LocationStateManager.swift
+16
View File
@@ -594,6 +594,22 @@ final class LocationStateManager: NSObject, CLLocationManagerDelegate, Observabl
}
}
/// Removes all persisted location state and resets the in-memory view.
/// Used by the panic wipe — selected channel, teleport set and bookmarks
/// (which reveal where the user has been) must not survive on device.
func panicWipe() {
storage.removeObject(forKey: selectedChannelKey)
storage.removeObject(forKey: teleportedStoreKey)
storage.removeObject(forKey: bookmarksKey)
storage.removeObject(forKey: bookmarkNamesKey)
teleportedSet.removeAll()
bookmarkMembership.removeAll()
bookmarks = []
bookmarkNames = [:]
teleported = false
selectedChannel = .mesh
}
private static func normalizeGeohash(_ s: String) -> String {
let allowed = Set("0123456789bcdefghjkmnpqrstuvwxyz")
return s
bitchat/ViewModels/ChatViewModel.swift
+11 -2
View File
@@ -1129,6 +1129,11 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, TransportEventDele
userDefaults.removeObject(forKey: "bitchat.noiseIdentityKey")
userDefaults.removeObject(forKey: "bitchat.messageRetentionKey")
// Wipe persisted location state (selected channel, teleport set,
// bookmarks). For an activist-safety wipe, where the user has been is
// exactly the data an adversary inspecting the device wants.
LocationStateManager.shared.panicWipe()
// Reset nickname to anonymous
nickname = "anon\(Int.random(in: 1000...9999))"
saveNickname()
@@ -1180,8 +1185,12 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, TransportEventDele
// Small delay to ensure cleanup completes
try? await Task.sleep(nanoseconds: TransportConfig.uiAsyncShortSleepNs) // 0.1 seconds
// Reinitialize Nostr relay manager with new identity
nostrRelayManager = NostrRelayManager()
// Reinitialize Nostr relay manager with new identity. Reuse the
// shared singleton — every other component (NostrTransport, geohash
// subscriptions, AppRuntime observers) is bound to `.shared`, so
// creating a fresh instance here would split relay state and leave
// sends running against a disconnected manager.
nostrRelayManager = NostrRelayManager.shared
setupNostrMessageHandling()
nostrRelayManager?.connect()
}
bitchatTests/Services/BLEAnnounceHandlerTests.swift
+4 -1
View File
@@ -173,7 +173,10 @@ struct BLEAnnounceHandlerTests {
#expect(recorder.uiEventDeliveries.count == 1)
#expect(recorder.uiEventDeliveries.first?.notifyPeerConnected == false)
#expect(recorder.uiEventDeliveries.first?.scheduleInitialSync == false)
#expect(recorder.persistedIdentities.count == 1)
// Identity persistence MUST NOT occur for unverified announces:
// persisting would let an attacker who replays a victim's noisePublicKey
// overwrite the victim's stored signing key/nickname (identity poisoning).
#expect(recorder.persistedIdentities.isEmpty)
#expect(recorder.trackedPackets.count == 1)
#expect(recorder.announceBacks == 1)
}
bitchatTests/Services/BLEPublicMessageHandlerTests.swift
+7 -1
View File
@@ -59,13 +59,17 @@ struct BLEPublicMessageHandlerTests {
let now = Date(timeIntervalSince1970: 1_000)
let recorder = Recorder()
recorder.peers = [remotePeerID: makePeerInfo(remotePeerID, nickname: "Alice", isVerified: true)]
// A valid packet signature is required even for a registry-verified peer:
// senderID is spoofable, so registry membership alone is not authentication.
recorder.signedName = "SignedAlice"
let handler = makeHandler(recorder: recorder, now: now)
let packet = makeMessagePacket(sender: remotePeerID, content: "hello mesh", timestamp: timestamp(now))
handler.handle(packet, from: remotePeerID)
#expect(recorder.peersSnapshotReads == 1)
#expect(recorder.signedNameQueries.isEmpty)
// Signature is verified, then the registry's collision-resolved name is preferred.
#expect(recorder.signedNameQueries == [remotePeerID])
#expect(recorder.trackedPackets.count == 1)
#expect(recorder.selfBroadcastTakes.isEmpty)
#expect(recorder.deliveries.count == 1)
@@ -154,6 +158,7 @@ struct BLEPublicMessageHandlerTests {
let now = Date(timeIntervalSince1970: 1_000)
let recorder = Recorder()
recorder.peers = [remotePeerID: makePeerInfo(remotePeerID, nickname: "Alice", isVerified: true)]
recorder.signedName = "SignedAlice"
let handler = makeHandler(recorder: recorder, now: now)
let packet = makeMessagePacket(sender: remotePeerID, payload: Data([0xFF, 0xFE, 0xFD]), timestamp: timestamp(now))
@@ -187,6 +192,7 @@ struct BLEPublicMessageHandlerTests {
let now = Date(timeIntervalSince1970: 1_000)
let recorder = Recorder()
recorder.peers = [remotePeerID: makePeerInfo(remotePeerID, nickname: "Alice", isVerified: true)]
recorder.signedName = "SignedAlice"
let handler = makeHandler(recorder: recorder, now: now)
let packet = makeMessagePacket(
sender: remotePeerID,
bitchatTests/Services/BLEReceivePipelineTests.swift
+4 -1
View File
@@ -14,7 +14,10 @@ struct BLEReceivePipelineTests {
let context = BLEReceivePipeline.context(for: packet, localPeerID: local)
#expect(context.senderID == sender)
#expect(context.messageID == "\(sender)-1234-\(MessageType.message.rawValue)")
// The message ID includes a payload digest so distinct packets sharing a
// sender/timestamp(ms)/type are not collapsed as duplicates.
let digest = packet.payload.sha256Hash().prefix(4).hexEncodedString()
#expect(context.messageID == "\(sender)-1234-\(MessageType.message.rawValue)-\(digest)")
#expect(context.messageType == .message)
#expect(context.shouldDeduplicate)
#expect(context.logsHandlingDetails)