fix: bound the work and traffic one peer can trigger through REQUEST_SYNC - #885
fix: bound the work and traffic one peer can trigger through REQUEST_SYNC#885Chessing234 wants to merge 4 commits into
Conversation
REQUEST_SYNC is a public packet, handled in PacketProcessor's unauthenticated branch and passed straight to GossipSyncManager. Nothing between the radio and the response limits how often a neighbor may send one, so a single peer decided how often we decode a GCS filter, run SHA-256 over every stored packet, and transmit whatever its filter claims to be missing. A filter that claims to hold nothing is a few bytes and makes us replay up to seenCapacity() packets — 500 by default. Repeating it costs the sender nothing and saturates a link measured in kbps. Give each requester a token bucket: one request per 5s with a burst of 2, so the scheduled first sync and a periodic one both land, and a response allowance that refills to seenCapacity() over the 30s periodic interval — the most a peer can legitimately be missing in one round. An honest peer never reaches either limit, so what it receives is unchanged. The bookkeeping is keyed by a peer ID off the wire, so it is an access-ordered LinkedHashMap capped at 256 entries; otherwise the mitigation would be its own memory exhaustion vector.
Pins both halves: a flood is shed after the burst allowance and cannot outrun the refill window, and a peer syncing normally still receives every packet it is missing. Also covers per-peer isolation, a backwards clock jump, and the cap on the requester table. Reverting the admission check fails four of the eight; the other four are the honest-peer guards and pass either way, which is what they are for.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48b85fb498
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val budget = requesterBudgets.getOrPut(fromPeerID) { | ||
| RequesterBudget( | ||
| requestTokens = AppConstants.Sync.REQUEST_BURST.toDouble(), | ||
| responseTokens = capacity, |
There was a problem hiding this comment.
Because REQUEST_SYNC is intentionally outside the signed public-packet set, fromPeerID here is just the sender ID parsed from the packet, so any neighbor can mint fresh buckets by rotating spoofed IDs or spend another peer's request tokens by using that peer's ID. With the 256-entry LRU, a 257-ID cycle keeps each request admitted before decode/hashing, so the flood path this change is meant to close remains available; key the budget to a non-spoofable ingress/link identity or authenticate the claimed peer before granting a new bucket.
AGENTS.md reference: AGENTS.md:L90-L91
Useful? React with 👍 / 👎.
SecurityManager.verifyPacketSignature only authenticates ANNOUNCE, MESSAGE, FILE_TRANSFER, VOICE_FRAME and LEAVE; every other type returns early as verified. REQUEST_SYNC is not in that set, so the sender ID is raw bytes off the wire and costs nothing to rotate. Keyed on that, the budget was bypassable: cycling more IDs than the LRU tracks gives every request a brand new bucket with a full allowance, so the flood this was meant to stop still went through. Found by Codex on the first push. Key it on RoutedPacket.ingressLinkID instead — assigned by the transport, never serialized onto the mesh, and distinct per link. A flooder is then bounded by the connections it can actually hold open rather than by the names it is willing to invent. Falls back to the sender ID only when a transport supplies no link identity.
|
good catch — REQUEST_SYNC isn't in the set verifyPacketSignature authenticates, so the sender id is free to rotate and the budget was bypassable exactly as described. rekeyed it on routed.ingressLinkID, which the transport assigns and never puts on the wire. two tests cover it: rotating ids on one link now shares one bucket, and separate links stay independent. |
AGENTS.md requires protocol and security changes to update the relevant specification. sync.md already carries the receiver-side DoS rules — the filter-length cap and the malformed-payload rejections — but says nothing about how often a request may be serviced or how much it may pull, which is the gap this branch closes in the Android client. Record it where the other receiver limits live, including why the budget cannot be keyed on the sender ID: REQUEST_SYNC carries no signature that receivers verify, so that field is free to rotate. Other implementations need some equivalent even if they pick different numbers.
|
also added the responder budget to docs/sync.md, since AGENTS.md asks for the spec to move with protocol changes and that file already carries the other receiver-side limits (filter cap, malformed-payload rejection) but nothing about servicing rate. wrote down the sender-id caveat there too — it applies to any implementation, not just this one. flagging the gate i can't meet: this touches packets and routing, so per AGENTS.md it wants mesh lab validation on physical devices. i don't have two, so the evidence here is the unit suite plus testDebugUnitTest/lintDebug locally. happy to rework if a lab run shows the 5s/30s budgets are too tight for a real multi-peer catch-up. |
1 participant
What
REQUEST_SYNCis handled inPacketProcessor's public/unauthenticated branch (PacketProcessor.kt:142) and handed straight toGossipSyncManager.handleRequestSync. Nothing on that path limits how often a neighbor may send one.So a single peer decides how often we:
PacketIdUtil.computeIdBytes, once per stored packet per request),A filter that claims to hold nothing is a few bytes on the wire (
p=1, m=1, data=[]) and makes us replay up toseenCapacity()packets — 500 by default. Repeating it costs the sender nothing, and the reply goes out over a link measured in kbps.Fix
A token bucket per requester, checked before any decoding or hashing:
scheduleInitialSyncToPeer, 5s) and a periodic one (30s) both land.seenCapacity()over the 30s periodic interval, which is the most a peer can legitimately be missing in one round.An honest peer never reaches either limit, so what it receives is unchanged.
The table is keyed by a peer ID read off the wire, so it's an access-ordered
LinkedHashMapcapped at 256 entries — otherwise the mitigation would be its own memory-exhaustion vector.Evidence
GossipSyncRequestBudgetTest, 8 tests. Four pin the limits, four pin that normal sync is untouched.Teeth check — with
admitRequestbypassed, exactly the four limit tests fail:The other four are the honest-peer guards and pass either way, which is the point of them.
Ran locally with JDK 21:
Measured the same way on
mainand on this branch, the only difference is the new class: 90 classes / 591 tests → 91 / 599. Lint is unchanged frommain(305 errors, 333 warnings, 17 hints — all pre-existing/baselined).Not run locally: instrumented tests and any on-device check. I don't have two radios to hold a real mesh flood against, so the throttle is verified against the manager's own interface rather than over BLE.