server: sweep dstNodes — unauthenticated unbounded map growth - #51
Merged
Conversation
dstNodes had no Delete anywhere. Entries were Stored in the discover handler (line 652) and only ever Loaded on the relay path (line 1135). It appeared in no sweep — reapStaleNodes covers nodes, punchRL, relayRL, discoverRL, gossip peers and nodePubKeys, but never this one. The node id is read straight off the wire (binary.BigEndian.Uint32(data[0:4])) in an unauthenticated datagram, so the key space is the full uint32 range and an attacker picks it freely. Steady, unbounded growth on a process that runs embedded in the production registry. Same treatment as nodePubKeys directly above: an entry is only meaningful while the node is still known, so drop it once the node has aged out of s.nodes. Mirrors the existing pattern exactly, in the sweep that already runs. Found during a codebase-wide sweep for resource leaks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TestRelayBudgetIsPerDatagramSource asserted a flat ceiling of maxRelaysPerSourcePerSecond after firing 3x that many packets in a loop. That assumed the whole burst fits inside one rate window. It does on a fast dev box (~0.25s) and does not on CI. The budget is a one-second window — allow() rolls it when nowNano-windowStart >= 1s — and dispatchRelay reads the real clock, so a loaded runner spending 2.3s in the loop legitimately opens three windows and passes ~3x the per-window budget. The test has been failing on main since it landed (e.g. run 30205771783, 2026-07-26, "enqueued 2344 ... budget is 1000"), so main has been red and every branch inherited it. The property under test is that rotating the SENDER ID does not multiply the per-source budget — not how fast the machine is. So the ceiling is now scaled by the number of windows the loop actually spanned, and a second assertion pins the real invariant independently of timing: not every attempt may be enqueued, which is exactly what would happen if per-source keying regressed. Verified passing repeatedly, including with -cpu=1 to approximate a contended runner. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
dstNodeshad noDeleteanywhere. Entries wereStored in the discover handler (line 652) and only everLoaded on the relay path (line 1135). It appeared in no sweep —reapStaleNodescoversnodes,punchRL,relayRL,discoverRL, gossip peers andnodePubKeys, but never this one.The node id is read straight off the wire (
binary.BigEndian.Uint32(data[0:4])) in an unauthenticated datagram, so the key space is the full uint32 range and an attacker picks it freely. Steady, unbounded growth — on a process that runs embedded in the production registry.Fix
Same treatment as
nodePubKeysdirectly above it: an entry is only meaningful while the node is still known, so drop it once the node has aged out ofs.nodes. Mirrors the existing pattern exactly, in the sweep that already runs.Build, vet and all beacon tests pass.
Found during a codebase-wide sweep for resource leaks.
🤖 Generated with Claude Code