fix(l1): reject short discv5 packet authdata instead of panicking#6964
fix(l1): reject short discv5 packet authdata instead of panicking#6964ElFantasma wants to merge 2 commits into
Conversation
🤖 Kimi Code ReviewThis PR correctly addresses a critical DoS vulnerability in the discv5 implementation where malformed packets with short Security AssessmentVulnerability Class: Unauthenticated remote DoS (panic/crash) Code Review
|
🤖 Claude Code ReviewReview: fix(l1): reject short discv5 packet authdata instead of panickingThis is a well-scoped, correct fix for a real unauthenticated DoS. Summary of what I verified:
Minor observations (non-blocking)
Overall: correct, minimal, well-tested fix for a legitimate unauthenticated panic/DoS vector. No consensus, gas, or state-trie implications since this is purely discovery-layer packet parsing. Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Codex Code ReviewNo findings. The patch correctly closes two peer-controlled panic paths in discv5 authdata parsing:
Residual gap: I could not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
Greptile SummaryThis PR fixes two unauthenticated panic sites in the discv5 packet-decode path where peer-controlled
Confidence Score: 4/5Safe to merge — both panic-inducing slice operations are now gated behind explicit length checks that return errors, and the actor's existing error-handling path discards malformed packets without restarting the actor. The fix correctly closes both panic sites with minimal, targeted changes and includes tests that cover the previously-crashing inputs. The one rough edge is that No files require special attention; both changed files are straightforward and the new
|
| Filename | Overview |
|---|---|
| crates/networking/p2p/discovery/discv5_handlers.rs | Routes ordinary-packet src-id extraction through the new length-checked Ordinary::src_id, preventing unauthenticated panic-based DoS of the discv5 actor; minor redundancy in that src_id is now validated twice per decrypted packet. |
| crates/networking/p2p/discv5/messages.rs | Introduces Ordinary::src_id with an upfront length guard, refactors Ordinary::decode to reuse it, adds an identical guard in WhoAreYou::decode, and adds three targeted unit tests covering short and well-formed authdata; all three panic sites are now error-returning. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[UDP packet received] --> B{flag?}
B -- 0x00 ordinary --> C[discv5_handle_ordinary]
B -- 0x01 WHOAREYOU --> D[discv5_handle_who_are_you]
B -- 0x02 handshake --> E[discv5_handle_handshake]
C --> F[Ordinary::src_id checks authdata len == 32]
F -- len != 32 --> G[Err InvalidSize - was panic before fix]
G --> H[actor logs and drops packet, keeps running]
F -- len == 32 --> I[session lookup by src_id]
I -- no session --> J[send WhoAreYou]
I -- has session --> K[Ordinary::decode]
K --> L[decrypt and parse message]
D --> M[WhoAreYou::decode checks authdata len == 24]
M -- len != 24 --> N[Err InvalidSize - was panic before fix]
N --> H
M -- len == 24 --> O[parse id_nonce and enr_seq]
E --> P[HandshakeAuthdata::decode checks len >= min - already guarded]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[UDP packet received] --> B{flag?}
B -- 0x00 ordinary --> C[discv5_handle_ordinary]
B -- 0x01 WHOAREYOU --> D[discv5_handle_who_are_you]
B -- 0x02 handshake --> E[discv5_handle_handshake]
C --> F[Ordinary::src_id checks authdata len == 32]
F -- len != 32 --> G[Err InvalidSize - was panic before fix]
G --> H[actor logs and drops packet, keeps running]
F -- len == 32 --> I[session lookup by src_id]
I -- no session --> J[send WhoAreYou]
I -- has session --> K[Ordinary::decode]
K --> L[decrypt and parse message]
D --> M[WhoAreYou::decode checks authdata len == 24]
M -- len != 24 --> N[Err InvalidSize - was panic before fix]
N --> H
M -- len == 24 --> O[parse id_nonce and enr_seq]
E --> P[HandshakeAuthdata::decode checks len >= min - already guarded]
Comments Outside Diff (1)
-
crates/networking/p2p/discovery/discv5_handlers.rs, line 74-85 (link)Redundant
src_id()call in decode pathOrdinary::src_id(&packet)is called here to extract the src-id for the session lookup (line 74), and thenOrdinary::decode(&packet, &key)is called later (line 85), which itself immediately callsSelf::src_id(packet)?again internally. The length validation andH256construction happen twice for every packet that reaches the decode path. This is harmless but wasteful; consider threading the already-extractedsrc_idintoOrdinary::decodeor adding an internal variant that accepts a pre-validated src-id.Prompt To Fix With AI
This is a comment left during a code review. Path: crates/networking/p2p/discovery/discv5_handlers.rs Line: 74-85 Comment: **Redundant `src_id()` call in decode path** `Ordinary::src_id(&packet)` is called here to extract the src-id for the session lookup (line 74), and then `Ordinary::decode(&packet, &key)` is called later (line 85), which itself immediately calls `Self::src_id(packet)?` again internally. The length validation and `H256` construction happen twice for every packet that reaches the decode path. This is harmless but wasteful; consider threading the already-extracted `src_id` into `Ordinary::decode` or adding an internal variant that accepts a pre-validated src-id. How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
crates/networking/p2p/discovery/discv5_handlers.rs:74-85
**Redundant `src_id()` call in decode path**
`Ordinary::src_id(&packet)` is called here to extract the src-id for the session lookup (line 74), and then `Ordinary::decode(&packet, &key)` is called later (line 85), which itself immediately calls `Self::src_id(packet)?` again internally. The length validation and `H256` construction happen twice for every packet that reaches the decode path. This is harmless but wasteful; consider threading the already-extracted `src_id` into `Ordinary::decode` or adding an internal variant that accepts a pre-validated src-id.
Reviews (1): Last reviewed commit: "fix(l1): reject short discv5 packet auth..." | Re-trigger Greptile
2 participants
Motivation
The discv5 packet-decode path read fixed-size fields out of the peer-controlled
authdatabefore validating its length, so a malformed packet could panic. Because the discv5 server runs as an actor whose runtime catches a handler panic and stops the loop (no restart), a single malformed packet could take the discv5 discovery actor down. The masking key is the destination node id (public), so an ordinary packet can be forged without a session — making the ordinary path reachable unauthenticated.Two sites:
discv5_handle_ordinaryextracted the 32-byte src-id viaH256::from_slice(&authdata)before the session lookup — panics on anyauthdata.len() != 32. Reachable unauthenticated (primary).WhoAreYou::decodeslicedauthdata[..16]before checking length — panics onauthdata.len() < 16. Reachable when a matching pending nonce exists.Description
Ordinary::src_id(&Packet) -> Result<H256, PacketCodecError>that returnsInvalidSizeunlessauthdata.len() == 32, and routeddiscv5_handle_ordinarythrough it (?).Ordinary::decodenow uses the same helper, so its length check is shared.WhoAreYou::decodenow rejectsauthdata.len() != 24(16-byte id-nonce + 8-byte enr-seq) up front, before slicing.Both return an error instead of panicking; the packet dispatch already logs-and-drops handler errors (
inspect_err), so a malformed packet is now discarded and the actor keeps running. Handshake decode was already length-guarded.Adds unit tests: short WHOAREYOU authdata and short ordinary authdata are rejected (previously panicked), and a well-formed 32-byte ordinary authdata still yields the src-id.
Checklist
cargo test -p ethrex-p2p discv5(incl. new authdata tests) andcargo clippy -p ethrex-p2ppass.