fix: let a resubscribing central recover from the announce rate limit - #1669
Open
Chessing234 wants to merge 1 commit into
Open
fix: let a resubscribing central recover from the announce rate limit#1669Chessing234 wants to merge 1 commit into
Chessing234 wants to merge 1 commit into
Conversation
BLESubscriptionAnnounceLimiter refreshed lastAnnounceTime on rejected
attempts as well as allowed ones. That field is the reference for both
the backoff comparison and pruneStaleEntries, so a central that
resubscribed faster than the capped 30s backoff kept resetting its own
clock: elapsed time never grew past the backoff, attemptCount climbed
past bleSubscriptionRateLimitMaxAttempts, and suppressAnnounce stayed
true for as long as it kept trying.
The BCH-01-004 limiter exists to blunt enumeration attacks, but a
legitimate central with a flapping BLE link resubscribes the same way --
and once suppressed it could never be re-admitted, so we stayed
invisible to that peer indefinitely.
Keep lastAnnounceTime at the last allowed announce, which is what
TransportConfig documents it as ("minimum interval between announces per
central"). Escalation is unchanged: attemptCount still climbs and
suppressAnnounce still engages, but a central is re-admitted once the
capped backoff has elapsed since it was last actually announced to.
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
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.
Found by reading
BLESubscriptionAnnounceLimiter, not from an issue.The bug
decision(for:now:)rebuilds the entry on a rejected attempt withlastAnnounceTime: now:That field is the reference for both comparisons in this type — the backoff
check (
now.timeIntervalSince(existing.lastAnnounceTime)) andpruneStaleEntries. Refreshing it on rejection means the elapsed time is resetby the very attempt that was refused, so for a central that keeps resubscribing:
currentBackoffSeconds, which has meanwhile beendoubled up to the 30 s cap, so every subsequent attempt is refused;
attemptCountclimbs pastbleSubscriptionRateLimitMaxAttempts(5), sosuppressAnnouncelatches true;pruneStaleEntriesnever fires either, because it measures staleness fromthe same refreshed timestamp.
The result is that the central is suppressed for as long as it keeps trying, with
no path back.
Why it matters beyond the attack case
The limiter is the BCH-01-004 enumeration defence, and against a hammering
attacker the behaviour looks like a feature. But a legitimate central with a
flapping BLE link resubscribes exactly the same way —
didSubscribeTofires onevery reconnect. Once it crosses 5 attempts we stop announcing to it
(
BLEService+LinkLayerPeripheralRole.swift:151-156returns early), so we stayinvisible to that peer until the app restarts or it stops reconnecting for a
full window — and it cannot stop reconnecting, because the link is flapping.
The fix
Keep
lastAnnounceTimeat the last allowed announce. That is whatTransportConfigalready documents it as — "Minimum interval between announcesper central" — and it makes the backoff mean what it says.
Escalation is deliberately unchanged:
attemptCountstill climbs, the backoffstill doubles to the 30 s cap, and
suppressAnnouncestill engages at 5attempts. The only change is that elapsed time is now measured from a fixed
point, so a central is re-admitted once the capped backoff has genuinely passed
— i.e. at most one announce per 30 s for someone hammering, instead of none ever.
Test plan
swift test --parallel— 2020 tests in 219 suites passed. Baseline onmain: 2018 in 219, also green. The delta is exactly the 2 tests added here.unmodified
main:Expectation failed: (admittedAt → nil) != nil— after 60 s of oneresubscribe per second the central was still
.rateLimited(backoffSeconds: 30.0, attemptCount: 120, suppressAnnounce: true).re-running: the recovery test fails, the hold-off test still passes, and the
three pre-existing tests are unaffected either way.
New coverage:
regression itself, and it asserts re-admission happens at or after the
capped backoff, not merely that it happens.
counterpart, so the fix cannot be "corrected" into admitting early and
weakening the enumeration defence.
The three existing tests (first-allowed/then-limited, suppression threshold,
stale pruning) are untouched and still pass.