Ico security issues fix
BUG=skia:3401
BUG=skia:3426
BUG=skia:3441
Review URL: https://codereview.chromium.org/996173005
diff --git a/resources/invalid_images/ico_fuzz0.ico b/resources/invalid_images/ico_fuzz0.ico
new file mode 100644
index 0000000..97efb89
--- /dev/null
+++ b/resources/invalid_images/ico_fuzz0.ico
Binary files differ
diff --git a/resources/invalid_images/ico_fuzz1.ico b/resources/invalid_images/ico_fuzz1.ico
new file mode 100644
index 0000000..5276123
--- /dev/null
+++ b/resources/invalid_images/ico_fuzz1.ico
Binary files differ
diff --git a/resources/invalid_images/ico_leak01.ico b/resources/invalid_images/ico_leak01.ico
new file mode 100644
index 0000000..2e28283
--- /dev/null
+++ b/resources/invalid_images/ico_leak01.ico
Binary files differ
diff --git a/src/images/SkImageDecoder_libico.cpp b/src/images/SkImageDecoder_libico.cpp
index 3ca1908..1fafd52 100644
--- a/src/images/SkImageDecoder_libico.cpp
+++ b/src/images/SkImageDecoder_libico.cpp
@@ -76,7 +76,8 @@
{
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;
}
@@ -91,8 +92,15 @@
}
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;
}
@@ -102,6 +110,7 @@
//otherwise, they could be used for error checking
int w = readByte(buf, 6);
int h = readByte(buf, 7);
+ SkASSERT(w >= 0 && h >= 0);
int colorCount = readByte(buf, 8);
//int reservedToo = readByte(buf, 9 + choice*16); //0
//int planes = read2Bytes(buf, 10 + choice*16); //1 - but often 0
@@ -109,6 +118,7 @@
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 +149,20 @@
//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
+ // 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,
@@ -180,6 +204,12 @@
//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 < (size_t) (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;
@@ -228,6 +258,45 @@
return kFailure;
}
+ // The AND mask is a 1-bit alpha mask for each pixel that comes after the
+ // XOR 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.
+ size_t 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.
+#ifdef SK_DEBUG
+ 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;
+ }
+ int maxXOROffset = xorOffset + maxByte;
+ SkASSERT(maxXOROffset < andOffset);
+#endif
+
SkAutoLockPixels alp(*bm);
for (int y = 0; y < h; y++)
diff --git a/tests/BadIcoTest.cpp b/tests/BadIcoTest.cpp
index 566f3d6..9543d4b 100644
--- a/tests/BadIcoTest.cpp
+++ b/tests/BadIcoTest.cpp
@@ -16,6 +16,9 @@
"sigabort_favicon.ico",
"sigsegv_favicon.ico",
"sigsegv_favicon_2.ico",
+ "ico_leak01.ico",
+ "ico_fuzz0.ico",
+ "ico_fuzz1.ico"
};
const char* badIcoFolder = "invalid_images";