Skip to content

fix: stop duplicate detection dropping distinct packets that share a 64-byte prefix - #882

Open
Chessing234 wants to merge 2 commits into
permissionlesstech:mainfrom
Chessing234:fix/dedup-identity-strength
Open

fix: stop duplicate detection dropping distinct packets that share a 64-byte prefix#882
Chessing234 wants to merge 2 commits into
permissionlesstech:mainfrom
Chessing234:fix/dedup-identity-strength

Conversation

@Chessing234

Copy link
Copy Markdown

Replay and duplicate detection in SecurityManager keyed on this:

val payloadHash = packet.payload.sliceArray(0 until minOf(64, packet.payload.size)).contentHashCode()
"${packet.timestamp}-$peerID-$payloadHash"

A 32-bit contentHashCode over at most the first 64 bytes of the payload. Two packets from the same peer in the same millisecond that agree on that prefix are the same packet as far as this cache is concerned, and a collision here is not a false alarm — it is a dropped message. validatePacket returns false, the packet is discarded, and nothing anywhere reports that it happened.

Why this is worth changing rather than tuning

The repo already has the right answer to "are these the same packet". PacketIdUtil — first 16 bytes of SHA-256 over type, senderID, timestamp and the whole payload — is what GossipSyncManager uses for sync membership and what MessageHandler uses to assign message IDs. iOS derives it identically (bitchat/Sync/PacketIdUtil.swift), so it is also the cross-platform notion.

So the security path was carrying its own weaker private definition while the strong one sat in the same source tree, already used for the same question. Using it makes the two agree, and the FRAGMENT special case disappears on the way — it existed only because the general branch truncated at 64 bytes, and the full payload is covered either way now.

Peer scoping is deliberately kept. PacketIdUtil covers the packet's own senderID, which is not the same thing as the peer it was received from once a packet has been relayed, so the key stays "$peerID-$packetId" rather than becoming the bare packet ID.

What is not claimed

This is not a fix for a forged-packet attack. validatePacket already records only packets that pass signature verification, with a comment explaining exactly why, so an attacker cannot poison the cache against a peer without that peer's signature. What changes here is accidental collision between two honest packets, and the fact that the security layer and the sync layer no longer disagree about packet identity.

Verification

Local run of the CI job (testDebugUnitTest lintDebug, JDK 21). Counts from --rerun-tasks on both sides so they are real full-suite runs, not incremental leftovers:

  • 608 → 611 tests, 0 failures, lint clean. Per-class diff shows one class changed — SecurityManagerTest, 24 → 27 — and nothing else moved.
  • The collision test fails on unmodified main, which is the point: two packets sharing a 64-byte prefix and a timestamp, second one silently dropped.
  • The other two pass before and after by design — replay of an identical packet still rejected, and the same packet from two different peers still tracked separately. They are there so that strengthening the identity cannot quietly weaken what the cache exists to do.

CI has not run. Fork PRs here sit at action_required until a maintainer approves the workflow, so the local run is the evidence, not a green check.

Replay and duplicate detection keyed on a 32-bit contentHashCode over at
most the first 64 bytes of the payload. Two packets from the same peer in
the same millisecond that agreed on that prefix were the same packet as
far as this cache was concerned, and a collision here is a dropped
message: the second is discarded and nothing reports it.

PacketIdUtil is the identity the rest of the stack already uses for this
question — gossip sync membership, and the message IDs MessageHandler
assigns — and iOS derives it identically: first 16 bytes of SHA-256 over
type, senderID, timestamp and the whole payload. The security path now
agrees with the sync path instead of carrying a weaker private notion of
"same packet", and the FRAGMENT special case disappears because the full
payload is covered either way.

Peer scoping is deliberately kept. PacketIdUtil covers the packet's own
senderID, which is not the peer it arrived from once relayed.
The collision case fails on main: two packets sharing a 64-byte prefix and
a timestamp, where the second was silently dropped.

The other two pass before and after on purpose. Replay of an identical
packet must still be caught, and the same packet arriving from two
different peers must still be tracked separately — strengthening the
identity must not quietly weaken either.
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