From 5a10ba6bf9a1688e25901ec9c7874143235a69cf Mon Sep 17 00:00:00 2001 From: krish rathi <148011352+krishrathi1@users.noreply.github.com> Date: Thu, 24 Sep 2026 13:04:14 +0530 Subject: [PATCH] Trigger Noise session rekey on absolute age, not just idle time (#1675) * Trigger Noise session rekey on absolute age, not just idle time needsRenegotiation() had two triggers: message count nearing the 1 billion-message ceiling, and lastActivityTime idle for longer than sessionTimeout (24h). Neither fires for a session with steady traffic: message count realistically never approaches a billion, and lastActivityTime resets on every encrypt/decrypt, so a continuously active conversation never looks idle. Meanwhile encrypt/decrypt both hard-fail with sessionExpired once sessionStartTime crosses the same 24h sessionTimeout, unconditionally, with no rekey ever having been scheduled to prevent it. Two peers who keep a conversation open past 24h (a stationary mesh node, or two phones left open overnight) hit this simultaneously: every message throws sessionExpired on both sides, isEstablished() still reports true so the UI keeps showing the session as secured, MessageRouter never falls back off the transport because canDeliverSecurely never goes false, and nothing re-handshakes because the one self-heal path (BLENoisePacketHandler's decrypt-failure recovery) requires an inbound ciphertext to fail to decrypt for a session-establishment reason, not sessionExpired specifically -- so it's excluded. The conversation is silently and permanently dead until an app restart or a BLE disconnect/reconnect forces revalidation. Adds a third needsRenegotiation() trigger on sessionStartTime age directly, firing at 90% of sessionTimeout -- the rekey timer polls every 60s (NoiseEncryptionService.rekeyCheckInterval), leaving over two hours of margin to complete a full handshake before the hard cutoff. No Xcode/Swift toolchain available here, so this is verified by tracing, not compiling: manually walked the new continuouslyActiveSessionStillNeedsRenegotiationBeforeExpiry test's inputs through both the pre-fix and post-fix needsRenegotiation() logic (confirmed the pre-fix code returns false for a session with fresh lastActivityTime and old sessionStartTime, and the fix flips it to true), checked every other call site of needsRenegotiation() and getSessionsNeedingRekey() in the test suite to confirm none depend on the old always-false behavior at this age, and confirmed via git apply -R / git apply that the source diff is exactly the traced change with no other drift. * Fix stale comment in needsRenegotiation idle time check --- bitchat/Noise/SecureNoiseSession.swift | 18 ++++++++-- bitchatTests/Noise/NoiseCoverageTests.swift | 38 +++++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/bitchat/Noise/SecureNoiseSession.swift b/bitchat/Noise/SecureNoiseSession.swift index 8f95842c..47adf140 100644 --- a/bitchat/Noise/SecureNoiseSession.swift +++ b/bitchat/Noise/SecureNoiseSession.swift @@ -66,12 +66,24 @@ final class SecureNoiseSession: NoiseSession { if messageCount >= messageThreshold { return true } - - // Check if last activity was more than 30 minutes ago + + // Check if last activity exceeded sessionTimeout if Date().timeIntervalSince(lastActivityTime) > NoiseSecurityConstants.sessionTimeout { return true } - + + // A session that never goes idle (a long-lived conversation with + // steady traffic) keeps refreshing lastActivityTime forever, so the + // check above never fires -- only sessionStartTime tracks how close + // encrypt/decrypt are to the hard sessionExpired cutoff above. Firing + // at 90% of sessionTimeout leaves the rekey timer's polling interval + // (checked every 60s, see NoiseEncryptionService.rekeyCheckInterval) + // room to complete a full handshake well before the old session + // starts throwing sessionExpired on every call. + if Date().timeIntervalSince(sessionStartTime) > NoiseSecurityConstants.sessionTimeout * 0.9 { + return true + } + return false } diff --git a/bitchatTests/Noise/NoiseCoverageTests.swift b/bitchatTests/Noise/NoiseCoverageTests.swift index 763f5816..c777d63f 100644 --- a/bitchatTests/Noise/NoiseCoverageTests.swift +++ b/bitchatTests/Noise/NoiseCoverageTests.swift @@ -893,6 +893,44 @@ struct NoiseCoverageTests { } } + @Test("A continuously active session still schedules a rekey before it hard-expires") + func continuouslyActiveSessionStillNeedsRenegotiationBeforeExpiry() throws { + // A session with steady traffic never goes idle, so lastActivityTime + // stays fresh right up to the moment sessionStartTime crosses the + // hard sessionTimeout cutoff above and encrypt/decrypt start throwing + // sessionExpired on every call. needsRenegotiation() has to notice + // the session is old on its own -- it can't rely on the idle check, + // because there is no idle time to find. + let session = SecureNoiseSession( + peerID: alicePeerID, + role: .initiator, + keychain: keychain, + localStaticKey: aliceStaticKey + ) + let responder = SecureNoiseSession( + peerID: bobPeerID, + role: .responder, + keychain: keychain, + localStaticKey: bobStaticKey + ) + try establishSessions(initiator: session, responder: responder) + + // Just under the hard cutoff, but past the 90% rekey threshold. + session.setSessionStartTimeForTesting( + Date().addingTimeInterval(-(NoiseSecurityConstants.sessionTimeout * 0.95)) + ) + session.setLastActivityTimeForTesting(Date()) + session.setMessageCountForTesting(0) + + #expect(session.needsRenegotiation()) + + // The session is still within its hard cutoff, so it must keep + // working right up until a rekey actually replaces it -- the fix is + // to schedule the rekey earlier, not to expire the session sooner. + let ciphertext = try session.encrypt(Data("still working".utf8)) + #expect(try responder.decrypt(ciphertext) == Data("still working".utf8)) + } + @Test("Rate limiter handles global message caps and per-peer resets") func rateLimiterGlobalMessageCapAndReset() async throws { let globalLimiter = NoiseRateLimiter()