Don't decode AND mask for an icon that already has alpha information

Source/platform/image-decoders/ico/ICOImageDecoder.cpp

 /*
  * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 198 matching lines @@
     const IconDirectoryEntry& dirEntry = m_dirEntries[index];
     const ImageType imageType = imageTypeAtIndex(index);
     if (imageType == Unknown)
         return false; // Not enough data to determine image type yet.
 
     if (imageType == BMP) {
         if (!m_bmpReaders[index]) {
             // We need to have already sized m_frameBufferCache before this, and
             // we must not resize it again later (see caution in frameCount()).
             ASSERT(m_frameBufferCache.size() == m_dirEntries.size());
-            m_bmpReaders[index] = adoptPtr(new BMPImageReader(this, dirEntry.m_imageOffset, 0, true));
-            m_bmpReaders[index]->setData(m_data.get());
+            // Make sure the image data doesn't go beyond the end of the file.
+            uint32_t imageSize = std::min(dirEntry.m_imageSize, m_data->size() - dirEntry.m_imageOffset);
+            RefPtr<SharedBuffer> bmpData(SharedBuffer::create(&m_data->data()[dirEntry.m_imageOffset], imageSize));
+            m_bmpReaders[index] = adoptPtr(new BMPImageReader(this, 0, 0, true));
+            m_bmpReaders[index]->setData(bmpData.get());

[Peter Kasting, 2014/11/18 20:59:05]

I have three concerns with this code, in decreasing order.

(1) We also call setData() on the BMP reader in ICOImageDecoder::setData()
above, for when e.g. more data comes in after an initial decode attempt.  If we
want to be correct, we have to either mirror this clamp there, or we have to
instead inform the BMP decoder about the size limits so it will always avoid
running over its data limit.

(2) Is the image size in the directory entry always trustworthy?  Do other
browsers always use it, do they ignore it if it's 0, do they use some other
number to calculate this limit?  There are so many malformed images on the web
that I'm worried about this patch fixing this case and breaking others.  Perhaps
instead of trusting the image size field, we should instead clamp to "however
much data we have before the start of the next entry"?  That seems potentially
safer.

(3) There's also a setDataForPNGDecoderAtIndex() function above.  You're only
clamping for BMP.  Perhaps we should also clamp for other types of embedded
images?

             m_bmpReaders[index]->setBuffer(&m_frameBufferCache[index]);
         }
         m_frameSize = dirEntry.m_size;
         bool result = m_bmpReaders[index]->decodeBMP(false);
         m_frameSize = IntSize();
         return result;
     }
 
     if (!m_pngDecoders[index]) {
         m_pngDecoders[index] = adoptPtr(
@@ skipping 76 matching lines @@
         height = 256;
     IconDirectoryEntry entry;
     entry.m_size = IntSize(width, height);
     if (m_fileType == CURSOR) {
         entry.m_bitCount = 0;
         entry.m_hotSpot = IntPoint(readUint16(4), readUint16(6));
     } else {
         entry.m_bitCount = readUint16(6);
         entry.m_hotSpot = IntPoint();
     }
+    entry.m_imageSize = readUint32(8);
     entry.m_imageOffset = readUint32(12);
 
     // Some icons don't have a bit depth, only a color count.  Convert the
     // color count to the minimum necessary bit depth.  It doesn't matter if
     // this isn't quite what the bitmap info header says later, as we only use
     // this value to determine which icon entry is best.
     if (!entry.m_bitCount) {
         int colorCount = static_cast<uint8_t>(m_data->data()[m_decodedOffset + 2]);
         if (!colorCount)
             colorCount = 256;  // Vague in the spec, needed by real-world icons.
@@ skipping 10 matching lines @@
     // Check if this entry is a BMP or a PNG; we need 4 bytes to check the magic
     // number.
     ASSERT_WITH_SECURITY_IMPLICATION(index < m_dirEntries.size());
     const uint32_t imageOffset = m_dirEntries[index].m_imageOffset;
     if ((imageOffset > m_data->size()) || ((m_data->size() - imageOffset) < 4))
         return Unknown;
     return strncmp(&m_data->data()[imageOffset], "\x89PNG", 4) ? BMP : PNG;
 }
 
 }