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 58 matching lines @@
         return (w + 1) << 2;
     }
     //  Otherwise return 0, which will allow it to be calculated automatically.
     return 0;
 }
 
 SkImageDecoder::Result SkICOImageDecoder::onDecode(SkStream* stream, SkBitmap* bm, Mode mode)
 {
     SkAutoMalloc autoMal;
     const size_t length = SkCopyStreamToStorage(&autoMal, stream);
-    if (0 == length) {
+    // Check that the buffer is large enough to read the directory header
+    if (length < 6) {
         return kFailure;
     }
 
     unsigned char* buf = (unsigned char*)autoMal.get();
 
     //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);
+    SkASSERT(w >= 0 && h >= 0);

[scroggo, 2015/03/12 21:19:29]

Is this because readByte is guaranteed to return a non-negative number? We might
need to instead return kFailure.

[msarett, 2015/03/12 22:18:22]

It is because readByte is guaranteed to return a non-negative number.  It
returns an unsigned char.

[scroggo, 2015/03/13 13:43:12]

Ah, got it, because buf is defined as pointer to unsigned char.

     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?)
+    
+    // For ico images, only a byte is used to store each dimension

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

The original decoder does not make this check.  I added this code because 0
values of w or h would mess up the bounds checking below.  We could
alternatively return kFailure here.

[scroggo, 2015/03/12 21:19:29]

If images in the wild use 0 to mean 256, we should include this.

[msarett, 2015/03/12 22:18:22]

This is how images in the wild are stored.  I will leave this.

+    // 0 is used to represent 256
+    if (w == 0) {
+        w = 256;
+    }
+    if (h == 0) {
+        h = 256;
+    }
+
+    // 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
+    // For bmp-in-icos, there should be 4 bytes per color
+    if (length < begin + 4*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 is a 1-bit alpha mask for each pixel that comes after the
+    // OR mask in the bmp.  If we check that the largest AND offset is safe,

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

This is a type I meant XOR not OR.  Will fix in next draft.

+    // 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);
+    if (length <= maxAndOffset) {
+        return kFailure;
+    }
+
+    // Here we assert that all reads from the buffer using the XOR offset are
+    // less than the AND offset.  This should be guaranteed based on the above
+    // calculations.
+    SkDEBUGCODE(

[scroggo, 2015/03/12 21:19:29]

nit: Typically we use SkDEBUGCODE for one line. For something like this, we use

#ifdef SK_DEBUG

[msarett, 2015/03/12 22:18:22]

Done.

+        int maxPixelNum = lineWidth*(h-1)+w-1;
+        int maxByte;
+        switch (bitCount) {
+            case 1:
+                maxByte = maxPixelNum >> 3;
+                break;
+            case 4:
+                maxByte = maxPixelNum >> 1;
+                break;
+            case 8:
+                maxByte = maxPixelNum;
+                break;
+            case 24:
+                maxByte = maxPixelNum * 3 + 2;
+                break;
+            case 32:
+                maxByte = maxPixelNum * 4 + 3;
+                break;
+            default:
+                SkASSERT(false);
+                return kFailure;

[scroggo, 2015/03/12 21:19:29]

This is strange. We (sort of) do something different in debug. But from your
assert, I see we should never reach here. Maybe this is the right thing to do.

[msarett, 2015/03/12 22:18:22]

Yes we should never reach here.  I will defer to you on what you think is the
right thing to style-wise.  I have no idea.

[scroggo, 2015/03/13 13:43:12]

I think it's fine as is.

+        }
+        int maxXOROffset = xorOffset + maxByte;
+    );
+    SkASSERT(maxXOROffset < andOffset);

[scroggo, 2015/03/12 21:19:29]

Can you include this inside the SkDEBUGCODE/SK_DEBUG check? It works in both
debug and release here, but I had to do a double take.

[msarett, 2015/03/12 22:18:22]

Yes that will make it easier to understand.

+
     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;
@@ skipping 133 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);