Skip to content

fix: stop NoiseRateLimiter retaining every peer it has ever seen - #1670

Open
Chessing234 wants to merge 1 commit into
permissionlesstech:mainfrom
Chessing234:fix/noise-rate-limiter-unbounded-peer-maps
Open

fix: stop NoiseRateLimiter retaining every peer it has ever seen#1670
Chessing234 wants to merge 1 commit into
permissionlesstech:mainfrom
Chessing234:fix/noise-rate-limiter-unbounded-peer-maps

Conversation

@Chessing234

Copy link
Copy Markdown
Contributor

Found by reading NoiseRateLimiter, not from an issue.

The bug

allowHandshake(from:) and allowMessage(from:) filter stale timestamps for
the peer being queried:

var timestamps = handshakeTimestamps[peerID] ?? []
timestamps = timestamps.filter { $0 > oneMinuteAgo }

Nothing filters the dictionaries themselves. A peer that handshakes or messages
once and never returns keeps its entry — with its now-meaningless timestamps —
for the lifetime of the process, because the only code that would drop it is the
filter that runs when that same peer is queried again.

So both maps grow with every distinct PeerID the node has ever admitted. That
is not a bounded set: geohash peers churn, and identities rotate.

Two things confirm nothing else reclaims them:

  • reset(for:) is the per-peer counterpart and is never called from
    production code
    — the only reference is its own test,
    test_reset_clearsPerPeerHandshakeLimit. Peer cleanup was evidently intended
    and never wired up.
  • resetAll() runs only from clearEphemeralStateForPanic().

It is also the component least able to afford this: an attacker can add an entry
per spoofed PeerID at the cost of one admitted handshake each, against the very
type that exists to bound handshake cost.

For contrast, the sibling SyncResponseRateLimiter already has
prune(now:)"Drops history outside the window so departed peers don't
accumulate"
. This type is the outlier.

The fix

Sweep peers whose timestamps have all aged out of their own window (60 s for
handshakes, 1 s for messages), so each map holds only peers seen inside that
window.

Rate-limited at once per second. Sweeping on every admission would put an
O(peers) walk on the message path, which runs up to
maxGlobalMessagesPerSecond times a second; once a second is frequent enough
given the windows being enforced are 1 s and 60 s.

Also injects the clock. Every sibling limiter takes now as a parameter —
SyncResponseRateLimiter.shouldRespond(to:now:),
BLESubscriptionAnnounceLimiter.decision(for:now:),
BLEAnnounceThrottle.shouldSend(force:now:) — while this one called Date()
inline. That is precisely why none of its time-dependent behaviour had coverage,
and why this went unnoticed. The default argument (= Date.init) leaves all nine
call sites untouched.

trackedPeerCount mirrors BLESubscriptionAnnounceLimiter.trackedCentralCount
so retention is observable from a test.

Test plan

  • swift test213 XCTest cases and 2018 swift-testing cases, 0 failures.
    Baseline on main: 210 XCTest and 2018 swift-testing, also green. The delta
    is exactly the 3 tests added here.
  • The two retention tests were written before the fix and confirmed to fail
    against unmodified main:
    XCTAssertEqual failed: ("21") is not equal to ("1") for handshakes and
    ("11") is not equal to ("1") for messages — i.e. every departed peer was
    still held.
  • Teeth re-checked after the fix by deleting only the two
    pruneStalePeersLocked(now:) calls and re-running: the same two tests fail
    and nothing else does.
  • No new warnings.

New coverage:

  • handshake map does not retain peers past their window — a burst of one-shot
    peers, then one query past 60 s; only the live peer should remain.
  • message map does not retain peers past their window — same shape against the
    1 s window.
  • pruning keeps a peer still inside its window — the counterpart. A peer that
    has exhausted its per-peer budget must stay rate limited across a prune,
    so the sweep cannot be "simplified" into something that resets budgets and
    makes the limit bypassable by waiting one second.

The five existing tests are untouched and still pass.

Not in scope

reset(for:) is still uncalled. Wiring it to peer departure would reclaim
entries sooner, but it needs a disconnect hook and is a separate change; the
sweep here bounds the maps regardless of whether that ever lands.

handshakeTimestamps and messageTimestamps were only ever filtered for
the peer being queried. A peer that handshaked or messaged once and
never came back kept its dictionary entry for the lifetime of the
process, so both maps grew with every distinct PeerID the node had ever
admitted -- and PeerIDs churn as geohash peers come and go and as
identities rotate.

reset(for:) is the per-peer counterpart and is never called from
production code (only from its own test), so nothing reclaimed them.
resetAll() only runs from clearEphemeralStateForPanic().

Sweep peers whose timestamps have all aged out of their own window,
at most once a second so the message admission path does not take an
O(peers) walk up to maxGlobalMessagesPerSecond times a second. Both
maps now hold only peers seen inside their window.

Also inject the clock. Every sibling limiter takes `now` as a parameter
-- SyncResponseRateLimiter, BLESubscriptionAnnounceLimiter,
BLEAnnounceThrottle -- while this one read Date() inline, which is why
none of its time-dependent behaviour had coverage. The default argument
keeps all nine call sites unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant