From 4fd0e5a949d2fd027001b1177f078e5f189302e7 Mon Sep 17 00:00:00 2001 From: memosr Date: Sat, 4 Jul 2026 09:56:54 +0300 Subject: [PATCH 1/2] fix: saturate Fin sequence arithmetic in proposal-part streaming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Fin stream message carries an unvalidated u64 `sequence` straight off the wire. `StreamState::insert` computed the stream's expected message count as `msg.sequence as usize + 1`, which a malicious peer could drive to overflow by sending `sequence = u64::MAX`: a panic under debug-assertions and a wrap-to-zero in release builds. Release builds happened to stay safe because the subsequent `buffer.len() == expected_messages` check never matches a wrapped-to-zero target, so the stream is left incomplete and later evicted. But the guarantee rested on wrapping behaviour rather than intent, and the accompanying comment justified the arithmetic with a false invariant: `expected_messages` is assigned from the unbounded `msg.sequence`, not from the bounded `message_count`. Use `saturating_add(1)` so the worst case is `usize::MAX` — an unreachable completion target the stream is evicted for — instead of a panic or a silent wrap. Comment corrected to describe the real bound. Co-Authored-By: Claude Opus 4.8 --- crates/malachite-app/src/streaming.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/malachite-app/src/streaming.rs b/crates/malachite-app/src/streaming.rs index 56f1bf55..4ec45bea 100644 --- a/crates/malachite-app/src/streaming.rs +++ b/crates/malachite-app/src/streaming.rs @@ -280,11 +280,16 @@ impl StreamState { // If we have received the fin message, we can determine when we will be done. // We are done if we have already received all messages from 0 to fin.sequence, // included. That is to say, if we have received `fin.sequence + 1` messages. - // Sequence is a u64 protocol field; on 64-bit targets usize == u64. - // The +1 cannot overflow because MAX_MESSAGES_PER_STREAM << u64::MAX. - #[allow(clippy::cast_possible_truncation, clippy::arithmetic_side_effects)] + // + // `msg.sequence` is an unvalidated u64 straight off the wire, so a + // malicious peer can set it to `u64::MAX`. `as usize + 1` would then + // overflow: panic under debug-assertions, wrap to 0 in release. Use a + // saturating add so the worst case is `usize::MAX` (an unreachable + // completion target the stream is later evicted for), never a panic + // or a spurious wrap-to-zero. + #[allow(clippy::cast_possible_truncation)] { - self.expected_messages = msg.sequence as usize + 1; + self.expected_messages = (msg.sequence as usize).saturating_add(1); } } From 62df0de9ba58b186d29a3829954183601cfda638 Mon Sep 17 00:00:00 2001 From: memosr Date: Fri, 14 Aug 2026 09:34:15 +0300 Subject: [PATCH 2/2] test: pin Fin saturation with a u64::MAX regression test, note the 64-bit cast assumption --- crates/malachite-app/src/streaming.rs | 30 +++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/crates/malachite-app/src/streaming.rs b/crates/malachite-app/src/streaming.rs index 4ec45bea..1bf3ed13 100644 --- a/crates/malachite-app/src/streaming.rs +++ b/crates/malachite-app/src/streaming.rs @@ -287,6 +287,13 @@ impl StreamState { // saturating add so the worst case is `usize::MAX` (an unreachable // completion target the stream is later evicted for), never a panic // or a spurious wrap-to-zero. + // + // The retained truncation allow assumes a 64-bit target, where + // `usize == u64` and the cast is lossless. On a 32-bit target the + // cast would truncate before the saturating add, letting a peer + // pick a small-but-wrong completion target; the stream would still + // be bounded and evicted, but node deployments are 64-bit and this + // code relies on that. #[allow(clippy::cast_possible_truncation)] { self.expected_messages = (msg.sequence as usize).saturating_add(1); @@ -756,6 +763,29 @@ mod tests { assert_eq!(map.streams.len(), 1, "Map should contain one active stream"); } + #[test] + fn test_fin_with_max_sequence_does_not_overflow() { + let peer_1 = PeerId::random(); + let stream_1 = make_stream_id(101); + + let mut map = PartStreamsMap::new(Height::new(1), NUM_VALIDATORS); + let init_msg = make_message(&stream_1, 0, make_init_part()); + // A malicious peer can put any u64 in `sequence`. Before the saturating + // add, `sequence as usize + 1` panicked here under debug assertions. + let fin_msg = make_fin_message(&stream_1, u64::MAX); + + assert!(map.must_insert(peer_1, init_msg).is_none()); + assert!( + map.must_insert(peer_1, fin_msg).is_none(), + "Stream must not complete on a bogus Fin sequence" + ); + assert_eq!( + map.streams.len(), + 1, + "Stream should stay pending until evicted, not complete or disappear" + ); + } + #[test] fn test_insert_in_order_completes_and_removes_stream() { let peer_1 = PeerId::random();