diff --git a/PROGRESS.md b/PROGRESS.md index f4df715..4d7c4b8 100644 --- a/PROGRESS.md +++ b/PROGRESS.md @@ -24,6 +24,32 @@ up instantly. Newest status at the top. channel CRUD with full per-channel Opus audio config, user moderation (kick/ban/move/server mute/server deafen/set permissions), and server account management. `dotnet test` of the Windows solution passes. Still to do: DRED/audio-quality polish. +- **Done:** **Fixed multi-user voice — relayed frames failed AEAD decryption (nonce desync)** + (2026-06-17, reported live: with 2+ people in a channel, audio was one-directional — "I can + hear them but they can't hear me" — and a 3rd joiner heard nobody). Root cause was in the SFU + relay (`server/src/media_relay.cpp`). The media AEAD nonce is an *implicit per-direction + monotonic counter*; `open()` reconstructs it from the 14-byte header's `seq` field (the AAD), + so the wire contract is `header.seq == the counter seal() used` (the client honors this at + `client.cpp:907`). The relay decrypted each inbound frame with the sender's key, then re-sealed + with the **recipient's** `send_crypto` (its own counter) but **forwarded the sender's header + verbatim** — so `header.seq` carried the sender's counter, not the recipient's. The recipient's + `open()` rebuilt the wrong nonce → every relayed frame failed auth and was silently dropped. It + only "worked" while the sender's counter coincidentally equalled the server→recipient counter + (a single first-ever sender into a fresh recipient), which is exactly why the first/sole talker + was heard but reverse/3rd-party audio was not. **Fix:** before re-sealing, the relay rewrites the + outgoing header's `seq` (bytes [8..9]) to the recipient's `peek_send_counter()`, so each + server→client direction is one contiguous monotonic counter and the nonce always matches (the + anti-replay window also stops seeing false replays from interleaved senders). Safe because the + jitter buffer orders by `timestamp`, not `seq` (`audio_engine.h`); `seq` exists only to carry the + AEAD counter. No wire-format/proto/ABI change. Regression test added in `tests/test_media_aead.cpp` + (`test_relay_interleaved_reseal`): two senders interleaved into one recipient all decrypt with the + fix, and the verbatim-seq path is asserted to fail. `ctest --preset m1-dev` — **18/18 green** (run + via PowerShell; Git Bash can't resolve the runtime DLLs. `vad_ptt_devices` is timing-flaky over + loopback — passes on re-run — unrelated to this fix). **Latent, separate:** the secondary + "3rd joiner sometimes can't see other users" report is a control-plane (TCP snapshot/UserEvent) + issue, not this AEAD bug — re-verify after live testing before investigating. Also still latent: + the 16-bit `seq` wraps after 65536 frames per direction (faster on a busy relay) with no ROC, so + the implicit counter desyncs on long continuous sessions (`crypto.cpp` open() TODO). - **Done:** **Fixed "randomly bumped to Lobby" in the Windows client — actors were excluded from their own state-change broadcasts** (2026-06-17, reported live: a connected client would intermittently snap from its joined channel back to Lobby in the UI). Root cause was a design diff --git a/server/src/media_relay.cpp b/server/src/media_relay.cpp index e93a3d5..d37dd56 100644 --- a/server/src/media_relay.cpp +++ b/server/src/media_relay.cpp @@ -102,8 +102,17 @@ void MediaRelay::on_udp_frame(const uint8_t* data, size_t len, crypto_aead_chacha20poly1305_ietf_ABYTES); } - // Copy header (re-use sender's header verbatim — ssrc, seq, ts pass through). + // Copy header (ssrc, ts, flags pass through for demux/playout), then rewrite the + // seq field to THIS recipient's next send counter. The media AEAD nonce is an + // implicit per-direction monotonic counter; open() reconstructs it from the seq in + // the header (the AAD). Since we re-seal with the recipient's send_crypto (its own + // counter), the verbatim sender seq would no longer match the nonce seal() uses and + // every relayed frame would fail auth. Set seq = peek_send_counter() BEFORE sealing + // so the header (which is the authenticated AAD) carries the matching counter. std::memcpy(seal_buf_.data(), data, voicecat::net::kVoiceHeaderSize); + const uint64_t send_ctr = send_crypto->peek_send_counter(); + seal_buf_[8] = static_cast((send_ctr >> 8) & 0xFF); + seal_buf_[9] = static_cast(send_ctr & 0xFF); uint8_t* out_payload = seal_buf_.data() + voicecat::net::kVoiceHeaderSize; long sealed_out = send_crypto->seal( diff --git a/tests/test_media_aead.cpp b/tests/test_media_aead.cpp index b3116f7..6f46556 100644 --- a/tests/test_media_aead.cpp +++ b/tests/test_media_aead.cpp @@ -3,6 +3,7 @@ * * Uses a synthetic 32-byte key directly (no TLS context needed for unit tests). */ +#include #include #include #include @@ -148,6 +149,108 @@ static void test_multiple_packets() { } } +// Build a 14-byte AAD (voice frame header) with a given ssrc + seq. +static std::vector make_aad_ssrc(uint32_t ssrc, uint16_t seq) { + VoiceFrame f; + f.ssrc = ssrc; + f.seq = seq; + std::vector aad(kVoiceHeaderSize); + serialize_header(f, aad.data()); + return aad; +} + +// Simulate one server relay hop for a single frame, sender → recipient R. +// - sender seals with its send key, setting header seq = its own send counter (client contract). +// - server opens with the sender's key, then re-seals with R's send key. +// - if rewrite_seq, the re-sealed header's seq is set to R's send counter (the fix); otherwise +// the sender's seq is forwarded verbatim (the bug). +// Returns true iff R successfully decrypts the relayed frame. +static bool relay_one(SodiumMediaCrypto& sender_send, SodiumMediaCrypto& server_recv, + SodiumMediaCrypto& r_send, SodiumMediaCrypto& r_recv, + uint32_t ssrc, const std::vector& plain, bool rewrite_seq) { + // Client A→server: seq carries the sender's send counter. + auto in_aad = make_aad_ssrc(ssrc, static_cast(sender_send.peek_send_counter())); + std::vector cipher(plain.size() + crypto_aead_chacha20poly1305_ietf_ABYTES); + long sealed = sender_send.seal(plain.data(), plain.size(), in_aad.data(), in_aad.size(), + cipher.data(), cipher.size()); + if (sealed < 0) return false; + + // Server decrypts the inbound frame. + std::vector recovered(plain.size()); + long opened = server_recv.open(cipher.data(), static_cast(sealed), + in_aad.data(), in_aad.size(), + recovered.data(), recovered.size()); + if (opened < 0) return false; + + // Server re-seals to R. Header passes through except seq, which (when fixed) is set to R's + // own send counter so R's open() reconstructs the matching nonce. + std::vector out_aad = in_aad; // copy header verbatim + if (rewrite_seq) { + uint64_t ctr = r_send.peek_send_counter(); + out_aad[8] = static_cast((ctr >> 8) & 0xFF); + out_aad[9] = static_cast(ctr & 0xFF); + } + std::vector relay_cipher(recovered.size() + crypto_aead_chacha20poly1305_ietf_ABYTES); + long resealed = r_send.seal(recovered.data(), static_cast(opened), + out_aad.data(), out_aad.size(), + relay_cipher.data(), relay_cipher.size()); + if (resealed < 0) return false; + + // R decrypts the relayed frame. + std::vector r_recovered(recovered.size()); + long r_opened = r_recv.open(relay_cipher.data(), static_cast(resealed), + out_aad.data(), out_aad.size(), + r_recovered.data(), r_recovered.size()); + return r_opened == static_cast(plain.size()); +} + +// Regression test for the relay nonce-desync bug: two senders (A, B) relayed into one recipient +// (R) interleaved. The media AEAD nonce is an implicit per-direction counter reconstructed from +// the header seq; if the relay forwards the sender's seq verbatim, it no longer matches R's send +// counter and frames fail to decrypt. The relay must rewrite seq = R's send counter. +static void test_relay_interleaved_reseal() { + auto make_key = [](const char* label) { + std::array k{}; + crypto_generichash(k.data(), k.size(), + reinterpret_cast(label), + std::strlen(label), nullptr, 0); + return k; + }; + auto kA = make_key("relay-A"); // A↔server direction + auto kB = make_key("relay-B"); // B↔server direction + auto kR = make_key("relay-R"); // server↔R direction + + std::vector plain(80, 0x3C); + + // Fixed path: interleaved A/B frames all decrypt at R. + { + SodiumMediaCrypto a_send(kA.data()), srv_recv_a(kA.data()); + SodiumMediaCrypto b_send(kB.data()), srv_recv_b(kB.data()); + SodiumMediaCrypto r_send(kR.data()), r_recv(kR.data()); + + bool all_ok = true; + for (int i = 0; i < 8; ++i) { + all_ok &= relay_one(a_send, srv_recv_a, r_send, r_recv, 0x1111, plain, /*rewrite=*/true); + all_ok &= relay_one(b_send, srv_recv_b, r_send, r_recv, 0x2222, plain, /*rewrite=*/true); + } + CHECK(all_ok); // with the fix, every interleaved relayed frame decrypts at R + } + + // Control: forwarding seq verbatim (the bug) must drop frames once the counters diverge. + { + SodiumMediaCrypto a_send(kA.data()), srv_recv_a(kA.data()); + SodiumMediaCrypto b_send(kB.data()), srv_recv_b(kB.data()); + SodiumMediaCrypto r_send(kR.data()), r_recv(kR.data()); + + int failures = 0; + for (int i = 0; i < 8; ++i) { + if (!relay_one(a_send, srv_recv_a, r_send, r_recv, 0x1111, plain, /*rewrite=*/false)) ++failures; + if (!relay_one(b_send, srv_recv_b, r_send, r_recv, 0x2222, plain, /*rewrite=*/false)) ++failures; + } + CHECK(failures > 0); // proves the verbatim-seq path is broken (locks in the regression) + } +} + int main() { if (sodium_init() < 0) { std::printf("FAIL: sodium_init failed\n"); @@ -158,6 +261,7 @@ int main() { test_anti_replay(); test_tamper_detection(); test_multiple_packets(); + test_relay_interleaved_reseal(); if (g_failures == 0) { std::printf("media_aead: all tests passed\n");