From 36e81ebec7cb5978cd94dc868f597bc9c282fb17 Mon Sep 17 00:00:00 2001 From: J3vb Date: Sat, 4 Apr 2026 23:30:36 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20address=20code=20review=20IMPORTANT=20is?= =?UTF-8?q?sues=20=E2=80=94=20atomic=20TOCTOU,=20updateKeyHolder=20race,?= =?UTF-8?q?=20dead=20keyReceived=20var?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Client/tauri-client/src/lib/e2eeCrypto.ts | 2 ++ Client/tauri-client/src/lib/livekitSession.ts | 3 -- Server/ws/voice_e2ee.go | 35 +++++++++++-------- Server/ws/voice_e2ee_test.go | 10 ------ 4 files changed, 22 insertions(+), 28 deletions(-) diff --git a/Client/tauri-client/src/lib/e2eeCrypto.ts b/Client/tauri-client/src/lib/e2eeCrypto.ts index f1492106..c694e821 100644 --- a/Client/tauri-client/src/lib/e2eeCrypto.ts +++ b/Client/tauri-client/src/lib/e2eeCrypto.ts @@ -24,9 +24,11 @@ if (typeof crypto === "undefined" || !crypto.subtle) { } const ECDH_CURVE = "P-256"; +// UTF-8 bytes of "owncord-voice-e2ee-v1" const HKDF_SALT = new Uint8Array([ 111, 119, 110, 99, 111, 114, 100, 45, 118, 111, 105, 99, 101, 45, 101, 50, 101, 101, 45, 118, 49, ]); +// UTF-8 bytes of "room-key-wrap" const HKDF_INFO = new Uint8Array([114, 111, 111, 109, 45, 107, 101, 121, 45, 119, 114, 97, 112]); const ROOM_KEY_BYTES = 32; // 256-bit AES key for LiveKit SFrame diff --git a/Client/tauri-client/src/lib/livekitSession.ts b/Client/tauri-client/src/lib/livekitSession.ts index 2d2cf554..275023bd 100644 --- a/Client/tauri-client/src/lib/livekitSession.ts +++ b/Client/tauri-client/src/lib/livekitSession.ts @@ -868,10 +868,8 @@ export class LiveKitSession { new Promise((_, reject) => { timeoutId = setTimeout(() => reject(new Error("E2EE key exchange timeout")), ms); }); - let keyReceived = false; try { await Promise.race([roomKeyPromise, makeTimeout(10_000)]); - keyReceived = true; } catch { // First attempt timed out — re-announce and retry once. if (timeoutId !== null) clearTimeout(timeoutId); @@ -882,7 +880,6 @@ export class LiveKitSession { }); try { await Promise.race([roomKeyPromise, makeTimeout(5_000)]); - keyReceived = true; } catch { log.error("E2EE: key exchange timed out after retry — disconnecting", { channelId }); this._roomKeyResolver = null; diff --git a/Server/ws/voice_e2ee.go b/Server/ws/voice_e2ee.go index 9a8a2226..ef653a3e 100644 --- a/Server/ws/voice_e2ee.go +++ b/Server/ws/voice_e2ee.go @@ -7,9 +7,11 @@ import ( "log/slog" ) -// decodeBase64Loose accepts both standard (padded) and raw (unpadded) base64. -// ECDH public keys exported from WebCrypto use raw base64 (no '=' padding); -// we accept both to avoid breaking existing clients. +// decodeBase64Loose accepts both padded (StdEncoding) and unpadded (RawStdEncoding) +// standard-alphabet base64. ECDH public keys exported from WebCrypto omit '=' +// padding; we accept both forms to avoid breaking existing clients. +// Note: URL-safe base64 (alphabet '-_') is not accepted; clients must use the +// standard alphabet ('+/'). func decodeBase64Loose(s string) ([]byte, error) { if b, err := base64.StdEncoding.DecodeString(s); err == nil { return b, nil @@ -20,8 +22,14 @@ func decodeBase64Loose(s string) ([]byte, error) { // updateKeyHolder scans connected clients to find the one with the lowest // userID currently in channelID, and records them as the key holder. // If no clients remain in the channel the entry is deleted. -// Must NOT be called while h.mu is held (it acquires h.mu.RLock internally). +// Must NOT be called while h.mu or h.keyHolderMu is held. +// Lock order: keyHolderMu (write) → h.mu (read). Holding keyHolderMu for +// the entire scan+write prevents two concurrent calls from racing and +// overwriting each other with a stale result. func (h *Hub) updateKeyHolder(channelID int64) { + h.keyHolderMu.Lock() + defer h.keyHolderMu.Unlock() + h.mu.RLock() var minUserID int64 found := false @@ -35,13 +43,11 @@ func (h *Hub) updateKeyHolder(channelID int64) { } h.mu.RUnlock() - h.keyHolderMu.Lock() if found { h.voiceKeyHolders[channelID] = minUserID } else { delete(h.voiceKeyHolders, channelID) } - h.keyHolderMu.Unlock() } // isVoiceKeyHolder reports whether userID is the current key holder for channelID. @@ -149,9 +155,11 @@ func (h *Hub) handleVoiceE2EEOffer(_ context.Context, c *Client, payload json.Ra return } - // Verify the target is in the same voice channel — lookup and channel - // check must be atomic (under the same lock hold) to prevent TOCTOU races - // where the target leaves between lookup and the channel comparison. + // Verify the target is in the same voice channel, then relay — all under + // one h.mu.RLock hold so the check and send are atomic. A concurrent + // voice_leave cannot remove the target from h.clients between the lookup + // and the channel comparison, nor between the comparison and the send. + msg := buildVoiceE2EEOffer(c.userID, p.EncryptedKey, p.IV) h.mu.RLock() target, ok := h.clients[p.TargetUserID] if !ok { @@ -159,16 +167,13 @@ func (h *Hub) handleVoiceE2EEOffer(_ context.Context, c *Client, payload json.Ra c.sendMsg(buildErrorMsg(ErrCodeBadPayload, "target user not connected")) return } - targetChID := target.getVoiceChID() - h.mu.RUnlock() - if targetChID != voiceChID { + if target.getVoiceChID() != voiceChID { + h.mu.RUnlock() c.sendMsg(buildErrorMsg(ErrCodeForbidden, "target user not in your voice channel")) return } - - // Relay the encrypted key offer to the target. - msg := buildVoiceE2EEOffer(c.userID, p.EncryptedKey, p.IV) target.sendMsg(msg) + h.mu.RUnlock() slog.Debug("voice e2ee: offer relayed", "from_user_id", c.userID, "to_user_id", p.TargetUserID, "channel_id", voiceChID) diff --git a/Server/ws/voice_e2ee_test.go b/Server/ws/voice_e2ee_test.go index 1993a937..aa9276aa 100644 --- a/Server/ws/voice_e2ee_test.go +++ b/Server/ws/voice_e2ee_test.go @@ -387,16 +387,6 @@ func TestE2EE_GetPubKey_ReturnsKeyAfterAnnounce(t *testing.T) { } } -// ─── I-7: login rate limit ────────────────────────────────────────────────── - -func TestLoginRateLimit_Is5(t *testing.T) { - // I-7: loginRateLimitPerMinute should be 5, not 60. - // We can't directly access the constant from ws_test, but we test the - // behavior via the API package test. This test is a placeholder that - // verifies the constant value via the api package's exported test helper. - // (See constants_test.go for the actual value assertion.) -} - // ─── is_key_holder in voice_token payload ──────────────────────────────────── func TestE2EE_VoiceToken_IncludesIsKeyHolder(t *testing.T) {