ICO: Prevent calling 'new' with large values

numImages is read from untrusted data, and as an unsigned short could
be up to 65,536. Avoid calling new with this number, which could result
in a crash if it pushes the device over the memory limit.

Change-Id: Ifff9e0ac6ade2b3d8584af656ea7d2f9eb4998e2
Reviewed-on: https://skia-review.googlesource.com/21269
Reviewed-by: Derek Sollenberger <djsollen@google.com>
Commit-Queue: Leon Scroggins <scroggo@google.com>

1 files changed, 22 insertions, 17 deletions

diff --git a/src/codec/SkIcoCodec.cpp b/src/codec/SkIcoCodec.cpp
index 14c6fc49e7..cde233a1bd 100644
--- a/src/codec/SkIcoCodec.cpp
+++ b/src/codec/SkIcoCodec.cpp
@@ -54,14 +54,6 @@ SkCodec* SkIcoCodec::NewFromStream(SkStream* stream) {
         return nullptr;
     }
 
-    // Ensure that we can read all of indicated directory entries
-    std::unique_ptr<uint8_t[]> entryBuffer(new uint8_t[numImages * kIcoDirEntryBytes]);
-    if (inputStream.get()->read(entryBuffer.get(), numImages*kIcoDirEntryBytes) !=
-            numImages*kIcoDirEntryBytes) {
-        SkCodecPrintf("Error: unable to read ico directory entries.\n");
-        return nullptr;
-    }
-
     // This structure is used to represent the vital information about entries
     // in the directory header.  We will obtain this information for each
     // directory entry.
@@ -69,10 +61,24 @@ SkCodec* SkIcoCodec::NewFromStream(SkStream* stream) {
         uint32_t offset;
         uint32_t size;
     };
-    std::unique_ptr<Entry[]> directoryEntries(new Entry[numImages]);
+    SkAutoFree dirEntryBuffer(sk_malloc_flags(sizeof(Entry) * numImages,
+                                              SK_MALLOC_TEMP));
+    if (!dirEntryBuffer) {
+        SkCodecPrintf("Error: OOM allocating ICO directory for %i images.\n",
+                      numImages);
+        return nullptr;
+    }
+    auto* directoryEntries = reinterpret_cast<Entry*>(dirEntryBuffer.get());
 
     // Iterate over directory entries
     for (uint32_t i = 0; i < numImages; i++) {
+        uint8_t entryBuffer[kIcoDirEntryBytes];
+        if (inputStream->read(entryBuffer, kIcoDirEntryBytes) !=
+                kIcoDirEntryBytes) {
+            SkCodecPrintf("Error: Dir entries truncated in ico.\n");
+            return nullptr;
+        }
+
         // The directory entry contains information such as width, height,
         // bits per pixel, and number of colors in the color palette.  We will
         // ignore these fields since they are repeated in the header of the
@@ -80,16 +86,16 @@ SkCodec* SkIcoCodec::NewFromStream(SkStream* stream) {
         // defer to the value in the embedded header anyway.
 
         // Specifies the size of the embedded image, including the header
-        uint32_t size = get_int(entryBuffer.get(), 8 + i*kIcoDirEntryBytes);
+        uint32_t size = get_int(entryBuffer, 8);
 
         // Specifies the offset of the embedded image from the start of file.
         // It does not indicate the start of the pixel data, but rather the
         // start of the embedded image header.
-        uint32_t offset = get_int(entryBuffer.get(), 12 + i*kIcoDirEntryBytes);
+        uint32_t offset = get_int(entryBuffer, 12);
 
         // Save the vital fields
-        directoryEntries.get()[i].offset = offset;
-        directoryEntries.get()[i].size = size;
+        directoryEntries[i].offset = offset;
+        directoryEntries[i].size = size;
     }
 
     // It is "customary" that the embedded images will be stored in order of
@@ -102,16 +108,15 @@ SkCodec* SkIcoCodec::NewFromStream(SkStream* stream) {
         }
     };
     EntryLessThan lessThan;
-    SkTQSort(directoryEntries.get(), directoryEntries.get() + numImages - 1,
-            lessThan);
+    SkTQSort(directoryEntries, &directoryEntries[numImages - 1], lessThan);
 
     // Now will construct a candidate codec for each of the embedded images
     uint32_t bytesRead = kIcoDirectoryBytes + numImages * kIcoDirEntryBytes;
     std::unique_ptr<SkTArray<std::unique_ptr<SkCodec>, true>> codecs(
             new (SkTArray<std::unique_ptr<SkCodec>, true>)(numImages));
     for (uint32_t i = 0; i < numImages; i++) {
-        uint32_t offset = directoryEntries.get()[i].offset;
-        uint32_t size = directoryEntries.get()[i].size;
+        uint32_t offset = directoryEntries[i].offset;
+        uint32_t size = directoryEntries[i].size;
 
         // Ensure that the offset is valid
         if (offset < bytesRead) {