From 33d889910ee7799d8ac4ac81e3589d26ec08913d Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Sat, 15 Aug 2026 03:56:37 +0700 Subject: [PATCH] fix(cluster): reject a truncated v3 gossip header instead of panicking the process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gossip wire v3 (#493) appended a 40-byte `sender_master_id` to the header. The length guard at the top of `deserialize_gossip` was deliberately left at the SMALLER v2 header size (2130) so a genuine v2 peer would still parse — but the v3 branch below it then read `data[HEADER_SIZE_V2..HEADER_SIZE]` (`data[2130..2170]`) unconditionally. Any frame declaring version 3 or above with a length in `2130..2170` indexed past the end of the slice. That is reachable from the network, not just from a fuzzer. `bus.rs` reads a peer-supplied length (capped at 64 KiB, which 2130 passes), reads exactly that many bytes, and hands them straight to `deserialize_gossip`. Measured against a cluster-enabled server built from ac2b036d: one unauthenticated 2130-byte frame to the bus port produces thread 'cluster-ctl' panicked at src/cluster/gossip.rs:278:47: range end index 2170 out of range for slice of length 2130 FATAL: thread 'cluster-ctl' panicked; aborting the whole process and the server is gone — subsequent PING is connection-refused. So the impact is a full remote denial of service on any cluster-enabled node, not a dropped connection. A v3 sender always writes the full 40 bytes, so a short v3 header is malformed and is now rejected with an error rather than zero-filled — fail-closed, and it leaves the v2 back-compat path (which is what the loose guard exists for) untouched. Red/green: `test_deserialize_rejects_v3_header_truncated_inside_master_id` walks every truncation point in `2130..2170` and panics at 278:47 without the fix. Re-verified end to end after the fix: the same frame now leaves the server alive and answering PING, with zero panics in the log. The `gossip_deser` fuzz target was already correct and already green on both #486 and #493 — it simply had not synthesised a valid 4-byte magic together with that exact 40-byte length window inside its 15-minute PR budget. Seeds for the panicking shape and for the legitimate v2-exact frame are added to `fuzz/corpus/gossip_deser` so the window is covered from the first iteration. Found by an adversarial review of the merged cluster work. Refs #493 author: Tin Dang --- CHANGELOG.md | 9 ++++ fuzz/corpus/gossip_deser/v2_header_exact | Bin 0 -> 2130 bytes .../gossip_deser/v3_truncated_master_id_2130 | Bin 0 -> 2130 bytes .../gossip_deser/v3_truncated_master_id_2140 | Bin 0 -> 2140 bytes .../gossip_deser/v3_truncated_master_id_2169 | Bin 0 -> 2169 bytes src/cluster/gossip.rs | 49 ++++++++++++++++++ 6 files changed, 58 insertions(+) create mode 100644 fuzz/corpus/gossip_deser/v2_header_exact create mode 100644 fuzz/corpus/gossip_deser/v3_truncated_master_id_2130 create mode 100644 fuzz/corpus/gossip_deser/v3_truncated_master_id_2140 create mode 100644 fuzz/corpus/gossip_deser/v3_truncated_master_id_2169 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 0000000000000000000000000000000000000000..34e296201a3d314f24df597da586e1cd6955094c GIT binary patch literal 2130 ycmWGaP03_n;HY9?Vqi!l5-^N{(GVC7fzc2c4S~@R7!85Z5P*h&bc9m|0|NjvBN9#k literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..bc2fd3b2403421c86b351739d8929e2e47a32116 GIT binary patch literal 2130 ycmWGaP03_n;HY9?W?)Dp5-^N{(GVC7fzc2c4S~@R7!85Z5P*h&bc9m|0|Nj%Y!XiZ literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..f71bcb7fcd0b1a233d2edcac267b6b3f07307671 GIT binary patch literal 2140 zcmWGaP03_n;HY9?W?)Dp5-^N{(GVC7fzc2c4S~@R7!85Z5P*h&bc9m|a$o=e@`4gi literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..7de482b7fd77e028320d55168ddcb68a04130a10 GIT binary patch literal 2169 zcmWGaP03_n;HY9?W?)Dp5-^N{(GVC7fzc2c4S~@R7!85Z5P*h&bc9m|(SZQ~1)>s9 literal 0 HcmV?d00001 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.