fix: stop an injected fragment from destroying or poisoning a reassembly - #880
Open
Chessing234 wants to merge 2 commits into
Open
fix: stop an injected fragment from destroying or poisoning a reassembly#880Chessing234 wants to merge 2 commits into
Chessing234 wants to merge 2 commits into
Conversation
Fragment packets are unauthenticated and bypass the deduplicator, and both halves of the reassembly key are attacker-choosable, so a stream in flight has to survive a crafted packet aimed at it. Two paths discarded the whole set on a single bad fragment: A header whose total or originalType disagrees with the pinned metadata now rejects that fragment and leaves the stream alone. Nothing from a conflicting header was ever stored, so keeping the set stays inside the same memory bound, and a stream that genuinely stalls is still reaped by the timeout sweep. A fragment that trips the per-set cumulative cap only clears the set when it created it. An oversized fragment must not be able to destroy an assembly it did not start — which is the rule the global-cap branch a few lines below already applied. Both are demonstrated against a real fragmented packet: each test fails on main and passes here. Ports the storage half of permissionlesstech/bitchat#1515.
The index map was written unconditionally, so a duplicate carrying different bytes replaced what the real sender had already delivered — last-wins, one packet, no accumulation needed. An index already held is now immutable: a differing duplicate is rejected and the held bytes stand. This protects indices already filled. An index still empty when the injected fragment arrives is filled by whichever copy wins the race, and nothing at this layer can tell them apart, so this raises the cost of corruption rather than removing it. Byte-identical redelivery stays accepted — duplicates are normal in a mesh, and only a differing one is an attack. That case is pinned too, and it is the one test here that passes both before and after.
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.
Ports the storage-layer hardening from permissionlesstech/bitchat#1515 to this reassembler. jack verified those bugs empirically on the iOS side ("we verified all three bugs with a side-by-side harness against main, and they're real") and said that PR should land; two of the three, plus index poisoning, are present here.
Threat model, same as iOS
.fragmentpackets are unauthenticated and bypass the deduplicator, and both halves of(fragmentID, sender)are attacker-choosable. A fragment stream is broadcast, so its ID is observable in radio range. So a stream in flight has to survive a crafted packet aimed at it.What was wrong
1. One packet destroys an assembly, via metadata conflict.
FragmentManagercorrectly pins(originalType, total)on the first fragment — that part was already right, and it is why the iOS truncation bug does not exist here. But on a mismatch it calledremoveFragmentSetLocked, so a single fragment claiming a differenttotalfor a stream someone else owns wipes it. The victim sees a transfer that never completes.2. One packet destroys an assembly, via the size cap. Tripping
MAX_FRAGMENT_TOTAL_BYTESalso discarded the whole set, so an injected oversized fragment at any index wipes a legitimate stream. The global-cap branch immediately below already got this right — it only clears the set whenisNewSet— so this was an inconsistency inside the same function.3. Index poisoning.
fragmentMap[index] = dataoverwrote unconditionally, so a duplicate at an already-held index carrying different bytes replaced what the real sender delivered. Last-wins, one packet.What this changes
A conflicting header is rejected without touching the stream. Nothing from a conflicting header was ever stored, so retaining the set stays inside the same memory bound, and a genuinely stalled stream is still reaped by the timeout sweep. An oversized fragment only clears a set it created. An index already held is immutable.
Byte-identical redelivery stays accepted. Duplicates are normal in a mesh; only a differing duplicate is an attack. Turning redelivery into a rejection would be its own bug, so that case is pinned as well.
What this does not fix
It does not make reassembly unforgeable, and the same caveat jack drew on #1515 applies here. First-wins protects indices already held. A fragment that reaches an index before the honest one does is the first accepted value there, and the stream reassembles around it. What changes is cost: full replacement and one-packet destruction stop working, and corrupting a specific byte range now needs the attacker to win a race at each index it wants.
Verification
Local run of the CI job (
testDebugUnitTest lintDebug, JDK 21), measured with--rerun-tasksso the counts are a real full-suite run rather than incremental leftovers:FragmentManagerTest, and no other class moved.main— the two conflict cases, the index-poisoning case, and the oversize case. The fifth isan identical duplicate is still accepted, which passes both before and after; it is there to catch this fix over-rejecting, so it is supposed to pass on main.createFragmentson a genuineBitchatPacketand inject against the stream's ownfragmentID, rather than hand-built bytes, so completion actually exercisesBitchatPacket.fromBinaryData.CI has not run. Fork PRs here sit at
action_requireduntil a maintainer approves the workflow, so the local run above is the evidence, not a green check.