Fix integer overflow in APNG parser and add bounds checks to StandardCodecSerializer#187701
Fix integer overflow in APNG parser and add bounds checks to StandardCodecSerializer#187701unknownhad wants to merge 13 commits into
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request adds overflow protection to APNG chunk size calculation and introduces bounds checks using a new GetRemaining method in ByteStreamReader during standard codec deserialization of strings, lists, maps, and vectors. The review feedback identifies a backward compatibility issue where the default implementation of GetRemaining returning 0 causes custom stream readers to fail bounds checks, and suggests returning std::numeric_limits<size_t>::max() instead.
2432eb8 to
a1fdd62
Compare
…andardCodecSerializer Add overflow detection in GetChunkSize to prevent size_t wrap on 32-bit platforms. Add remaining-bytes validation in StandardCodecSerializer before string/vector/list/map allocations to reject malformed messages early.
a1fdd62 to
81ec8d7
Compare
…e/flutter/byte_streams.h Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
jason-simmons
left a comment
There was a problem hiding this comment.
Thank you for finding the APNG chunk size overflow issue!
I've forked the APNG example in this PR into another PR with a different way to avoid overflow: #187949
The StandardCodecSerializer changes are unrelated. Platform channels are used as an internal communication mechanism between the application, framework, and engine. They are not intended to receive untrusted data that has not been written using the encoders provided in the engine and framework. Stream implementations such as ByteBufferStreamReader::ReadBytes will do bounds checking.
| const size_t data_length = static_cast<size_t>(chunk->get_data_length()); | ||
| const size_t total = sizeof(ChunkHeader) + data_length + kChunkCrcSize; | ||
| if (total < data_length) { | ||
| return std::numeric_limits<size_t>::max(); |
There was a problem hiding this comment.
I'd prefer to avoid returning a value that does not accurately reflect the chunk's size.
I have a PR that instead validates the data length in IsValidChunkHeader before GetChunkSize is called.
| return ReadVector<double>(stream); | ||
| case EncodedType::kList: { | ||
| size_t length = ReadSize(stream); | ||
| if (length > stream->GetRemaining()) { |
There was a problem hiding this comment.
The length field in a serialized list is a count of elements in the list, not bytes.
| } | ||
| case EncodedType::kMap: { | ||
| size_t length = ReadSize(stream); | ||
| if (length > stream->GetRemaining()) { |
There was a problem hiding this comment.
Similar to lists - the length field here is a count of entries in the map.
|
Thanks for the review, @jason-simmons. Understood on the StandardCodecSerializer — makes sense that platform channels are a trusted boundary. Happy to close this PR in favor of #187949 for the APNG fix. Let me know if there's anything else needed. |
…tChunkSize to avoid potential overflow in the chunk size calculation (flutter#187949) Before this PR, APNGImageGenerator::IsValidChunkHeader was calling GetChunkSize to check whether the buffer had sufficient capacity for the chunk. The chunk contains a 32-bit data length field, and GetChunkSize calculates the chunk size as a size_t. If size_t is 32-bit and the chunk data length is malformed, then the calculation could overflow and return an incorrect result. This PR verifies that the chunk's data length fits within the remaining capacity of the buffer before using the length in calculations. See flutter#187701 --------- Co-authored-by: Himanshu Anand <anand.himanshu17@gmail.com>
|
How can I get CVE for this issue? |
…tChunkSize to avoid potential overflow in the chunk size calculation (flutter#187949) Before this PR, APNGImageGenerator::IsValidChunkHeader was calling GetChunkSize to check whether the buffer had sufficient capacity for the chunk. The chunk contains a 32-bit data length field, and GetChunkSize calculates the chunk size as a size_t. If size_t is 32-bit and the chunk data length is malformed, then the calculation could overflow and return an incorrect result. This PR verifies that the chunk's data length fits within the remaining capacity of the buffer before using the length in calculations. See flutter#187701 --------- Co-authored-by: Himanshu Anand <anand.himanshu17@gmail.com>
Description
This PR fixes two memory safety issues in the Flutter engine's C++ code:
1. Integer overflow in APNG demuxer
GetChunkSize(image_generator_apng.h)GetChunkSizecomputessizeof(ChunkHeader) + data_length + kChunkCrcSizeassize_t. On 32-bit targets, largedata_lengthvalues cause this to wrap, which can leadIsValidChunkHeaderto accept malformed chunks. Added an overflow check that returnsSIZE_MAXon wrap, causing the existing bounds validation to correctly reject such chunks.2. Missing bounds validation in
StandardCodecSerializer(standard_codec.cc)The C++ platform-channel message decoder reads size fields from the message and uses them to allocate memory (
string.resize,vector.resize,list.reserve) without first checking that the declared size fits within the remaining message bytes. AddedGetRemaining()toByteStreamReaderand validation checks before each allocation inReadValueOfTypeandReadVector, consistent with the approach used by the Linux GObject codec (fl_standard_message_codec.cc).Changes
image_generator_apng.h: Overflow guard inGetChunkSizebyte_streams.h: Added virtualGetRemaining()toByteStreamReaderbyte_buffer_streams.h: OverrideGetRemaining()inByteBufferStreamReaderstandard_codec.cc: Bounds checks before string/vector/list/map allocationsTesting
Verified with AddressSanitizer that the APNG overflow no longer triggers on 32-bit builds, and that the StandardCodec correctly rejects malformed messages instead of attempting multi-GB allocations.
Pre-launch Checklist