From 14bd786730f513bd95dd5c5512605f30aab26257 Mon Sep 17 00:00:00 2001 From: Leon Scroggins III Date: Fri, 3 May 2019 15:19:04 +0000 Subject: [PATCH] Check for overflow in PNGImageReader On a 32 bit machine, an extra large chunk length can result in overflow when added to other small numbers. Check for that overflow in Parse/ParseSize and mark the decoder as failed if it occurs. DCHECK that the length is valid in Decode methods, since it has already been checked by Parse. In ParseSize, check for length > PNG_UINT_31_MAX. This matches Parse, and the PNG spec (https://www.w3.org/TR/PNG/), which demands that all four byte integers are <= PNG_UINT_31_MAX. Bug: 954983 Change-Id: I86d92c2ce1db54b2326188d41d378f3c172bf67c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1588943 Reviewed-by: Noel Gordon Reviewed-by: Madeleine Barowsky Commit-Queue: Leon Scroggins Cr-Commit-Position: refs/heads/master@{#656379} --- .../image-decoders/png/png_image_reader.cc | 48 +++++++++++++------ 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/third_party/blink/renderer/platform/image-decoders/png/png_image_reader.cc b/third_party/blink/renderer/platform/image-decoders/png/png_image_reader.cc index 3cfa5490a3cb4e..14cdea9b01373f 100644 --- a/third_party/blink/renderer/platform/image-decoders/png/png_image_reader.cc +++ b/third_party/blink/renderer/platform/image-decoders/png/png_image_reader.cc @@ -39,6 +39,7 @@ #include "third_party/blink/renderer/platform/image-decoders/png/png_image_reader.h" #include +#include "base/numerics/checked_math.h" #include "third_party/blink/renderer/platform/image-decoders/fast_shared_buffer_reader.h" #include "third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.h" #include "third_party/blink/renderer/platform/image-decoders/segment_reader.h" @@ -128,7 +129,7 @@ const png_byte* ReadAsConstPngBytep(const FastSharedBufferReader& reader, size_t read_offset, size_t length, char* buffer) { - DCHECK(length <= kPngReadBufferSize); + DCHECK_LE(length, kPngReadBufferSize); return reinterpret_cast( reader.GetConsecutiveData(read_offset, length, buffer)); } @@ -244,17 +245,22 @@ bool PNGImageReader::ProgressivelyDecodeFirstFrame( char read_buffer[8]; // At the beginning of each loop, the offset is at the start of a chunk. const png_byte* chunk = ReadAsConstPngBytep(reader, offset, 8, read_buffer); + + // A large length would have been rejected in Parse. const png_uint_32 length = png_get_uint_32(chunk); - DCHECK(length <= PNG_UINT_31_MAX); + DCHECK_LE(length, PNG_UINT_31_MAX); // When an fcTL or IEND chunk is encountered, the frame data has ended. - // Return true, since all frame data is decoded. - if (IsChunk(chunk, "fcTL") || IsChunk(chunk, "IEND")) + if (IsChunk(chunk, "fcTL") || IsChunk(chunk, "IEND")) { return true; + } + + const size_t chunk_end_offset = offset + length + 12; + DCHECK_GT(chunk_end_offset, offset); // If this chunk was already decoded, move on to the next. - if (progressive_decode_offset_ >= offset + length + 12) { - offset += length + 12; + if (progressive_decode_offset_ >= chunk_end_offset) { + offset = chunk_end_offset; continue; } @@ -263,8 +269,6 @@ bool PNGImageReader::ProgressivelyDecodeFirstFrame( // Continue from there. // 2) This is an fdAT chunk. Convert it to an IDAT chunk to decode. // 3) This is any other chunk. Pass it to libpng for processing. - size_t end_offset_chunk = offset + length + 12; - if (progressive_decode_offset_ >= offset + 8) { offset = progressive_decode_offset_; } else if (IsChunk(chunk, "fdAT")) { @@ -276,7 +280,7 @@ bool PNGImageReader::ProgressivelyDecodeFirstFrame( offset += 8; } - size_t bytes_left_in_chunk = end_offset_chunk - offset; + size_t bytes_left_in_chunk = chunk_end_offset - offset; size_t bytes_decoded = ProcessData(reader, offset, bytes_left_in_chunk); progressive_decode_offset_ = offset + bytes_decoded; if (bytes_decoded < bytes_left_in_chunk) @@ -316,7 +320,7 @@ void PNGImageReader::DecodeFrame(const FastSharedBufferReader& reader, while (offset < end_offset) { const png_byte* chunk = ReadAsConstPngBytep(reader, offset, 8, read_buffer); const png_uint_32 length = png_get_uint_32(chunk); - DCHECK(length <= PNG_UINT_31_MAX); + DCHECK_LE(length, PNG_UINT_31_MAX); if (IsChunk(chunk, "fdAT")) { ProcessFdatChunkAsIdat(length); @@ -338,7 +342,7 @@ static bool CheckCrc(const FastSharedBufferReader& reader, size_t chunk_length) { constexpr size_t kSizeNeededForfcTL = 26 + 4; char read_buffer[kSizeNeededForfcTL]; - DCHECK(chunk_length + 4 <= kSizeNeededForfcTL); + DCHECK_LE(chunk_length + 4u, kSizeNeededForfcTL); const png_byte* chunk = ReadAsConstPngBytep(reader, chunk_start + 4, chunk_length + 4, read_buffer); @@ -405,6 +409,12 @@ bool PNGImageReader::Parse(SegmentReader& data, ParseQuery query) { const size_t length = png_get_uint_32(chunk); if (length > PNG_UINT_31_MAX) return false; + size_t chunk_end_offset; + if (!base::CheckAdd(read_offset_, base::CheckAdd(12, length)) + .AssignIfValid(&chunk_end_offset)) { + // Overflow + return false; + } const bool idat = IsChunk(chunk, "IDAT"); if (idat && !expect_idats_) @@ -463,7 +473,7 @@ bool PNGImageReader::Parse(SegmentReader& data, ParseQuery query) { new_frame_.start_offset = 0; } - if (reader.size() < read_offset_ + 12 + length) + if (reader.size() < chunk_end_offset) return true; if (IsChunk(chunk, "IEND")) { @@ -486,7 +496,7 @@ bool PNGImageReader::Parse(SegmentReader& data, ParseQuery query) { return false; } - read_offset_ += 12 + length; + read_offset_ = chunk_end_offset; } return true; } @@ -539,10 +549,20 @@ bool PNGImageReader::ParseSize(const FastSharedBufferReader& reader) { // Process APNG chunks manually, pass other chunks to libpng. for (png_uint_32 length = 0; reader.size() >= read_offset_ + 8; + // This call will not overflow since it was already checked below, after + // calculating chunk_end_offset. read_offset_ += length + 12) { const png_byte* chunk = ReadAsConstPngBytep(reader, read_offset_, 8, read_buffer); length = png_get_uint_32(chunk); + if (length > PNG_UINT_31_MAX) + return false; + size_t chunk_end_offset; + if (!base::CheckAdd(read_offset_, base::CheckAdd(12, length)) + .AssignIfValid(&chunk_end_offset)) { + // Overflow + return false; + } if (IsChunk(chunk, "IDAT")) { // Done with header chunks. @@ -566,7 +586,7 @@ bool PNGImageReader::ParseSize(const FastSharedBufferReader& reader) { } // Wait until the entire chunk is available for parsing simplicity. - if (reader.size() < read_offset_ + length + 12) + if (reader.size() < chunk_end_offset) break; if (IsChunk(chunk, "acTL")) {