Merge pull request #1233 from J3vb/fix/e2ee-repin-toctou

fix(e2ee): close voice-E2EE re-pin TOCTOU (MITM) + forward-secrecy gaps

Client/tauri-client/src/components/ChannelSidebar.ts

@@ -107,10 +107,20 @@ 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).
+      //
+      // 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.
-      void rePinPeerIdentity(userId).catch((err: unknown) => {
+      void rePinPeerIdentity(userId, publishedKey).catch((err: unknown) => {
         log.error("E2EE: failed to re-pin peer identity", err);
       });
     },

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;
@@ -1296,23 +1300,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<boolean> {
+  async rePinPeerIdentity(userId: number, verifiedKey: string): Promise<boolean> {
     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 +1463,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<void> {
+    const hadPeerKey = this._peerPublicKeys.has(userId);
     this._peerPublicKeys.delete(userId);
     clearPeerVerification(userId);
 
@@ -1541,7 +1554,25 @@ 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.
+      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();
       }
     }
   }
@@ -1604,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<void> {
+    if (this._rotationPending) {
+      this._rotationPending = false;
+      await this.rotateKeyPeriodically();
+      return;
+    }
     this.startKeyRotationTimer();
   }
 
@@ -1618,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();

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(() => ({
@@ -1488,8 +1489,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 +1516,40 @@ 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();
+  });
+
+  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();
   });
 

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,85 @@ 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("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);