diff --git a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/platform/MetadataStrip.kt b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/platform/MetadataStrip.kt index ad56294316..ed9089fe9b 100644 --- a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/platform/MetadataStrip.kt +++ b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/platform/MetadataStrip.kt @@ -170,6 +170,7 @@ fun stripGifMetadata(data: ByteArray): ByteArray { val hasGCT = (packed and 0x80) != 0 val gctSize = if (hasGCT) 3 * (1 shl ((packed and 0x07) + 1)) else 0 if (hasGCT) { + if (13 + gctSize > data.size) throw IllegalArgumentException("Invalid GIF: GCT (size $gctSize) extends past file (size ${data.size})") out.write(data, 13, gctSize) } @@ -256,6 +257,7 @@ private fun skipExtensionBlock(data: ByteArray, start: Int): Int = /** Copy an Image Descriptor block + optional Local Color Table + image data. Returns new position. */ private fun copyImageDescriptor(data: ByteArray, start: Int, out: java.io.ByteArrayOutputStream): Int { // Image Descriptor is 10 bytes (0x2C + 9 bytes) + if (start + 10 > data.size) throw IllegalArgumentException("Invalid GIF: truncated Image Descriptor at offset $start") out.write(data, start, 10) var pos = start + 10 @@ -264,11 +266,13 @@ private fun copyImageDescriptor(data: ByteArray, start: Int, out: java.io.ByteAr val hasLCT = (packed and 0x80) != 0 if (hasLCT) { val lctSize = 3 * (1 shl ((packed and 0x07) + 1)) + if (pos + lctSize > data.size) throw IllegalArgumentException("Invalid GIF: LCT (size $lctSize) extends past file at offset $pos") out.write(data, pos, lctSize) pos += lctSize } // LZW Minimum Code Size + if (pos >= data.size) throw IllegalArgumentException("Invalid GIF: truncated before LZW min code size at offset $pos") out.write(data[pos].toInt() and 0xFF) pos++ @@ -315,6 +319,9 @@ fun stripWebPMetadata(data: ByteArray): ByteArray { } if (fourCC == "VP8X") { + // Per spec VP8X payload is fixed at 10 bytes. If it's smaller, our flags-byte write + // at the post-loop fixup would land on the next chunk's FourCC and corrupt structure. + if (chunkSize < 10) throw IllegalArgumentException("Invalid WebP: VP8X chunk too small ($chunkSize bytes, need 10)") // +8 to skip the chunk header (fourCC + size), points to the flags byte in the output vp8xOffset = out.size() + 8 } @@ -606,6 +613,9 @@ fun stripVideoMetadata(inputPath: String, outputPath: String) { } pos += boxSize } + // Fail closed if any bytes remain — silently dropping them would shrink the container, + // shifting absolute file offsets that stco/co64 in moov reference into mdat. + if (pos != end) throw IllegalArgumentException("MP4 container has ${end - pos} trailing byte(s) at end of range $offset..$end") return containerOut.toByteArray() } @@ -803,10 +813,14 @@ fun stripAviMetadata(inputPath: String, outputPath: String) { while (p + 8 <= end) { val fourCC = String(d, p, 4, Charsets.US_ASCII) val chunkSize = readLE32(d, p + 4) - if (chunkSize < 0) break + // Fail closed: silent break would shorten the rewritten LIST and could skip past real + // metadata chunks that follow a malformed one. Match the MKV-side fail-closed posture. + if (chunkSize < 0) throw IllegalArgumentException("AVI header chunk '$fourCC' has negative size at offset $p: $chunkSize") val paddedSize = if (chunkSize % 2 == 0) chunkSize.toLong() else chunkSize.toLong() + 1L val totalSizeLong = 8L + paddedSize - if (totalSizeLong > Int.MAX_VALUE || p + totalSizeLong > end) break + if (totalSizeLong > Int.MAX_VALUE || p + totalSizeLong > end) { + throw IllegalArgumentException("AVI header chunk '$fourCC' at offset $p overruns container: totalSize=$totalSizeLong, remaining=${end - p}") + } val totalSize = totalSizeLong.toInt() if (fourCC == "LIST" && p + 12 <= end) { @@ -853,7 +867,9 @@ fun stripAviMetadata(inputPath: String, outputPath: String) { raf.readFully(chunkHeader) val fourCC = String(chunkHeader, 0, 4, Charsets.US_ASCII) val chunkSize = readLE32(chunkHeader, 4) - if (chunkSize < 0) break + // Fail closed: a silent break would truncate the output (RIFF size is rewritten at the end + // to match), letting a crafted file pass through with arbitrary tail data silently dropped. + if (chunkSize < 0) throw IllegalArgumentException("AVI top-level chunk '$fourCC' has negative size: $chunkSize") // Use Long to avoid overflow for odd chunkSize near Int.MAX_VALUE val paddedSize = if (chunkSize % 2 == 0) chunkSize.toLong() else chunkSize.toLong() + 1L diff --git a/apps/multiplatform/common/src/commonTest/kotlin/chat/simplex/app/MetadataStripTest.kt b/apps/multiplatform/common/src/commonTest/kotlin/chat/simplex/app/MetadataStripTest.kt index 07d42a349b..14e573bb15 100644 --- a/apps/multiplatform/common/src/commonTest/kotlin/chat/simplex/app/MetadataStripTest.kt +++ b/apps/multiplatform/common/src/commonTest/kotlin/chat/simplex/app/MetadataStripTest.kt @@ -2259,3 +2259,125 @@ class AnimImageFileDispatchTest { } } } + +/** Regression tests for fail-closed gaps flagged in PR #6999 review. */ +class ReviewFailClosedTest { + + // --- GIF bounds checks (review #4, #5) --- + + @Test fun testGifTruncatedGctThrowsIllegalArgument() { + // GIF89a header + LSD where the packed byte advertises a GCT (0x80) of 3*2^(0+1) = 6 bytes, + // but the file ends right after the LSD. Without the bounds check this throws IOB from + // ByteArrayOutputStream.write; with it the stripper throws IllegalArgumentException. + val truncated = byteArrayOf( + 0x47, 0x49, 0x46, 0x38, 0x39, 0x61, // "GIF89a" + 0x01, 0x00, 0x01, 0x00, // 1x1 + 0x80.toByte(), // packed: GCT bit set, size code 0 → 6 bytes + 0x00, 0x00 // bg, aspect + ) + assertFailsWith { stripGifMetadata(truncated) } + } + + @Test fun testGifTruncatedImageDescriptorThrowsIllegalArgument() { + // Header + LSD (no GCT) + image-descriptor sentinel only — descriptor needs 10 bytes total. + val truncated = byteArrayOf( + 0x47, 0x49, 0x46, 0x38, 0x39, 0x61, // "GIF89a" + 0x01, 0x00, 0x01, 0x00, // 1x1 + 0x00, 0x00, 0x00, // no GCT, bg, aspect + 0x2C, 0x00 // image-descriptor sentinel + 1 stray byte + ) + assertFailsWith { stripGifMetadata(truncated) } + } + + // --- WebP VP8X size check (review #6) --- + + @Test fun testWebpVp8xTooSmallThrowsIllegalArgument() { + // VP8X with chunkSize = 0 (spec mandates 10). Without the check, vp8xOffset would land on + // the next chunk's FourCC and the post-loop flag-clear would corrupt that byte. + val malformed = byteArrayOf( + 0x52, 0x49, 0x46, 0x46, // "RIFF" + 0x14, 0x00, 0x00, 0x00, // file size - 8 = 20 + 0x57, 0x45, 0x42, 0x50, // "WEBP" + 0x56, 0x50, 0x38, 0x58, // "VP8X" + 0x00, 0x00, 0x00, 0x00, // chunkSize = 0 (truncated, malformed) + 0x45, 0x58, 0x49, 0x46, // "EXIF" — would be flag-corrupted by bug + 0x00, 0x00, 0x00, 0x00 // EXIF chunkSize = 0 + ) + assertFailsWith { stripWebPMetadata(malformed) } + } + + // --- MP4 processContainer trailing bytes (review #1) --- + + @Test fun testMp4ContainerWithTrailingBytesThrowsIllegalArgument() { + // moov containing a single trak that DOES NOT TILE — i.e., trak's declared size leaves + // a few bytes inside moov unaccounted for. Without the trailing-byte throw, processContainer + // silently drops those bytes, shrinking moov and shifting mdat → broken stco/co64. + val mvhd = box("mvhd", ByteArray(100)) + val trak = box("trak", ByteArray(100)) + val moov = box("moov", mvhd + trak + ByteArray(3)) // 3 trailing bytes inside moov + val ftyp = box("ftyp", "isom".toByteArray() + ByteArray(8)) + val mdat = box("mdat", ByteArray(8)) + + val inputFile = File.createTempFile("test_mp4_trailing_", ".mp4") + inputFile.writeBytes(ftyp + moov + mdat) + val outputFile = File.createTempFile("test_mp4_trailing_out_", ".mp4") + try { + assertFailsWith { + stripVideoMetadata(inputFile.absolutePath, outputFile.absolutePath) + } + } finally { + inputFile.delete(); outputFile.delete() + } + } + + // --- AVI silent break → throw (review #2, #3) --- + + @Test fun testAviTopLevelNegativeChunkSizeThrows() { + // Top-level chunk with the high bit set in chunkSize. Previously silent-break, now throws. + val data = byteArrayOf( + 0x52, 0x49, 0x46, 0x46, // "RIFF" + 0x14, 0x00, 0x00, 0x00, // file size - 8 = 20 + 0x41, 0x56, 0x49, 0x20, // "AVI " + 0x4A, 0x55, 0x4E, 0x4B, // "JUNK" chunk + 0xFF.toByte(), 0xFF.toByte(), 0xFF.toByte(), 0xFF.toByte() // chunkSize = -1 (top bit set) + ) + val inputFile = File.createTempFile("test_avi_neg_", ".avi") + inputFile.writeBytes(data) + val outputFile = File.createTempFile("test_avi_neg_out_", ".avi") + try { + assertFailsWith { + stripAviMetadata(inputFile.absolutePath, outputFile.absolutePath) + } + } finally { + inputFile.delete(); outputFile.delete() + } + } + + @Test fun testAviHdrlNegativeChunkSizeThrows() { + // hdrl LIST containing an inner chunk with negative size. Previously silent-break, + // now throws so subsequent _PMX/strd/IDIT metadata can't be silently dropped. + val innerChunk = byteArrayOf( + 0x61, 0x76, 0x69, 0x68, // "avih" + 0xFF.toByte(), 0xFF.toByte(), 0xFF.toByte(), 0xFF.toByte() // chunkSize = -1 + ) + val hdrlListBody = "hdrl".toByteArray() + innerChunk + val hdrlList = byteArrayOf(0x4C, 0x49, 0x53, 0x54) + // "LIST" + byteArrayOf(hdrlListBody.size.toByte(), 0, 0, 0) + // size + hdrlListBody + val data = byteArrayOf( + 0x52, 0x49, 0x46, 0x46, // "RIFF" + (12 + hdrlList.size).toByte(), 0, 0, 0, // file size - 8 + 0x41, 0x56, 0x49, 0x20 // "AVI " + ) + hdrlList + val inputFile = File.createTempFile("test_avi_hdrl_neg_", ".avi") + inputFile.writeBytes(data) + val outputFile = File.createTempFile("test_avi_hdrl_neg_out_", ".avi") + try { + assertFailsWith { + stripAviMetadata(inputFile.absolutePath, outputFile.absolutePath) + } + } finally { + inputFile.delete(); outputFile.delete() + } + } +}