Ico security issues fix

src/images/SkImageDecoder_libico.cpp

Index: src/images/SkImageDecoder_libico.cpp
diff --git a/src/images/SkImageDecoder_libico.cpp b/src/images/SkImageDecoder_libico.cpp
index 3ca19084daf39aa2e1b7c86e16e69eb55336e005..fc45bb85ee8f11e16206bc2689c1177ec2256379 100644
--- a/src/images/SkImageDecoder_libico.cpp
+++ b/src/images/SkImageDecoder_libico.cpp
@@ -82,6 +82,10 @@ SkImageDecoder::Result SkICOImageDecoder::onDecode(SkStream* stream, SkBitmap* b
 
     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
@@ -91,8 +95,15 @@ SkImageDecoder::Result SkICOImageDecoder::onDecode(SkStream* stream, SkBitmap* b
     }
 
     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;
     }
@@ -109,6 +120,7 @@ SkImageDecoder::Result SkICOImageDecoder::onDecode(SkStream* stream, SkBitmap* b
     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;
     }
@@ -139,6 +151,11 @@ SkImageDecoder::Result SkICOImageDecoder::onDecode(SkStream* stream, SkBitmap* b
     //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,
@@ -180,6 +197,12 @@ SkImageDecoder::Result SkICOImageDecoder::onDecode(SkStream* stream, SkBitmap* b
     //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;
@@ -228,6 +251,14 @@ SkImageDecoder::Result SkICOImageDecoder::onDecode(SkStream* stream, SkBitmap* b
         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++)