mirror of
https://github.com/permissionlesstech/bitchat.git
synced 2026-07-25 11:05:19 +00:00
fix: clear DH shared secrets after Noise handshake operations
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 <noreply@anthropic.com>
This commit is contained in:
@@ -610,14 +610,20 @@ final class NoiseHandshakeState {
|
|||||||
throw NoiseError.missingKeys
|
throw NoiseError.missingKeys
|
||||||
}
|
}
|
||||||
let shared = try localEphemeral.sharedSecretFromKeyAgreement(with: remoteStatic)
|
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 {
|
} else {
|
||||||
guard let localStatic = localStaticPrivate,
|
guard let localStatic = localStaticPrivate,
|
||||||
let remoteEphemeral = remoteEphemeralPublic else {
|
let remoteEphemeral = remoteEphemeralPublic else {
|
||||||
throw NoiseError.missingKeys
|
throw NoiseError.missingKeys
|
||||||
}
|
}
|
||||||
let shared = try localStatic.sharedSecretFromKeyAgreement(with: remoteEphemeral)
|
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:
|
case .se:
|
||||||
@@ -628,14 +634,20 @@ final class NoiseHandshakeState {
|
|||||||
throw NoiseError.missingKeys
|
throw NoiseError.missingKeys
|
||||||
}
|
}
|
||||||
let shared = try localStatic.sharedSecretFromKeyAgreement(with: remoteEphemeral)
|
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 {
|
} else {
|
||||||
guard let localEphemeral = localEphemeralPrivate,
|
guard let localEphemeral = localEphemeralPrivate,
|
||||||
let remoteStatic = remoteStaticPublic else {
|
let remoteStatic = remoteStaticPublic else {
|
||||||
throw NoiseError.missingKeys
|
throw NoiseError.missingKeys
|
||||||
}
|
}
|
||||||
let shared = try localEphemeral.sharedSecretFromKeyAgreement(with: remoteStatic)
|
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:
|
case .ss:
|
||||||
@@ -724,7 +736,10 @@ final class NoiseHandshakeState {
|
|||||||
throw NoiseError.missingKeys
|
throw NoiseError.missingKeys
|
||||||
}
|
}
|
||||||
let shared = try localEphemeral.sharedSecretFromKeyAgreement(with: remoteEphemeral)
|
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:
|
case .es:
|
||||||
if role == .initiator {
|
if role == .initiator {
|
||||||
@@ -778,7 +793,10 @@ final class NoiseHandshakeState {
|
|||||||
throw NoiseError.missingKeys
|
throw NoiseError.missingKeys
|
||||||
}
|
}
|
||||||
let shared = try localStatic.sharedSecretFromKeyAgreement(with: remoteStatic)
|
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:
|
case .e, .s:
|
||||||
break
|
break
|
||||||
|
|||||||
@@ -65,3 +65,90 @@ final class MockKeychain: KeychainManagerProtocol {
|
|||||||
|
|
||||||
/// Typealias for backwards compatibility with tests using MockKeychainHelper
|
/// Typealias for backwards compatibility with tests using MockKeychainHelper
|
||||||
typealias MockKeychainHelper = MockKeychain
|
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")
|
"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")
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user