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 <noel@chromium.org>
Reviewed-by: Madeleine Barowsky <mbarowsky@chromium.org>
Commit-Queue: Leon Scroggins <scroggo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#656379}

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 <memory>
+#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<const png_byte*>(
       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")) {