fix: bound future-dated timestamps on every packet type, not just LEAVE - #888
Open
Chessing234 wants to merge 2 commits into
Open
fix: bound future-dated timestamps on every packet type, not just LEAVE#888Chessing234 wants to merge 2 commits into
Chessing234 wants to merge 2 commits into
Conversation
validatePacket bounds the timestamp of exactly one packet type. LEAVE
gets a symmetric five-minute window; ANNOUNCE gets one in
AnnouncementIdentityValidator. Everything else — MESSAGE, FRAGMENT,
FILE_TRANSFER, VOICE_FRAME — is admitted with any timestamp its sender
signs, because a signature authenticates the sender's choice without
sanity-checking it.
That timestamp then decides ordering downstream. GossipSyncManager builds
its advertised sync filter with sortByDescending { it.timestamp } and
takes the top N, so broadcasts stamped far in the future occupy every
slot and keep real traffic out of gossip sync — not just locally, but for
every neighbor that syncs against us. The public timeline orders on the
same field.
Bound the future direction for every type at the allowance that already
admits the peer's announcement. A device skewed further than that cannot
get an ANNOUNCE verified, so it has no working session today and nothing
that currently works starts failing. LEAVE keeps its tighter symmetric
window.
The past deliberately stays unbounded: store-and-forward replays packets
cached for up to twelve hours, and bounding it would drop them silently.
Extends the existing LEAVE replay-window test to the relayed types. Pins that the bound is future-only — a six-hour-old message still delivers so store-and-forward keeps working — and that it is no tighter than the announcement allowance. Removing the check fails the two rejection tests and leaves the three admission tests green.
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.
What
SecurityManager.validatePacketbounds a packet's timestamp for exactly one type:ANNOUNCE is bounded separately, in
AnnouncementIdentityValidator(10 minutes, symmetric). Everything else — MESSAGE, FRAGMENT, FILE_TRANSFER, VOICE_FRAME — is admitted with any timestamp its sender signs. A signature authenticates that the sender chose that value; it doesn't sanity-check it.That timestamp then drives ordering downstream. The one that matters most is
GossipSyncManager.buildGcsPayload:list.sortByDescending { it.timestamp.toLong() } ... val takeN = minOf(nMax, cap, list.size)Broadcasts stamped far in the future sort first and occupy every advertised slot, so real traffic stops being advertised for gossip sync — not just locally, but to every neighbour that syncs against us. The public timeline orders on the same field.
Fix
Bound the future direction for every packet type, at the allowance that already admits the peer's announcement (
AnnouncementIdentityValidator.MAX_CLOCK_SKEW_MS, nowinternalso the two cannot drift apart).Choosing that specific bound matters: a device skewed further than this cannot get an ANNOUNCE verified, so it has no working session today. Nothing that currently works starts failing. LEAVE keeps its tighter symmetric window.
The past stays deliberately unbounded.
StoreForwardcaches for 12 hours and replays those packets, so a symmetric bound would drop store-and-forward delivery silently.Evidence
Four tests added to
SecurityManagerTest(24 → 28), extending the existing LEAVE replay-window test to the relayed types. Two pin rejection; two pin that the bound is future-only and no tighter than the announcement allowance.Removing the check fails exactly the two rejection tests:
The three admission tests — tolerable forward skew, and a six-hour-old message for store-and-forward — pass either way, which is what they are for.
Measured the same way on
mainand here, the only class that changed isSecurityManagerTest: 591 → 595 tests. Lint unchanged frommain.What I could not verify
No on-device or two-radio test — this is verified against
SecurityManager's own interface. The judgement call worth a second opinion is the allowance: I tied it to the announcement's 10 minutes rather than the 5 minutes LEAVE uses, on the reasoning above. If you'd rather every type shared LEAVE's 5-minute window, that's a one-constant change, but it would start dropping traffic from peers whose ANNOUNCE we still accept.