Indexed PNG decoding: Ensure color table is large enough that the bit depth of the image will not allow reads beyond its end.
BUG=skia:3440
R=rmistry@google.com, scroggo@google.com
Committed: https://skia.googlesource.com/skia/+/493c1ce1cd406ef28683203146274154783452ce
Review URL: https://codereview.chromium.org/948163002
diff --git a/gyp/tests.gypi b/gyp/tests.gypi
index d4ca3af..f03488d 100644
--- a/gyp/tests.gypi
+++ b/gyp/tests.gypi
@@ -126,6 +126,7 @@
'../tests/ImageGeneratorTest.cpp',
'../tests/ImageIsOpaqueTest.cpp',
'../tests/ImageNewShaderTest.cpp',
+ '../tests/IndexedPngOverflowTest.cpp',
'../tests/InfRectTest.cpp',
'../tests/InterpolatorTest.cpp',
'../tests/InvalidIndexedPngTest.cpp',
diff --git a/src/images/SkImageDecoder_libpng.cpp b/src/images/SkImageDecoder_libpng.cpp
index c074268..245654d 100644
--- a/src/images/SkImageDecoder_libpng.cpp
+++ b/src/images/SkImageDecoder_libpng.cpp
@@ -97,7 +97,7 @@
SkPNGImageIndex* fImageIndex;
bool onDecodeInit(SkStream* stream, png_structp *png_ptrp, png_infop *info_ptrp);
- bool decodePalette(png_structp png_ptr, png_infop info_ptr,
+ bool decodePalette(png_structp png_ptr, png_infop info_ptr, int bitDepth,
bool * SK_RESTRICT hasAlphap, bool *reallyHasAlphap,
SkColorTable **colorTablep);
bool getBitmapColorType(png_structp, png_infop, SkColorType*, bool* hasAlpha,
@@ -350,7 +350,7 @@
SkColorTable* colorTable = NULL;
if (pngColorType == PNG_COLOR_TYPE_PALETTE) {
- decodePalette(png_ptr, info_ptr, &hasAlpha, &reallyHasAlpha, &colorTable);
+ decodePalette(png_ptr, info_ptr, bitDepth, &hasAlpha, &reallyHasAlpha, &colorTable);
}
SkAutoUnref aur(colorTable);
@@ -657,7 +657,8 @@
typedef uint32_t (*PackColorProc)(U8CPU a, U8CPU r, U8CPU g, U8CPU b);
bool SkPNGImageDecoder::decodePalette(png_structp png_ptr, png_infop info_ptr,
- bool *hasAlphap, bool *reallyHasAlphap,
+ int bitDepth, bool *hasAlphap,
+ bool *reallyHasAlphap,
SkColorTable **colorTablep) {
int numPalette;
png_colorp palette;
@@ -666,13 +667,6 @@
png_get_PLTE(png_ptr, info_ptr, &palette, &numPalette);
- /* BUGGY IMAGE WORKAROUND
-
- We hit some images (e.g. fruit_.png) who contain bytes that are == colortable_count
- which is a problem since we use the byte as an index. To work around this we grow
- the colortable by 1 (if its < 256) and duplicate the last color into that slot.
- */
- int colorCount = numPalette + (numPalette < 256);
SkPMColor colorStorage[256]; // worst-case storage
SkPMColor* colorPtr = colorStorage;
@@ -711,9 +705,19 @@
palette++;
}
- // see BUGGY IMAGE WORKAROUND comment above
- if (numPalette < 256) {
- *colorPtr = colorPtr[-1];
+ /* BUGGY IMAGE WORKAROUND
+
+ Invalid images could contain pixel values that are greater than the number of palette
+ entries. Since we use pixel values as indices into the palette this could result in reading
+ beyond the end of the palette which could leak the contents of uninitialized memory. To
+ ensure this doesn't happen, we grow the colortable to the maximum size that can be
+ addressed by the bitdepth of the image and fill it with the last palette color or black if
+ the palette is empty (really broken image).
+ */
+ int colorCount = SkTMax(numPalette, 1 << SkTMin(bitDepth, 8));
+ SkPMColor lastColor = index > 0 ? colorPtr[-1] : SkPackARGB32(0xFF, 0, 0, 0);
+ for (; index < colorCount; index++) {
+ *colorPtr++ = lastColor;
}
*colorTablep = SkNEW_ARGS(SkColorTable, (colorStorage, colorCount));
@@ -803,7 +807,7 @@
SkColorTable* colorTable = NULL;
if (pngColorType == PNG_COLOR_TYPE_PALETTE) {
- decodePalette(png_ptr, info_ptr, &hasAlpha, &reallyHasAlpha, &colorTable);
+ decodePalette(png_ptr, info_ptr, bitDepth, &hasAlpha, &reallyHasAlpha, &colorTable);
}
SkAutoUnref aur(colorTable);
diff --git a/tests/IndexedPngOverflowTest.cpp b/tests/IndexedPngOverflowTest.cpp
new file mode 100644
index 0000000..914053a
--- /dev/null
+++ b/tests/IndexedPngOverflowTest.cpp
@@ -0,0 +1,44 @@
+/*
+ * Copyright 2015 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "SkBitmap.h"
+#include "SkCanvas.h"
+#include "SkForceLinking.h"
+#include "SkImageDecoder.h"
+#include "SkImageInfo.h"
+#include "SkSurface.h"
+#include "Test.h"
+
+// A 20x1 image with 8 bits per pixel and a palette size of 2. Pixel values are 255, 254... Run
+// this test with ASAN to make sure we don't try to access before/after any palette-sized buffers.
+unsigned char gPng[] = {
+ 0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d,
+ 0x49, 0x48, 0x44, 0x52, 0x00, 0x00, 0x00, 0x14, 0x00, 0x00, 0x00, 0x01,
+ 0x08, 0x03, 0x00, 0x00, 0x00, 0xe9, 0x4c, 0x7e, 0x17, 0x00, 0x00, 0x00,
+ 0x09, 0x70, 0x48, 0x59, 0x73, 0x00, 0x00, 0x00, 0x1c, 0x00, 0x00, 0x00,
+ 0x1c, 0x00, 0x0f, 0x01, 0xb9, 0x8f, 0x00, 0x00, 0x00, 0x06, 0x50, 0x4c,
+ 0x54, 0x45, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xa5, 0x67, 0xb9, 0xcf,
+ 0x00, 0x00, 0x00, 0x20, 0x49, 0x44, 0x41, 0x54, 0x78, 0xda, 0xed, 0xfd,
+ 0x07, 0x01, 0x00, 0x20, 0x08, 0x00, 0x41, 0xbc, 0x5b, 0xe8, 0xdf, 0x97,
+ 0x99, 0xe3, 0x92, 0xa0, 0xf2, 0xdf, 0x3d, 0x7b, 0x0d, 0xda, 0x04, 0x1c,
+ 0x03, 0xad, 0x00, 0x38, 0x5c, 0x2e, 0xad, 0x12, 0x00, 0x00, 0x00, 0x00,
+ 0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82
+};
+
+DEF_TEST(IndexedPngOverflow, reporter) {
+ SkBitmap image;
+ SkForceLinking(false);
+ bool success = SkImageDecoder::DecodeMemory(
+ gPng, sizeof(gPng), &image, SkColorType::kUnknown_SkColorType,
+ SkImageDecoder::kDecodePixels_Mode);
+ REPORTER_ASSERT(reporter, success);
+
+ SkAutoTUnref<SkSurface> surface(SkSurface::NewRaster(SkImageInfo::MakeN32Premul(20, 1)));
+ SkCanvas* canvas = surface->getCanvas();
+ SkRect destRect = SkRect::MakeXYWH(0, 0, 20, 1);
+ canvas->drawBitmapRect(image, destRect, NULL);
+}
