diff --git a/CHANGELOG.md b/CHANGELOG.md index ff8d4c8dc..721eb44e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 dispatcher — so `COMMAND COUNT` was advertising verbs Moon could not run. ### Fixed +- **Remote panic on the cluster bus: a truncated v3 gossip header killed the process.** The gossip + wire v3 (#493) appended a 40-byte `sender_master_id`, but the deserializer's length guard still + admitted any frame of at least the v2 header size so that a genuine v2 peer would still parse — + and the v3 branch then read `data[2130..2170]` unconditionally. Any frame carrying version 3 with + a length in `2130..2170` indexed past the end. The cluster bus listener hands this function + peer-supplied bytes, so a single unauthenticated 2130-byte frame to the bus port panicked the + `cluster-ctl` thread, which by policy aborts the whole server. A short v3 header is now rejected + as malformed. Seeded into `fuzz/corpus/gossip_deser` — the target was correct but had not + synthesised the 4-byte magic plus that 40-byte length window within its PR budget. - **`CLUSTER INFO` no longer claims `cluster_enabled`, and a slotless node no longer claims health.** Two integration assertions encoded the pre-fix behaviour and contradicted the measured oracle: redis-server 8.6.1 reports `cluster_enabled` in `INFO` only — `CLUSTER INFO` never carries it — diff --git a/fuzz/corpus/gossip_deser/v2_header_exact b/fuzz/corpus/gossip_deser/v2_header_exact new file mode 100644 index 000000000..34e296201 Binary files /dev/null and b/fuzz/corpus/gossip_deser/v2_header_exact differ diff --git a/fuzz/corpus/gossip_deser/v3_truncated_master_id_2130 b/fuzz/corpus/gossip_deser/v3_truncated_master_id_2130 new file mode 100644 index 000000000..bc2fd3b24 Binary files /dev/null and b/fuzz/corpus/gossip_deser/v3_truncated_master_id_2130 differ diff --git a/fuzz/corpus/gossip_deser/v3_truncated_master_id_2140 b/fuzz/corpus/gossip_deser/v3_truncated_master_id_2140 new file mode 100644 index 000000000..f71bcb7fc Binary files /dev/null and b/fuzz/corpus/gossip_deser/v3_truncated_master_id_2140 differ diff --git a/fuzz/corpus/gossip_deser/v3_truncated_master_id_2169 b/fuzz/corpus/gossip_deser/v3_truncated_master_id_2169 new file mode 100644 index 000000000..7de482b7f Binary files /dev/null and b/fuzz/corpus/gossip_deser/v3_truncated_master_id_2169 differ diff --git a/src/cluster/gossip.rs b/src/cluster/gossip.rs index bad59e059..d09918f19 100644 --- a/src/cluster/gossip.rs +++ b/src/cluster/gossip.rs @@ -271,10 +271,21 @@ pub fn deserialize_gossip(data: &[u8]) -> Result { // v3 appended the sender's master id. A v2 (or v1) peer sent no such field // and its sections start 40 bytes earlier — so the header layout, not just // the flags encoding, depends on the version. + // The length guard above admits anything >= HEADER_SIZE_V2 so a legitimate + // v2 peer parses; a v3 header cut off inside this field would otherwise + // index past the end. A real v3 sender always writes the full 40 bytes, so + // a short one is malformed and is rejected rather than zero-filled. let mut sender_master_id = [0u8; 40]; let header_len = if version <= GOSSIP_VERSION_NO_MASTER_ID { HEADER_SIZE_V2 } else { + if data.len() < HEADER_SIZE { + return Err(format!( + "v{version} header truncated inside sender_master_id: {} < {}", + data.len(), + HEADER_SIZE + )); + } sender_master_id.copy_from_slice(&data[HEADER_SIZE_V2..HEADER_SIZE]); HEADER_SIZE }; @@ -981,6 +992,44 @@ mod tests { /// A v2 peer sends no master id, and must still parse — sections included. /// + /// A v3 header truncated INSIDE `sender_master_id` must be rejected, not + /// panic. + /// + /// The length guard at the top of `deserialize_gossip` admits any frame of + /// at least `HEADER_SIZE_V2` (2130) bytes so a legitimate v2 peer can be + /// parsed — but the v3 branch then reads `data[2130..2170]` + /// unconditionally. Every length in `2130..2170` carrying version 3 or + /// above therefore indexes past the end. The cluster bus listener feeds + /// this function peer-supplied bytes, so the failure mode is a remote + /// panic — and `cluster-ctl` aborts the process — not a bad parse. + #[test] + fn test_deserialize_rejects_v3_header_truncated_inside_master_id() { + let msg = GossipMessage { + msg_type: GossipMsgType::Ping, + sender_node_id: [b'a'; 40], + sender_slots: Box::new([0u8; 2048]), + config_epoch: 1, + sender_ip: [ + b'1', b'2', b'7', b'.', b'0', b'.', b'0', b'.', b'1', 0, 0, 0, 0, 0, 0, 0, + ], + sender_port: 7000, + sender_bus_port: 17000, + sender_master_id: [b'm'; 40], + gossip_sections: vec![], + }; + let full = serialize_gossip(&msg); + assert!(full.len() >= HEADER_SIZE); + + // Every truncation point strictly inside the appended field. + for len in HEADER_SIZE_V2..HEADER_SIZE { + let truncated = &full[..len]; + assert!( + deserialize_gossip(truncated).is_err(), + "a v3 header cut at {len} bytes must be rejected, not panic" + ); + } + } + /// The field decodes as absent, which is indistinguishable from "I am a /// master". That is exactly the pre-v3 behaviour and the reason adding it /// required a version bump rather than a silent field append.