Memory safety (buffer underflow/overflow) in APNG fdAT chunk parsing

// Proof-of-concept (C++-style pseudocode snippet using the same API pattern as Flutter's APNG decoder tests) // This demonstrates that an APNG with an fdAT chunk whose data_length is less than 4 bytes // is rejected (returns nullptr) rather than causing a crash or memory corruption. #include "flutter/lib/ui/painting/image_generator_apng.h" #include "third_party/skia/include/core/SkData.h" #include <vector> std::vector<uint8_t> BuildMaliciousApng(uint32_t fdat_data_length) { // Minimal APNG with IHDR, acTL, fcTL, and a malicious fdAT with short length std::vector<uint8_t> apng; // PNG signature const uint8_t sig[] = {137,80,78,71,13,10,26,10}; apng.insert(apng.end(), sig, sig+8); // IHDR (simplified) + acTL, fcTL would be appended similarly to test harness // ... skip detailed construction for brevity; the key is the fdAT with short length // Malicious fdAT chunk: // AppendChunk(apng, "fdAT", std::vector<uint8_t>(fdat_data_length, 0)); // IEND and other required chunks would follow here in a real APNG return apng; } // Example usage in a test harness (pseudocode): // auto apng_bytes = BuildMaliciousApng(0); // or 2 // auto data = SkData::MakeWithCopy(apng_bytes.data(), apng_bytes.size()); // auto generator = APNGImageGenerator::MakeFromData(data); // EXPECT_EQ(generator, nullptr); // should be nullptr due to bounds checks // This PoC demonstrates that with a crafted APNG where the fdAT length is less than 4, // the decoder should return null (reject the chunk) rather than crashing or corrupting memory.

