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 1/2] 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); From 8b8632774e36f0cf575e33801fe814f70eba0263 Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Fri, 24 Jul 2026 09:11:08 +0200 Subject: [PATCH 2/2] fix(e2ee): coalesce concurrent room-key rotations; refuse blind re-pin Follow-up to the F3 re-pin/forward-secrecy fix, closing two residuals found by adversarial re-review of the first fix: - Concurrent-leave rotation drop (medium): rotateKeyPeriodically's _rotatingKey guard silently skipped a rotation already in flight, so a keyed peer that left mid-rotation kept a live room key until the next periodic (<=5 min) rotation. A coincident keyed-peer leave now DEFERS its rekey (_rotationPending) instead of dropping it; the completing rotation drains it via a shared drainPendingRotationOrArmTimer, excluding the departed member. Applied to both the become-holder and periodic rotation paths; reset in clearE2EEState. - Blind re-pin (info, defense-in-depth): the mismatch modal's Trust action pinned publishedKey even when its fingerprint could not be computed (nothing shown to verify). onAccept now refuses to pin when fingerprint is null. Client gates green: typecheck, lint (0 errors), prettier, vitest (3364). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/components/ChannelSidebar.ts | 11 ++++-- Client/tauri-client/src/lib/livekitSession.ts | 38 ++++++++++++++++--- .../tests/unit/channel-sidebar.test.ts | 33 ++++++++++++++++ .../tests/unit/livekit-session.test.ts | 33 ++++++++++++++++ 4 files changed, 106 insertions(+), 9 deletions(-) diff --git a/Client/tauri-client/src/components/ChannelSidebar.ts b/Client/tauri-client/src/components/ChannelSidebar.ts index 586f889d..ce11477a 100644 --- a/Client/tauri-client/src/components/ChannelSidebar.ts +++ b/Client/tauri-client/src/components/ChannelSidebar.ts @@ -110,10 +110,13 @@ async function openIdentityMismatchModal( // 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; + // verification window and get its key pinned instead (TOCTOU). + // + // Only pin a key whose fingerprint was actually SHOWN: publishedKey null + // means the server stripped the key, and fingerprint null means it could + // not be computed (malformed key). In both cases the user saw nothing to + // verify, so pinning would be a blind accept — refuse it. + if (publishedKey === null || fingerprint === 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. diff --git a/Client/tauri-client/src/lib/livekitSession.ts b/Client/tauri-client/src/lib/livekitSession.ts index c58a6937..6cc121c6 100644 --- a/Client/tauri-client/src/lib/livekitSession.ts +++ b/Client/tauri-client/src/lib/livekitSession.ts @@ -169,6 +169,10 @@ export class LiveKitSession { private _roomKeyRejector: ((err: Error) => void) | null = null; /** Guard: true while a key rotation is in progress (prevents concurrent rotations). */ private _rotatingKey = false; + /** Set when a keyed-peer leave coincides with an in-flight rotation: the rekey + * is deferred (not dropped) and re-run when the current rotation finishes, so + * a member that left mid-rotation is excluded from the fresh room key. */ + private _rotationPending = false; /** Monotonic counter incremented on every key rotation. handleE2EEOffer captures the * epoch before async work and discards the result if epoch changed (stale offer). */ private _e2eeEpoch = 0; @@ -1550,16 +1554,26 @@ export class LiveKitSession { log.error("E2EE: failed to rotate room key", err); } finally { this._rotatingKey = false; - this.startKeyRotationTimer(); } + // If a keyed peer left while this become-holder rotation was in flight, its + // rekey was deferred (not dropped) — run it now so the departed member is + // excluded from the fresh key; otherwise re-arm the periodic timer. + await this.drainPendingRotationOrArmTimer(); } 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(); + // the next periodic rotation. + if (this._rotatingKey) { + // A rotation is already in flight and may already have sent the current + // key to this leaver before they left. Don't DROP the rekey (that would + // leave the departed member holding a live key) — defer it so it re-runs + // when the in-flight rotation completes, excluding them. + this._rotationPending = true; + } else { + await this.rotateKeyPeriodically(); + } } } @@ -1621,7 +1635,20 @@ export class LiveKitSession { this._rotatingKey = false; } - // Re-arm the timer for the next rotation. + // Re-arm the periodic timer, or run a rotation deferred by a keyed-peer leave + // that coincided with this one. + await this.drainPendingRotationOrArmTimer(); + } + + /** After a rotation completes: if a keyed-peer leave coincided with it (its + * rekey was deferred, not dropped), run one more rotation to exclude the + * departed member; otherwise re-arm the periodic rotation timer. */ + private async drainPendingRotationOrArmTimer(): Promise { + if (this._rotationPending) { + this._rotationPending = false; + await this.rotateKeyPeriodically(); + return; + } this.startKeyRotationTimer(); } @@ -1635,6 +1662,7 @@ export class LiveKitSession { clearPeerVerifications(); this._isKeyHolder = false; this._rotatingKey = false; + this._rotationPending = false; this._e2eeEpoch = 0; this._pendingAnnounces.length = 0; this.clearKeyRotationTimer(); diff --git a/Client/tauri-client/tests/unit/channel-sidebar.test.ts b/Client/tauri-client/tests/unit/channel-sidebar.test.ts index f9eb538b..8db8556b 100644 --- a/Client/tauri-client/tests/unit/channel-sidebar.test.ts +++ b/Client/tauri-client/tests/unit/channel-sidebar.test.ts @@ -39,6 +39,7 @@ import { voiceStore, updateVoiceState } from "../../src/stores/voice.store"; import type { PeerVerification } from "../../src/stores/voice.store"; import { membersStore } from "../../src/stores/members.store"; import type { ReadyChannel } from "../../src/lib/types"; +import { computeKeyFingerprint } from "@lib/e2eeCrypto"; function resetStores(): void { channelsStore.setState(() => ({ @@ -1520,6 +1521,38 @@ describe("ChannelSidebar voice identity badge", () => { expect(document.body.querySelector(".modal-overlay")).toBeNull(); }); + it("does not re-pin when the fingerprint could not be computed (no blind accept)", async () => { + addVoiceUser(VOICE_CH, 10, "Alice"); + 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); + // The changed key's fingerprint cannot be computed → the modal shows no + // fingerprint, so Trust must not pin a key the user never got to verify. + (computeKeyFingerprint as any).mockRejectedValueOnce(new Error("bad key")); + sidebar.mount(container); + + (badgeFor(10) as HTMLElement).click(); + const trustBtn = await vi.waitFor(() => { + const btn = document.body.querySelector(".modal-overlay .btn-danger") as HTMLButtonElement; + expect(btn).not.toBeNull(); + return btn; + }); + trustBtn.click(); + + expect(mockRePinPeerIdentity).not.toHaveBeenCalled(); + expect(document.body.querySelector(".modal-overlay")).toBeNull(); + }); + it("closes an open mismatch modal on sidebar destroy", async () => { addVoiceUser(VOICE_CH, 10, "Alice"); setPeerVerif(10, "mismatch", null); diff --git a/Client/tauri-client/tests/unit/livekit-session.test.ts b/Client/tauri-client/tests/unit/livekit-session.test.ts index 196793b9..2087f631 100644 --- a/Client/tauri-client/tests/unit/livekit-session.test.ts +++ b/Client/tauri-client/tests/unit/livekit-session.test.ts @@ -2328,6 +2328,39 @@ describe("LiveKitSession", () => { expect((session as any)._e2eeEpoch).toBe(epochBefore); }); + it("defers a keyed-peer leave rotation instead of dropping it when one is in flight", async () => { + seedPeer("peer-identity-b64"); + const ws = { send: vi.fn() }; + await joinAsKeyHolder(ws); + await session.handleE2EEAnnounce(PEER_ID, "cGVlcg==", "sig"); // peer holds the room key + (mockVoiceState as any).voiceUsers = new Map([[1, new Map([[1, {}]])]]); + // A rotation is already in flight (e.g. an earlier peer's leave). + (session as any)._rotatingKey = true; + const epochBefore = (session as any)._e2eeEpoch; + + await session.handleParticipantLeft(PEER_ID); + + // Not dropped by the _rotatingKey guard: the rekey is queued and no second + // rotation ran underneath the in-flight one. + expect((session as any)._rotationPending).toBe(true); + expect((session as any)._e2eeEpoch).toBe(epochBefore); + }); + + it("runs the deferred rotation once the in-flight one completes", async () => { + const ws = { send: vi.fn() }; + await joinAsKeyHolder(ws); + (mockVoiceState as any).voiceUsers = new Map([[1, new Map([[1, {}]])]]); + // A keyed-peer leave was deferred while a rotation was in flight. + (session as any)._rotationPending = true; + const epochBefore = (session as any)._e2eeEpoch; + + // The completing rotation must drain the pending one (excludes the leaver). + await (session as any).rotateKeyPeriodically(); + + expect((session as any)._e2eeEpoch).toBe(epochBefore + 2); + expect((session as any)._rotationPending).toBe(false); + }); + it("verifies a server-substituted key when drained from the pending queue", async () => { seedPeer("peer-identity-b64"); (verifyEphemeralKeySignature as any).mockResolvedValue(false);