From a6706eee2888a6a1ffa7c99cf11f76076dac76ee Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 26 Oct 2026 21:11:39 -0700 Subject: [PATCH 1/2] chore(quest): claim quest/m1/cut-audit/announce-decode-complete.md Quest-Claim: f428bc20d9103f59 From 4a58bf37ab41dadd31a547dab95259369e72b259 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 26 Oct 2026 21:20:44 -0700 Subject: [PATCH 2/2] fix(net): refuse malformed complete announce bodies Co-Authored-By: GPT-6 --- quest/m1/cut-audit/README.md | 1 - .../m1/cut-audit/announce-decode-complete.md | 38 ----------- rs/moq-net/fuzz/README.md | 7 ++- .../regressions/lite/complete-announce-lite05 | 1 + .../regressions/lite/complete-announce-lite06 | Bin 0 -> 5 bytes .../regressions/lite/complete-announce-lite07 | Bin 0 -> 5 bytes rs/moq-net/src/fuzz.rs | 29 ++++++++- rs/moq-net/src/lite/announce.rs | 59 +++++++++--------- rs/moq-net/src/lite/subscriber.rs | 41 ++++++++++++ 9 files changed, 104 insertions(+), 72 deletions(-) delete mode 100644 quest/m1/cut-audit/announce-decode-complete.md create mode 100644 rs/moq-net/fuzz/regressions/lite/complete-announce-lite05 create mode 100644 rs/moq-net/fuzz/regressions/lite/complete-announce-lite06 create mode 100644 rs/moq-net/fuzz/regressions/lite/complete-announce-lite07 diff --git a/quest/m1/cut-audit/README.md b/quest/m1/cut-audit/README.md index a84b26c68c0..fd7d37ec3c2 100644 --- a/quest/m1/cut-audit/README.md +++ b/quest/m1/cut-audit/README.md @@ -59,7 +59,6 @@ leaving JS-only redesigns of `js/net` to [rs2ts](/quest/m2/rs2ts/README.md). - [A route too large to re-encode is skipped](/quest/m1/cut-audit/announce-reencode.md) - one oversized route never resets a downstream announce stream - [A reserved group survives a route returning](/quest/m1/cut-audit/reserved-route-return.md) - a route flip A to B to A no longer panics a peeked reader - [JS tracks end clean only on a deliberate finish](/quest/m1/cut-audit/js-close-cause.md) - local close, violations, and unanswered requests end with errors -- [A malformed lite message closes the session](/quest/m1/cut-audit/announce-decode-complete.md) - a bad ANNOUNCE body no longer jams its stream - [Withdrawing a moq-transport FETCH or TRACK_STATUS ends it](/quest/m1/cut-audit/ietf-withdraw-resolve.md) - withdrawal during resolution releases the request - [JS moq-transport sessions close on peer violations](/quest/m1/cut-audit/js-ietf-violations.md) - right close codes, no unhandled rejection, bounded reflected namespaces - [A media hole in the player keeps the timeline](/quest/m1/cut-audit/watch-audio-hole.md) - a skipped group re-primes decoders without flushing audio or re-anchoring sync diff --git a/quest/m1/cut-audit/announce-decode-complete.md b/quest/m1/cut-audit/announce-decode-complete.md deleted file mode 100644 index b4591952838..00000000000 --- a/quest/m1/cut-audit/announce-decode-complete.md +++ /dev/null @@ -1,38 +0,0 @@ -# [S] A malformed lite message closes the session instead of jamming its stream - -## Goal - -A lite ANNOUNCE body whose fields run past its own declared length closes the -session with PROTOCOL_VIOLATION, as JS already does, instead of leaving the -announce stream waiting for bytes that will never come. Lite fuzzing asserts -that no complete body ever decodes as `Short`, and covers the lite-07 TRACKS -and AUTH decoders. - -## Plan - -Found by the 2026-10-24 pre-cut audit at 151a529670. Codec half reproduced -(`decode_slice` returns `Short` for `[0,1,5,..]` on lite-06/07 and `[1,1]` on -lite-05); the jam is source-inferred. Inherited. - -`lite::Announce::decode` (`rs/moq-net/src/lite/announce.rs:340-381`) decodes -the lite-06+ body and the legacy body inside `buf.sub(size)` without -`DecodeError::complete`, unlike `message.rs` and `tracks.rs`. A Hop Count -larger than the hops that follow returns `Short`; `Reader::poll_with_sized` -(`coding/reader.rs:147`) then grows its window a byte at a time, and -`lite/error.rs:34` deliberately never maps `Short` to a violation. Every -later announcement re-runs the failed decode until FIN. - -- Decode both bodies with `.map_err(DecodeError::complete)`, or through - `message::decode_with`. Grep the lite and IETF codecs for any other `sub()` - body decoded without it. -- In `fuzz.rs` `roundtrip`, assert `Err(Short)` implies the decoder needed - more bytes than it was given, so this class fails the fuzzer. -- Add `lite_wire` arms for `Tracks`/`TracksEvent` and - `Auth`/`AuthOk`/`AuthError`/`AuthReply`, all peer-received lite-07 - decoders with no target today. - -Public API: none. Wire: none. - -## Related - -- [Wire fuzz coverage](/quest/m2/wire-fuzz-coverage.md) - the remaining decoder and unsafe targets diff --git a/rs/moq-net/fuzz/README.md b/rs/moq-net/fuzz/README.md index 566ba0c3067..6c877b91a13 100644 --- a/rs/moq-net/fuzz/README.md +++ b/rs/moq-net/fuzz/README.md @@ -40,6 +40,8 @@ symbol stripping. Prefix direct calls with Beyond "does not panic": +- A short decode requires more outer input. A field running short inside a complete + length-prefixed body is malformed and cannot ask for bytes from the next message. - Whatever we encode, we decode back, consuming every byte. The peer's decoder is this same code, so emitting something we would reject is a bug on the wire. - Encoding is stable: encode, decode, encode again yields identical bytes. Skipped @@ -53,8 +55,9 @@ Beyond "does not panic": - `Path::relative` inverts `Path::resolve`, and never produces a reference that walks above the root. -A decode that fails is not a finding, and neither is an encode that fails: a value can -decode at a version that cannot express it again. +A refused decode is not a finding unless it asks for more bytes despite a complete +body. An encode failure is legitimate: a value can decode at a version that cannot +express it again. ## When something crashes diff --git a/rs/moq-net/fuzz/regressions/lite/complete-announce-lite05 b/rs/moq-net/fuzz/regressions/lite/complete-announce-lite05 new file mode 100644 index 00000000000..0a8cb8c252d --- /dev/null +++ b/rs/moq-net/fuzz/regressions/lite/complete-announce-lite05 @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/rs/moq-net/fuzz/regressions/lite/complete-announce-lite06 b/rs/moq-net/fuzz/regressions/lite/complete-announce-lite06 new file mode 100644 index 0000000000000000000000000000000000000000..2ae928704e6b9a797083c019ad24acb8bbaba938 GIT binary patch literal 5 McmZQ&Wng3l0021v5dZ)H literal 0 HcmV?d00001 diff --git a/rs/moq-net/fuzz/regressions/lite/complete-announce-lite07 b/rs/moq-net/fuzz/regressions/lite/complete-announce-lite07 new file mode 100644 index 0000000000000000000000000000000000000000..1b15c27bc70c5055ae72897f1bfba6139fbf8824 GIT binary patch literal 5 McmZQ$Wng3l002G!5&!@I literal 0 HcmV?d00001 diff --git a/rs/moq-net/src/fuzz.rs b/rs/moq-net/src/fuzz.rs index 602c78a6034..d226d30515e 100644 --- a/rs/moq-net/src/fuzz.rs +++ b/rs/moq-net/src/fuzz.rs @@ -105,7 +105,7 @@ const IETF_VERSIONS: &[ietf::Version] = &[ ]; /// How many types [`lite_wire`] dispatches over. -const LITE_KINDS: u8 = 22; +const LITE_KINDS: u8 = 28; /// How many types [`ietf_wire`] dispatches over. const IETF_KINDS: u8 = 44; @@ -131,8 +131,14 @@ where T: Decode + Encode, V: Copy + Into
, { - let Ok((decoded, _)) = T::decode_slice(data, version) else { - return false; + let mut reader = Decoder::new(data, version.into()); + let decoded = match T::decode(&mut reader, version) { + Ok(decoded) => decoded, + Err(coding::DecodeError::Short) => { + assert!(reader.need() > data.len(), "short read inside a complete body"); + return false; + } + Err(_) => return false, }; let Ok(first) = decoded.encode_bytes(version) else { @@ -224,6 +230,12 @@ pub fn lite_wire(data: &[u8]) -> bool { 19 => datagram(rest, version), 20 => roundtrip::(rest, version), 21 => roundtrip::(rest, version), + 22 => roundtrip::, _>(rest, version), + 23 => roundtrip::, _>(rest, version), + 24 => roundtrip::(rest, version), + 25 => roundtrip::(rest, version), + 26 => roundtrip::(rest, version), + 27 => roundtrip::(rest, version), _ => unreachable!("kind is taken modulo LITE_KINDS"), } } @@ -1074,6 +1086,17 @@ pub fn seeds() -> Vec { } } + // A typed empty TRACKS_SYNCED and AUTH_OK reply need nonuniform envelope bytes. + for (kind, body) in [(23, &[4, 0][..]), (27, &[0, 3, 0, 0, 0][..])] { + let mut data = vec![(LITE_VERSIONS.len() - 1) as u8, kind]; + data.extend_from_slice(body); + seeds.push(Seed { + target: "lite", + kind, + data, + }); + } + for version in 0..IETF_VERSIONS.len() as u8 { for kind in 0..IETF_KINDS { for fill in FILLS { diff --git a/rs/moq-net/src/lite/announce.rs b/rs/moq-net/src/lite/announce.rs index 834ed8423ac..6d955150ade 100644 --- a/rs/moq-net/src/lite/announce.rs +++ b/rs/moq-net/src/lite/announce.rs @@ -339,33 +339,36 @@ impl Decode for AnnounceBroadcast<'_> { let typ = buf.varint()?; let size = decode_size(buf, MAX_MESSAGE_SIZE)?; let mut body = buf.sub(size)?; - let msg = match typ { - ANNOUNCE_START => Self::Active { - suffix: PathRef::decode(&mut body, version)?, - epoch: super::epoch::decode_epoch(&mut body, version)?, - hops: HopsRef::decode(&mut body, version)?, - cost: Cost::decode(&mut body, version)?, - }, - SYNCED if version.has_synced() => Self::Synced, - ANNOUNCE_END => Self::EndedId { id: body.varint()? }, - ANNOUNCE_UPDATE => Self::Update { - id: body.varint()?, - hops: HopsRef::decode(&mut body, version)?, - cost: Cost::decode(&mut body, version)?, - }, - ANNOUNCE_RESTART if version.has_announce_restart() => Self::Restart { - id: body.varint()?, - epoch: super::epoch::decode_epoch(&mut body, version)?, - hops: HopsRef::decode(&mut body, version)?, - cost: Cost::decode(&mut body, version)?, - }, - // Unknown types are skipped by length so an earlier Lite06 build - // negotiating the same ALPN does not kill the announce stream. - _ => { - body.rest(); - Self::Skipped - } - }; + let msg = (|| { + Ok(match typ { + ANNOUNCE_START => Self::Active { + suffix: PathRef::decode(&mut body, version)?, + epoch: super::epoch::decode_epoch(&mut body, version)?, + hops: HopsRef::decode(&mut body, version)?, + cost: Cost::decode(&mut body, version)?, + }, + SYNCED if version.has_synced() => Self::Synced, + ANNOUNCE_END => Self::EndedId { id: body.varint()? }, + ANNOUNCE_UPDATE => Self::Update { + id: body.varint()?, + hops: HopsRef::decode(&mut body, version)?, + cost: Cost::decode(&mut body, version)?, + }, + ANNOUNCE_RESTART if version.has_announce_restart() => Self::Restart { + id: body.varint()?, + epoch: super::epoch::decode_epoch(&mut body, version)?, + hops: HopsRef::decode(&mut body, version)?, + cost: Cost::decode(&mut body, version)?, + }, + // Unknown types are skipped by length so an earlier Lite06 build + // negotiating the same ALPN does not kill the announce stream. + _ => { + body.rest(); + Self::Skipped + } + }) + })() + .map_err(DecodeError::complete)?; if !body.is_empty() { return Err(DecodeError::Long); } @@ -375,7 +378,7 @@ impl Decode for AnnounceBroadcast<'_> { // Older versions: a single size-prefixed ANNOUNCE_BROADCAST with an inner status. let size = decode_size(buf, MAX_MESSAGE_SIZE)?; let mut body = buf.sub(size)?; - let msg = Self::decode_legacy(&mut body, version)?; + let msg = Self::decode_legacy(&mut body, version).map_err(DecodeError::complete)?; if !body.is_empty() { return Err(DecodeError::Long); } diff --git a/rs/moq-net/src/lite/subscriber.rs b/rs/moq-net/src/lite/subscriber.rs index 3ccbdf5c7ae..aa3a14bea19 100644 --- a/rs/moq-net/src/lite/subscriber.rs +++ b/rs/moq-net/src/lite/subscriber.rs @@ -5930,6 +5930,47 @@ mod tests { assert!(cursor.is_synced()); } + /// A complete body cannot be repaired by bytes from the next announcement. + /// Keep the peer stream open: FIN must not be what discovers the malformed body. + #[moq_clock::test] + async fn malformed_complete_announce_closes_before_more_bytes() { + for version in [Version::Lite05, Version::Lite06, Version::Lite07] { + let mut script = announce_script(version, &[&announce_ok(version, 1)]); + script.extend_from_slice(match version { + Version::Lite05 => &[1, 1], + _ => &[0, 1, 5], + }); + let session = crate::lite::test_transport::ScriptedSession::new(script); + let log = session.log.clone(); + let (mut prefix, _consumer) = root_prefix(version, session); + let result = prefix.poll(&kio::Waiter::noop()); + assert!( + matches!(result, Poll::Ready(Err(Error::ProtocolViolation))), + "{version:?}: {result:?}" + ); + assert_eq!(log.closes()[0].0, crate::SessionError::ProtocolViolation.to_code()); + } + } + + /// Only an incomplete outer frame waits, then resumes on its missing byte. + #[moq_clock::test] + async fn an_incomplete_announce_waits_for_its_body() { + for version in [Version::Lite05, Version::Lite06, Version::Lite07] { + let mut script = announce_script(version, &[&announce_ok(version, 1), &announce_start(version, "room")]); + let last = script.pop().unwrap(); + let session = crate::lite::test_transport::ScriptedSession::new(script); + let log = session.log.clone(); + let (mut prefix, consumer) = root_prefix(version, session.clone()); + let mut cursor = consumer.announced(); + assert!(prefix.poll(&kio::Waiter::noop()).is_pending()); + assert!(log.closes().is_empty()); + session.push(&[last]); + assert!(prefix.poll(&kio::Waiter::noop()).is_pending()); + cursor.assert_next_active("room"); + assert!(log.closes().is_empty()); + } + } + #[moq_clock::test] async fn a_duplicate_synced_is_refused() { const VERSION: Version = Version::Lite07;