diff --git a/engine/src/flutter/lib/ui/BUILD.gn b/engine/src/flutter/lib/ui/BUILD.gn index 51e10b43af8b4..6ed8db4f6e4e2 100644 --- a/engine/src/flutter/lib/ui/BUILD.gn +++ b/engine/src/flutter/lib/ui/BUILD.gn @@ -271,6 +271,7 @@ if (enable_unittests) { "painting/image_decoder_no_gl_unittests.h", "painting/image_dispose_unittests.cc", "painting/image_encoding_unittests.cc", + "painting/image_generator_apng_unittests.cc", "painting/image_generator_registry_unittests.cc", "painting/paint_unittests.cc", "painting/path_unittests.cc", 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 9e2d52986f78e..6b21aebb51d08 100644 --- a/engine/src/flutter/lib/ui/painting/image_generator_apng.cc +++ b/engine/src/flutter/lib/ui/painting/image_generator_apng.cc @@ -286,6 +286,9 @@ std::unique_ptr<ImageGenerator> APNGImageGenerator::MakeFromData( } } + if (chunk->get_data_length() < sizeof(AnimationControlChunkData)) { + return nullptr; + } const AnimationControlChunkData* animation_data = CastChunkData<AnimationControlChunkData>(chunk); @@ -430,6 +433,9 @@ APNGImageGenerator::DemuxNextImage(const void* buffer_p, // The presence of an fcTL chunk is optional for the first (default) image // of a PNG. Both cases are handled in APNGImage. if (chunk->get_type() == kFrameControlChunkType) { + if (chunk->get_data_length() < sizeof(FrameControlChunkData)) { + return std::make_pair(std::nullopt, nullptr); + } control_data = CastChunkData<FrameControlChunkData>(chunk); ImageGenerator::FrameInfo frame_info; @@ -491,7 +497,10 @@ APNGImageGenerator::DemuxNextImage(const void* buffer_p, // sequence number prepended to its data, so subtract that space from // the buffer. if (chunk->get_type() == kFrameDataChunkType) { - chunk_space -= 4; + if (chunk->get_data_length() < kFrameDataSequenceNumberSize) { + return std::make_pair(std::nullopt, nullptr); + } + chunk_space -= kFrameDataSequenceNumberSize; } } @@ -527,17 +536,21 @@ APNGImageGenerator::DemuxNextImage(const void* buffer_p, // Copy the image data/ancillary chunks. for (const ChunkHeader* c : image_chunks) { if (c->get_type() == kFrameDataChunkType) { + FML_DCHECK(c->get_data_length() >= kFrameDataSequenceNumberSize); + // Write a new IDAT chunk header. ChunkHeader* write_header = reinterpret_cast<ChunkHeader*>(write_cursor); - write_header->set_data_length(c->get_data_length() - 4); + write_header->set_data_length(c->get_data_length() - + kFrameDataSequenceNumberSize); write_header->set_type(kImageDataChunkType); write_cursor += sizeof(ChunkHeader); // Copy all of the data except for the 4 byte sequence number at the // beginning of the fdAT data. memcpy(write_cursor, - reinterpret_cast<const uint8_t*>(c) + sizeof(ChunkHeader) + 4, + reinterpret_cast<const uint8_t*>(c) + sizeof(ChunkHeader) + + kFrameDataSequenceNumberSize, write_header->get_data_length()); write_cursor += write_header->get_data_length(); @@ -645,8 +658,13 @@ void APNGImageGenerator::ChunkHeader::UpdateChunkCrc32() { uint32_t APNGImageGenerator::ChunkHeader::ComputeChunkCrc32() { // Exclude the length field at the beginning of the chunk header. size_t length = sizeof(ChunkHeader) - 4 + get_data_length(); - uint8_t* chunk_data_p = reinterpret_cast<uint8_t*>(this) + 4; + const uint8_t* chunk_data = reinterpret_cast<const uint8_t*>(this) + 4; + return ComputeCrc32(chunk_data, length); +} + +uint32_t APNGImageGenerator::ComputeCrc32(const uint8_t* data, size_t length) { uint32_t crc = 0; + const uint8_t* data_p = data; // zlib's crc32 can only take 16 bits at a time for the length, but PNG // supports a 32 bit chunk length, so looping is necessary here. @@ -658,9 +676,9 @@ uint32_t APNGImageGenerator::ChunkHeader::ComputeChunkCrc32() { length16 = std::numeric_limits<uint16_t>::max(); } - crc = crc32(crc, chunk_data_p, length16); + crc = crc32(crc, data_p, length16); length -= length16; - chunk_data_p += length16; + data_p += length16; } while (length > 0); return crc; diff --git a/engine/src/flutter/lib/ui/painting/image_generator_apng.h b/engine/src/flutter/lib/ui/painting/image_generator_apng.h index 1bf1c26e298d0..6aa1634cc46e9 100644 --- a/engine/src/flutter/lib/ui/painting/image_generator_apng.h +++ b/engine/src/flutter/lib/ui/painting/image_generator_apng.h @@ -53,10 +53,16 @@ class APNGImageGenerator : public ImageGenerator { static std::unique_ptr<ImageGenerator> MakeFromData(sk_sp<SkData> data); + /// Computes the CRC of the data in a PNG chunk. + static uint32_t ComputeCrc32(const uint8_t* data, size_t length); + private: static constexpr uint8_t kPngSignature[8] = {137, 80, 78, 71, 13, 10, 26, 10}; static constexpr size_t kChunkCrcSize = 4; + /// The size of the sequence number at the beginning of an fdAT chunk. + static constexpr size_t kFrameDataSequenceNumberSize = 4; + enum ChunkType { kImageHeaderChunkType = 'IHDR', kAnimationControlChunkType = 'acTL', 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 new file mode 100644 index 0000000000000..4765004b180ae --- /dev/null +++ b/engine/src/flutter/lib/ui/painting/image_generator_apng_unittests.cc @@ -0,0 +1,127 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/lib/ui/painting/image_generator_apng.h" + +#include <cstdint> +#include <cstring> +#include <vector> + +#include "flutter/lib/ui/painting/image_generator_registry.h" +#include "flutter/testing/testing.h" +#include "third_party/skia/include/core/SkData.h" + +namespace flutter { +namespace testing { + +namespace { + +// Writes a big-endian uint32_t to a buffer. +void WriteBE32(std::vector<uint8_t>& buf, uint32_t val) { + buf.push_back((val >> 24) & 0xFF); + buf.push_back((val >> 16) & 0xFF); + buf.push_back((val >> 8) & 0xFF); + buf.push_back(val & 0xFF); +} + +// Writes a big-endian uint16_t to a buffer. +void WriteBE16(std::vector<uint8_t>& buf, uint16_t val) { + buf.push_back((val >> 8) & 0xFF); + buf.push_back(val & 0xFF); +} + +// Appends a PNG chunk (length + type + data + CRC) to the buffer. +void AppendChunk(std::vector<uint8_t>& buf, + const char type[4], + const std::vector<uint8_t>& data) { + FML_CHECK(data.size() <= std::numeric_limits<uint32_t>::max()); + WriteBE32(buf, static_cast<uint32_t>(data.size())); + size_t type_start = buf.size(); + buf.insert(buf.end(), type, type + 4); + buf.insert(buf.end(), data.begin(), data.end()); + uint32_t crc = APNGImageGenerator::ComputeCrc32(buf.data() + type_start, + 4 + data.size()); + WriteBE32(buf, crc); +} + +// 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. +std::vector<uint8_t> BuildMaliciousApng(uint32_t fdat_data_length) { + std::vector<uint8_t> apng; + + // PNG signature + const uint8_t sig[] = {137, 80, 78, 71, 13, 10, 26, 10}; + apng.insert(apng.end(), sig, sig + 8); + + // IHDR: 1x1 RGBA, 8-bit + { + std::vector<uint8_t> ihdr; + WriteBE32(ihdr, 1); // width + WriteBE32(ihdr, 1); // height + ihdr.push_back(8); // bit depth + ihdr.push_back(6); // color type (RGBA) + ihdr.push_back(0); // compression + ihdr.push_back(0); // filter + ihdr.push_back(0); // interlace + AppendChunk(apng, "IHDR", ihdr); + } + + // acTL: 1 frame, loop forever + { + std::vector<uint8_t> actl; + WriteBE32(actl, 1); // num_frames + WriteBE32(actl, 0); // num_plays (0 = infinite) + AppendChunk(apng, "acTL", actl); + } + + // fcTL for frame 0 + { + std::vector<uint8_t> fctl; + WriteBE32(fctl, 0); // sequence_number + WriteBE32(fctl, 1); // width + WriteBE32(fctl, 1); // height + WriteBE32(fctl, 0); // x_offset + WriteBE32(fctl, 0); // y_offset + WriteBE16(fctl, 1); // delay_num + WriteBE16(fctl, 10); // delay_den + fctl.push_back(0); // dispose_op + fctl.push_back(0); // blend_op + AppendChunk(apng, "fcTL", fctl); + } + + // Malicious fdAT for frame 0: data_length < 4 + // An fdAT chunk must have at least 4 bytes (sequence number). + // With data_length < 4, the subtraction in DemuxNextImage() underflows. + AppendChunk(apng, "fdAT", std::vector<uint8_t>(fdat_data_length, 0)); + + // IEND + AppendChunk(apng, "IEND", {}); + + return apng; +} + +} // namespace + +// Verify that the APNG decoder can handle fdAT chunks whose length is shorter +// than the required 4-byte sequence number. +TEST(APNGImageGeneratorTest, FdATWithShortDataLengthDoesNotCrash) { + ImageGeneratorRegistry registry; + + auto make_generator = [](uint32_t fdat_length) -> auto { + auto apng_bytes = BuildMaliciousApng(fdat_length); + auto data = SkData::MakeWithCopy(apng_bytes.data(), apng_bytes.size()); + return APNGImageGenerator::MakeFromData(data); + }; + + // The decoder should reject fdAT chunks that are less than 4 bytes long. + EXPECT_EQ(make_generator(0), nullptr); + EXPECT_EQ(make_generator(2), nullptr); + + // Creating the generator should succeed if the fdAT has sufficient length. + EXPECT_NE(make_generator(4), nullptr); +} + +} // namespace testing +} // namespace flutter