Fix a potential buffer overflow in the animated PNG decoder when parsing malformed fdAT chunks#186700
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements bounds checking for APNG chunk data lengths within the APNGImageGenerator to mitigate potential integer underflows and out-of-bounds memory access. It also refactors CRC32 computation into a static helper and introduces unit tests to verify the decoder's handling of malformed fdAT chunks. Feedback focuses on maintaining const-correctness for read-only buffer pointers, adhering to the Flutter style guide by documenting public members, and addressing a minor typo in the test suite.
There was a problem hiding this comment.
Code Review
This pull request introduces bounds checks in the APNG image generator to prevent integer underflows and out-of-bounds memory access when processing acTL, fcTL, and fdAT chunks. It also refactors CRC32 computation into a reusable static method and replaces magic numbers with the kFrameDataSequenceNumberSize constant. A new unit test file is added to verify the decoder's resilience against malicious APNG data. Feedback focuses on improving const-correctness for data pointers and adding documentation for new members as required by the style guide.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
lgtm for the most part. i agree with the constness that gemini pointed out and some other minor notes
|
|
||
| // 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); |
There was a problem hiding this comment.
add a static assert for little endian please
static_assert(std::endian::native == std::endian::little,
"This code requires a Little Endian architecture.");There was a problem hiding this comment.
This does not depend on the endianness of the host. The bytes will be written in order from most significant to least significant on any architecture.
There was a problem hiding this comment.
This function has the assumption that val is in little endian. That will only be the case when the host is little endian. If we are on a big endian machine we don't have to do this swizzling.
There was a problem hiding this comment.
val is a uint32_t, and the shifts of val done by this function will behave consistently regardless of the host architecture's endianness.
This function writes the value of val into buf in the big-endian format required by the PNG spec. It will work as intended on both big- and little-endian architectures.
There was a problem hiding this comment.
Jason's right, this will operate the same across the different machines, lgtm
| 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); |
There was a problem hiding this comment.
Just noting for awareness:
Up until now, tests for the APNG demuxer have lived a level above this at the codec level in engine/src/flutter/testing/dart/codec_test.dart with pre-generated APNG fixtures. This is mainly due to the image generator state machine being so intertwined with the classes driving it.
I kinda like the idea of generating bad APNGs inline, since it makes the structure of the bad file easier to decipher, and IMO since this is a "crash on initial parse" bug rather than a "crash after advancing N frames" bug... seems fine to place it this deep.
guard against fdAT data_length < 4 underflow in APNGImageGenerator::DemuxNextImage and add a ui_unittests regression test + minimal fixtures.
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 flutter#56928: frame offset bounds check - PR flutter#57025: integer wrapping fix for the bounds check Neither addressed the fdAT data_length underflow.
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)
Extend the data_length validation to cover two additional chunk types that use CastChunkData<T>() 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.
* 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.
|
Added a change to fix a clang-tidy warning (8f60267) - PTAL |
jason-simmons
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
6 participants
An fdAT (frame data) chunk in an APNG should contain a sequence number
followed by image data. The APNG decoder needs to reject invalid fdAT
chunks that do not have the expected contents.
Based on #184301 and #183180
Fixes #183179