p2p: bound relay address query responses - #4642
Conversation
Relay address resolution read the HTTP response body with io.ReadAll and no size limit, using a zero-value http.Client with no timeout, in a loop that runs for the process lifetime. A malicious or compromised configured relay could stream an endless response and grow the heap until the node was OOM killed. Limit the response to 64KB, which is well above a valid ENR string or multiaddr array, set a 10s per-attempt client timeout, and close the response body on the non-2xx retry path where it was leaked. category: bug ticket: none
|
There was a problem hiding this comment.
Pull request overview
Mitigates a potential OOM/connection-leak issue in long-lived relay address resolution by bounding HTTP response body reads and adding a per-attempt HTTP client timeout, and adds regression tests to validate the bounds behavior.
Changes:
- Bound relay address query response bodies to 64KiB and retry on oversized responses (instead of parsing truncated data).
- Add a 10s per-attempt HTTP client timeout and ensure response bodies are closed on non-2xx retry paths.
- Add internal tests covering valid, oversized, boundary-sized, unbounded streaming, and non-200 relay responses.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| p2p/bootnode.go | Adds response size limiting, per-attempt timeout, and fixes response-body close on non-2xx retries in relay address resolution. |
| p2p/bootnode_internal_test.go | Adds tests to ensure response reads are bounded and retries behave correctly across edge cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4642 +/- ##
==========================================
+ Coverage 58.16% 58.26% +0.10%
==========================================
Files 247 247
Lines 34063 34069 +6
==========================================
+ Hits 19814 19852 +38
+ Misses 11774 11734 -40
- Partials 2475 2483 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Do we have metrics from the size of the relay data that was exchanged? If so, is it the case that those limits would suffice? With both QUIC and TCP. I generally think this PR has higher ROI than the other transport-related PRs, as it is relay related. |
No, we don't have historical metrics. I queried the production relays directly to get a real number:
The 64KB limit is ~350× the actual production response (~180 bytes) and would still fit ~600 multiaddrs, so it's safe for both transports and any plausible relay configuration. Note that the limit applies only to the HTTP address-discovery query; the relayed p2p traffic itself (QUIC or TCP) is unaffected. |
Relay address resolution read the HTTP response body with
io.ReadAlland no size limit, using a zero-valuehttp.Clientwith no timeout, in a loop that runs for the process lifetime. A configured relay returning an endless response body could therefore grow the heap until the node was OOM killed. Reported via the bug bounty program.Limits the response to 64KB, which is well above a valid ENR string or multiaddr array, and rejects anything larger instead of parsing a truncated body. Adds a 10s per-attempt client timeout, since the request was previously bound only to the long-lived application context. Closes the response body on the non-2xx retry path, where it was leaked along with its connection on every retry.
Oversized responses warn and retry, matching how the existing non-2xx, JSON and multiaddr failures are handled; returning an error instead would make
resolveRelayabandon the relay for the rest of the process lifetime.Adds
p2p/bootnode_internal_test.go, which had no coverage.TestQueryRelayAddrsBoundsResponsecounts the bytes a streaming server manages to write and asserts a hard bound, so it exercises the size limit independently of the timeout. With the limit removed but the timeout in place, that server writes 2.8GB within the 10s window, so the timeout alone is not sufficient mitigation. The oversized case uses valid JSON padded past the limit so the size check rejects it rather than the parser, and a case at exactly the limit covers the boundary.category: bug
ticket: none