fix: skip unknown file TLVs instead of dropping the transfer

`BitchatFilePacket.decode` resolved every tag through the four-value
`TLVType` enum and bailed on the first miss:

    val t = TLVType.from(data[off].toUByte()) ?: return null

So a file packet carrying one tag this build does not know is not
partially understood — it is discarded whole, media included. The
receiver shows nothing and logs nothing; the sender sees a successful
transfer. Both apps look healthy.

iOS has always skipped unknown tags (`case nil: continue` in its own
`BitchatFilePacket.decode`), so the two implementations disagreed about
what a valid packet is, and the tag list stopped being extensible in
practice: any optional field added by a newer or third-party client
costs every Android peer the whole file rather than just that field.

Unknown tags now advance past the value, exactly as iOS does. Known-tag
handling, the 4-byte CONTENT length, multi-CONTENT concatenation and
every existing rejection are unchanged.

The skip path is deliberately allocation- and log-free per TLV, because
its iteration count is chosen by the sender: a zero-length unknown TLV
costs 3 bytes, so a padded packet would otherwise mean millions of empty
array copies and formatted log lines monopolising the mesh handler. The
count is reported once after the loop instead.

Tests: `decode should skip unknown TLV types instead of dropping the
whole file` (extension before CONTENT), `decode should skip an unknown
TLV that trails the content` (after it), and `decode should handle a
packet padded with many zero-length unknown TLVs` (200k of them). The
first two fail on the old decoder.
This commit is contained in:
Vincenzo Palazzo 2026-07-27 22:03:01 +02:00
parent 97d6e8a479
commit 46de1a3623
2 changed files with 117 additions and 1 deletions

View File

