Memory safety / Integer overflow

PoC outline: - Craft an APNG with a chunk (fdAT) whose declared data_length is an extreme value (e.g., 0xFFFFFFFF) but with far fewer actual payload bytes provided. The parser would previously compute the chunk end using the vulnerable length and could overflow the size_t calculation, potentially causing out-of-bounds reads or a crash. The fix prevents this by validating the remaining buffer against the declared length before using it and by ensuring there is enough room for header, data, and CRC before computing any end positions. Concrete PoC steps (aligned with the repository test): 1) Build a minimal APNG structure including IHDR, acTL, fcTL, and a malformed fdAT chunk with declared_length = 0xFFFFFFFF and only 8 actual data bytes, followed by a valid IEND chunk. 2) Feed the bytes to the APNG image generator (e.g., APNGImageGenerator::MakeFromData in the Flutter test harness). 3) Observe that, with the buggy code, the parser could overflow and misbehave; after the fix, MakeFromData should reject the malformed APNG (generator == nullptr). Representative test equivalent (from the commit): AppendChunkWithFakeLength(apng, "fdAT", 0xFFFFFFFF, {0, 0, 0, 1, 0, 0, 0, 0}); AppendChunk(apng, "IEND", {}); Python-like pseudocode for generating the APNG payload (conceptual): import struct def be32(n): return struct.pack('>I', n) apng = bytearray() apng += b'\x89PNG\r\n\x1a\n' # IHDR, acTL, fcTL chunks would be appended here with valid data (omitted for brevity) # fdAT with declared length overflow and 8 actual bytes apng += be32(0xFFFFFFFF) + b'fdAT' + bytes([0,0,0,1,0,0,0,0]) + be32(0) # IEND apng += be32(0) + b'IEND' + b'' + be32(0) # The PoC runs in your test harness: pass the apng bytes to APNG parser and assert it returns null/no crash.

diff --git a/engine/src/flutter/lib/ui/painting/image_generator_apng.cc b/engine/src/flutter/lib/ui/painting/image_generator_apng.cc index 307bb5b5fc34d..dc646b593dd1c 100644 --- a/engine/src/flutter/lib/ui/painting/image_generator_apng.cc +++ b/engine/src/flutter/lib/ui/painting/image_generator_apng.cc @@ -325,23 +325,28 @@ std::unique_ptr<ImageGenerator> APNGImageGenerator::MakeFromData( bool APNGImageGenerator::IsValidChunkHeader(const void* buffer, size_t size, const ChunkHeader* chunk) { - // Ensure the chunk doesn't start before the beginning of the buffer. - if (reinterpret_cast<const uint8_t*>(chunk) < - static_cast<const uint8_t*>(buffer)) { + // Ensure that the chunk starts within the bounds of the buffer. + const uint8_t* chunk_ptr = reinterpret_cast<const uint8_t*>(chunk); + const uint8_t* buffer_ptr = static_cast<const uint8_t*>(buffer); + if (chunk_ptr < buffer_ptr || chunk_ptr >= buffer_ptr + size) { return false; } - // Ensure the buffer is large enough to contain at least the chunk header. - if (reinterpret_cast<const uint8_t*>(chunk) + sizeof(ChunkHeader) > - static_cast<const uint8_t*>(buffer) + size) { + // Ensure that the buffer has enough space for the chunk header before using + // any fields in the header. + size_t buffer_bytes_remaining = size - (chunk_ptr - buffer_ptr); + if (buffer_bytes_remaining < sizeof(ChunkHeader)) { return false; } - - // Ensure the buffer is large enough to contain the chunk's given data size - // and CRC. - const uint8_t* chunk_end = - reinterpret_cast<const uint8_t*>(chunk) + GetChunkSize(chunk); - if (chunk_end > static_cast<const uint8_t*>(buffer) + size) { + // Ensure that the buffer has enough space for the chunk data and CRC. + // Do not use the chunk data length in pointer arithmetic until it is known to + // be valid. + size_t data_length = chunk->get_data_length(); + if (buffer_bytes_remaining - sizeof(ChunkHeader) < data_length) { + return false; + } + if (buffer_bytes_remaining - sizeof(ChunkHeader) - data_length < + kChunkCrcSize) { return false; } diff --git a/engine/src/flutter/lib/ui/painting/image_generator_apng_unittests.cc b/engine/src/flutter/lib/ui/painting/image_generator_apng_unittests.cc index 283d07d5fb7ab..e626621711edb 100644 --- a/engine/src/flutter/lib/ui/painting/image_generator_apng_unittests.cc +++ b/engine/src/flutter/lib/ui/painting/image_generator_apng_unittests.cc @@ -45,6 +45,18 @@ void AppendChunk(std::vector<uint8_t>& buf, WriteBE32(buf, crc); } +// Appends a chunk with a declared data_length that may differ from the actual +// data bytes written. Used to test handling of malformed chunks. +void AppendChunkWithFakeLength(std::vector<uint8_t>& buf, + const char type[4], + uint32_t declared_length, + const std::vector<uint8_t>& actual_data) { + WriteBE32(buf, declared_length); + buf.insert(buf.end(), type, type + 4); + buf.insert(buf.end(), actual_data.begin(), actual_data.end()); + WriteBE32(buf, 0); // CRC placeholder +} + // Builds a minimal valid APNG with a malicious fdAT chunk whose // data_length is less than 4, which would trigger an integer underflow // in DemuxNextImage() without the bounds check fix. @@ -120,5 +132,53 @@ TEST(APNGImageGeneratorTest, FdATWithShortDataLengthDoesNotCrash) { EXPECT_NE(make_generator(4), nullptr); } +TEST(APNGImageGeneratorTest, FdATWithOverflowDataLengthIsRejected) { + std::vector<uint8_t> apng(APNGImageGenerator::kPngSignature.begin(), + APNGImageGenerator::kPngSignature.end()); + + // IHDR + { + std::vector<uint8_t> ihdr; + WriteBE32(ihdr, 1); + WriteBE32(ihdr, 1); + ihdr.push_back(8); + ihdr.push_back(6); + ihdr.push_back(0); + ihdr.push_back(0); + ihdr.push_back(0); + AppendChunk(apng, "IHDR", ihdr); + } + // acTL + { + std::vector<uint8_t> actl; + WriteBE32(actl, 1); + WriteBE32(actl, 0); + AppendChunk(apng, "acTL", actl); + } + // fcTL + { + std::vector<uint8_t> fctl; + WriteBE32(fctl, 0); + WriteBE32(fctl, 1); + WriteBE32(fctl, 1); + WriteBE32(fctl, 0); + WriteBE32(fctl, 0); + WriteBE16(fctl, 1); + WriteBE16(fctl, 10); + fctl.push_back(0); + fctl.push_back(0); + AppendChunk(apng, "fcTL", fctl); + } + // fdAT with declared data_length=0xFFFFFFFF but only 8 actual bytes + AppendChunkWithFakeLength(apng, "fdAT", 0xFFFFFFFF, {0, 0, 0, 1, 0, 0, 0, 0}); + // IEND + AppendChunk(apng, "IEND", {}); + + auto data = SkData::MakeWithCopy(apng.data(), apng.size()); + auto generator = APNGImageGenerator::MakeFromData(data); + // The generator should reject the malformed APNG without crashing. + EXPECT_EQ(generator, nullptr); +} + } // namespace testing } // namespace flutter