fix(node): bound replay of authenticated gossip ref-update events - #334
Open
beardthelion wants to merge 5 commits into
Open
fix(node): bound replay of authenticated gossip ref-update events#334beardthelion wants to merge 5 commits into
beardthelion wants to merge 5 commits into
Conversation
Builds the two layers that bound replay of an authenticated ref-update, with nothing calling them yet; the ingest path is wired in the next commit. The freshness check is deliberately two-directional rather than an absolute delta: an abs() window admits an event stamped up to the window ahead and pins its seen-set slot until the clock catches up. Producers were enumerated before settling the unparseable arm; the sole production publish site emits RFC-3339, so an unparseable timestamp is refused rather than admitted. The replay guard keys on SHA-256 of the canonical signing bytes, not the raw wire bytes, because one signature verifies against many encodings and only the canonical form collapses them to a single key. A golden digest is frozen for the pre-version artifact, and the same constant is asserted for its v-injected twin, so the collapse is pinned rather than described. Reservations settle through a drop guard so only a confirmed entry outlives the ingest call, which keeps a transient write failure from permanently burning an event's slot. Replayed and StaleTimestamp are separate outcomes because they diagnose different conditions, a mesh replay against a broken clock or a healing partition, and folding them would be the same observability lie the unsigned shed variant already exists to avoid.
Wires the freshness check and the replay guard into ingest, immediately after signature verification and above the author debit. The lower placement, just above the writes, stops the duplicate row and the duplicate sync but still lets every replay drain the victim author's budget, which is the harm the defect names: a captured signature replayed 500 times empties the victim's window and their next genuine push is refused. The guard runs on the verified path only. Unsigned event bytes are predictable, so applying it there would let an attacker pre-send a victim's expected event and have the genuine one dropped as a replay, which is a censorship primitive rather than a defense. A replay flood still costs a parse and one Ed25519 verify, because the guard has to sit below verification for the reason above. What it removes is the peer_exists round trip, the victim's author debit, the ref-update row and the sync enqueue. The existing author-budget test signed one event and ingested the same bytes five hundred times, so it had to re-sign per iteration to keep exercising the budget under a shared guard. Giving it a fresh guard per call would have kept it green while gutting the property it exists to prove. Its over-budget probe needed the same treatment, since the replay gate sits above the author gate and would have refused the burst's last bytes before the budget assertion ran. Both saturation tests now take a shared lock so each keeps an exact assertion on a process-wide counter; a lower bound would stay green if the fail-open branch ever double-counted.
At capacity the inline sweep ran on every event, so a full O(capacity) retain happened under the lock once per message and reclaimed nothing when nothing had expired. The guard that bounds replay became a CPU amplifier in exactly the state an attacker drives toward. The sweep is now rate limited to once a second; the periodic sweep still reclaims on its own cadence, so only the redundant rescans go away. An unparseable timestamp was echoed into the refusal detail verbatim. That value is attacker-controlled and arbitrary length, so it reached a warn! as both a log-injection and an unbounded-size sink. Only that arm needs sanitizing; the other two ran through the parser first. The capacity rationale cited a count of registered DIDs, which this same file says elsewhere an attacker mints freely through the announce path, so it was not a bound at all. It now cites the bound that is real: reaching saturation costs a hundred thousand durable rows and as many sync enqueues inside one retention horizon, which the database makes loud. ingest_now was read twice per ingest while its own doc comment claimed the two layers share one reading, which is the invariant the retention derivation rests on. Now read once and passed to both. Also: the Unparseable outcome is driven through ingest rather than only as a pure function, the periodic sweep is observable without a live swarm, the restart exposure is written down where the rest of the tradeoffs already are, and the saturation-counter lock covers every test that can reach Saturated rather than the two I first found.
…e sibling window Seven tests for the six gaps a review found. The reservation's release path was reasoned rather than executed: the drop guard was only ever proven through an early refusal, while the case its own doc comment names, a transient write failure burning the event's slot, was never driven. Both write directions now are. An expired entry at capacity must be replaced in place rather than answer Saturated, confirm on an already-swept entry is pinned as a deliberate no-op, and the single-critical-section shape is now driven concurrently rather than asserted sequentially. Restart behavior was documented but untested; a fresh guard readmits a seen event and the freshness window still bounds it, which is the composition that makes the restart exposure finite. check_created in gitlawb-core used a symmetric abs() window, so a request stamped 299 seconds ahead was accepted and a signer could roughly double a signature's effective validity by stamping forward. It is now two comparisons like the gossip path, 300s late and 60s early, with the error naming the direction so a fast peer and a slow one need different operator action. There was no future-direction test at all; there are now five covering both. Every caller was checked and none depended on the symmetry. Adding a freshness window obliges auditing the siblings, which is how this one surfaced: the repo argued both ways in two files for a week.
…ng the window check created is parsed as an unrestricted i64 straight from the Signature-Input header, so the sender picks it. At i64::MIN the past-side subtraction overflows, which panics a debug build and wraps a release one into a value that can read as inside the window. Confirmed by execution before the fix: 'attempt to subtract with overflow'. Saturating subtraction answers correctly at both ends, since a timestamp that far out is refused by whichever side it saturates toward. A test drives all four extremes of the type and asserts each is refused by a named direction; reverting to plain subtraction reddens it on the overflow. The symmetric abs() form this replaced had the same hazard, so splitting the window into two comparisons did not introduce it, but it did not remove it either. Found by a cross-family review pass after six same-family reviewers and I had all read the line.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
beardthelion
added
crate:core
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
1 participant
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.
Bounds replay of authenticated gossip ref-update events. Stacked on #325, which is what makes these events authenticated in the first place.
Why this is separate from #325 rather than folded into it
With #325 merged and this not yet merged, an attacker's capability is strictly lower than it was before #325: forging arbitrary ref-updates for any peer required no key at all, and afterwards they are reduced to replaying events a legitimate node actually published, with the same repo, refs, and shas. The intermediate state is better than the pre-state, not worse, so the split does not leave a window where the system is more exposed than it was.
That is the test I applied, and it is the one the #173/#321 split failed: that resolver hardening withheld the provider CID for unrepaired rows, so deploying it alone actively broke existing pins, and the ordering argument only held as long as nothing shipped in the gap. Writing this out here rather than leaving it in a comment is the other half of that lesson.
What it does
Two layers, following the shape production gossip systems use, where a dedup-layer fix and an authentication-layer fix are paired rather than either being treated as sufficient.
A freshness window on the already-signature-covered
timestamp: 600 seconds into the past, 60 into the future. Two comparisons, never a distance. Anabs()window accepts a future-dated event as readily as a late one, and a future-dated event also pins a seen-set slot while sitting outside the past-window check until the clock catches up.A bounded seen-set keyed on SHA-256 of the canonical signing bytes, not the raw wire bytes. That distinction is the whole point: one signature verifies against a family of wire encodings, so a raw-bytes key deduplicates nothing. The frozen pre-version artifact and the same artifact with
"v":0injected are different lengths, both verify against one signature, and produce identical signing bytes.The guard runs immediately after signature verification and above the per-author debit. Below the debit it would still stop the duplicate row and the duplicate sync while letting every replay drain the victim author's budget, which is the harm this is about. It runs on the verified path only: unsigned bytes are predictable, so applying dedup there would let an attacker pre-send a victim's expected event and have the genuine one dropped as a replay.
A key is recorded only on acceptance, through a reservation whose drop releases it, so a transient write failure does not permanently burn an event's slot. At capacity the guard fails open and counts the degradation, because a saturated set that dropped all fresh gossip would convert a loud resource attack into quiet mesh-wide censorship.
Also included:
check_createdin gitlawb-core used a symmetricabs()window, so it tolerated 300 seconds of future-dating and a signer could roughly double a signature's effective validity by stamping forward. It is now two comparisons like the gossip path. Adding a freshness window obliges auditing the siblings, which is how that surfaced.What is proven rather than argued
Every guard here is backed by a mutation that turns a named test red, and they are present-but-wrong re-implementations rather than deletions, because each of these compiles and reads sensibly: keying on raw wire bytes, keying fresh per ingest, keying on
(repo, ref, sha), anabs()freshness window, recording before accept, failing closed at saturation, placing the guard below the author debit, a sweep that stops evicting, confirming on a write failure, and treating an expired entry as absent at capacity.Two are worth calling out because the obvious version of each test cannot catch what it names. The revert case needs three events, since two never collide under a
(repo, ref, sha)key. The layer-composition case has to run at the future-skew edge, since a present-stamped probe is rejected by freshness anyway and stays green under a shortened retention.An attacker-supplied
createdcould overflow the window subtraction and panic a debug build; that is fixed with saturating arithmetic and a test driving all four extremes of the type.Known open, not closed here
A compromised or malicious registered signer can still mint fresh signed events with fresh timestamps, each of which gets a distinct key and passes freshness. This bounds third-party replay of a captured event; it is not an aggregate write-volume defense and should not be described as one.
The seen-set is in-process, so a restart readmits any event still inside its freshness window, once per restart. Bounded by the window rather than unbounded, and documented on the guard.
POST /api/v1/sync/notifyreaches the same two sinks with no dedup. Bounded differently there, since that handler still accepts unsigned notifications naming any known peer DID, so forgery already beats replay and the per-IP limiter is the real bound. Worth closing separately.The freshness window is an availability bound with no resume: a peer returning from a partition longer than the window drains a backlog stamped at push time and every one of those events is dropped, with no republish and no repair path. That is a product decision about what federation should do with stale-but-genuine updates, not something to patch quietly.
Full suite, clippy and fmt clean locally; CI has the authoritative run for this head.