From 1e886e96bae489b9fa82a1a40e5c4f0ba4074bc9 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Tue, 28 Jul 2026 19:17:45 +0300 Subject: [PATCH 1/2] =?UTF-8?q?server:=20sweep=20dstNodes=20=E2=80=94=20un?= =?UTF-8?q?authenticated=20unbounded=20map=20growth?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- server.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/server.go b/server.go index d041812..434ed2b 100644 --- a/server.go +++ b/server.go @@ -1311,6 +1311,24 @@ func (s *Server) reapStaleNodes() { } return true }) + + // dstNodes had no Delete anywhere: entries were Stored in the discover + // handler and only ever Loaded on the relay path. The node id comes + // straight off the wire (data[0:4]) in an unauthenticated datagram, so + // the key space is the full uint32 range and an attacker chose it + // freely — steady, unbounded growth on a process that is 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. + s.dstNodes.Range(func(k, _ interface{}) bool { + id, ok := k.(uint32) + if ok && !s.nodes.Has(id) { + s.dstNodes.Delete(k) + } + return true + }) } // --- Gossip --- From 3d2e757fdb1615fba8e4edfb78553d1e73c0f44a Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Wed, 29 Jul 2026 14:51:39 +0300 Subject: [PATCH 2/2] test: make the relay-budget assertion time-aware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- zz_relay_source_limit_test.go | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/zz_relay_source_limit_test.go b/zz_relay_source_limit_test.go index f71303e..68c63b7 100644 --- a/zz_relay_source_limit_test.go +++ b/zz_relay_source_limit_test.go @@ -49,14 +49,37 @@ func TestRelayBudgetIsPerDatagramSource(t *testing.T) { // Three times the per-source budget, every packet claiming a // different sender id. const attempts = maxRelaysPerSourcePerSecond * 3 + start := time.Now() for i := 0; i < attempts; i++ { s.dispatchRelay(relayFrame(uint32(i+1), dest, "x"), relaySourceForUDP(src)) } + elapsed := time.Since(start) got := drainRelayCh(s) - if got > maxRelaysPerSourcePerSecond { - t.Fatalf("one datagram source enqueued %d relays with rotating sender ids; budget is %d per second", - got, maxRelaysPerSourcePerSecond) + + // The budget is a 1-second window (allow() rolls the window when + // nowNano-windowStart >= 1s), so the ceiling has to account for how + // long the loop actually took. dispatchRelay reads the real clock — + // allow() takes nowNano but the caller supplies time.Now() — so a + // loaded CI runner that spends 2.3s in this loop legitimately opens + // three windows and passes ~3x the per-window budget. + // + // Asserting a flat budget assumed the whole burst fit inside one + // window. That held on a fast dev box (~0.25s) and failed on CI, + // where this test has been red on main since it landed. The property + // under test is that rotating the SENDER ID does not multiply the + // budget — not how fast the machine is. + windows := int(elapsed/time.Second) + 1 + ceiling := maxRelaysPerSourcePerSecond * windows + if got > ceiling { + t.Fatalf("one datagram source enqueued %d relays with rotating sender ids in %v; "+ + "budget is %d per second and at most %d window(s) elapsed (ceiling %d)", + got, elapsed, maxRelaysPerSourcePerSecond, windows, ceiling) + } + // Independently of timing, the limiter must actually limit: without + // per-source keying every one of the attempts would be enqueued. + if got >= attempts { + t.Fatalf("all %d relays enqueued — rotating the sender id bypassed the per-source budget entirely", got) } if got == 0 { t.Fatalf("no relays enqueued at all; the limiter rejected everything")