From 9c84d4ce4f8e67af44b4d6df3d2ed417da6c3a82 Mon Sep 17 00:00:00 2001 From: jack <212554440+jackjackbits@users.noreply.github.com> Date: Sat, 8 Aug 2026 12:12:27 +0200 Subject: [PATCH] Fail closed on unverifiable handshake peer IDs + check SecRandomCopyBytes results (#1645) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two hardening fixes from the repo evaluation: - NoiseSessionManager.authenticatedRemoteKey returned true for peer IDs that are neither 16-hex wire IDs nor full Noise-key IDs — an accept-any-key fallback kept for test harnesses. It now fails closed; the Noise/integration/E2E tests that relied on it address peers by key-derived wire IDs instead (the pattern NoiseCoverageTests already used), and a new regression test pins the rejection. - Four SecRandomCopyBytes call sites discarded the return status. The two verification nonces now fail their operation on error, the Nostr device identity seed uses CryptoKit key generation (cannot fail, and can no longer silently persist an all-zero seed), and BIP-340 aux randomness throws on failure like the adjacent nonce path. Co-authored-by: jack Co-authored-by: Claude Fable 5 --- bitchat/Noise/NoiseSessionManager.swift | 6 +++--- bitchat/Nostr/NostrIdentityBridge.swift | 7 +++---- bitchat/Nostr/NostrProtocol.swift | 5 ++++- bitchat/Services/VerificationService.swift | 3 ++- .../ChatVerificationCoordinator.swift | 3 ++- .../EndToEnd/PrivateChatE2ETests.swift | 19 ++++++++++------- .../Integration/IntegrationTests.swift | 14 +++++++------ .../Integration/LargeTopologyTests.swift | 2 +- .../Integration/TestNetworkHelper.swift | 10 +++++---- bitchatTests/Noise/NoiseCoverageTests.swift | 20 ++++++++++++++++++ bitchatTests/Noise/NoiseProtocolTests.swift | 21 ++++++++++++------- 11 files changed, 75 insertions(+), 35 deletions(-) diff --git a/bitchat/Noise/NoiseSessionManager.swift b/bitchat/Noise/NoiseSessionManager.swift index 6e2b3c23..c2aacbb0 100644 --- a/bitchat/Noise/NoiseSessionManager.swift +++ b/bitchat/Noise/NoiseSessionManager.swift @@ -1126,8 +1126,8 @@ final class NoiseSessionManager { /// Mesh handshakes normally use a 16-hex wire ID. Full Noise-key IDs are /// also accepted by internal callers when they exactly match the static - /// key. Non-wire identifiers remain available to protocol test harnesses; - /// BLE packet ingress always supplies a short hexadecimal ID. + /// key. Anything else fails closed: an identifier that can't be checked + /// against the remote static key must never complete a handshake. private func authenticatedRemoteKey( _ remoteKey: Curve25519.KeyAgreement.PublicKey, matches claimedPeerID: PeerID @@ -1139,7 +1139,7 @@ final class NoiseSessionManager { if let claimedNoiseKey = claimedPeerID.noiseKey { return claimedNoiseKey == rawKey } - return true + return false } // MARK: - Encryption/Decryption diff --git a/bitchat/Nostr/NostrIdentityBridge.swift b/bitchat/Nostr/NostrIdentityBridge.swift index 2e18a235..51d14e84 100644 --- a/bitchat/Nostr/NostrIdentityBridge.swift +++ b/bitchat/Nostr/NostrIdentityBridge.swift @@ -76,10 +76,9 @@ final class NostrIdentityBridge { deviceSeedCache = existing return existing } - var seed = Data(count: 32) - _ = seed.withUnsafeMutableBytes { ptr in - SecRandomCopyBytes(kSecRandomDefault, 32, ptr.baseAddress!) - } + // CryptoKit key generation cannot fail, unlike SecRandomCopyBytes — + // a discarded failure here would persist an all-zero identity seed. + let seed = SymmetricKey(size: .bits256).withUnsafeBytes { Data($0) } // Ensure availability after first unlock to prevent unintended rotation when locked keychain.save(key: deviceSeedKey, data: seed, service: keychainService, accessible: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly) deviceSeedCache = seed diff --git a/bitchat/Nostr/NostrProtocol.swift b/bitchat/Nostr/NostrProtocol.swift index bfef85e2..2e3e35d8 100644 --- a/bitchat/Nostr/NostrProtocol.swift +++ b/bitchat/Nostr/NostrProtocol.swift @@ -836,9 +836,12 @@ struct NostrEvent: Codable { // Sign with Schnorr (BIP-340) var messageBytes = [UInt8](eventIdHash) var auxRand = [UInt8](repeating: 0, count: 32) - _ = auxRand.withUnsafeMutableBytes { ptr in + let auxStatus = auxRand.withUnsafeMutableBytes { ptr in SecRandomCopyBytes(kSecRandomDefault, 32, ptr.baseAddress!) } + guard auxStatus == errSecSuccess else { + throw NostrError.cryptographicFailure + } let schnorrSignature = try key.signature(message: &messageBytes, auxiliaryRand: &auxRand) let signatureHex = schnorrSignature.dataRepresentation.hexEncodedString() diff --git a/bitchat/Services/VerificationService.swift b/bitchat/Services/VerificationService.swift index ae983cca..8aa46ed1 100644 --- a/bitchat/Services/VerificationService.swift +++ b/bitchat/Services/VerificationService.swift @@ -84,7 +84,8 @@ final class VerificationService { let signKey = transport.noiseSigningPublicKeyData().hexEncodedString() let ts = Int64(Date().timeIntervalSince1970) var nonce = Data(count: 16) - _ = nonce.withUnsafeMutableBytes { SecRandomCopyBytes(kSecRandomDefault, 16, $0.baseAddress!) } + let status = nonce.withUnsafeMutableBytes { SecRandomCopyBytes(kSecRandomDefault, 16, $0.baseAddress!) } + guard status == errSecSuccess else { return nil } let nonceB64 = nonce.base64EncodedString().replacingOccurrences(of: "+", with: "-").replacingOccurrences(of: "/", with: "_").replacingOccurrences(of: "=", with: "") let payload = VerificationQR(v: 1, noiseKeyHex: noiseKey, signKeyHex: signKey, npub: npub, nickname: nickname, ts: ts, nonceB64: nonceB64, sigHex: "") let msg = payload.canonicalBytes() diff --git a/bitchat/ViewModels/ChatVerificationCoordinator.swift b/bitchat/ViewModels/ChatVerificationCoordinator.swift index f6499988..42b39a94 100644 --- a/bitchat/ViewModels/ChatVerificationCoordinator.swift +++ b/bitchat/ViewModels/ChatVerificationCoordinator.swift @@ -294,7 +294,8 @@ final class ChatVerificationCoordinator { } var nonce = Data(count: 16) - _ = nonce.withUnsafeMutableBytes { SecRandomCopyBytes(kSecRandomDefault, 16, $0.baseAddress!) } + let status = nonce.withUnsafeMutableBytes { SecRandomCopyBytes(kSecRandomDefault, 16, $0.baseAddress!) } + guard status == errSecSuccess else { return false } var pending = PendingVerification( noiseKeyHex: qr.noiseKeyHex, signKeyHex: qr.signKeyHex, diff --git a/bitchatTests/EndToEnd/PrivateChatE2ETests.swift b/bitchatTests/EndToEnd/PrivateChatE2ETests.swift index dc8de7ab..d3bb2ab9 100644 --- a/bitchatTests/EndToEnd/PrivateChatE2ETests.swift +++ b/bitchatTests/EndToEnd/PrivateChatE2ETests.swift @@ -115,13 +115,18 @@ struct PrivateChatE2ETests { let aliceManager = NoiseSessionManager(localStaticKey: aliceKey, keychain: mockKeychain) let bobManager = NoiseSessionManager(localStaticKey: bobKey, keychain: mockKeychain) - + + // Manager sessions are keyed by key-derived wire IDs: handshake + // completion fails closed on IDs the remote key can't vouch for. + let aliceNoiseID = PeerID(publicKey: aliceKey.publicKey.rawRepresentation) + let bobNoiseID = PeerID(publicKey: bobKey.publicKey.rawRepresentation) + // Establish encrypted session do { - let handshake1 = try aliceManager.initiateHandshake(with: bob.peerID) - let handshake2 = try bobManager.handleIncomingHandshake(from: alice.peerID, message: handshake1)! - let handshake3 = try aliceManager.handleIncomingHandshake(from: bob.peerID, message: handshake2)! - _ = try bobManager.handleIncomingHandshake(from: alice.peerID, message: handshake3) + let handshake1 = try aliceManager.initiateHandshake(with: bobNoiseID) + let handshake2 = try bobManager.handleIncomingHandshake(from: aliceNoiseID, message: handshake1)! + let handshake3 = try aliceManager.handleIncomingHandshake(from: bobNoiseID, message: handshake2)! + _ = try bobManager.handleIncomingHandshake(from: aliceNoiseID, message: handshake3) } catch { Issue.record("Failed to establish Noise session: \(error)") } @@ -134,7 +139,7 @@ struct PrivateChatE2ETests { let message = BitchatMessage(packet.payload), message.isPrivate { do { - let encrypted = try aliceManager.encrypt(packet.payload, for: bob.peerID) + let encrypted = try aliceManager.encrypt(packet.payload, for: bobNoiseID) let encryptedPacket = BitchatPacket( type: 0x02, // Encrypted message type senderID: packet.senderID, @@ -155,7 +160,7 @@ struct PrivateChatE2ETests { // Decrypt incoming encrypted messages if packet.type == 0x02 { do { - let decrypted = try bobManager.decrypt(packet.payload, from: alice.peerID) + let decrypted = try bobManager.decrypt(packet.payload, from: aliceNoiseID) if let message = BitchatMessage(decrypted) { #expect(message.content == TestConstants.testMessage1) #expect(message.isPrivate) diff --git a/bitchatTests/Integration/IntegrationTests.swift b/bitchatTests/Integration/IntegrationTests.swift index e07f0fae..70311b49 100644 --- a/bitchatTests/Integration/IntegrationTests.swift +++ b/bitchatTests/Integration/IntegrationTests.swift @@ -18,10 +18,10 @@ struct IntegrationTests { private var helper = TestNetworkHelper() init() { - helper.createNode("Alice", peerID: PeerID(str: UUID().uuidString)) - helper.createNode("Bob", peerID: PeerID(str: UUID().uuidString)) - helper.createNode("Charlie", peerID: PeerID(str: UUID().uuidString)) - helper.createNode("David", peerID: PeerID(str: UUID().uuidString)) + helper.createNode("Alice") + helper.createNode("Bob") + helper.createNode("Charlie") + helper.createNode("David") } // MARK: - Multi-Peer Scenarios @@ -266,9 +266,11 @@ struct IntegrationTests { helper.nodes["Alice"]!.sendPrivateMessage("Before restart", to: helper.nodes["Bob"]!.peerID, recipientNickname: "Bob") } - // Simulate Bob restart by recreating his Noise manager + // Simulate Bob restart by recreating his Noise manager. A new static + // key means a new key-derived wire ID, just like production. let bobKey = Curve25519.KeyAgreement.PrivateKey() helper.noiseManagers["Bob"] = NoiseSessionManager(localStaticKey: bobKey, keychain: helper.mockKeychain) + helper.nodes["Bob"]!.myPeerID = PeerID(publicKey: bobKey.publicKey.rawRepresentation) // Re-establish Noise handshake explicitly via managers do { @@ -320,7 +322,7 @@ struct IntegrationTests { @Test func largeScaleNetwork() async throws { // Create larger network for i in 5...10 { - helper.createNode("Node\(i)", peerID: PeerID(str: "PEER\(i)")) + helper.createNode("Node\(i)") } // Connect in ring topology with cross-connections diff --git a/bitchatTests/Integration/LargeTopologyTests.swift b/bitchatTests/Integration/LargeTopologyTests.swift index 3f4c44dc..a87be438 100644 --- a/bitchatTests/Integration/LargeTopologyTests.swift +++ b/bitchatTests/Integration/LargeTopologyTests.swift @@ -37,7 +37,7 @@ struct LargeTopologyTests { private func makeNodes(_ names: [String]) { for name in names { - helper.createNode(name, peerID: PeerID(str: UUID().uuidString)) + helper.createNode(name) } } diff --git a/bitchatTests/Integration/TestNetworkHelper.swift b/bitchatTests/Integration/TestNetworkHelper.swift index 0131739c..e11d1917 100644 --- a/bitchatTests/Integration/TestNetworkHelper.swift +++ b/bitchatTests/Integration/TestNetworkHelper.swift @@ -22,15 +22,17 @@ final class TestNetworkHelper { // MARK: - Node/Manager management @discardableResult - func createNode(_ name: String, peerID: PeerID) -> MockBLEService { + func createNode(_ name: String) -> MockBLEService { let node = MockBLEService(bus: bus) - node.myPeerID = peerID + // Wire IDs must derive from the node's Noise static key: handshake + // completion fails closed on IDs the remote key can't vouch for. + let key = Curve25519.KeyAgreement.PrivateKey() + node.myPeerID = PeerID(publicKey: key.publicKey.rawRepresentation) node.mockNickname = name nodes[name] = node - + // This synchronous helper directly drives all three XX messages and // has no transport callback loop for delayed collision recovery. - let key = Curve25519.KeyAgreement.PrivateKey() noiseManagers[name] = NoiseSessionManager( localStaticKey: key, keychain: mockKeychain, diff --git a/bitchatTests/Noise/NoiseCoverageTests.swift b/bitchatTests/Noise/NoiseCoverageTests.swift index d083bd47..763f5816 100644 --- a/bitchatTests/Noise/NoiseCoverageTests.swift +++ b/bitchatTests/Noise/NoiseCoverageTests.swift @@ -578,6 +578,26 @@ struct NoiseCoverageTests { #expect(failingManager.getSession(for: charliePeerID) == nil) } + @Test("Handshake completion fails closed on non-wire peer IDs") + func handshakeCompletionRejectsNonWirePeerIDs() throws { + let aliceManager = NoiseSessionManager(localStaticKey: aliceStaticKey, keychain: keychain) + let bobManager = NoiseSessionManager(localStaticKey: bobStaticKey, keychain: keychain) + + // Alice addresses Bob by an identifier no static key can vouch for: + // neither a 16-hex wire ID nor a full Noise-key ID. Completion must + // reject it rather than accept any remote static key. + let nonWireID = PeerID(str: "not-a-wire-identifier") + let msg1 = try aliceManager.initiateHandshake(with: nonWireID) + let msg2 = try #require( + try bobManager.handleIncomingHandshake(from: bobPeerID, message: msg1) + ) + + #expect(throws: (any Error).self) { + try aliceManager.handleIncomingHandshake(from: nonWireID, message: msg2) + } + #expect(aliceManager.getSession(for: nonWireID)?.isEstablished() != true) + } + @Test("Session manager cleans up initiator sessions after start-handshake failures") func sessionManagerCleansUpInitiatorSessionsAfterStartHandshakeFailures() { let manager = NoiseSessionManager( diff --git a/bitchatTests/Noise/NoiseProtocolTests.swift b/bitchatTests/Noise/NoiseProtocolTests.swift index ac8ef93d..007bbef0 100644 --- a/bitchatTests/Noise/NoiseProtocolTests.swift +++ b/bitchatTests/Noise/NoiseProtocolTests.swift @@ -68,22 +68,29 @@ struct NoiseProtocolTests { private let bobKey = Curve25519.KeyAgreement.PrivateKey() private let mockKeychain = MockKeychain() - private let alicePeerID = PeerID(str: UUID().uuidString) - private let bobPeerID = PeerID(str: UUID().uuidString) - + // Manager sessions are keyed by the remote peer. Keep the historical + // names, but derive each wire ID from the static key that the + // corresponding manager authenticates during the handshake. + private var alicePeerID: PeerID { + PeerID(publicKey: bobKey.publicKey.rawRepresentation) + } + private var bobPeerID: PeerID { + PeerID(publicKey: aliceKey.publicKey.rawRepresentation) + } + private let aliceSession: NoiseSession private let bobSession: NoiseSession - + init() { aliceSession = NoiseSession( - peerID: alicePeerID, + peerID: PeerID(publicKey: bobKey.publicKey.rawRepresentation), role: .initiator, keychain: mockKeychain, localStaticKey: aliceKey ) - + bobSession = NoiseSession( - peerID: bobPeerID, + peerID: PeerID(publicKey: aliceKey.publicKey.rawRepresentation), role: .responder, keychain: mockKeychain, localStaticKey: bobKey