fix(proto): bound wire collections during deserialization
Gemini audit finding NET-05 ("unbounded collection deserialization before
semantic validation", Low). `LibrarySnapshot::games` and
`CallToPlayAuthorSnapshot::events` were plain `Vec`s. Their semantic
validators reject more than 4,096 elements, but serde had already built
the complete vector by then, so a peer could make a receiver allocate up
to a full 8 MiB response frame's worth of parsed elements before the
limit was applied. The 64 KiB request bound already covers inbound
requests; this closes the same gap for responses a client accepts from a
peer it connected to.
Both fields now deserialize through a bounded visitor that keeps at most
`maximum + 1` elements and drains the remainder as `IgnoredAny` without
allocating. Keeping exactly one element past the limit is deliberate: the
existing `validate` methods still observe `len() > maximum` and report
`TooManyItems`, and `Response::decode` keeps leaving that judgement to
the caller so an invalid Call-to-Play domain does not discard a valid
library (and vice versa). Frames at or below the limit are byte-for-byte
unchanged, and the encoder still refuses to produce oversize frames.
Tests build oversize JSON by hand (the encoder cannot) and check that a
sequence four times the limit decodes to limit + 1 elements, that the
semantic validator then reports `TooManyItems`, and that a sequence
exactly at the limit is untouched. The existing domain-isolation test
asserts the truncated length as well.
Ported from the parallel security branch (lanspread2 commit 5139ec1),
which bundled it with the request frame bound this branch already has.
Test plan:
- `cargo test -p lanspread-proto`: 24 passed.
- `cargo clippy -p lanspread-proto --all-targets -- -D warnings`: clean.
Claude-Session: https://claude.ai/code/session_01QRkCv4a4GqkajyamxmbSuA
This commit is contained in:
@@ -102,7 +102,10 @@ When a peer is discovered:
|
||||
accepts one frame followed by request EOF and sends at most one response.
|
||||
Inbound request frames are capped at 64 KiB (the QUIC receive window per
|
||||
server stream matches it), response frames at 8 MiB, and control I/O has
|
||||
ten-second deadlines.
|
||||
ten-second deadlines. Wire collections (library games, Call to Play author
|
||||
events) are deserialized through a bounded visitor that materializes at most
|
||||
one element past the semantic limit and discards the rest, so an oversize
|
||||
frame cannot claim more memory than a valid one before validation rejects it.
|
||||
|
||||
### Call to Play replication
|
||||
|
||||
|
||||
@@ -1,12 +1,19 @@
|
||||
use std::{
|
||||
fmt::{self, Write as _},
|
||||
marker::PhantomData,
|
||||
net::SocketAddr,
|
||||
str::FromStr,
|
||||
};
|
||||
|
||||
use bytes::Bytes;
|
||||
pub use lanspread_db::content_manifest::{CanonicalCatalogPath, ContentId};
|
||||
use serde::{Deserialize, Deserializer, Serialize, Serializer, de::DeserializeOwned};
|
||||
use serde::{
|
||||
Deserialize,
|
||||
Deserializer,
|
||||
Serialize,
|
||||
Serializer,
|
||||
de::{DeserializeOwned, IgnoredAny, SeqAccess, Visitor},
|
||||
};
|
||||
|
||||
pub const PROTOCOL_VERSION: u32 = 8;
|
||||
pub const ALPN_PROTOCOL: &[u8] = b"lanspread/8";
|
||||
@@ -388,6 +395,7 @@ pub struct GameAvailability {
|
||||
#[serde(deny_unknown_fields)]
|
||||
pub struct LibrarySnapshot {
|
||||
pub revision: u64,
|
||||
#[serde(deserialize_with = "deserialize_library_games")]
|
||||
pub games: Vec<GameAvailability>,
|
||||
}
|
||||
|
||||
@@ -437,6 +445,7 @@ pub enum CallToPlayAction {
|
||||
pub struct CallToPlayAuthorSnapshot {
|
||||
pub revision: u64,
|
||||
pub display_name: String,
|
||||
#[serde(deserialize_with = "deserialize_call_to_play_events")]
|
||||
pub events: Vec<CallToPlayAuthorEvent>,
|
||||
}
|
||||
|
||||
@@ -958,6 +967,85 @@ fn validate_game_id(game_id: &str) -> Result<(), ControlValidationError> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn deserialize_library_games<'de, D>(deserializer: D) -> Result<Vec<GameAvailability>, D::Error>
|
||||
where
|
||||
D: Deserializer<'de>,
|
||||
{
|
||||
deserialize_bounded_vec(deserializer, MAX_LIBRARY_GAMES, "library games")
|
||||
}
|
||||
|
||||
fn deserialize_call_to_play_events<'de, D>(
|
||||
deserializer: D,
|
||||
) -> Result<Vec<CallToPlayAuthorEvent>, D::Error>
|
||||
where
|
||||
D: Deserializer<'de>,
|
||||
{
|
||||
deserialize_bounded_vec(
|
||||
deserializer,
|
||||
MAX_CALL_TO_PLAY_EVENTS_PER_AUTHOR,
|
||||
"Call to Play author events",
|
||||
)
|
||||
}
|
||||
|
||||
/// Deserializes a wire sequence without ever materialising more than
|
||||
/// `maximum + 1` elements.
|
||||
///
|
||||
/// The semantic validators (`LibrarySnapshot::validate`,
|
||||
/// `CallToPlayAuthorSnapshot::validate`) report `TooManyItems` when a
|
||||
/// collection exceeds its limit, and `Response::decode` deliberately leaves
|
||||
/// that judgement to the caller so one invalid domain does not discard the
|
||||
/// other. Truncating to exactly one element past the limit keeps both
|
||||
/// behaviours intact while bounding the memory an oversize frame can claim
|
||||
/// during parsing: the remaining elements are consumed as `IgnoredAny` and
|
||||
/// never allocated.
|
||||
fn deserialize_bounded_vec<'de, D, T>(
|
||||
deserializer: D,
|
||||
maximum: usize,
|
||||
field: &'static str,
|
||||
) -> Result<Vec<T>, D::Error>
|
||||
where
|
||||
D: Deserializer<'de>,
|
||||
T: Deserialize<'de>,
|
||||
{
|
||||
struct BoundedVecVisitor<T> {
|
||||
maximum: usize,
|
||||
field: &'static str,
|
||||
marker: PhantomData<T>,
|
||||
}
|
||||
|
||||
impl<'de, T> Visitor<'de> for BoundedVecVisitor<T>
|
||||
where
|
||||
T: Deserialize<'de>,
|
||||
{
|
||||
type Value = Vec<T>;
|
||||
|
||||
fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
|
||||
write!(formatter, "a bounded sequence of {}", self.field)
|
||||
}
|
||||
|
||||
fn visit_seq<A>(self, mut sequence: A) -> Result<Self::Value, A::Error>
|
||||
where
|
||||
A: SeqAccess<'de>,
|
||||
{
|
||||
let mut values = Vec::new();
|
||||
while let Some(value) = sequence.next_element()? {
|
||||
values.push(value);
|
||||
if values.len() > self.maximum {
|
||||
while sequence.next_element::<IgnoredAny>()?.is_some() {}
|
||||
break;
|
||||
}
|
||||
}
|
||||
Ok(values)
|
||||
}
|
||||
}
|
||||
|
||||
deserializer.deserialize_seq(BoundedVecVisitor {
|
||||
maximum,
|
||||
field,
|
||||
marker: PhantomData,
|
||||
})
|
||||
}
|
||||
|
||||
fn validate_bounded_chars(
|
||||
value: &str,
|
||||
maximum: usize,
|
||||
@@ -1503,6 +1591,55 @@ mod tests {
|
||||
assert!(Response::decode(Bytes::from(serde_json::to_vec(&value).expect("json"))).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn oversize_wire_collections_are_truncated_to_one_past_their_limit() {
|
||||
// Build the JSON by hand: the encoder refuses to produce these frames.
|
||||
let event_json = serde_json::to_string(&event(CallToPlayAction::Rsvp)).expect("event json");
|
||||
let events =
|
||||
std::iter::repeat_n(event_json.as_str(), MAX_CALL_TO_PLAY_EVENTS_PER_AUTHOR * 4)
|
||||
.collect::<Vec<_>>()
|
||||
.join(",");
|
||||
let snapshot = format!(r#"{{"revision":1,"display_name":"Alice","events":[{events}]}}"#);
|
||||
let decoded: CallToPlayAuthorSnapshot =
|
||||
serde_json::from_str(&snapshot).expect("oversize sequence still parses");
|
||||
assert_eq!(decoded.events.len(), MAX_CALL_TO_PLAY_EVENTS_PER_AUTHOR + 1);
|
||||
assert!(matches!(
|
||||
decoded.validate(),
|
||||
Err(ControlValidationError::TooManyItems {
|
||||
field: "Call to Play author events",
|
||||
..
|
||||
})
|
||||
));
|
||||
|
||||
let content_id = content(1);
|
||||
let games = (0..MAX_LIBRARY_GAMES * 4)
|
||||
.map(|index| format!(r#"{{"game_id":"game-{index:05}","content_id":"{content_id}"}}"#))
|
||||
.collect::<Vec<_>>()
|
||||
.join(",");
|
||||
let library = format!(r#"{{"revision":1,"games":[{games}]}}"#);
|
||||
let decoded: LibrarySnapshot =
|
||||
serde_json::from_str(&library).expect("oversize sequence still parses");
|
||||
assert_eq!(decoded.games.len(), MAX_LIBRARY_GAMES + 1);
|
||||
assert!(matches!(
|
||||
decoded.validate(),
|
||||
Err(ControlValidationError::TooManyItems {
|
||||
field: "library games",
|
||||
..
|
||||
})
|
||||
));
|
||||
|
||||
// A sequence exactly at the limit is untouched.
|
||||
let games = (0..MAX_LIBRARY_GAMES)
|
||||
.map(|index| format!(r#"{{"game_id":"game-{index:05}","content_id":"{content_id}"}}"#))
|
||||
.collect::<Vec<_>>()
|
||||
.join(",");
|
||||
let decoded: LibrarySnapshot =
|
||||
serde_json::from_str(&format!(r#"{{"revision":1,"games":[{games}]}}"#))
|
||||
.expect("sequence at the limit parses");
|
||||
assert_eq!(decoded.games.len(), MAX_LIBRARY_GAMES);
|
||||
decoded.validate().expect("sequence at the limit is valid");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn response_decode_isolates_domain_validation() {
|
||||
let invalid_call_to_play = state_snapshot(vec![
|
||||
@@ -1518,6 +1655,10 @@ mod tests {
|
||||
panic!("decoded the wrong response variant");
|
||||
};
|
||||
decoded.library.validate().expect("library is valid");
|
||||
assert_eq!(
|
||||
decoded.call_to_play.events.len(),
|
||||
MAX_CALL_TO_PLAY_EVENTS_PER_AUTHOR + 1
|
||||
);
|
||||
assert!(decoded.call_to_play.validate().is_err());
|
||||
assert!(
|
||||
Response::HelloSnapshot(invalid_call_to_play)
|
||||
|
||||
Reference in New Issue
Block a user