@ -14,6 +14,13 @@ import java.nio.ByteOrder
* Length field for TLV is 2 bytes (UInt16, big-endian) for all TLVs.
* For large files, CONTENT is chunked into multiple TLVs of up to 65535 bytes each.
*
* Unknown TLV types are SKIPPED, not rejected: the tag list above is a floor,
* not a ceiling, and a decoder that bails on the first tag it does not know
* makes the format unextendable the whole file is lost over a field that,
* by construction, the sender considered optional. The iOS client has always
* done this (`case nil: continue` in its own decoder), so rejecting here also
* meant the two implementations disagreed about what a valid packet is.
*
* Note: The outer BitchatPacket uses version 2 (4-byte payload length), so this
* TLV payload can exceed 64 KiB even though each TLV value is limited to 65535 bytes.
* Transport-level fragmentation then splits the final packet for BLE MTU.
@ -90,8 +97,11 @@ data class BitchatFilePacket(
var size: Long? = null
var mime: String? = null
var contentBytes: ByteArray? = null
var skippedUnknownTLVs = 0
while (off + 3 <= data.size) { // minimum TLV header size (type + 2 bytes length)
val t = TLVType.from(data[off].toUByte()) ?: return null
// A null `t` is an unknown tag: read its length like any
// other 2-byte TLV and skip its value, matching iOS.
val t = TLVType.from(data[off].toUByte())
off += 1
// CONTENT uses 4-byte length; others use 2-byte length
val len: Int
@ -105,6 +115,18 @@ data class BitchatFilePacket(
off += 2
}
if (len < 0 || off + len > data.size) return null
if (t == null) {
// Unknown tag: advance past the value without copying it
// and without logging. A peer can pad a packet with
// zero-length unknown TLVs — 3 bytes each — so anything
// per-TLV here is attacker-scaled: at the payload
// ceiling that is millions of copies and formatted log
// lines monopolising the mesh handler. Counted and
// reported once after the loop instead.
off += len
skippedUnknownTLVs += 1
continue
}
val value = data.copyOfRange(off, off + len)
off += len
when (t) {
@ -124,6 +146,9 @@ data class BitchatFilePacket(
}
}
}
if (skippedUnknownTLVs > 0) {
android.util.Log.d("BitchatFilePacket", "⏭️ Skipped $skippedUnknownTLVs unknown TLV(s)")
}
val n = name ?: return null
val c = contentBytes ?: return null
val s = size ?: c.size.toLong()

View File

@ -139,6 +139,97 @@ class FileTransferTest {
assertEquals(32L, decoded.fileSize)
}
@Test
fun `decode should skip unknown TLV types instead of dropping the whole file`() {
// Given: a packet from a peer that added one TLV this build does not
// know (a message id, tag 0x05), placed before CONTENT the way an
// encoder that appends content last would emit it.
val content = ByteArray(64) { (it % 256).toByte() }
val fileName = "photo.jpg".toByteArray(Charsets.UTF_8)
val mimeType = "image/jpeg".toByteArray(Charsets.UTF_8)
val unknownValue = "some-message-id".toByteArray(Charsets.UTF_8)
val buf = ByteBuffer.allocate(
(1 + 2 + fileName.size) + (1 + 2 + 4) + (1 + 2 + mimeType.size) +
(1 + 2 + unknownValue.size) + (1 + 4 + content.size)
).order(ByteOrder.BIG_ENDIAN)
buf.put(0x01.toByte()); buf.putShort(fileName.size.toShort()); buf.put(fileName)
buf.put(0x02.toByte()); buf.putShort(4); buf.putInt(content.size)
buf.put(0x03.toByte()); buf.putShort(mimeType.size.toShort()); buf.put(mimeType)
buf.put(0x05.toByte()); buf.putShort(unknownValue.size.toShort()); buf.put(unknownValue)
buf.put(0x04.toByte()); buf.putInt(content.size); buf.put(content)
// When: Decoding
val decoded = BitchatFilePacket.decode(buf.array())
// Then: the unknown TLV costs nothing — the media still arrives.
// Rejecting it made every such transfer vanish with no error on either
// side, while iOS decoded the very same bytes fine.
assertNotNull(decoded)
assertEquals("photo.jpg", decoded!!.fileName)
assertEquals("image/jpeg", decoded.mimeType)
assertEquals(content.size.toLong(), decoded.fileSize)
assertEquals(content.size, decoded.content.size)
for (i in content.indices) {
assertEquals(content[i], decoded.content[i])
}
}
@Test
fun `decode should skip an unknown TLV that trails the content`() {
// Given: the same kind of extension appended AFTER content, which a
// decoder that stops at the first unknown tag also loses.
val content = ByteArray(16) { 0x7F }
val fileName = "note.m4a".toByteArray(Charsets.UTF_8)
val trailing = ByteArray(4) { 0x11 }
val buf = ByteBuffer.allocate(
(1 + 2 + fileName.size) + (1 + 4 + content.size) + (1 + 2 + trailing.size)
).order(ByteOrder.BIG_ENDIAN)
buf.put(0x01.toByte()); buf.putShort(fileName.size.toShort()); buf.put(fileName)
buf.put(0x04.toByte()); buf.putInt(content.size); buf.put(content)
buf.put(0x7F.toByte()); buf.putShort(trailing.size.toShort()); buf.put(trailing)
// When: Decoding
val decoded = BitchatFilePacket.decode(buf.array())
// Then: Defaults still apply and the content is intact
assertNotNull(decoded)
assertEquals("note.m4a", decoded!!.fileName)
assertEquals("application/octet-stream", decoded.mimeType)
assertEquals(content.size.toLong(), decoded.fileSize)
assertEquals(content.size, decoded.content.size)
}
@Test
fun `decode should handle a packet padded with many zero-length unknown TLVs`() {
// Given: the cheapest padding a peer can send — a zero-length unknown
// TLV is 3 bytes, so one packet can carry hundreds of thousands of
// them. Anything the skip path does per TLV (copying the empty value,
// formatting a log line) is scaled by the sender, not by us.
val padCount = 200_000
val content = ByteArray(8) { 0x5A }
val fileName = "padded.bin".toByteArray(Charsets.UTF_8)
val buf = ByteBuffer.allocate(
(1 + 2 + fileName.size) + (padCount * 3) + (1 + 4 + content.size)
).order(ByteOrder.BIG_ENDIAN)
buf.put(0x01.toByte()); buf.putShort(fileName.size.toShort()); buf.put(fileName)
repeat(padCount) {
buf.put(0x05.toByte()); buf.putShort(0)
}
buf.put(0x04.toByte()); buf.putInt(content.size); buf.put(content)
// When: Decoding
val decoded = BitchatFilePacket.decode(buf.array())
// Then: the real fields still come through and the padding is ignored
assertNotNull(decoded)
assertEquals("padded.bin", decoded!!.fileName)
assertEquals(content.size.toLong(), decoded.fileSize)
assertEquals(content.size, decoded.content.size)
}
@Test
fun `replaceFilePathInContent should correctly format content markers for different file types`() {
// Given: Different file types