Merge branch 'main' into fix/deduplication-service-thread-safety

2026-07-25 01:45:20 +00:00 · 2026-01-04 13:42:17 -10:00
parent 6cc3a8cde7 b4a3ee5777
commit 507cb19b91
5 changed files with 390 additions and 17 deletions
@@ -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
        }
@@ -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()
+            }
        }
    }

@@ -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)
+    }
+}
@@ -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")
+    }
 }
@@ -0,0 +1,108 @@
+//
+// NostrTransportTests.swift
+// bitchat
+//
+// This is free and unencumbered software released into the public domain.
+// For more information, see <https://unlicense.org>
+//
+
+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..<iterations {
+                group.addTask {
+                    let receipt = ReadReceipt(
+                        originalMessageID: UUID().uuidString,
+                        readerID: PeerID(str: String(format: "%016x", i)),
+                        readerNickname: "Reader\(i)"
+                    )
+                    let peerID = PeerID(str: String(format: "%016x", i))
+                    transport.sendReadReceipt(receipt, to: peerID)
+                }
+            }
+        }
+
+        // If we reach here without crashing, the test passes
+        // The concurrent enqueue operations completed without data races
+    }
+
+    @Test("Read queue processes under concurrent load")
+    @MainActor
+    func readQueueProcessingUnderLoad() async throws {
+        let keychain = MockKeychain()
+        let idBridge = NostrIdentityBridge(keychain: keychain)
+        let transport = NostrTransport(keychain: keychain, idBridge: idBridge)
+
+        // Rapidly enqueue many receipts from multiple concurrent sources
+        let iterations = 50
+
+        // First batch - rapid fire
+        for i in 0..<iterations {
+            let receipt = ReadReceipt(
+                originalMessageID: UUID().uuidString,
+                readerID: PeerID(str: String(format: "%016x", i)),
+                readerNickname: "Reader\(i)"
+            )
+            transport.sendReadReceipt(receipt, to: PeerID(str: String(format: "%016x", i)))
+        }
+
+        // Give some time for processing to start
+        try await Task.sleep(nanoseconds: 100_000_000) // 100ms
+
+        // Second batch - while first might be processing
+        await withTaskGroup(of: Void.self) { group in
+            for i in iterations..<(iterations * 2) {
+                group.addTask {
+                    let receipt = ReadReceipt(
+                        originalMessageID: UUID().uuidString,
+                        readerID: PeerID(str: String(format: "%016x", i)),
+                        readerNickname: "Reader\(i)"
+                    )
+                    transport.sendReadReceipt(receipt, to: PeerID(str: String(format: "%016x", i)))
+                }
+            }
+        }
+
+        // If we reach here without crashing or deadlocking, test passes
+    }
+
+    @Test("isPeerReachable is thread safe")
+    @MainActor
+    func isPeerReachableThreadSafety() async throws {
+        let keychain = MockKeychain()
+        let idBridge = NostrIdentityBridge(keychain: keychain)
+        let transport = NostrTransport(keychain: keychain, idBridge: idBridge)
+
+        let iterations = 100
+
+        // Concurrent reads on isPeerReachable
+        await withTaskGroup(of: Bool.self) { group in
+            for i in 0..<iterations {
+                group.addTask {
+                    let peerID = PeerID(str: String(format: "%016x", i))
+                    return transport.isPeerReachable(peerID)
+                }
+            }
+
+            // Collect results (all should be false since no favorites configured)
+            for await result in group {
+                #expect(result == false)
+            }
+        }
+    }
+}