From 3736bbdc730414467232ab258b5de48d57bc723d Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Mon, 20 Jul 2026 23:13:59 +0200 Subject: [PATCH] fix(core): streamed-AU hardening from the security-review pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review verdict: no exploitable memory-safety / amplification / splice / resurrection issue; nonce order and two-lane seal byte-identical for the legacy path. Actionable findings applied: - explicit per-block data_shards cross-check next to the recovery_shards one (defense-in-depth — the geometry invariants make it unreachable today, but have_data indexing assumes it) - partial delivery skips an UNPINNED streamed frame (frame_bytes still 0) instead of truncating its max-sized buffer to an empty "partial" - regression tests for the two untested load-bearing guards: the final-first out-of-range-sentinel drop (the exact-sized-buffer guard) and second-final-with-different-totals rejection Co-Authored-By: Claude Fable 5 --- .../punktfunk-core/src/packet/reassemble.rs | 13 ++- crates/punktfunk-core/src/packet/tests.rs | 108 ++++++++++++++++++ 2 files changed, 120 insertions(+), 1 deletion(-) diff --git a/crates/punktfunk-core/src/packet/reassemble.rs b/crates/punktfunk-core/src/packet/reassemble.rs index 5514eb08..c571e972 100644 --- a/crates/punktfunk-core/src/packet/reassemble.rs +++ b/crates/punktfunk-core/src/packet/reassemble.rs @@ -497,6 +497,14 @@ impl Reassembler { drop(stats); return Ok(None); } + // Defense-in-depth (2026-07 security review): the geometry invariants above guarantee + // every packet of a block agrees on K, so this can't fire today — but `have_data` + // indexing and the recovery-slot math below assume it, and an explicit check keeps a + // future firewall refactor from turning that assumption into an OOB panic. + if block.data_shards != data_shards { + drop(stats); + return Ok(None); + } if block.done { // A data shard the parity reconstruct already restored (`!have_data`) was late, not // lost — net it out of the `fec_recovered_shards` it was counted into (see the @@ -694,7 +702,10 @@ impl ReassemblyWindow { // where shards are missing (the codec's block walk skips zero windows). // Newest-wins if several age out in one prune. Still counted dropped below. if let Some(sink) = partial_sink.as_deref_mut() { - if f.user_flags & USER_FLAG_CHUNK_ALIGNED != 0 { + // `frame_bytes > 0` also excludes an UNPINNED streamed frame (its total is + // still the 0 sentinel value): truncating its max-sized buffer to 0 would + // deliver an empty "partial" — worse than the plain drop it gets instead. + if f.user_flags & USER_FLAG_CHUNK_ALIGNED != 0 && f.frame_bytes > 0 { let mut buf = std::mem::take(&mut f.buf); buf.truncate(f.frame_bytes); let newer = sink diff --git a/crates/punktfunk-core/src/packet/tests.rs b/crates/punktfunk-core/src/packet/tests.rs index e456eaa5..6222a6b5 100644 --- a/crates/punktfunk-core/src/packet/tests.rs +++ b/crates/punktfunk-core/src/packet/tests.rs @@ -931,3 +931,111 @@ fn streamed_open_amplification_is_budget_bounded() { "the fifth max-sized open must be refused by the in-flight budget" ); } + +/// The final-first order's single buffer-safety guard (2026-07 security review finding 3): a +/// frame opened by its FINAL block allocates an EXACT-sized buffer; a sentinel aimed at (or +/// past) the pinned final slot must be dropped — without the guard its full-K write would land +/// outside that buffer. And the reject must not corrupt the frame: it still completes. +#[test] +fn streamed_out_of_range_sentinel_after_final_first_is_dropped() { + let mut r = Reassembler::new(limits()); + let coder = coder_for(FecScheme::Gf8); + let stats = StatsCounters::default(); + // Final block opens the frame: block_count = 1, frame_bytes = 32 → K = 2. Send shard 0 + // only, so the frame stays in flight with the totals pinned. + let mut fin = base_header(); + fin.block_count = 1; + fin.frame_bytes = 32; + fin.data_shards = 2; + fin.recovery_shards = 0; + fin.shard_index = 0; + assert!(r + .push(&packet(fin), coder.as_ref(), &stats) + .unwrap() + .is_none()); + // Sentinels at the final slot (0) and past it (1): both non-final-impossible under the + // pinned block_count = 1 → dropped, never written into the 32-byte buffer. + for bi in 0..2u16 { + let mut h = base_header(); + h.block_count = 0; + h.frame_bytes = 0; + h.data_shards = 8; + h.recovery_shards = 0; + h.block_index = bi; + assert!(r + .push(&packet(h), coder.as_ref(), &stats) + .unwrap() + .is_none()); + } + assert_eq!(stats.snapshot().packets_dropped, 2); + // The frame is unharmed: its real second shard completes it at the exact pinned length. + let mut fin2 = fin; + fin2.shard_index = 1; + let got = r + .push(&packet(fin2), coder.as_ref(), &stats) + .unwrap() + .expect("frame must still complete after the rejected sentinels"); + assert_eq!(got.data.len(), 32); + assert!(got.complete); +} + +/// A second "final" header with DIFFERENT totals must be rejected once a streamed frame is +/// pinned (re-pinning would re-interpret already-landed shards), and the frame must still +/// complete under the first totals. +#[test] +fn streamed_second_final_with_different_totals_is_rejected() { + let mut r = Reassembler::new(limits()); + let coder = coder_for(FecScheme::Gf8); + let stats = StatsCounters::default(); + let sentinel_shard = |shard_index: u16| { + let mut h = base_header(); + h.block_count = 0; + h.frame_bytes = 0; + h.data_shards = 8; + h.recovery_shards = 0; + h.block_index = 0; + h.shard_index = shard_index; + h + }; + let final_shard = |frame_bytes: u32, data_shards: u16, shard_index: u16| { + let mut h = base_header(); + h.block_count = 2; + h.frame_bytes = frame_bytes; + h.data_shards = data_shards; + h.recovery_shards = 0; + h.block_index = 1; + h.shard_index = shard_index; + h + }; + // Sentinel opens block 0, then the real final pins totals: 10 shards = 160 bytes. + assert!(r + .push(&packet(sentinel_shard(0)), coder.as_ref(), &stats) + .unwrap() + .is_none()); + assert!(r + .push(&packet(final_shard(160, 2, 0)), coder.as_ref(), &stats) + .unwrap() + .is_none()); + // A second final claiming 144 bytes (K = 1): geometry-valid alone, but it contradicts the + // pinned totals → dropped. + let before = stats.snapshot().packets_dropped; + assert!(r + .push(&packet(final_shard(144, 1, 0)), coder.as_ref(), &stats) + .unwrap() + .is_none()); + assert_eq!(stats.snapshot().packets_dropped, before + 1); + // The frame still completes under the FIRST totals: the rest of block 0 + the final tail. + let mut got = None; + for s in 1..8u16 { + assert!(got.is_none()); + got = r + .push(&packet(sentinel_shard(s)), coder.as_ref(), &stats) + .unwrap(); + } + assert!(got.is_none(), "block 1 still owes a shard"); + let got = r + .push(&packet(final_shard(160, 2, 1)), coder.as_ref(), &stats) + .unwrap() + .expect("frame completes under the first pinned totals"); + assert_eq!(got.data.len(), 160); +}