From ef1aca8a676c9f32150f45a3375a80909540b7f2 Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Fri, 24 Jul 2026 08:55:07 +0200 Subject: [PATCH] fix(e2ee): pin the verified identity key on re-pin, rekey on keyed-peer leave Multi-agent F3 security review surfaced two voice-E2EE defects: - Re-pin TOCTOU (voice-E2EE MITM): the identity-mismatch modal showed a fingerprint from one membersStore read, but rePinPeerIdentity re-read the server-writable store to decide what to pin. A malicious server (F3's threat model) could swap in an attacker key via a user_update during the human out-of-band verification window and have it pinned, silently defeating the mismatch prompt. rePinPeerIdentity now takes the exact verified key as a parameter; ChannelSidebar passes the bytes whose fingerprint it displayed. - Membership forward secrecy: the key holder rotated the room key only when the holder ROLE transferred, so a departed non-key-holder kept a valid room key until the next periodic (<=5 min) rotation. The holder now also rotates when a peer that held the key leaves (reusing rotateKeyPeriodically), gated on the leaver having actually held a key. Client gates green: typecheck, lint (0 errors), prettier, vitest (3361). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/components/ChannelSidebar.ts | 9 +++- Client/tauri-client/src/lib/livekitSession.ts | 45 +++++++++++----- .../tests/unit/channel-sidebar.test.ts | 19 ++++++- .../tests/unit/livekit-session.test.ts | 51 ++++++++++++++++++- 4 files changed, 105 insertions(+), 19 deletions(-) diff --git a/Client/tauri-client/src/components/ChannelSidebar.ts b/Client/tauri-client/src/components/ChannelSidebar.ts index 9da27914..586f889d 100644 --- a/Client/tauri-client/src/components/ChannelSidebar.ts +++ b/Client/tauri-client/src/components/ChannelSidebar.ts @@ -107,10 +107,17 @@ async function openIdentityMismatchModal( fingerprint, onAccept: () => { closeIdentityModal(); + // Pin the EXACT key whose fingerprint we displayed and the user verified + // out-of-band (captured above), NOT a fresh membersStore re-read — a + // malicious server could mutate the store (user_update) during the human + // verification window and get its key pinned instead (TOCTOU). If the + // server had stripped the key, publishedKey is null and there was nothing + // to verify, so there is nothing to re-pin. + if (publishedKey === null) return; // Surface keyring/IO failures instead of dropping them — this re-pins a // trust anchor, so a silent failure would leave the user believing they // recovered when they did not. - void rePinPeerIdentity(userId).catch((err: unknown) => { + void rePinPeerIdentity(userId, publishedKey).catch((err: unknown) => { log.error("E2EE: failed to re-pin peer identity", err); }); }, diff --git a/Client/tauri-client/src/lib/livekitSession.ts b/Client/tauri-client/src/lib/livekitSession.ts index 187f6442..c58a6937 100644 --- a/Client/tauri-client/src/lib/livekitSession.ts +++ b/Client/tauri-client/src/lib/livekitSession.ts @@ -1296,23 +1296,28 @@ export class LiveKitSession { } /** - * F3 TOFU re-pin recovery (finding #4). Accept the peer's CURRENT published - * identity key, overwriting the stored pin for {host,userId} and clearing the - * mismatch block — the identity-key analogue of accepting a changed TLS cert. - * A legitimate key rotation (reinstall / new device / wiped keyring) is thus - * recoverable instead of a permanent lockout; the next announce re-verifies - * against the new pin. Returns false when there is no host or no published key - * to pin. The voice-panel mismatch confirm should call this. + * F3 TOFU re-pin recovery (finding #4). Pin the EXACT identity key + * `verifiedKey` — the bytes whose fingerprint the caller displayed and the + * user confirmed out-of-band — overwriting the stored pin for {host,userId} + * and clearing the mismatch block (the identity-key analogue of accepting a + * changed TLS cert). A legitimate key rotation (reinstall / new device / + * wiped keyring) is thus recoverable instead of a permanent lockout; the next + * announce re-verifies against the new pin. + * + * The verified key MUST be passed in, never re-read from membersStore here: + * the store is server-writable (a `user_update` mutates it), so re-reading it + * would let a malicious server swap in an attacker key during the human + * out-of-band verification window and have us pin THAT — a TOCTOU that + * silently defeats the mismatch prompt. Returns false when there is no host + * or no key to pin. */ - async rePinPeerIdentity(userId: number): Promise { + async rePinPeerIdentity(userId: number, verifiedKey: string): Promise { const host = this.serverHost; - const publishedIdentity = - membersStore.getState().members.get(userId)?.identityPublicKey ?? null; - if (!host || !publishedIdentity) { - log.warn("E2EE: cannot re-pin peer without a host and published identity key", { userId }); + if (!host || !verifiedKey) { + log.warn("E2EE: cannot re-pin peer without a host and the verified identity key", { userId }); return false; } - await storeIdentityPin(host, String(userId), publishedIdentity); + await storeIdentityPin(host, String(userId), verifiedKey); clearPeerVerification(userId); log.info("E2EE: re-pinned peer identity key (TOFU recovery)", { userId }); return true; @@ -1454,13 +1459,17 @@ export class LiveKitSession { /** * Handle a participant leaving the voice channel. If we become the new key - * holder, rotate the room key and distribute to remaining peers. + * holder, rotate the room key and distribute to remaining peers. If we are + * ALREADY the key holder and a peer that held the room key left, we also + * rotate — so the departed member's copy can no longer decrypt future audio + * against the untrusted SFU (membership forward secrecy). * * Key holder election: the participant with the lowest user ID among remaining * participants is elected. This is deterministic and does not depend on Map * insertion order (which is not guaranteed to match server join order). */ async handleParticipantLeft(userId: number): Promise { + const hadPeerKey = this._peerPublicKeys.has(userId); this._peerPublicKeys.delete(userId); clearPeerVerification(userId); @@ -1543,6 +1552,14 @@ export class LiveKitSession { this._rotatingKey = false; this.startKeyRotationTimer(); } + } else if (wasKeyHolder && hadPeerKey) { + // Membership forward secrecy: I remain the key holder and a peer that held + // the room key left, so rotate + redistribute to the CURRENT peer set + // (which already excludes the leaver, deleted above) — otherwise the + // departed member keeps a valid room key against the untrusted SFU until + // the next periodic rotation. rotateKeyPeriodically self-guards on + // _isKeyHolder / _rotatingKey. + await this.rotateKeyPeriodically(); } } diff --git a/Client/tauri-client/tests/unit/channel-sidebar.test.ts b/Client/tauri-client/tests/unit/channel-sidebar.test.ts index 9109e5a6..f9eb538b 100644 --- a/Client/tauri-client/tests/unit/channel-sidebar.test.ts +++ b/Client/tauri-client/tests/unit/channel-sidebar.test.ts @@ -1488,8 +1488,22 @@ describe("ChannelSidebar voice identity badge", () => { }); }); - it("re-pins the peer when the mismatch modal's Trust button is clicked", async () => { + it("re-pins the displayed key when the mismatch modal's Trust button is clicked", async () => { addVoiceUser(VOICE_CH, 10, "Alice"); + // The peer must have a published key so its fingerprint is shown and there + // is a concrete verified key to re-pin. + membersStore.setState((prev) => { + const members = new Map(prev.members); + members.set(10, { + id: 10, + username: "Alice", + avatar: null, + role: "member", + status: "online", + identityPublicKey: "alice-published-key-b64", + }); + return { ...prev, members }; + }); setPeerVerif(10, "mismatch", null); sidebar.mount(container); @@ -1501,7 +1515,8 @@ describe("ChannelSidebar voice identity badge", () => { }); trustBtn.click(); - expect(mockRePinPeerIdentity).toHaveBeenCalledWith(10); + // Pins the exact key whose fingerprint was displayed, not a bare userId. + expect(mockRePinPeerIdentity).toHaveBeenCalledWith(10, "alice-published-key-b64"); expect(document.body.querySelector(".modal-overlay")).toBeNull(); }); diff --git a/Client/tauri-client/tests/unit/livekit-session.test.ts b/Client/tauri-client/tests/unit/livekit-session.test.ts index 1d39cb50..196793b9 100644 --- a/Client/tauri-client/tests/unit/livekit-session.test.ts +++ b/Client/tauri-client/tests/unit/livekit-session.test.ts @@ -2263,8 +2263,9 @@ describe("LiveKitSession", () => { ); // User accepts the new key (analogous to accepting a changed TLS cert): - // re-pin overwrites the stored pin and clears the mismatch block. - const recovered = await session.rePinPeerIdentity(PEER_ID); + // re-pin overwrites the stored pin with the verified key and clears the + // mismatch block. + const recovered = await session.rePinPeerIdentity(PEER_ID, "new-identity-b64"); expect(recovered).toBe(true); expect(storeIdentityPin).toHaveBeenCalledWith(HOST, String(PEER_ID), "new-identity-b64"); @@ -2281,6 +2282,52 @@ describe("LiveKitSession", () => { expect(offerSends(ws)).toHaveLength(1); }); + it("re-pins the verified key, not a store re-read a malicious server mutated (TOCTOU)", async () => { + // The store holds whatever the server most recently pushed. If re-pin + // re-read the store it would pin the attacker's swapped-in key; it must + // instead pin the exact key it was handed — the one whose fingerprint the + // user verified out-of-band. + seedPeer("attacker-swapped-key-b64"); + const ws = { send: vi.fn() }; + await joinAsKeyHolder(ws); + + const pinned = await session.rePinPeerIdentity(PEER_ID, "verified-key-b64"); + + expect(pinned).toBe(true); + expect(storeIdentityPin).toHaveBeenCalledWith(HOST, String(PEER_ID), "verified-key-b64"); + expect(storeIdentityPin).not.toHaveBeenCalledWith( + HOST, + String(PEER_ID), + "attacker-swapped-key-b64", + ); + }); + + it("rotates the room key when a keyed peer leaves while I stay key holder (forward secrecy)", async () => { + seedPeer("peer-identity-b64"); + const ws = { send: vi.fn() }; + await joinAsKeyHolder(ws); // I hold the key for channel 1 + await session.handleE2EEAnnounce(PEER_ID, "cGVlcg==", "sig"); // peer now holds the room key + // A participant remains, so the leave handler proceeds past the empty check. + (mockVoiceState as any).voiceUsers = new Map([[1, new Map([[1, {}]])]]); + const epochBefore = (session as any)._e2eeEpoch; + + await session.handleParticipantLeft(PEER_ID); + + // Room key rotated (epoch advanced) so the departed peer's copy is dead. + expect((session as any)._e2eeEpoch).toBe(epochBefore + 1); + }); + + it("does not rotate the room key when the leaver never held it", async () => { + const ws = { send: vi.fn() }; + await joinAsKeyHolder(ws); + (mockVoiceState as any).voiceUsers = new Map([[1, new Map([[1, {}]])]]); + const epochBefore = (session as any)._e2eeEpoch; + + await session.handleParticipantLeft(999); // 999 never announced → held no key + + expect((session as any)._e2eeEpoch).toBe(epochBefore); + }); + it("verifies a server-substituted key when drained from the pending queue", async () => { seedPeer("peer-identity-b64"); (verifyEphemeralKeySignature as any).mockResolvedValue(false);