Ico security issues fix

src/images/SkImageDecoder_libico.cpp

 /*
  * Copyright 2006 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkColorPriv.h"
 #include "SkImageDecoder.h"
 #include "SkStream.h"
@@ skipping 64 matching lines @@
 SkImageDecoder::Result SkICOImageDecoder::onDecode(SkStream* stream, SkBitmap* bm, Mode mode)
 {
     SkAutoMalloc autoMal;
     const size_t length = SkCopyStreamToStorage(&autoMal, stream);
     if (0 == length) {
         return kFailure;
     }
 
     unsigned char* buf = (unsigned char*)autoMal.get();
 
+    // Check that the buffer is large enough to read the directory header
+    if (length < 6) {

[scroggo, 2015/03/12 13:25:54]

Why not merge this with the earlier check? We did not do anything interesting
that needs to be between the two.

+        return kFailure;
+    }
     //these should always be the same - should i use for error checking? - what about files that have some
     //incorrect values, but still decode properly?
     int reserved = read2Bytes(buf, 0);    // 0
     int type = read2Bytes(buf, 2);        // 1
     if (reserved != 0 || type != 1) {
         return kFailure;
     }
 
     int count = read2Bytes(buf, 4);
+    // Check that there are directory entries
+    if (count < 1) {
+        return kFailure;
+    }
 
-    //need to at least have enough space to hold the initial table of info
+    // Check that buffer is large enough to read directory entries.
+    // We are guaranteed that count is at least 1.  We might as well assume
+    // count is 1 because this deprecated decoder only looks at the first
+    // directory entry.
     if (length < (size_t)(6 + count*16)) {
         return kFailure;
     }
 
     //skip ahead to the correct header
     //commented out lines are not used, but if i switch to other read method, need to know how many to skip
     //otherwise, they could be used for error checking
     int w = readByte(buf, 6);
     int h = readByte(buf, 7);
     int colorCount = readByte(buf, 8);
     //int reservedToo = readByte(buf, 9 + choice*16);   //0
     //int planes = read2Bytes(buf, 10 + choice*16);       //1 - but often 0
     //int fakeBitCount = read2Bytes(buf, 12 + choice*16); //should be real - usually 0
     const size_t size = read4Bytes(buf, 14);           //matters?
     const size_t offset = read4Bytes(buf, 18);
     // promote the sum to 64-bits to avoid overflow
+    // Check that buffer is large enough to read image data
     if (offset > length || size > length || ((uint64_t)offset + size) > length) {
         return kFailure;
     }
 
     // Check to see if this is a PNG image inside the ICO
     {
         SkMemoryStream subStream(buf + offset, size, false);
         SkAutoTDelete<SkImageDecoder> otherDecoder(SkImageDecoder::Factory(&subStream));
         if (otherDecoder.get() != NULL) {
             // Disallow nesting ICO files within one another
@@ skipping 10 matching lines @@
             if (result != kFailure) {
                 return result;
             }
         }
     }
 
     //int infoSize = read4Bytes(buf, offset);             //40
     //int width = read4Bytes(buf, offset+4);              //should == w
     //int height = read4Bytes(buf, offset+8);             //should == 2*h
     //int planesToo = read2Bytes(buf, offset+12);         //should == 1 (does it?)
+
+    // Check that buffer is large enough to read the bit depth
+    if (length < offset + 16) {
+        return kFailure;
+    }
     int bitCount = read2Bytes(buf, offset+14);
 
     void (*placePixel)(const int pixelNo, const unsigned char* buf,
         const int xorOffset, int& x, int y, const int w,
         SkBitmap* bm, int alphaByte, int m, int shift, SkPMColor* colors) = NULL;
     switch (bitCount)
     {
         case 1:
             placePixel = &editPixelBit1;
             colorCount = 2;
@@ skipping 21 matching lines @@
 
     //these should all be zero, but perhaps are not - need to check
     //int compression = read4Bytes(buf, offset+16);       //0
     //int imageSize = read4Bytes(buf, offset+20);         //0 - sometimes has a value
     //int xPixels = read4Bytes(buf, offset+24);           //0
     //int yPixels = read4Bytes(buf, offset+28);           //0
     //int colorsUsed = read4Bytes(buf, offset+32)         //0 - might have an actual value though
     //int colorsImportant = read4Bytes(buf, offset+36);   //0
 
     int begin = SkToInt(offset + 40);
+    // Check that the buffer is large enough to read the color table
+    int bytesPerColor = 4;

[scroggo, 2015/03/12 13:25:54]

This can be inlined, since it's only used once. (Maybe with a comment that it's
always 4 bytes per color - at least the ones we support...).

[msarett, 2015/03/12 15:58:52]

Sure.  FWIW, 4 shows up in the code later.  We could alternately replace these
later uses of 4 with bytesPerColor, but I think it's best to avoid wasting time
trying to improve this code.

+    if (length < begin + bytesPerColor*colorCount) {
+        return kFailure;
+    }
+
     //this array represents the colortable
     //if i allow other types of bitmaps, it may actually be used as a part of the bitmap
     SkPMColor* colors = NULL;
     int blue, green, red;
     if (colorCount)
     {
         colors = new SkPMColor[colorCount];
         for (int j = 0; j < colorCount; j++)
         {
             //should this be a function - maybe a #define?
@@ skipping 28 matching lines @@
         delete[] colors;
         return kSuccess;
     }
 
     if (!this->allocPixelRef(bm, NULL))
     {
         delete[] colors;
         return kFailure;
     }
 
+    // The AND mask comes after the OR mask in the bmp.  If we check that the
+    // largest AND offset is safe, it should mean all other buffer accesses
+    // will be at smaller indices and will therefore be safe.
+    int maxAndOffset = andOffset + ((andLineWidth*(h-1)+(w-1)) >> 3);

[scroggo, 2015/03/12 13:25:54]

nit: This seems like it's the max offset of the alpha byte.

[msarett, 2015/03/12 15:58:52]

The AND mask is a 1 bit mask for each pixel indicating transparent or opaque. 
It comes after the XOR mask (actual pixel data).  I should make this more clear.
 I think the designer interprets all AND bytes as "alpha bytes".

+    if (length <= maxAndOffset) {
+        return kFailure;
+    }
+
     SkAutoLockPixels alp(*bm);
 
     for (int y = 0; y < h; y++)
     {
         for (int x = 0; x < w; x++)
         {
             //U32* address = bm->getAddr32(x, y);
 
             //check the alpha bit first, but pass it along to the function to figure out how to deal with it
             int andPixelNo = andLineWidth*(h-y-1)+x;
             //only need to get a new alphaByte when x %8 == 0
             //but that introduces an if and a mod - probably much slower
             //that's ok, it's just a read of an array, not a stream
             int alphaByte = readByte(buf, andOffset + (andPixelNo >> 3));
             int shift = 7 - (andPixelNo & 0x7);
             int m = 1 << shift;
 
             int pixelNo = lineWidth*(h-y-1)+x;
             placePixel(pixelNo, buf, xorOffset, x, y, w, bm, alphaByte, m, shift, colors);

[scroggo, 2015/03/12 13:25:54]

Here's the part where I get nervous. We pass buf to one of our placePixel
functions. Are they guaranteed not to read off the end off the array?

Maybe so... It looks like each call to read the buffer always starts from
xorOffset + f(pixelNo), (where f is some function). So long as that part is
smaller than maxAndOffset, the read should be safe. (I don't know whether we can
guarantee that.)

[msarett, 2015/03/12 15:58:52]

The XOR offset SHOULD always be less than the AND offset.  I have performed a
more thorough look at the code, and I feel that this will always be the case,
given the calculations.  I am adding an SkASSERT that clarifies this assumption.

 
         }
     }
 
     delete [] colors;
     //ensure we haven't read off the end?
     //of course this doesn't help us if the andOffset was a lie...
     //return andOffset + (andLineWidth >> 3) <= length;
     return kSuccess;
 }   //onDecode
@@ skipping 114 matching lines @@
 static SkImageDecoder_DecodeReg gReg(sk_libico_dfactory);
 
 static SkImageDecoder::Format get_format_ico(SkStreamRewindable* stream) {
     if (is_ico(stream)) {
         return SkImageDecoder::kICO_Format;
     }
     return SkImageDecoder::kUnknown_Format;
 }
 
 static SkImageDecoder_FormatReg gFormatReg(get_format_ico);