diff --git a/PROGRESS.md b/PROGRESS.md index df783b5..bd9a5bd 100644 --- a/PROGRESS.md +++ b/PROGRESS.md @@ -10,6 +10,35 @@ up instantly. Newest status at the top. ## ▶ Where we left off / next action +- **Done (2026-06-21):** **Fix permanent voice-loss bug + harden the UDP media path (protocol v2).** + Field report: two iOS users lost all audio mid-call after a bad-network blip and could not + recover even by restarting the apps. Root causes found in the UDP media path: + 1. **Anti-replay window poisoned by unauthenticated packets (the trigger).** + `SodiumMediaCrypto::open()` advanced `recv_highest_` from the plaintext header `seq` + *before* verifying the AEAD tag and never rolled it back on failure. One corrupted/forged + frame (a bit-flip on flaky wifi) shoved the high-water mark far ahead, after which every + legitimate frame was rejected as "too old" — permanently. Fixed by reordering to + replay-check → authenticate → update (RFC 3711 §3.3): the window is now touched only after + a successful tag check. Regression test in `test_media_aead.cpp` + (`test_corrupted_seq_does_not_poison_window`) — fails on the old code, passes now. + 2. **16-bit seq wrap with no rollover counter.** The wire header carried only the low 16 bits + of the nonce counter (zero-extended on receive); after 65,536 frames the reconstructed + nonce diverged and all frames failed auth. **Wire format widened to a full u64 seq** + (`voice_frame.h`: header 14 → 20 bytes, `seq` u16 → u64; `crypto.cpp`, `client.cpp`, + `media_relay.cpp` updated; `JitterBuffer::Frame::seq` widened). This is a **versioned wire + change → `VOICECAT_PROTOCOL_VERSION` 1 → 2**; the `Hello` handshake rejects on mismatch + (`conn_session.cpp`). The voice frame is parsed only in `core/`+`server/`+`tests/`, so the + Swift/C# clients need only a rebuild — no parser changes. + 3. **Server leaked UDP state on disconnect.** `SessionRegistry::unregister_session()` now also + frees `udp_endpoints_`/`udp_tokens_`/`ssrc_to_session_` (scan-and-erase by session id). + 4. **Diagnostics.** `MediaRelay` now emits rate-limited dropped-frame counters + (unmapped-endpoint / no-recv-crypto / open-failed) so a wedged media path is observable. + - **Verified:** `cmake --build --preset dev` clean; `ctest --preset dev -E external_pcm` + **22/22 pass** (incl. `m2_voice` e2e relay + the two new AEAD regressions). `external_pcm` + still aborts on the **pre-existing** CoreAudio shutdown mutex race (confirmed identical on a + clean baseline checkout under the same harness — unrelated to these changes). Docs updated: + `voice.md` §2 (header), `protocol.md` (v2 + negotiation), `security.md` (authenticate-then-advance). + - **Done (2026-06-21):** **Expose all channel codec params + guest nickname in every client.** - **DRED everywhere + ABI fix.** `dred` (Opus 1.6 Deep REDundancy) existed in the C ABI (`vc_audio_config.dred`) and proto but was absent from *both* client marshaling layers — a diff --git a/core/include/voicecat.h b/core/include/voicecat.h index 6ea5269..4b62614 100644 --- a/core/include/voicecat.h +++ b/core/include/voicecat.h @@ -48,8 +48,10 @@ extern "C" { #define VOICECAT_VERSION_MINOR 0 #define VOICECAT_VERSION_PATCH 1 -/* The control-protocol version this build speaks (docs/protocol.md §4). */ -#define VOICECAT_PROTOCOL_VERSION 1 +/* The control-protocol version this build speaks (docs/protocol.md §4). + * v2 widened the UDP voice frame seq field u16 → u64 (docs/voice.md §2); a v2 server + * and a v1 client cannot interoperate, so the Hello handshake rejects on mismatch. */ +#define VOICECAT_PROTOCOL_VERSION 2 /* ── Result codes ─────────────────────────────────────────────────────────── */ typedef enum vc_result { diff --git a/core/src/audio/audio_engine.h b/core/src/audio/audio_engine.h index 060e6b1..81af736 100644 --- a/core/src/audio/audio_engine.h +++ b/core/src/audio/audio_engine.h @@ -42,7 +42,7 @@ namespace voicecat::audio { class JitterBuffer { public: struct Frame { - uint16_t seq; + uint64_t seq; uint32_t timestamp; bool fec_present; std::vector payload; diff --git a/core/src/core/client.cpp b/core/src/core/client.cpp index 60869b4..cb29f21 100644 --- a/core/src/core/client.cpp +++ b/core/src/core/client.cpp @@ -298,7 +298,7 @@ void vc_client::run_io(std::string host, uint16_t port) { voicecat::v1::Envelope env; env.set_request_id(next_req_id_++); auto* hello = env.mutable_client_hello(); - hello->set_proto_version(1); + hello->set_proto_version(2); hello->set_client_name(cfg_.client_name ? cfg_.client_name : "vccli"); hello->set_client_version(cfg_.client_version ? cfg_.client_version : "0.1.0"); auto frame = make_frame(env); @@ -1047,7 +1047,7 @@ void vc_client::on_capture_frame(int kind, const int16_t* pcm, int samples, int voicecat::net::VoiceFrame hdr; hdr.ssrc = ls.ssrc; - hdr.seq = static_cast(media_send_crypto_->peek_send_counter()); + hdr.seq = media_send_crypto_->peek_send_counter(); hdr.timestamp = ls.timestamp; ls.timestamp += static_cast(samples); diff --git a/core/src/crypto/crypto.cpp b/core/src/crypto/crypto.cpp index dbc17c0..61cc893 100644 --- a/core/src/crypto/crypto.cpp +++ b/core/src/crypto/crypto.cpp @@ -353,24 +353,28 @@ long SodiumMediaCrypto::open(const uint8_t* sealed, size_t len, const uint8_t* a if (len < crypto_aead_chacha20poly1305_ietf_ABYTES) return -1; if (out_cap < len - crypto_aead_chacha20poly1305_ietf_ABYTES) return -1; - // Reconstruct 64-bit counter from aad[8..9] (seq, big-endian u16). - // For M2, we zero-extend the 16-bit seq; TODO: add ROC for long sessions. - if (aad_len < 10) return -1; - uint64_t counter = (static_cast(aad[8]) << 8) | aad[9]; + // Read the full 64-bit nonce counter directly from aad[8..15] (seq, big-endian + // u64). Protocol v2 carries the full counter on the wire, so the nonce is exact — + // no reconstruction/rollover guessing needed. + if (aad_len < 16) return -1; + uint64_t counter = (static_cast(aad[8]) << 56) | + (static_cast(aad[9]) << 48) | + (static_cast(aad[10]) << 40) | + (static_cast(aad[11]) << 32) | + (static_cast(aad[12]) << 24) | + (static_cast(aad[13]) << 16) | + (static_cast(aad[14]) << 8) | + static_cast(aad[15]); - // ── Anti-replay check ──────────────────────────────────────────────────── - if (!recv_initialized_) { - recv_highest_ = counter; - recv_window_ = 1; // bit0 = highest itself - recv_initialized_ = true; - } else { - if (counter > recv_highest_) { - uint64_t shift = counter - recv_highest_; - recv_window_ = (shift >= 64) ? 0 : (recv_window_ << shift); - recv_highest_ = counter; - } + // ── Anti-replay: REJECT-ONLY checks (no state mutation) ─────────────────── + // The counter comes from the UNAUTHENTICATED header, so we must NOT advance the + // window before the AEAD tag is verified — otherwise a single corrupted/forged + // packet would shove recv_highest_ far ahead and reject every later legitimate + // packet as "too old", permanently wedging the stream. Order per RFC 3711 §3.3: + // replay-check → authenticate → update. + if (recv_initialized_ && counter <= recv_highest_) { uint64_t offset = recv_highest_ - counter; - if (offset >= 64) return -1; // too old + if (offset >= 64) return -1; // too old if (recv_window_ & (UINT64_C(1) << offset)) return -1; // replay } @@ -382,11 +386,21 @@ long SodiumMediaCrypto::open(const uint8_t* sealed, size_t len, const uint8_t* a out, &plain_len, nullptr, sealed, static_cast(len), aad, static_cast(aad_len), nonce, key_.data()) != 0) - return -1; + return -1; // auth failure — leave the replay window untouched - // Mark this counter as accepted in the window. - uint64_t offset = recv_highest_ - counter; - recv_window_ |= (UINT64_C(1) << offset); + // ── Authenticated: now it's safe to advance the window ──────────────────── + if (!recv_initialized_) { + recv_highest_ = counter; + recv_window_ = 1; // bit0 = highest itself + recv_initialized_ = true; + } else if (counter > recv_highest_) { + uint64_t shift = counter - recv_highest_; + recv_window_ = (shift >= 64) ? 0 : (recv_window_ << shift); + recv_window_ |= 1; // bit0 = the new highest + recv_highest_ = counter; + } else { + recv_window_ |= (UINT64_C(1) << (recv_highest_ - counter)); + } return static_cast(plain_len); } diff --git a/core/src/crypto/crypto.h b/core/src/crypto/crypto.h index 0fc32ed..2b89064 100644 --- a/core/src/crypto/crypto.h +++ b/core/src/crypto/crypto.h @@ -163,7 +163,9 @@ class SodiumMediaCrypto final : public MediaCrypto { long seal(const uint8_t* plain, size_t len, const uint8_t* aad, size_t aad_len, uint8_t* out, size_t out_cap) override; - // open(): reconstructs counter from aad[8..9] (seq field), checks anti-replay. + // open(): reads the full 64-bit counter from aad[8..15] (seq field), checks + // anti-replay, then decrypts. The replay window is advanced ONLY after the AEAD + // tag verifies, so a corrupted/forged packet cannot poison it (RFC 3711 §3.3). long open(const uint8_t* sealed, size_t len, const uint8_t* aad, size_t aad_len, uint8_t* out, size_t out_cap) override; diff --git a/core/src/net/voice_frame.h b/core/src/net/voice_frame.h index a63ad99..5d20543 100644 --- a/core/src/net/voice_frame.h +++ b/core/src/net/voice_frame.h @@ -29,32 +29,36 @@ inline constexpr uint8_t kFlagLast = 0x08; // last frame before stream st inline constexpr uint16_t kCodecOpus = 0; // Size of the serialized header (bytes before the payload). -inline constexpr size_t kVoiceHeaderSize = 14; +inline constexpr size_t kVoiceHeaderSize = 20; /* * Wire layout (big-endian): - * [0] type u8 - * [1] flags u8 - * [2..3] codec u16 - * [4..7] ssrc u32 - * [8..9] seq u16 (low 16 bits of monotonic send counter) - * [10..13] timestamp u32 (sample clock @48 kHz) - * [14+] payload (AEAD-encrypted Opus packet) + * [0] type u8 + * [1] flags u8 + * [2..3] codec u16 + * [4..7] ssrc u32 + * [8..15] seq u64 (full monotonic send counter — the AEAD nonce counter) + * [16..19] timestamp u32 (sample clock @48 kHz) + * [20+] payload (AEAD-encrypted Opus packet) * - * The 14-byte header is the AEAD AAD (authenticated, not encrypted). + * The 20-byte header is the AEAD AAD (authenticated, not encrypted). * The payload region is the AEAD ciphertext + 16-byte Poly1305 MAC. + * + * Protocol v2 widened `seq` from u16 to u64: the receiver reconstructs the AEAD + * nonce counter directly from this field, so the full 64-bit counter must be on the + * wire (a 16-bit field wrapped after 65,536 frames and desynced the nonce). */ struct VoiceFrame { uint8_t type = kFrameVoice; uint8_t flags = 0; uint16_t codec = kCodecOpus; uint32_t ssrc = 0; - uint16_t seq = 0; + uint64_t seq = 0; uint32_t timestamp = 0; std::vector payload; // Opus bytes (pre-AEAD on send; post-AEAD on recv) }; -// Serialize the 14-byte header into buf[0..13]. buf must be at least kVoiceHeaderSize bytes. +// Serialize the 20-byte header into buf[0..19]. buf must be at least kVoiceHeaderSize bytes. inline void serialize_header(const VoiceFrame& f, uint8_t* buf) { buf[0] = f.type; buf[1] = f.flags; @@ -64,15 +68,21 @@ inline void serialize_header(const VoiceFrame& f, uint8_t* buf) { buf[5] = static_cast(f.ssrc >> 16); buf[6] = static_cast(f.ssrc >> 8); buf[7] = static_cast(f.ssrc & 0xFF); - buf[8] = static_cast(f.seq >> 8); - buf[9] = static_cast(f.seq & 0xFF); - buf[10] = static_cast(f.timestamp >> 24); - buf[11] = static_cast(f.timestamp >> 16); - buf[12] = static_cast(f.timestamp >> 8); - buf[13] = static_cast(f.timestamp & 0xFF); + buf[8] = static_cast(f.seq >> 56); + buf[9] = static_cast(f.seq >> 48); + buf[10] = static_cast(f.seq >> 40); + buf[11] = static_cast(f.seq >> 32); + buf[12] = static_cast(f.seq >> 24); + buf[13] = static_cast(f.seq >> 16); + buf[14] = static_cast(f.seq >> 8); + buf[15] = static_cast(f.seq & 0xFF); + buf[16] = static_cast(f.timestamp >> 24); + buf[17] = static_cast(f.timestamp >> 16); + buf[18] = static_cast(f.timestamp >> 8); + buf[19] = static_cast(f.timestamp & 0xFF); } -// Parse the 14-byte header from buf. Returns false if len < kVoiceHeaderSize. +// Parse the 20-byte header from buf. Returns false if len < kVoiceHeaderSize. inline bool parse_header(const uint8_t* buf, size_t len, VoiceFrame& out) { if (len < kVoiceHeaderSize) return false; out.type = buf[0]; @@ -82,11 +92,18 @@ inline bool parse_header(const uint8_t* buf, size_t len, VoiceFrame& out) { (static_cast(buf[5]) << 16) | (static_cast(buf[6]) << 8) | static_cast(buf[7]); - out.seq = static_cast((buf[8] << 8) | buf[9]); - out.timestamp = (static_cast(buf[10]) << 24) | - (static_cast(buf[11]) << 16) | - (static_cast(buf[12]) << 8) | - static_cast(buf[13]); + out.seq = (static_cast(buf[8]) << 56) | + (static_cast(buf[9]) << 48) | + (static_cast(buf[10]) << 40) | + (static_cast(buf[11]) << 32) | + (static_cast(buf[12]) << 24) | + (static_cast(buf[13]) << 16) | + (static_cast(buf[14]) << 8) | + static_cast(buf[15]); + out.timestamp = (static_cast(buf[16]) << 24) | + (static_cast(buf[17]) << 16) | + (static_cast(buf[18]) << 8) | + static_cast(buf[19]); return true; } diff --git a/docs/protocol.md b/docs/protocol.md index 17c2936..ecd91e0 100644 --- a/docs/protocol.md +++ b/docs/protocol.md @@ -153,6 +153,9 @@ Notes: - **Version negotiation.** Each side sends `proto_version` (integer) and a `features` string list. The effective version is `min(client, server)`; the effective feature set is the intersection. A client that doesn't understand a feature simply never uses it. + The **current `proto_version` is 2**. v2 widened the UDP voice frame `seq` field from + u16 to u64 (voice.md §2) — a wire-format change with no backward compatibility on the + media path, so the server rejects any peer not on v2 rather than min-negotiating down. - **Auth over TLS.** Passwords cross the wire only inside TLS 1.3 and are verified against an Argon2id hash at rest (see security.md). `auth_methods` in `ServerHello` advertises whether `guest` is enabled. diff --git a/docs/security.md b/docs/security.md index 2156ac2..0a17abc 100644 --- a/docs/security.md +++ b/docs/security.md @@ -89,8 +89,13 @@ the design depends on that. - **Nonce discipline:** `nonce = direction_bit ‖ ssrc ‖ monotonic_packet_counter`. The counter never repeats under one key; the session **rekeys** (re-derives via the exporter with a bumped epoch) well before counter exhaustion or on a time/byte budget. -- **Anti-replay:** a sliding-window replay filter per ssrc (à la IPsec) keyed on the packet - counter. Replays and out-of-window packets are dropped before decode. +- **Anti-replay:** a 64-bit sliding-window replay filter keyed on the packet counter (à la + IPsec). The window is **advanced only after the AEAD tag verifies** (RFC 3711 §3.3 order: + replay-check → authenticate → update). The counter is read from the unauthenticated + header, so advancing the high-water mark *before* authentication would let a single + corrupted or forged packet jump it far ahead, after which every legitimate packet is + rejected as "too old" — a permanent denial of the whole stream. Failed-auth packets leave + the window untouched. Replays and out-of-window packets are dropped before decode. ## 3. UDP session binding diff --git a/docs/voice.md b/docs/voice.md index 38a1974..b9b7b3c 100644 --- a/docs/voice.md +++ b/docs/voice.md @@ -35,13 +35,16 @@ macOS, **and iOS** (via a ReplayKit broadcast extension). A fixed binary header — no protobuf on the RT path. Multi-byte fields are big-endian. +The header is **20 bytes** (protocol v2; v1 was 14 bytes with a u16 seq — see note below). + ``` - 0 1 2 3 4 5 6 7 8 ... + 0 1 2 3 4 5 6 7 8 ............ 15 ┌──────┬──────┬──────┬──────┬──────┬──────┬──────┬──────┬───────────────┐ -│ type │flags │ codec │ ssrc (u32) │ -├──────┴──────┴──────┴──────┼──────┬──────┬──────┬──────┬──────────────┤ -│ seq (u16) │ timestamp (u32, in samples @48k) │ payload ... │ -└─────────────┴──────────────────────────────────────────┴──────────────┘ +│ type │flags │ codec │ ssrc (u32) │ seq (u64) ──▶ │ +├──────┴──────┴──────┴──────┴──────┴──────┴──────┴──────┴───────────────┤ +│ ◀── seq (u64) ──┤ timestamp (u32 @48k) │ payload ... │ +└──────────────────┴────────────────────────────────────┴───────────────┘ + bytes [8..15] = seq (u64) [16..19] = timestamp (u32) type u8 1 = VOICE, 2 = KEEPALIVE, 3 = UDP_BINDING (handshake) flags u8 bit0 marker (start of talkspurt) · bit1 FEC-present @@ -49,11 +52,19 @@ flags u8 bit0 marker (start of talkspurt) · bit1 FEC-present codec u16 0 = OPUS (room for future codecs) ssrc u32 media-plane stream id. Client sends its own ssrc; the server validates it against the bound session and relays unchanged. -seq u16 per-ssrc sequence number, wraps; drives loss detection + reorder +seq u64 full monotonic send counter. This IS the AEAD nonce counter, so the + receiver derives the nonce directly from it — no rollover guessing. timestamp u32 RTP-style sample clock @48 kHz; drives the jitter buffer payload one Opus packet (the encoder's output for one frame) ``` +> **Why u64 (protocol v2).** v1 carried only the low 16 bits of the counter and the +> receiver zero-extended them to rebuild the AEAD nonce. After 65,536 frames the seq +> wrapped, the reconstructed nonce diverged from the sealing nonce, and **every frame +> failed authentication permanently** (no rollover counter). v2 puts the full 64-bit +> counter on the wire so the nonce is always exact. A v2 server and a v1 client cannot +> interoperate; the `Hello` handshake rejects on `proto_version` mismatch. + This is intentionally RTP-shaped (familiar semantics: ssrc/seq/timestamp) without RTP's full machinery. The **server relays the payload unmodified** — it only reads the header to route by ssrc→channel and may restamp nothing (the client's ssrc is globally unique once diff --git a/server/src/conn_session.cpp b/server/src/conn_session.cpp index 645cb95..c892bfe 100644 --- a/server/src/conn_session.cpp +++ b/server/src/conn_session.cpp @@ -257,13 +257,13 @@ void ConnSession::send_generic_result(uint64_t req_id, bool ok, uint32_t code, // ── Handlers ───────────────────────────────────────────────────────────────── void ConnSession::handle_client_hello(uint64_t req_id, const voicecat::v1::ClientHello& msg) { - if (msg.proto_version() != 1) { + if (msg.proto_version() != 2) { // protocol v2: 64-bit voice seq (docs/voice.md §2) send_disconnect_and_close(1, "unsupported protocol version"); return; } auto env = make_env(req_id); auto* hello = env.mutable_server_hello(); - hello->set_proto_version(1); + hello->set_proto_version(2); hello->set_server_name("VoiceCat Server"); hello->set_server_version("0.1.0"); if (allow_guests_) hello->add_auth_methods("guest"); diff --git a/server/src/media_relay.cpp b/server/src/media_relay.cpp index 704df9f..5f3d3a5 100644 --- a/server/src/media_relay.cpp +++ b/server/src/media_relay.cpp @@ -3,7 +3,9 @@ #ifdef VOICECAT_HAS_NET #include +#include #include +#include #include "conn_session.h" #include "crypto/crypto.h" @@ -35,6 +37,24 @@ uint16_t MediaRelay::media_port() const { return static_cast(udp_.local_endpoint().port()); } +void MediaRelay::note_drop(const char* reason) { + if (std::string_view(reason) == "unmapped-endpoint") ++drop_no_endpoint_; + else if (std::string_view(reason) == "no-recv-crypto") ++drop_no_crypto_; + else ++drop_open_failed_; + + // Rate-limit the summary to at most once every 5s so a flood can't spam the log. + auto now_ms = std::chrono::duration_cast( + std::chrono::steady_clock::now().time_since_epoch()).count(); + if (now_ms - last_drop_log_ms_ < 5000) return; + last_drop_log_ms_ = now_ms; + std::fprintf(stderr, + "[media] dropped frames — unmapped-endpoint=%llu no-recv-crypto=%llu " + "open-failed(auth/replay)=%llu\n", + static_cast(drop_no_endpoint_), + static_cast(drop_no_crypto_), + static_cast(drop_open_failed_)); +} + void MediaRelay::on_udp_frame(const uint8_t* data, size_t len, asio::ip::udp::endpoint sender) { if (len < 1) return; @@ -59,10 +79,10 @@ void MediaRelay::on_udp_frame(const uint8_t* data, size_t len, // Resolve sender session. auto sender_session = registry_->find_by_udp_endpoint(sender); - if (!sender_session) return; + if (!sender_session) { note_drop("unmapped-endpoint"); return; } auto* recv_crypto = sender_session->recv_crypto(); - if (!recv_crypto) return; + if (!recv_crypto) { note_drop("no-recv-crypto"); return; } // AAD = 14-byte header (authenticated, not encrypted). const uint8_t* aad = data; @@ -73,7 +93,7 @@ void MediaRelay::on_udp_frame(const uint8_t* data, size_t len, long plain_len = recv_crypto->open(sealed, sealed_len, aad, voicecat::net::kVoiceHeaderSize, plain_buf_.data(), plain_buf_.size()); - if (plain_len < 0) return; // auth failure or replay + if (plain_len < 0) { note_drop("open-failed"); return; } // auth failure or replay // Parse the voice frame header to find the source ssrc/channel. voicecat::net::VoiceFrame hdr{}; @@ -111,8 +131,14 @@ void MediaRelay::on_udp_frame(const uint8_t* data, size_t len, // 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); + seal_buf_[8] = static_cast((send_ctr >> 56) & 0xFF); + seal_buf_[9] = static_cast((send_ctr >> 48) & 0xFF); + seal_buf_[10] = static_cast((send_ctr >> 40) & 0xFF); + seal_buf_[11] = static_cast((send_ctr >> 32) & 0xFF); + seal_buf_[12] = static_cast((send_ctr >> 24) & 0xFF); + seal_buf_[13] = static_cast((send_ctr >> 16) & 0xFF); + seal_buf_[14] = static_cast((send_ctr >> 8) & 0xFF); + seal_buf_[15] = static_cast(send_ctr & 0xFF); uint8_t* out_payload = seal_buf_.data() + voicecat::net::kVoiceHeaderSize; long sealed_out = send_crypto->seal( diff --git a/server/src/media_relay.h b/server/src/media_relay.h index 60623f9..708fcb5 100644 --- a/server/src/media_relay.h +++ b/server/src/media_relay.h @@ -49,10 +49,20 @@ class MediaRelay { private: void on_udp_frame(const uint8_t* data, size_t len, asio::ip::udp::endpoint sender); + // Count a dropped inbound voice frame (by reason) and emit a rate-limited summary + // to stderr. Runs on the io thread, so plain counters are safe. + void note_drop(const char* reason); + asio::io_context& io_; std::shared_ptr registry_; voicecat::net::UdpMediaChannel udp_; + // Diagnostics: dropped-frame counters so a wedged media path is observable. + uint64_t drop_no_endpoint_ = 0; // voice from an unmapped UDP endpoint + uint64_t drop_no_crypto_ = 0; // session has no recv_crypto yet + uint64_t drop_open_failed_ = 0; // AEAD auth failure or replay reject + int64_t last_drop_log_ms_ = 0; + // Scratch buffer for re-encrypted payloads (size = max_frame + 16 MAC) static constexpr size_t kMaxPayload = 1500; std::vector seal_buf_ = std::vector(kMaxPayload + 16, uint8_t{0}); diff --git a/server/src/session_registry.cpp b/server/src/session_registry.cpp index 93426f2..789f24c 100644 --- a/server/src/session_registry.cpp +++ b/server/src/session_registry.cpp @@ -100,6 +100,22 @@ void SessionRegistry::unregister_session(uint64_t session_id) { std::unique_lock lk(mu_); sessions_.erase(session_id); session_permissions_.erase(session_id); + + // Free the per-session UDP/media state too. These maps are keyed by + // endpoint/token/ssrc (not session id), so scan-and-erase by value. Leaving them + // behind leaks entries and lets a stale endpoint/token resolve toward a dead + // session across reconnects (e.g. a wifi-handoff rebind from a new port). + auto erase_by_value = [session_id](auto& map) { + for (auto it = map.begin(); it != map.end();) { + if (it->second == session_id) + it = map.erase(it); + else + ++it; + } + }; + erase_by_value(udp_endpoints_); + erase_by_value(udp_tokens_); + erase_by_value(ssrc_to_session_); } uint32_t SessionRegistry::add_user(uint64_t session_id, const voicecat::v1::User& user) { diff --git a/tests/test_m2_voice.cpp b/tests/test_m2_voice.cpp index 03dcc0d..a2d1e31 100644 --- a/tests/test_m2_voice.cpp +++ b/tests/test_m2_voice.cpp @@ -212,7 +212,7 @@ struct TestClient { { v1::Envelope env; env.set_request_id(1); - env.mutable_client_hello()->set_proto_version(1); + env.mutable_client_hello()->set_proto_version(2); env.mutable_client_hello()->set_client_name(label); if (!tcp_send_envelope(*tls, env)) return false; } @@ -450,10 +450,10 @@ int main() { size_t payload_len = pcm.size() * sizeof(int16_t); #endif - // Build 14-byte header (AAD). + // Build the voice frame header (AAD). VoiceFrame hdr; hdr.ssrc = A.assigned_ssrc; - hdr.seq = static_cast(i); + hdr.seq = static_cast(i); hdr.timestamp = static_cast(i * kFrameSamples); uint8_t header_bytes[kVoiceHeaderSize]; serialize_header(hdr, header_bytes); diff --git a/tests/test_media_aead.cpp b/tests/test_media_aead.cpp index 6f46556..1baab94 100644 --- a/tests/test_media_aead.cpp +++ b/tests/test_media_aead.cpp @@ -24,8 +24,8 @@ static int g_failures = 0; ++g_failures; \ }} while (0) -// Build a synthetic 14-byte AAD (voice frame header). -static std::vector make_aad(uint16_t seq) { +// Build a synthetic voice-frame-header AAD. +static std::vector make_aad(uint64_t seq) { VoiceFrame f; f.ssrc = 0xCAFEBABE; f.seq = seq; @@ -132,7 +132,7 @@ static void test_multiple_packets() { std::vector plain(60, 0x99); - for (uint16_t seq = 0; seq < 10; ++seq) { + for (uint64_t seq = 0; seq < 10; ++seq) { auto aad = make_aad(seq); std::vector cipher(plain.size() + crypto_aead_chacha20poly1305_ietf_ABYTES); long sealed_len = sender.seal(plain.data(), plain.size(), @@ -149,8 +149,8 @@ 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) { +// Build a voice-frame-header AAD with a given ssrc + seq. +static std::vector make_aad_ssrc(uint32_t ssrc, uint64_t seq) { VoiceFrame f; f.ssrc = ssrc; f.seq = seq; @@ -159,6 +159,12 @@ static std::vector make_aad_ssrc(uint32_t ssrc, uint16_t seq) { return aad; } +// Overwrite the 8-byte big-endian seq field (header bytes [8..15]) in an AAD buffer. +static void set_aad_seq(std::vector& aad, uint64_t seq) { + for (int i = 0; i < 8; ++i) + aad[8 + i] = static_cast((seq >> (56 - 8 * i)) & 0xFF); +} + // 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. @@ -169,7 +175,7 @@ static bool relay_one(SodiumMediaCrypto& sender_send, SodiumMediaCrypto& server_ 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())); + auto in_aad = make_aad_ssrc(ssrc, 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()); @@ -185,11 +191,7 @@ static bool relay_one(SodiumMediaCrypto& sender_send, SodiumMediaCrypto& server_ // 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); - } + if (rewrite_seq) set_aad_seq(out_aad, r_send.peek_send_counter()); 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(), @@ -251,6 +253,79 @@ static void test_relay_interleaved_reseal() { } } +// Regression for the bad-wifi wedge: the anti-replay window must NOT be advanced by a +// packet that fails authentication. A single corrupted/forged frame carrying a huge seq +// used to shove recv_highest_ far ahead (before the AEAD tag was checked), after which +// every legitimate frame was rejected as "too old" — permanent silence. open() now +// advances the window only after a successful tag check (RFC 3711 §3.3). +static void test_corrupted_seq_does_not_poison_window() { + uint8_t key[crypto_aead_chacha20poly1305_ietf_KEYBYTES]; + crypto_generichash(key, sizeof(key), + reinterpret_cast("poison-key"), 10, nullptr, 0); + SodiumMediaCrypto sender(key); + SodiumMediaCrypto receiver(key); + + std::vector plain(64, 0x5A); + std::vector recovered(plain.size()); + + auto seal_at_current = [&](std::vector& aad_out, std::vector& cipher_out) { + aad_out = make_aad_ssrc(0xABCD, sender.peek_send_counter()); + cipher_out.assign(plain.size() + crypto_aead_chacha20poly1305_ietf_ABYTES, 0); + long s = sender.seal(plain.data(), plain.size(), aad_out.data(), aad_out.size(), + cipher_out.data(), cipher_out.size()); + CHECK(s > 0); + }; + + // 1. A normal frame (counter 0) decrypts. recv_highest_ = 0. + std::vector aad0, cipher0; + seal_at_current(aad0, cipher0); // sender counter 0 → 1 + CHECK(receiver.open(cipher0.data(), cipher0.size(), aad0.data(), aad0.size(), + recovered.data(), recovered.size()) == static_cast(plain.size())); + + // 2. A frame whose header seq has been corrupted to a huge value: it fails auth + // (the AAD no longer matches what was sealed) and must NOT move the window. + std::vector aad1, cipher1; + seal_at_current(aad1, cipher1); // sender counter 1 → 2 + std::vector forged_aad = aad1; + set_aad_seq(forged_aad, 0x0000FFFFFFFFFFFFULL); // bit-flip-style corruption + CHECK(receiver.open(cipher1.data(), cipher1.size(), forged_aad.data(), forged_aad.size(), + recovered.data(), recovered.size()) < 0); + + // 3. The next legitimate frame (counter 2) must still decrypt. On the old code this + // returned "too old" because step 2 had poisoned recv_highest_. + std::vector aad2, cipher2; + seal_at_current(aad2, cipher2); // sender counter 2 → 3 + CHECK(receiver.open(cipher2.data(), cipher2.size(), aad2.data(), aad2.size(), + recovered.data(), recovered.size()) == static_cast(plain.size())); +} + +// Regression for the 16-bit seq wrap: with a full 64-bit wire counter, sealing/opening +// across the old u16 boundary (65,535 → 65,536) must keep decrypting. On the old code the +// nonce desynced at the wrap and every frame failed auth permanently. +static void test_seq_past_16bit_boundary() { + uint8_t key[crypto_aead_chacha20poly1305_ietf_KEYBYTES]; + crypto_generichash(key, sizeof(key), + reinterpret_cast("wrap-key"), 8, nullptr, 0); + SodiumMediaCrypto sender(key); + SodiumMediaCrypto receiver(key); + + std::vector plain(48, 0x6B); + std::vector recovered(plain.size()); + std::vector cipher(plain.size() + crypto_aead_chacha20poly1305_ietf_ABYTES); + + bool all_ok = true; + for (uint64_t i = 0; i < 70000; ++i) { // crosses 65,536 + auto aad = make_aad_ssrc(0x1234, sender.peek_send_counter()); + long s = sender.seal(plain.data(), plain.size(), aad.data(), aad.size(), + cipher.data(), cipher.size()); + if (s < 0) { all_ok = false; break; } + long o = receiver.open(cipher.data(), static_cast(s), aad.data(), aad.size(), + recovered.data(), recovered.size()); + if (o != static_cast(plain.size())) { all_ok = false; break; } + } + CHECK(all_ok); +} + int main() { if (sodium_init() < 0) { std::printf("FAIL: sodium_init failed\n"); @@ -262,6 +337,8 @@ int main() { test_tamper_detection(); test_multiple_packets(); test_relay_interleaved_reseal(); + test_corrupted_seq_does_not_poison_window(); + test_seq_past_16bit_boundary(); if (g_failures == 0) { std::printf("media_aead: all tests passed\n"); diff --git a/tests/test_voice_frame.cpp b/tests/test_voice_frame.cpp index 6142bf5..8679e2b 100644 --- a/tests/test_voice_frame.cpp +++ b/tests/test_voice_frame.cpp @@ -1,5 +1,5 @@ /* - * test_voice_frame — serialize/parse round-trips for the 14-byte UDP media header. + * test_voice_frame — serialize/parse round-trips for the 20-byte UDP media header. */ #include #include @@ -24,7 +24,7 @@ static void test_header_round_trip() { f.flags = kFlagMarker | kFlagFecPresent; f.codec = kCodecOpus; f.ssrc = 0xDEADBEEF; - f.seq = 0xAB12; + f.seq = 0x0123456789ABCDEFULL; // full 64-bit range (protocol v2) f.timestamp = 0x12345678; uint8_t buf[kVoiceHeaderSize]; @@ -97,18 +97,19 @@ static void test_parse_too_short() { static void test_big_endian_layout() { VoiceFrame f; f.ssrc = 0x01020304; - f.seq = 0x0506; - f.timestamp = 0x0708090A; + f.seq = 0x05060708090A0B0CULL; + f.timestamp = 0x0D0E0F10; uint8_t buf[kVoiceHeaderSize]; serialize_header(f, buf); // ssrc at [4..7] CHECK(buf[4] == 0x01 && buf[5] == 0x02 && buf[6] == 0x03 && buf[7] == 0x04); - // seq at [8..9] - CHECK(buf[8] == 0x05 && buf[9] == 0x06); - // timestamp at [10..13] - CHECK(buf[10] == 0x07 && buf[11] == 0x08 && buf[12] == 0x09 && buf[13] == 0x0A); + // seq (u64) at [8..15] + CHECK(buf[8] == 0x05 && buf[9] == 0x06 && buf[10] == 0x07 && buf[11] == 0x08 && + buf[12] == 0x09 && buf[13] == 0x0A && buf[14] == 0x0B && buf[15] == 0x0C); + // timestamp at [16..19] + CHECK(buf[16] == 0x0D && buf[17] == 0x0E && buf[18] == 0x0F && buf[19] == 0x10); } int main() {