From 6ec10ef7747d9137eb6dff7e74aaf2005a77a6f6 Mon Sep 17 00:00:00 2001 From: Taksh Date: Sat, 15 Aug 2026 11:10:51 +0530 Subject: [PATCH 1/3] Stop an injected fragment from destroying an in-flight assembly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../bitchat/android/mesh/FragmentManager.kt | 17 ++- .../android/mesh/FragmentManagerTest.kt | 108 ++++++++++++++++++ 2 files changed, 123 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/mesh/FragmentManager.kt b/app/src/main/java/com/bitchat/android/mesh/FragmentManager.kt index 3a10f998..56ca4cd8 100644 --- a/app/src/main/java/com/bitchat/android/mesh/FragmentManager.kt +++ b/app/src/main/java/com/bitchat/android/mesh/FragmentManager.kt @@ -197,8 +197,15 @@ class FragmentManager { synchronized(fragmentStateLock) { fragmentMetadata[fragmentIDString]?.let { (expectedType, expectedTotal, _) -> if (expectedTotal != fragmentPayload.total || expectedType != fragmentPayload.originalType) { + // Reject the fragment, keep the stream. Fragment packets + // are unauthenticated and both halves of the key are + // attacker-choosable, so discarding the set here let one + // crafted packet destroy a legitimate in-flight + // reassembly. Nothing from a conflicting header is ever + // stored, so keeping the set stays inside the same + // memory bound; a stream that genuinely stalls is reaped + // by the timeout sweep. Log.w(TAG, "Rejecting fragment for $fragmentIDString: inconsistent metadata") - removeFragmentSetLocked(fragmentIDString) return null } } @@ -239,7 +246,13 @@ class FragmentManager { val maxTotalBytes = com.bitchat.android.util.AppConstants.Fragmentation.MAX_FRAGMENT_TOTAL_BYTES if (newSize > maxTotalBytes) { Log.w(TAG, "Rejecting fragment for $fragmentIDString: cumulative size $newSize exceeds cap $maxTotalBytes") - removeFragmentSetLocked(fragmentIDString) + // Only a set this fragment itself created has nothing worth + // preserving. An oversized fragment must not be able to + // destroy an assembly it did not start — same reasoning the + // global-cap branch below already applies. + if (isNewSet) { + removeFragmentSetLocked(fragmentIDString) + } return null } diff --git a/app/src/test/java/com/bitchat/android/mesh/FragmentManagerTest.kt b/app/src/test/java/com/bitchat/android/mesh/FragmentManagerTest.kt index bf3e3d01..1df9a2bd 100644 --- a/app/src/test/java/com/bitchat/android/mesh/FragmentManagerTest.kt +++ b/app/src/test/java/com/bitchat/android/mesh/FragmentManagerTest.kt @@ -252,4 +252,112 @@ class FragmentManagerTest { } return result } + + // Injected-fragment hardening. Fragment packets are unauthenticated and + // both halves of the reassembly key are attacker-choosable, so a stream in + // flight has to survive a crafted packet aimed at it. Ports the storage + // rules from permissionlesstech/bitchat#1515. + + /** A real packet, fragmented the way the sender would fragment it. */ + private fun honestStream(): Pair> { + val payload = ByteArray(1200) { (it % 251).toByte() } + val packet = BitchatPacket( + version = 1u, + type = MessageType.FILE_TRANSFER.value, + senderID = hexStringToByteArray(senderID), + recipientID = hexStringToByteArray(recipientID), + timestamp = 1u, + payload = payload, + ttl = 7u + ) + val fragments = fragmentManager.createFragments(packet) + assertTrue("Need a multi-fragment stream", fragments.size >= 3) + return packet to fragments + } + + /** Same stream key as [honest], but attacker-chosen contents. */ + private fun injected( + honest: BitchatPacket, + index: Int, + total: Int, + data: ByteArray, + originalType: UByte + ): BitchatPacket { + val decoded = FragmentPayload.decode(honest.payload)!! + return honest.copy( + payload = FragmentPayload( + fragmentID = decoded.fragmentID, + index = index, + total = total, + originalType = originalType, + data = data + ).encode() + ) + } + + private fun feedAllButLast(fragments: List) { + fragments.dropLast(1).forEach { + assertNull("No fragment before the last should complete", fragmentManager.handleFragment(it)) + } + } + + private fun assertCompletes(original: BitchatPacket, last: BitchatPacket) { + val reassembled = fragmentManager.handleFragment(last) + assertNotNull("The honest stream must still complete", reassembled) + assertTrue( + "Reassembled payload must be the honest bytes", + original.payload.contentEquals(reassembled!!.payload) + ) + } + + @Test + fun `a conflicting total is rejected without destroying the stream`() { + val (original, fragments) = honestStream() + feedAllButLast(fragments) + + val decoded = FragmentPayload.decode(fragments[0].payload)!! + assertNull( + fragmentManager.handleFragment( + injected(fragments[0], index = 0, total = decoded.total - 1, + data = ByteArray(4), originalType = decoded.originalType) + ) + ) + + assertCompletes(original, fragments.last()) + } + + @Test + fun `a conflicting original type is rejected without destroying the stream`() { + val (original, fragments) = honestStream() + feedAllButLast(fragments) + + val decoded = FragmentPayload.decode(fragments[0].payload)!! + assertNull( + fragmentManager.handleFragment( + injected(fragments[0], index = 0, total = decoded.total, + data = ByteArray(4), originalType = MessageType.ANNOUNCE.value) + ) + ) + + assertCompletes(original, fragments.last()) + } + + @Test + fun `an oversized fragment cannot wipe an assembly it did not start`() { + val (original, fragments) = honestStream() + feedAllButLast(fragments) + + val decoded = FragmentPayload.decode(fragments[0].payload)!! + val oversized = ByteArray( + com.bitchat.android.util.AppConstants.Fragmentation.MAX_FRAGMENT_TOTAL_BYTES + 1 + ) + assertNull( + fragmentManager.handleFragment( + injected(fragments[0], index = decoded.total - 1, total = decoded.total, + data = oversized, originalType = decoded.originalType) + ) + ) + + assertCompletes(original, fragments.last()) + } } From 3c34c29d0f49a44852edd7ad08165d77038a7bec Mon Sep 17 00:00:00 2001 From: Taksh Date: Sat, 15 Aug 2026 11:11:25 +0530 Subject: [PATCH 2/3] Keep the first bytes at an index already held MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../bitchat/android/mesh/FragmentManager.kt | 14 ++++++++- .../android/mesh/FragmentManagerTest.kt | 31 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/bitchat/android/mesh/FragmentManager.kt b/app/src/main/java/com/bitchat/android/mesh/FragmentManager.kt index 56ca4cd8..edd7169f 100644 --- a/app/src/main/java/com/bitchat/android/mesh/FragmentManager.kt +++ b/app/src/main/java/com/bitchat/android/mesh/FragmentManager.kt @@ -241,7 +241,19 @@ class FragmentManager { return null } - val oldEntrySize = fragmentMap[fragmentPayload.index]?.size ?: 0 + val heldEntry = fragmentMap[fragmentPayload.index] + if (heldEntry != null && !heldEntry.contentEquals(fragmentPayload.data)) { + // Duplicate delivery is normal in a mesh, but one index of a + // stream must always carry the same bytes. Overwriting + // last-wins let a crafted duplicate replace bytes already + // received from the real sender. This protects indices + // already held; an index still empty when the injected + // fragment arrives is filled by whichever copy wins the + // race, and nothing here can tell them apart. + Log.w(TAG, "Rejecting fragment for $fragmentIDString: index ${fragmentPayload.index} already holds different bytes") + return null + } + val oldEntrySize = heldEntry?.size ?: 0 val newSize = currentSize - oldEntrySize + fragmentPayload.data.size val maxTotalBytes = com.bitchat.android.util.AppConstants.Fragmentation.MAX_FRAGMENT_TOTAL_BYTES if (newSize > maxTotalBytes) { diff --git a/app/src/test/java/com/bitchat/android/mesh/FragmentManagerTest.kt b/app/src/test/java/com/bitchat/android/mesh/FragmentManagerTest.kt index 1df9a2bd..17a264ff 100644 --- a/app/src/test/java/com/bitchat/android/mesh/FragmentManagerTest.kt +++ b/app/src/test/java/com/bitchat/android/mesh/FragmentManagerTest.kt @@ -342,6 +342,37 @@ class FragmentManagerTest { assertCompletes(original, fragments.last()) } + @Test + fun `an index already held keeps its first bytes`() { + val (original, fragments) = honestStream() + feedAllButLast(fragments) + + // Same stream, same index, different bytes: last-wins would swap the + // real sender's bytes out from under the reassembly. + val decoded = FragmentPayload.decode(fragments[0].payload)!! + assertNull( + fragmentManager.handleFragment( + injected(fragments[0], index = 0, total = decoded.total, + data = ByteArray(decoded.data.size) { 0x66 }, + originalType = decoded.originalType) + ) + ) + + assertCompletes(original, fragments.last()) + } + + @Test + fun `an identical duplicate is still accepted`() { + // Duplicate delivery is normal in a mesh; only a *differing* duplicate + // is an attack, so redelivery must not be turned into a rejection. + val (original, fragments) = honestStream() + feedAllButLast(fragments) + + assertNull(fragmentManager.handleFragment(fragments[0])) + + assertCompletes(original, fragments.last()) + } + @Test fun `an oversized fragment cannot wipe an assembly it did not start`() { val (original, fragments) = honestStream()