From 151b68a497802d5b517e59eed04542e7f4dbfe0d Mon Sep 17 00:00:00 2001 From: jack Date: Sun, 4 Jan 2026 13:21:01 -1000 Subject: [PATCH 1/2] fix: clear DH shared secrets after Noise handshake operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added secureClear() calls for all 6 DH operations in NoiseProtocol.swift to properly clear sensitive shared secrets from memory after use: - writeMessage(): .es initiator/responder, .se initiator/responder - performDHOperation(): .ee and .ss operations This fixes a forward secrecy vulnerability where shared secrets could persist in memory after handshake completion. Also adds: - TrackingMockKeychain to count secureClear calls in tests - 3 new tests verifying secureClear is called during handshake 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- bitchat/Noise/NoiseProtocol.swift | 34 ++++- bitchatTests/Mocks/MockKeychain.swift | 87 +++++++++++ bitchatTests/Noise/NoiseProtocolTests.swift | 155 ++++++++++++++++++++ 3 files changed, 268 insertions(+), 8 deletions(-) diff --git a/bitchat/Noise/NoiseProtocol.swift b/bitchat/Noise/NoiseProtocol.swift index 021dc9fc..e36745d9 100644 --- a/bitchat/Noise/NoiseProtocol.swift +++ b/bitchat/Noise/NoiseProtocol.swift @@ -610,14 +610,20 @@ final class NoiseHandshakeState { throw NoiseError.missingKeys } let shared = try localEphemeral.sharedSecretFromKeyAgreement(with: remoteStatic) - symmetricState.mixKey(shared.withUnsafeBytes { Data($0) }) + var sharedData = shared.withUnsafeBytes { Data($0) } + symmetricState.mixKey(sharedData) + // Clear sensitive shared secret + keychain.secureClear(&sharedData) } else { guard let localStatic = localStaticPrivate, let remoteEphemeral = remoteEphemeralPublic else { throw NoiseError.missingKeys } let shared = try localStatic.sharedSecretFromKeyAgreement(with: remoteEphemeral) - symmetricState.mixKey(shared.withUnsafeBytes { Data($0) }) + var sharedData = shared.withUnsafeBytes { Data($0) } + symmetricState.mixKey(sharedData) + // Clear sensitive shared secret + keychain.secureClear(&sharedData) } case .se: @@ -628,14 +634,20 @@ final class NoiseHandshakeState { throw NoiseError.missingKeys } let shared = try localStatic.sharedSecretFromKeyAgreement(with: remoteEphemeral) - symmetricState.mixKey(shared.withUnsafeBytes { Data($0) }) + var sharedData = shared.withUnsafeBytes { Data($0) } + symmetricState.mixKey(sharedData) + // Clear sensitive shared secret + keychain.secureClear(&sharedData) } else { guard let localEphemeral = localEphemeralPrivate, let remoteStatic = remoteStaticPublic else { throw NoiseError.missingKeys } let shared = try localEphemeral.sharedSecretFromKeyAgreement(with: remoteStatic) - symmetricState.mixKey(shared.withUnsafeBytes { Data($0) }) + var sharedData = shared.withUnsafeBytes { Data($0) } + symmetricState.mixKey(sharedData) + // Clear sensitive shared secret + keychain.secureClear(&sharedData) } case .ss: @@ -724,8 +736,11 @@ final class NoiseHandshakeState { throw NoiseError.missingKeys } let shared = try localEphemeral.sharedSecretFromKeyAgreement(with: remoteEphemeral) - symmetricState.mixKey(shared.withUnsafeBytes { Data($0) }) - + var sharedData = shared.withUnsafeBytes { Data($0) } + symmetricState.mixKey(sharedData) + // Clear sensitive shared secret + keychain.secureClear(&sharedData) + case .es: if role == .initiator { guard let localEphemeral = localEphemeralPrivate, @@ -778,8 +793,11 @@ final class NoiseHandshakeState { throw NoiseError.missingKeys } let shared = try localStatic.sharedSecretFromKeyAgreement(with: remoteStatic) - symmetricState.mixKey(shared.withUnsafeBytes { Data($0) }) - + var sharedData = shared.withUnsafeBytes { Data($0) } + symmetricState.mixKey(sharedData) + // Clear sensitive shared secret + keychain.secureClear(&sharedData) + case .e, .s: break } diff --git a/bitchatTests/Mocks/MockKeychain.swift b/bitchatTests/Mocks/MockKeychain.swift index c3ffa9d3..9f6673f5 100644 --- a/bitchatTests/Mocks/MockKeychain.swift +++ b/bitchatTests/Mocks/MockKeychain.swift @@ -65,3 +65,90 @@ final class MockKeychain: KeychainManagerProtocol { /// Typealias for backwards compatibility with tests using MockKeychainHelper typealias MockKeychainHelper = MockKeychain + +/// Mock keychain that tracks secureClear calls for testing DH secret clearing +final class TrackingMockKeychain: KeychainManagerProtocol { + private var storage: [String: Data] = [:] + private var serviceStorage: [String: [String: Data]] = [:] + + /// Thread-safe counter for secureClear calls + private let lock = NSLock() + private var _secureClearDataCallCount = 0 + private var _secureClearStringCallCount = 0 + + var secureClearDataCallCount: Int { + lock.lock() + defer { lock.unlock() } + return _secureClearDataCallCount + } + + var secureClearStringCallCount: Int { + lock.lock() + defer { lock.unlock() } + return _secureClearStringCallCount + } + + var totalSecureClearCallCount: Int { + return secureClearDataCallCount + secureClearStringCallCount + } + + func resetCounts() { + lock.lock() + defer { lock.unlock() } + _secureClearDataCallCount = 0 + _secureClearStringCallCount = 0 + } + + func saveIdentityKey(_ keyData: Data, forKey key: String) -> Bool { + storage[key] = keyData + return true + } + + func getIdentityKey(forKey key: String) -> Data? { + storage[key] + } + + func deleteIdentityKey(forKey key: String) -> Bool { + storage.removeValue(forKey: key) + return true + } + + func deleteAllKeychainData() -> Bool { + storage.removeAll() + serviceStorage.removeAll() + return true + } + + func secureClear(_ data: inout Data) { + lock.lock() + _secureClearDataCallCount += 1 + lock.unlock() + data = Data() + } + + func secureClear(_ string: inout String) { + lock.lock() + _secureClearStringCallCount += 1 + lock.unlock() + string = "" + } + + func verifyIdentityKeyExists() -> Bool { + storage["identity_noiseStaticKey"] != nil + } + + func save(key: String, data: Data, service: String, accessible: CFString?) { + if serviceStorage[service] == nil { + serviceStorage[service] = [:] + } + serviceStorage[service]?[key] = data + } + + func load(key: String, service: String) -> Data? { + serviceStorage[service]?[key] + } + + func delete(key: String, service: String) { + serviceStorage[service]?.removeValue(forKey: key) + } +} diff --git a/bitchatTests/Noise/NoiseProtocolTests.swift b/bitchatTests/Noise/NoiseProtocolTests.swift index 6083e134..fea921ba 100644 --- a/bitchatTests/Noise/NoiseProtocolTests.swift +++ b/bitchatTests/Noise/NoiseProtocolTests.swift @@ -789,4 +789,159 @@ struct NoiseProtocolTests { "Message \(index + 1): Decrypted payload should match original") } } + + // MARK: - DH Shared Secret Clearing Tests + + @Test func secureClearCalledDuringHandshake() throws { + // Use TrackingMockKeychain to verify secureClear is called + let trackingKeychain = TrackingMockKeychain() + + let aliceKey = Curve25519.KeyAgreement.PrivateKey() + let bobKey = Curve25519.KeyAgreement.PrivateKey() + + let alice = NoiseSession( + peerID: PeerID(str: "alice-test"), + role: .initiator, + keychain: trackingKeychain, + localStaticKey: aliceKey + ) + + let bob = NoiseSession( + peerID: PeerID(str: "bob-test"), + role: .responder, + keychain: trackingKeychain, + localStaticKey: bobKey + ) + + // Perform handshake + let msg1 = try alice.startHandshake() + let msg2 = try bob.processHandshakeMessage(msg1)! + let msg3 = try alice.processHandshakeMessage(msg2)! + _ = try bob.processHandshakeMessage(msg3) + + // In Noise XX pattern handshake: + // - Message 1 (initiator): e token only (no DH) + // - Message 2 (responder): e, ee, s, es tokens (2 DH operations: ee, es) + // - Message 3 (initiator): s, se tokens (1 DH operation: se) + // Total in writeMessage: 3 DH operations (ee, es, se) + // + // In readMessage (performDHOperation): + // - After msg1: no DH + // - After msg2: ee, es (2 DH operations) + // - After msg3: se (1 DH operation) + // Total in performDHOperation: 3 DH operations + // + // Grand total: 6 DH operations requiring secureClear + // + // Note: .ss pattern is only used in certain handshake patterns, not XX + let expectedMinimumCalls = 6 + #expect( + trackingKeychain.secureClearDataCallCount >= expectedMinimumCalls, + "Expected at least \(expectedMinimumCalls) secureClear calls for DH secrets, got \(trackingKeychain.secureClearDataCallCount)" + ) + } + + @Test func encryptionWorksAfterSecureClear() throws { + // Verify that encryption/decryption still works correctly after adding secureClear + let trackingKeychain = TrackingMockKeychain() + + let aliceKey = Curve25519.KeyAgreement.PrivateKey() + let bobKey = Curve25519.KeyAgreement.PrivateKey() + + let alice = NoiseSession( + peerID: PeerID(str: "alice-test-enc"), + role: .initiator, + keychain: trackingKeychain, + localStaticKey: aliceKey + ) + + let bob = NoiseSession( + peerID: PeerID(str: "bob-test-enc"), + role: .responder, + keychain: trackingKeychain, + localStaticKey: bobKey + ) + + // Perform handshake + let msg1 = try alice.startHandshake() + let msg2 = try bob.processHandshakeMessage(msg1)! + let msg3 = try alice.processHandshakeMessage(msg2)! + _ = try bob.processHandshakeMessage(msg3) + + // Verify both sessions are established + #expect(alice.isEstablished()) + #expect(bob.isEstablished()) + + // Verify secureClear was called (basic sanity check) + #expect(trackingKeychain.secureClearDataCallCount > 0) + + // Test encryption from Alice to Bob + let plaintext1 = "Hello from Alice after secureClear!".data(using: .utf8)! + let ciphertext1 = try alice.encrypt(plaintext1) + let decrypted1 = try bob.decrypt(ciphertext1) + #expect(decrypted1 == plaintext1) + + // Test encryption from Bob to Alice + let plaintext2 = "Hello from Bob after secureClear!".data(using: .utf8)! + let ciphertext2 = try bob.encrypt(plaintext2) + let decrypted2 = try alice.decrypt(ciphertext2) + #expect(decrypted2 == plaintext2) + + // Test multiple messages to verify cipher state is correct + for i in 1...10 { + let msg = "Message \(i) from Alice".data(using: .utf8)! + let cipher = try alice.encrypt(msg) + let dec = try bob.decrypt(cipher) + #expect(dec == msg) + } + } + + @Test func secureClearCalledInBothWriteAndReadPaths() throws { + // Verify secureClear is called in both writeMessage and readMessage paths + // We do this by checking the count increases at each step + + let aliceKeychain = TrackingMockKeychain() + let bobKeychain = TrackingMockKeychain() + + let aliceKey = Curve25519.KeyAgreement.PrivateKey() + let bobKey = Curve25519.KeyAgreement.PrivateKey() + + let alice = NoiseSession( + peerID: PeerID(str: "alice-paths"), + role: .initiator, + keychain: aliceKeychain, + localStaticKey: aliceKey + ) + + let bob = NoiseSession( + peerID: PeerID(str: "bob-paths"), + role: .responder, + keychain: bobKeychain, + localStaticKey: bobKey + ) + + // Message 1: Alice writes (e token only, no DH) + let msg1 = try alice.startHandshake() + let aliceCountAfterMsg1 = aliceKeychain.secureClearDataCallCount + // No DH in message 1 for initiator + #expect(aliceCountAfterMsg1 == 0, "No DH secrets in message 1 write") + + // Bob reads message 1 (no DH) and writes message 2 (ee, es DH operations) + let msg2 = try bob.processHandshakeMessage(msg1)! + let bobCountAfterMsg2 = bobKeychain.secureClearDataCallCount + // Bob should have cleared secrets for: ee (read), es (read), ee (write), es (write) + #expect(bobCountAfterMsg2 >= 2, "Bob should clear DH secrets when processing/writing message 2") + + // Alice reads message 2 (ee, es) and writes message 3 (se) + let msg3 = try alice.processHandshakeMessage(msg2)! + let aliceCountAfterMsg3 = aliceKeychain.secureClearDataCallCount + // Alice should have cleared: ee (read), es (read), se (write) + #expect(aliceCountAfterMsg3 >= 3, "Alice should clear DH secrets when processing/writing message 3") + + // Bob reads message 3 (se) + _ = try bob.processHandshakeMessage(msg3) + let bobFinalCount = bobKeychain.secureClearDataCallCount + // Bob should have additionally cleared: se (read) + #expect(bobFinalCount > bobCountAfterMsg2, "Bob should clear DH secrets when processing message 3") + } } From e887e04f40cbfdb4c44c1f48cba60ffda9dae6d0 Mon Sep 17 00:00:00 2001 From: jack Date: Sun, 4 Jan 2026 13:23:32 -1000 Subject: [PATCH 2/2] fix: add thread safety to NostrTransport read receipt queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Synchronized access to readQueue and isSendingReadAcks using the existing concurrent DispatchQueue with barrier flags: - sendReadReceipt(): wrap enqueue in barrier async - processReadQueueIfNeeded(): extract item within barrier context - scheduleNextReadAck(): wrap callback in barrier async This fixes race conditions where concurrent calls could corrupt the read queue or cause check-then-act bugs on isSendingReadAcks. Also adds thread safety tests: - concurrentReadReceiptEnqueue: 100 concurrent enqueue operations - readQueueProcessingUnderLoad: concurrent enqueue during processing - isPeerReachableThreadSafety: concurrent read access test 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- bitchat/Services/NostrTransport.swift | 23 ++-- .../Services/NostrTransportTests.swift | 108 ++++++++++++++++++ 2 files changed, 122 insertions(+), 9 deletions(-) create mode 100644 bitchatTests/Services/NostrTransportTests.swift diff --git a/bitchat/Services/NostrTransport.swift b/bitchat/Services/NostrTransport.swift index 294274bc..3e2dccdc 100644 --- a/bitchat/Services/NostrTransport.swift +++ b/bitchat/Services/NostrTransport.swift @@ -149,8 +149,11 @@ final class NostrTransport: Transport, @unchecked Sendable { func sendReadReceipt(_ receipt: ReadReceipt, to peerID: PeerID) { // Enqueue and process with throttling to avoid relay rate limits - readQueue.append(QueuedRead(receipt: receipt, peerID: peerID)) - processReadQueueIfNeeded() + // Use barrier to synchronize access to readQueue + queue.async(flags: .barrier) { [weak self] in + self?.readQueue.append(QueuedRead(receipt: receipt, peerID: peerID)) + self?.processReadQueueIfNeeded() + } } func sendFavoriteNotification(to peerID: PeerID, isFavorite: Bool) { @@ -260,16 +263,17 @@ extension NostrTransport { // MARK: - Private Helpers extension NostrTransport { + /// Must be called within a barrier on `queue` private func processReadQueueIfNeeded() { guard !isSendingReadAcks else { return } guard !readQueue.isEmpty else { return } isSendingReadAcks = true - sendNextReadAck() + let item = readQueue.removeFirst() + sendReadAckItem(item) } - private func sendNextReadAck() { - guard !readQueue.isEmpty else { isSendingReadAcks = false; return } - let item = readQueue.removeFirst() + /// Sends a single read ack item (called after extraction from queue within barrier) + private func sendReadAckItem(_ item: QueuedRead) { Task { @MainActor in guard let recipientNpub = resolveRecipientNpub(for: item.peerID) else { scheduleNextReadAck(); return } guard let senderIdentity = try? idBridge.getCurrentNostrIdentity() else { scheduleNextReadAck(); return } @@ -301,9 +305,10 @@ extension NostrTransport { private func scheduleNextReadAck() { DispatchQueue.main.asyncAfter(deadline: .now() + readAckInterval) { [weak self] in - guard let self = self else { return } - self.isSendingReadAcks = false - self.processReadQueueIfNeeded() + self?.queue.async(flags: .barrier) { [weak self] in + self?.isSendingReadAcks = false + self?.processReadQueueIfNeeded() + } } } diff --git a/bitchatTests/Services/NostrTransportTests.swift b/bitchatTests/Services/NostrTransportTests.swift new file mode 100644 index 00000000..ce92228d --- /dev/null +++ b/bitchatTests/Services/NostrTransportTests.swift @@ -0,0 +1,108 @@ +// +// NostrTransportTests.swift +// bitchat +// +// This is free and unencumbered software released into the public domain. +// For more information, see +// + +import Foundation +import Testing +@testable import bitchat + +@Suite("NostrTransport Thread Safety Tests") +struct NostrTransportTests { + + @Test("Concurrent read receipt enqueue does not crash") + @MainActor + func concurrentReadReceiptEnqueue() async throws { + let keychain = MockKeychain() + let idBridge = NostrIdentityBridge(keychain: keychain) + let transport = NostrTransport(keychain: keychain, idBridge: idBridge) + + // Create 100 concurrent read receipt submissions + let iterations = 100 + + await withTaskGroup(of: Void.self) { group in + for i in 0..