fix(cluster): reject a truncated v3 gossip header instead of panicking the process - #495
Conversation
…g the process 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 ac2b036: 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
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe gossip deserializer now rejects truncated version-3 headers before reading ChangesGossip header safety
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change rejects truncated v3 gossip headers instead of allowing a process crash, and no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The defect
Gossip wire v3 (#493) appended a 40-byte
sender_master_idto the header. The length guard at the top ofdeserialize_gossipwas deliberately left at the smaller v2 header size (2130) so a genuine v2 peer would still parse — but the v3 branch below it then readdata[HEADER_SIZE_V2..HEADER_SIZE](data[2130..2170]) unconditionally.Any frame declaring version ≥ 3 with a length in
2130..2170indexes past the end of the slice.Why this is a remote DoS, not a bad parse
bus.rsreads a peer-supplied length (capped at 64 KiB — 2130 passes), reads exactly that many bytes, and hands them straight todeserialize_gossip. Measured against a cluster-enabled server built from the mergedac2b036d, sending one unauthenticated 2130-byte frame to the bus port:The process is gone; the next
PINGis connection-refused. Any cluster-enabled node can be killed by a single unauthenticated frame.The fix
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. The loose guard that the v2 back-compat path depends on is untouched.
Evidence
test_deserialize_rejects_v3_header_truncated_inside_master_idwalks every truncation point in2130..2170; without the fix it panics atgossip.rs:278:47.PING, zero panics in the log.cluster_client_bootstrap20/20;fmt --checkclean;clippy --all-targets -D warningsclean on both feature sets.Why the fuzzer missed it
gossip_deserwas already correct and already green on both #486 and #493. It had simply not synthesised a valid 4-byte magic ("Redi") together with that exact 40-byte length window inside the 15-minute PR budget. Seeds for the panicking shape (2130 / 2140 / 2169) and for the legitimate v2-exact frame are added tofuzz/corpus/gossip_deser, so the window is covered from the first iteration.Found by an adversarial review of the merged cluster work.
Refs #493
Summary by CodeRabbit
Bug Fixes
Documentation