From 7825a6f08a46791b154fa636c22403183a8beb0b Mon Sep 17 00:00:00 2001 From: 1seal Date: Tue, 3 Mar 2026 21:45:06 +0100 Subject: [PATCH 1/8] engine apng: reject short fdAT chunk lengths guard against fdAT data_length < 4 underflow in APNGImageGenerator::DemuxNextImage and add a ui_unittests regression test + minimal fixtures. --- engine/src/flutter/lib/ui/BUILD.gn | 3 ++ .../lib/ui/fixtures/apng_fdat_len0.apng | Bin 0 -> 115 bytes .../lib/ui/fixtures/apng_fdat_len4.apng | Bin 0 -> 119 bytes .../lib/ui/painting/image_generator_apng.cc | 4 +++ .../image_generator_apng_unittests.cc | 31 ++++++++++++++++++ 5 files changed, 38 insertions(+) create mode 100644 engine/src/flutter/lib/ui/fixtures/apng_fdat_len0.apng create mode 100644 engine/src/flutter/lib/ui/fixtures/apng_fdat_len4.apng create mode 100644 engine/src/flutter/lib/ui/painting/image_generator_apng_unittests.cc diff --git a/engine/src/flutter/lib/ui/BUILD.gn b/engine/src/flutter/lib/ui/BUILD.gn index 51e10b43af8b4..39093cafc5fe3 100644 --- a/engine/src/flutter/lib/ui/BUILD.gn +++ b/engine/src/flutter/lib/ui/BUILD.gn @@ -232,6 +232,8 @@ if (enable_unittests) { "fixtures/DisplayP3Logo.png", "fixtures/Horizontal.jpg", "fixtures/Horizontal.png", + "fixtures/apng_fdat_len0.apng", + "fixtures/apng_fdat_len4.apng", "fixtures/heart_end.png", "fixtures/hello_loop_2.gif", "fixtures/hello_loop_2.webp", @@ -271,6 +273,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/fixtures/apng_fdat_len0.apng b/engine/src/flutter/lib/ui/fixtures/apng_fdat_len0.apng new file mode 100644 index 0000000000000000000000000000000000000000..95323d13e0f6c543a2e46cf8b13a960dc477e403 GIT binary patch literal 115 zcmeAS@N?(olHy`uVBq!ia0vp^j3CUx1|*??BQZI|2gqiE2>@xSG>8~T2&@4~E(O6( POK}W=@jPAqTtI99EfEOB literal 0 HcmV?d00001 diff --git a/engine/src/flutter/lib/ui/fixtures/apng_fdat_len4.apng b/engine/src/flutter/lib/ui/fixtures/apng_fdat_len4.apng new file mode 100644 index 0000000000000000000000000000000000000000..d7c428acaea7979524e06461e161a37d9acb552c GIT binary patch literal 119 zcmeAS@N?(olHy`uVBq!ia0vp^j3CUx1|*??BQZI|2gqiE2>@xSG>8~T2&@4~E(OkJ SNlS4If%AY2Pgg$|5FY?c1qjFh literal 0 HcmV?d00001 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..b59d4691f4db4 100644 --- a/engine/src/flutter/lib/ui/painting/image_generator_apng.cc +++ b/engine/src/flutter/lib/ui/painting/image_generator_apng.cc @@ -527,6 +527,10 @@ APNGImageGenerator::DemuxNextImage(const void* buffer_p, // Copy the image data/ancillary chunks. for (const ChunkHeader* c : image_chunks) { if (c->get_type() == kFrameDataChunkType) { + if (c->get_data_length() < 4) { + return std::make_pair(std::nullopt, nullptr); + } + // Write a new IDAT chunk header. ChunkHeader* write_header = reinterpret_cast(write_cursor); 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..750f4bc9caccd --- /dev/null +++ b/engine/src/flutter/lib/ui/painting/image_generator_apng_unittests.cc @@ -0,0 +1,31 @@ +// 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 "flutter/shell/common/shell_test.h" +#include "flutter/testing/testing.h" + +namespace flutter { +namespace testing { + +TEST_F(ShellTest, RejectsApngWithFrameDataLengthLessThanFour) { + auto data = OpenFixtureAsSkData("apng_fdat_len0.apng"); + ASSERT_NE(data, nullptr); + + auto generator = APNGImageGenerator::MakeFromData(std::move(data)); + ASSERT_EQ(generator, nullptr); +} + +TEST_F(ShellTest, AcceptsApngWithFrameDataLengthEqualToFour) { + auto data = OpenFixtureAsSkData("apng_fdat_len4.apng"); + ASSERT_NE(data, nullptr); + + auto generator = APNGImageGenerator::MakeFromData(std::move(data)); + ASSERT_NE(generator, nullptr); +} + +} // namespace testing +} // namespace flutter + From 3e4616182ea9dd54e08fb43f24443b5526c7711f Mon Sep 17 00:00:00 2001 From: mohammadmseet-hue Date: Sun, 29 Mar 2026 04:46:35 +0200 Subject: [PATCH 2/8] Reject fdAT chunks with data_length < 4 in APNG parser When converting fdAT chunks to IDAT chunks during APNG demuxing, DemuxNextImage() subtracts 4 from data_length to remove the fdAT sequence number. If an fdAT chunk has data_length < 4, this uint32_t subtraction underflows to ~4GB (0xFFFFFFFC for data_length=0). The underflowed value is then used as the length argument to memcpy, causing a heap buffer overflow. A crafted APNG image served from any HTTP server can trigger this when loaded by a Flutter application via Image.network() or similar APIs. The APNG parser is registered as a default image decoder and runs on all image data with a valid PNG signature + acTL chunk. This commit adds bounds checks in both the chunk_space calculation loop and the fdAT-to-IDAT conversion loop to reject fdAT chunks with insufficient data before the subtraction occurs. Related prior fixes in this file: - PR #56928: frame offset bounds check - PR #57025: integer wrapping fix for the bounds check Neither addressed the fdAT data_length underflow. --- engine/src/flutter/lib/ui/painting/image_generator_apng.cc | 3 +++ 1 file changed, 3 insertions(+) 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 b59d4691f4db4..bebdaabf625c6 100644 --- a/engine/src/flutter/lib/ui/painting/image_generator_apng.cc +++ b/engine/src/flutter/lib/ui/painting/image_generator_apng.cc @@ -491,6 +491,9 @@ APNGImageGenerator::DemuxNextImage(const void* buffer_p, // sequence number prepended to its data, so subtract that space from // the buffer. if (chunk->get_type() == kFrameDataChunkType) { + if (chunk->get_data_length() < 4) { + return std::make_pair(std::nullopt, nullptr); + } chunk_space -= 4; } } From f3ff035c039fa85262488cd3170677cdaa442461 Mon Sep 17 00:00:00 2001 From: mohammadmseet-hue Date: Sun, 29 Mar 2026 04:51:40 +0200 Subject: [PATCH 3/8] Add unit tests for fdAT data_length validation in APNG parser Tests that malformed APNG images with fdAT chunks whose data_length is less than the required 4 bytes (for the sequence number) do not cause crashes. Without the bounds check, the uint32_t subtraction in DemuxNextImage() underflows and causes a heap buffer overflow. Three test cases: - fdAT with data_length=0 (underflows to 0xFFFFFFFC) - fdAT with data_length=2 (underflows to 0xFFFFFFFE) - fdAT with data_length=8 (valid, should be accepted) --- .../image_generator_apng_unittests.cc | 193 ++++++++++++++++-- 1 file changed, 181 insertions(+), 12 deletions(-) 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 750f4bc9caccd..310a10a2bef53 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 @@ -4,28 +4,197 @@ #include "flutter/lib/ui/painting/image_generator_apng.h" -#include "flutter/shell/common/shell_test.h" +#include +#include +#include + #include "flutter/testing/testing.h" +#include "third_party/skia/include/core/SkData.h" namespace flutter { namespace testing { -TEST_F(ShellTest, RejectsApngWithFrameDataLengthLessThanFour) { - auto data = OpenFixtureAsSkData("apng_fdat_len0.apng"); - ASSERT_NE(data, nullptr); +namespace { + +// Writes a big-endian uint32_t to a buffer. +void WriteBE32(std::vector& 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& buf, uint16_t val) { + buf.push_back((val >> 8) & 0xFF); + buf.push_back(val & 0xFF); +} + +// Computes CRC32 over chunk type + data (standard PNG CRC). +uint32_t ComputePngCrc32(const uint8_t* data, size_t length) { + uint32_t crc = 0xFFFFFFFF; + for (size_t i = 0; i < length; i++) { + crc ^= data[i]; + for (int j = 0; j < 8; j++) { + crc = (crc >> 1) ^ (0xEDB88320 & (-(crc & 1))); + } + } + return crc ^ 0xFFFFFFFF; +} + +// Appends a PNG chunk (length + type + data + CRC) to the buffer. +void AppendChunk(std::vector& buf, + const char type[4], + const std::vector& data) { + WriteBE32(buf, static_cast(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 = + ComputePngCrc32(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 BuildMaliciousApng(uint32_t fdat_data_length) { + std::vector 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 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: 2 frames, loop forever + { + std::vector actl; + WriteBE32(actl, 2); // num_frames + WriteBE32(actl, 0); // num_plays (0 = infinite) + AppendChunk(apng, "acTL", actl); + } - auto generator = APNGImageGenerator::MakeFromData(std::move(data)); - ASSERT_EQ(generator, nullptr); + // fcTL for frame 0 + { + std::vector 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); + } + + // IDAT for frame 0: minimal valid zlib-compressed 1x1 RGBA + { + // zlib header (78 01) + deflate block for filter_byte(0) + RGBA(0,0,0,255) + const uint8_t idat_data[] = {0x78, 0x01, 0x62, 0x60, 0x60, 0x60, + 0xF8, 0x0F, 0x00, 0x00, 0x05, 0x00, + 0x01}; + std::vector idat(idat_data, idat_data + sizeof(idat_data)); + AppendChunk(apng, "IDAT", idat); + } + + // fcTL for frame 1 + { + std::vector fctl; + WriteBE32(fctl, 1); // 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 1: data_length < 4 + // An fdAT chunk must have at least 4 bytes (sequence number). + // With data_length < 4, the subtraction in DemuxNextImage() underflows. + { + std::vector fdat; + for (uint32_t i = 0; i < fdat_data_length; i++) { + fdat.push_back(0); + } + AppendChunk(apng, "fdAT", fdat); + } + + // IEND + AppendChunk(apng, "IEND", {}); + + return apng; } -TEST_F(ShellTest, AcceptsApngWithFrameDataLengthEqualToFour) { - auto data = OpenFixtureAsSkData("apng_fdat_len4.apng"); - ASSERT_NE(data, nullptr); +} // namespace + +// Verify that an APNG with an fdAT chunk of data_length=0 does not crash. +// Without the fix, this would cause a uint32_t underflow (0 - 4 = 0xFFFFFFFC) +// leading to a heap buffer overflow in DemuxNextImage(). +TEST(APNGImageGeneratorTest, FdATWithZeroDataLengthDoesNotCrash) { + auto apng_bytes = BuildMaliciousApng(0); + auto data = SkData::MakeWithCopy(apng_bytes.data(), apng_bytes.size()); + auto generator = APNGImageGenerator::MakeFromData(data); - auto generator = APNGImageGenerator::MakeFromData(std::move(data)); - ASSERT_NE(generator, nullptr); + // The generator may fail to create (if the malformed fdAT is caught during + // initial parsing) or may create successfully but fail during frame decode. + // Either way, it must NOT crash. + if (generator) { + // If the generator was created, try to decode frame 1 which references + // the malicious fdAT. This must not crash. + auto info = generator->GetInfo(); + std::vector pixels(info.computeMinByteSize()); + generator->GetPixels(info, pixels.data(), info.minRowBytes(), 1, + std::nullopt); + } +} + +// Verify that an fdAT chunk with data_length=2 (less than the required 4-byte +// sequence number) also does not crash. +TEST(APNGImageGeneratorTest, FdATWithShortDataLengthDoesNotCrash) { + auto apng_bytes = BuildMaliciousApng(2); + auto data = SkData::MakeWithCopy(apng_bytes.data(), apng_bytes.size()); + auto generator = APNGImageGenerator::MakeFromData(data); + + if (generator) { + auto info = generator->GetInfo(); + std::vector pixels(info.computeMinByteSize()); + generator->GetPixels(info, pixels.data(), info.minRowBytes(), 1, + std::nullopt); + } +} + +// Verify that a valid fdAT chunk (data_length >= 4) still works correctly. +TEST(APNGImageGeneratorTest, ValidFdATIsAccepted) { + // data_length=8: 4 bytes sequence number + 4 bytes compressed data + auto apng_bytes = BuildMaliciousApng(8); + auto data = SkData::MakeWithCopy(apng_bytes.data(), apng_bytes.size()); + auto generator = APNGImageGenerator::MakeFromData(data); + + // A valid APNG with proper fdAT should create a generator successfully. + // Frame decode may still fail due to invalid compressed data, but the + // generator creation should not be rejected by the data_length check. + if (generator) { + EXPECT_GE(generator->GetFrameCount(), 1u); + } } } // namespace testing } // namespace flutter - From c287f57217d787861a45bd3250639a7f39450474 Mon Sep 17 00:00:00 2001 From: mohammadmseet-hue Date: Mon, 30 Mar 2026 05:47:55 +0200 Subject: [PATCH 4/8] Add data_length validation for acTL and fcTL chunks Extend the data_length validation to cover two additional chunk types that use CastChunkData() without size checks: - acTL (line 272): reject if data_length < sizeof(AnimationControlChunkData) Prevents heap OOB read of num_frames and num_plays fields. - fcTL (line 419): reject if data_length < sizeof(FrameControlChunkData) Prevents heap OOB read of width, height, offsets, delay, and blend/dispose operation fields. Same vulnerability class as the fdAT fix in the previous commit. --- engine/src/flutter/lib/ui/painting/image_generator_apng.cc | 6 ++++++ 1 file changed, 6 insertions(+) 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 bebdaabf625c6..94d01d10d348c 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 APNGImageGenerator::MakeFromData( } } + if (chunk->get_data_length() < sizeof(AnimationControlChunkData)) { + return nullptr; + } const AnimationControlChunkData* animation_data = CastChunkData(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(chunk); ImageGenerator::FrameInfo frame_info; From fb7ed508f6f9d33004a2fa7ec7effa88a5adf4b4 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Fri, 15 May 2026 21:04:32 +0000 Subject: [PATCH 5/8] Updates to the APNG fdAT chunk parsing fix * Define a constant for the size of the sequence number in the fdAT chunk * Use an assert in the chunk copying loop to confirm that the data was validated by the parser. * Add an ImageGeneratorRegistry to the test which will register the required PNG codec in Skia. * Simplify the test to create an APNG containing a single frame with the potentially malformed fdAT chunk. The original version of the test did not reach the fdAT parsing code because its first frame got an error from SkCodec::MakeFromStream. Skia could not recognize the frame because the codecs had not been registered. * Use the CRC implementation from APNGImageGenerator in the test. --- engine/src/flutter/lib/ui/BUILD.gn | 2 - .../lib/ui/fixtures/apng_fdat_len0.apng | Bin 115 -> 0 bytes .../lib/ui/fixtures/apng_fdat_len4.apng | Bin 119 -> 0 bytes .../lib/ui/painting/image_generator_apng.cc | 25 ++-- .../lib/ui/painting/image_generator_apng.h | 4 + .../image_generator_apng_unittests.cc | 128 ++++-------------- 6 files changed, 49 insertions(+), 110 deletions(-) delete mode 100644 engine/src/flutter/lib/ui/fixtures/apng_fdat_len0.apng delete mode 100644 engine/src/flutter/lib/ui/fixtures/apng_fdat_len4.apng diff --git a/engine/src/flutter/lib/ui/BUILD.gn b/engine/src/flutter/lib/ui/BUILD.gn index 39093cafc5fe3..6ed8db4f6e4e2 100644 --- a/engine/src/flutter/lib/ui/BUILD.gn +++ b/engine/src/flutter/lib/ui/BUILD.gn @@ -232,8 +232,6 @@ if (enable_unittests) { "fixtures/DisplayP3Logo.png", "fixtures/Horizontal.jpg", "fixtures/Horizontal.png", - "fixtures/apng_fdat_len0.apng", - "fixtures/apng_fdat_len4.apng", "fixtures/heart_end.png", "fixtures/hello_loop_2.gif", "fixtures/hello_loop_2.webp", diff --git a/engine/src/flutter/lib/ui/fixtures/apng_fdat_len0.apng b/engine/src/flutter/lib/ui/fixtures/apng_fdat_len0.apng deleted file mode 100644 index 95323d13e0f6c543a2e46cf8b13a960dc477e403..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 115 zcmeAS@N?(olHy`uVBq!ia0vp^j3CUx1|*??BQZI|2gqiE2>@xSG>8~T2&@4~E(O6( POK}W=@jPAqTtI99EfEOB diff --git a/engine/src/flutter/lib/ui/fixtures/apng_fdat_len4.apng b/engine/src/flutter/lib/ui/fixtures/apng_fdat_len4.apng deleted file mode 100644 index d7c428acaea7979524e06461e161a37d9acb552c..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 119 zcmeAS@N?(olHy`uVBq!ia0vp^j3CUx1|*??BQZI|2gqiE2>@xSG>8~T2&@4~E(OkJ SNlS4If%AY2Pgg$|5FY?c1qjFh 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 94d01d10d348c..510fe7424b8e4 100644 --- a/engine/src/flutter/lib/ui/painting/image_generator_apng.cc +++ b/engine/src/flutter/lib/ui/painting/image_generator_apng.cc @@ -497,10 +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) { - if (chunk->get_data_length() < 4) { + if (chunk->get_data_length() < kFrameDataSequenceNumberSize) { return std::make_pair(std::nullopt, nullptr); } - chunk_space -= 4; + chunk_space -= kFrameDataSequenceNumberSize; } } @@ -536,21 +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) { - if (c->get_data_length() < 4) { - return std::make_pair(std::nullopt, nullptr); - } + FML_DCHECK(c->get_data_length() >= kFrameDataSequenceNumberSize); // Write a new IDAT chunk header. ChunkHeader* write_header = reinterpret_cast(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(c) + sizeof(ChunkHeader) + 4, + reinterpret_cast(c) + sizeof(ChunkHeader) + + kFrameDataSequenceNumberSize, write_header->get_data_length()); write_cursor += write_header->get_data_length(); @@ -658,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(this) + 4; + uint8_t* chunk_data = reinterpret_cast(this) + 4; + return ComputeCrc32(chunk_data, length); +} + +uint32_t APNGImageGenerator::ComputeCrc32(uint8_t* data, size_t length) { uint32_t crc = 0; + 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. @@ -671,9 +676,9 @@ uint32_t APNGImageGenerator::ChunkHeader::ComputeChunkCrc32() { length16 = std::numeric_limits::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..c407b1148fe22 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,14 @@ class APNGImageGenerator : public ImageGenerator { static std::unique_ptr MakeFromData(sk_sp data); + static uint32_t ComputeCrc32(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; + 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 index 310a10a2bef53..25f158ee17863 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 @@ -8,6 +8,7 @@ #include #include +#include "flutter/lib/ui/painting/image_generator_registry.h" #include "flutter/testing/testing.h" #include "third_party/skia/include/core/SkData.h" @@ -30,18 +31,6 @@ void WriteBE16(std::vector& buf, uint16_t val) { buf.push_back(val & 0xFF); } -// Computes CRC32 over chunk type + data (standard PNG CRC). -uint32_t ComputePngCrc32(const uint8_t* data, size_t length) { - uint32_t crc = 0xFFFFFFFF; - for (size_t i = 0; i < length; i++) { - crc ^= data[i]; - for (int j = 0; j < 8; j++) { - crc = (crc >> 1) ^ (0xEDB88320 & (-(crc & 1))); - } - } - return crc ^ 0xFFFFFFFF; -} - // Appends a PNG chunk (length + type + data + CRC) to the buffer. void AppendChunk(std::vector& buf, const char type[4], @@ -50,8 +39,8 @@ void AppendChunk(std::vector& buf, size_t type_start = buf.size(); buf.insert(buf.end(), type, type + 4); buf.insert(buf.end(), data.begin(), data.end()); - uint32_t crc = - ComputePngCrc32(buf.data() + type_start, 4 + data.size()); + uint32_t crc = APNGImageGenerator::ComputeCrc32(buf.data() + type_start, + 4 + data.size()); WriteBE32(buf, crc); } @@ -78,10 +67,10 @@ std::vector BuildMaliciousApng(uint32_t fdat_data_length) { AppendChunk(apng, "IHDR", ihdr); } - // acTL: 2 frames, loop forever + // acTL: 1 frame, loop forever { std::vector actl; - WriteBE32(actl, 2); // num_frames + WriteBE32(actl, 1); // num_frames WriteBE32(actl, 0); // num_plays (0 = infinite) AppendChunk(apng, "acTL", actl); } @@ -89,44 +78,19 @@ std::vector BuildMaliciousApng(uint32_t fdat_data_length) { // fcTL for frame 0 { std::vector 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); - } - - // IDAT for frame 0: minimal valid zlib-compressed 1x1 RGBA - { - // zlib header (78 01) + deflate block for filter_byte(0) + RGBA(0,0,0,255) - const uint8_t idat_data[] = {0x78, 0x01, 0x62, 0x60, 0x60, 0x60, - 0xF8, 0x0F, 0x00, 0x00, 0x05, 0x00, - 0x01}; - std::vector idat(idat_data, idat_data + sizeof(idat_data)); - AppendChunk(apng, "IDAT", idat); - } - - // fcTL for frame 1 - { - std::vector fctl; - WriteBE32(fctl, 1); // 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 + 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 1: data_length < 4 + // 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. { @@ -145,55 +109,23 @@ std::vector BuildMaliciousApng(uint32_t fdat_data_length) { } // namespace -// Verify that an APNG with an fdAT chunk of data_length=0 does not crash. -// Without the fix, this would cause a uint32_t underflow (0 - 4 = 0xFFFFFFFC) -// leading to a heap buffer overflow in DemuxNextImage(). -TEST(APNGImageGeneratorTest, FdATWithZeroDataLengthDoesNotCrash) { - auto apng_bytes = BuildMaliciousApng(0); - auto data = SkData::MakeWithCopy(apng_bytes.data(), apng_bytes.size()); - auto generator = APNGImageGenerator::MakeFromData(data); - - // The generator may fail to create (if the malformed fdAT is caught during - // initial parsing) or may create successfully but fail during frame decode. - // Either way, it must NOT crash. - if (generator) { - // If the generator was created, try to decode frame 1 which references - // the malicious fdAT. This must not crash. - auto info = generator->GetInfo(); - std::vector pixels(info.computeMinByteSize()); - generator->GetPixels(info, pixels.data(), info.minRowBytes(), 1, - std::nullopt); - } -} - -// Verify that an fdAT chunk with data_length=2 (less than the required 4-byte -// sequence number) also does not crash. +// Verify that the APNG decoder can handle fdAT chunks whose length is shorter +// that the required 4-byte sequence number. TEST(APNGImageGeneratorTest, FdATWithShortDataLengthDoesNotCrash) { - auto apng_bytes = BuildMaliciousApng(2); - auto data = SkData::MakeWithCopy(apng_bytes.data(), apng_bytes.size()); - auto generator = APNGImageGenerator::MakeFromData(data); - - if (generator) { - auto info = generator->GetInfo(); - std::vector pixels(info.computeMinByteSize()); - generator->GetPixels(info, pixels.data(), info.minRowBytes(), 1, - std::nullopt); - } -} + ImageGeneratorRegistry registry; -// Verify that a valid fdAT chunk (data_length >= 4) still works correctly. -TEST(APNGImageGeneratorTest, ValidFdATIsAccepted) { - // data_length=8: 4 bytes sequence number + 4 bytes compressed data - auto apng_bytes = BuildMaliciousApng(8); - auto data = SkData::MakeWithCopy(apng_bytes.data(), apng_bytes.size()); - auto generator = APNGImageGenerator::MakeFromData(data); - - // A valid APNG with proper fdAT should create a generator successfully. - // Frame decode may still fail due to invalid compressed data, but the - // generator creation should not be rejected by the data_length check. - if (generator) { - EXPECT_GE(generator->GetFrameCount(), 1u); - } + 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 From 4750fe2e0cf0454fa58dade333c05b70abafd917 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Mon, 18 May 2026 20:25:58 +0000 Subject: [PATCH 6/8] feedback --- engine/src/flutter/lib/ui/painting/image_generator_apng.cc | 6 +++--- engine/src/flutter/lib/ui/painting/image_generator_apng.h | 4 +++- .../lib/ui/painting/image_generator_apng_unittests.cc | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) 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 510fe7424b8e4..6b21aebb51d08 100644 --- a/engine/src/flutter/lib/ui/painting/image_generator_apng.cc +++ b/engine/src/flutter/lib/ui/painting/image_generator_apng.cc @@ -658,13 +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 = reinterpret_cast(this) + 4; + const uint8_t* chunk_data = reinterpret_cast(this) + 4; return ComputeCrc32(chunk_data, length); } -uint32_t APNGImageGenerator::ComputeCrc32(uint8_t* data, size_t length) { +uint32_t APNGImageGenerator::ComputeCrc32(const uint8_t* data, size_t length) { uint32_t crc = 0; - uint8_t* data_p = data; + 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. 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 c407b1148fe22..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,12 +53,14 @@ class APNGImageGenerator : public ImageGenerator { static std::unique_ptr MakeFromData(sk_sp data); - static uint32_t ComputeCrc32(uint8_t* data, size_t length); + /// 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 { 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 25f158ee17863..d5c4949e7164e 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 @@ -110,7 +110,7 @@ std::vector BuildMaliciousApng(uint32_t fdat_data_length) { } // namespace // Verify that the APNG decoder can handle fdAT chunks whose length is shorter -// that the required 4-byte sequence number. +// than the required 4-byte sequence number. TEST(APNGImageGeneratorTest, FdATWithShortDataLengthDoesNotCrash) { ImageGeneratorRegistry registry; From 74bb1ed421610dbdb384d4a447c9a6cd5b02a831 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Mon, 18 May 2026 20:48:44 +0000 Subject: [PATCH 7/8] test assert --- .../flutter/lib/ui/painting/image_generator_apng_unittests.cc | 1 + 1 file changed, 1 insertion(+) 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 d5c4949e7164e..390c292c17274 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 @@ -35,6 +35,7 @@ void WriteBE16(std::vector& buf, uint16_t val) { void AppendChunk(std::vector& buf, const char type[4], const std::vector& data) { + FML_CHECK(data.size() <= std::numeric_limits::max()); WriteBE32(buf, static_cast(data.size())); size_t type_start = buf.size(); buf.insert(buf.end(), type, type + 4); From 8f60267ff260cc19b4e1516a744f9668e7c1ed82 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Thu, 21 May 2026 15:51:27 +0000 Subject: [PATCH 8/8] clang-tidy --- .../lib/ui/painting/image_generator_apng_unittests.cc | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) 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 390c292c17274..4765004b180ae 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 @@ -94,13 +94,7 @@ std::vector BuildMaliciousApng(uint32_t fdat_data_length) { // 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. - { - std::vector fdat; - for (uint32_t i = 0; i < fdat_data_length; i++) { - fdat.push_back(0); - } - AppendChunk(apng, "fdAT", fdat); - } + AppendChunk(apng, "fdAT", std::vector(fdat_data_length, 0)); // IEND AppendChunk(apng, "IEND", {});