From 5d8a0be45aad78c5a22c8d099118ee26ef8144af Mon Sep 17 00:00:00 2001 From: wiredfool Date: Sun, 25 Sep 2016 10:44:22 +0100 Subject: [PATCH 1/9] Memory error in Storage.c when accepting negative image size arguments --- Tests/images/negative_size.ppm | Bin 0 -> 19 bytes Tests/test_file_ppm.py | 12 ++++++++++++ libImaging/Storage.c | 4 ++++ 3 files changed, 16 insertions(+) create mode 100755 Tests/images/negative_size.ppm diff --git a/Tests/images/negative_size.ppm b/Tests/images/negative_size.ppm new file mode 100755 index 0000000000000000000000000000000000000000..257b8c29c8ead2324a99166bfb6af1ed1fb0e5a5 GIT binary patch literal 19 UcmWGAGd5B%Hno5NAV-l404Om7!T Date: Thu, 29 Sep 2016 07:05:00 -0700 Subject: [PATCH 2/9] Map.c overflow fixes --- Tests/images/l2rgb_read.bmp | Bin 0 -> 57 bytes Tests/test_map.py | 25 +++++++++++++++++++++++++ map.c | 10 ++++++++++ 3 files changed, 35 insertions(+) create mode 100644 Tests/images/l2rgb_read.bmp create mode 100644 Tests/test_map.py diff --git a/Tests/images/l2rgb_read.bmp b/Tests/images/l2rgb_read.bmp new file mode 100644 index 0000000000000000000000000000000000000000..838e3226b07aa7214876e6fed83681b61c743a68 GIT binary patch literal 57 kcmZ?rHGqNt|LZjv7#M(D1_Ln70wlqFm SIZE_MAX bytes in the image or if + # the file encodes an offset that makes + # (offset + size(bytes)) > SIZE_MAX + + # Note that this image triggers the decompression bomb warning: + max_pixels = Image.MAX_IMAGE_PIXELS + Image.MAX_IMAGE_PIXELS = None + + # This image hits the offset test. + im = Image.open('Tests/images/l2rgb_read.bmp') + with self.assertRaises((ValueError, MemoryError)): + im.load() + + Image.MAX_IMAGE_PIXELS = max_pixels + + +if __name__ == '__main__': + unittest.main() diff --git a/map.c b/map.c index 7309a7bd7..3637ee86a 100644 --- a/map.c +++ b/map.c @@ -342,8 +342,18 @@ PyImaging_MapBuffer(PyObject* self, PyObject* args) stride = xsize * 4; } + if (ysize > INT_MAX / stride) { + PyErr_SetString(PyExc_MemoryError, "Integer overflow in ysize"); + return NULL; + } + size = (Py_ssize_t) ysize * stride; + if (offset > SIZE_MAX - size) { + PyErr_SetString(PyExc_MemoryError, "Integer overflow in offset"); + return NULL; + } + /* check buffer size */ if (PyImaging_GetBuffer(target, &view) < 0) return NULL; From 0f2d6e0cc5ec216c9f740eb119934ccd52d1d757 Mon Sep 17 00:00:00 2001 From: wiredfool Date: Thu, 29 Sep 2016 07:37:01 -0700 Subject: [PATCH 3/9] Changes, Release Notes for 3.3.2 --- CHANGES.rst | 9 +++++++++ docs/releasenotes/3.3.2.rst | 40 +++++++++++++++++++++++++++++++++++++ docs/releasenotes/index.rst | 1 + 3 files changed, 50 insertions(+) create mode 100644 docs/releasenotes/3.3.2.rst diff --git a/CHANGES.rst b/CHANGES.rst index 8cd9f2d1f..a9265f999 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -124,6 +124,15 @@ Changelog (Pillow) - Retain a reference to core image object in PyAccess #2009 [homm] +3.3.2 (2016-10-03) +------------------ + +- Fix negative image sizes in Storage.c #2105 + [wiredfool] + +- Fix integer overflow in map.c #2105 + [wiredfool] + 3.3.1 (2016-08-18) ------------------ diff --git a/docs/releasenotes/3.3.2.rst b/docs/releasenotes/3.3.2.rst new file mode 100644 index 000000000..141413093 --- /dev/null +++ b/docs/releasenotes/3.3.2.rst @@ -0,0 +1,40 @@ + +3.3.2 +===== + +Integer overflow in Map.c +------------------------- + +Pillow prior to 3.3.2 may experience integer overflow errors in map.c +when reading specially crafted image files. This may lead to memory +disclosure or corruption. + +Specifically, when parameters from the image are passed into +``Image.core.map_buffer``, the size of the image was calculated with +``xsize``*``ysize``*``bytes_per_pixel``. This will overflow if the +result is larger than SIZE_MAX. This is possible on a 32-bit system. + +Furthermore this ``size`` value was added to a potentially attacker +provided ``offset`` value and compared to the size of the buffer +without checking for overflow or negative values. + +These values were then used for creating pointers, at which point +Pillow could read the memory and include it in other images. The image +was marked readonly, so Pillow would not ordinarily write to that +memory without duplicating the image first. + +This issue was found by Cris Neckar at Divergent Security. + +Sign Extension in Storage.c +--------------------------- + +Pillow prior to 3.3.2 and PIL 1.1.7 (at least) do not check for +negative image sizes in ``ImagingNew`` in ``Storage.c``. A negative +image size can lead to a smaller allocation than expected, leading to +arbitrary writes. + +This issue was found by Cris Neckar at Divergent Security. + + + + diff --git a/docs/releasenotes/index.rst b/docs/releasenotes/index.rst index 2ffe37980..8c484af44 100644 --- a/docs/releasenotes/index.rst +++ b/docs/releasenotes/index.rst @@ -7,6 +7,7 @@ Release Notes :maxdepth: 2 3.4.0 + 3.3.2 3.3.0 3.2.0 3.1.2 From c0d2d2a912c7e70bce8f5b9054f60703b179006d Mon Sep 17 00:00:00 2001 From: wiredfool Date: Mon, 3 Oct 2016 02:54:23 -0700 Subject: [PATCH 4/9] whitespace: mixed 8ch tabs + spaces -> spaces --- libImaging/File.c | 134 +++++++++++++++++++++++----------------------- 1 file changed, 67 insertions(+), 67 deletions(-) diff --git a/libImaging/File.c b/libImaging/File.c index ac9ec3be1..b669ab665 100644 --- a/libImaging/File.c +++ b/libImaging/File.c @@ -30,26 +30,26 @@ ImagingOpenPPM(const char* infile) Imaging im; if (!infile) - return ImagingError_ValueError(NULL); + return ImagingError_ValueError(NULL); fp = fopen(infile, "rb"); if (!fp) - return ImagingError_IOError(); + return ImagingError_IOError(); /* PPM magic */ if (fgetc(fp) != 'P') - goto error; + goto error; switch (fgetc(fp)) { case '4': /* FIXME: 1-bit images are not yet supported */ - goto error; + goto error; case '5': - mode = "L"; - break; + mode = "L"; + break; case '6': - mode = "RGB"; - break; + mode = "RGB"; + break; default: - goto error; + goto error; } i = 0; @@ -59,66 +59,66 @@ ImagingOpenPPM(const char* infile) while (i < 3) { - /* Ignore optional comment fields */ - while (c == '\n') { - c = fgetc(fp); - if (c == '#') { - do { - c = fgetc(fp); - if (c == EOF) - goto error; - } while (c != '\n'); - c = fgetc(fp); - } - } + /* Ignore optional comment fields */ + while (c == '\n') { + c = fgetc(fp); + if (c == '#') { + do { + c = fgetc(fp); + if (c == EOF) + goto error; + } while (c != '\n'); + c = fgetc(fp); + } + } - /* Skip forward to next value */ - while (isspace(c)) - c = fgetc(fp); + /* Skip forward to next value */ + while (isspace(c)) + c = fgetc(fp); - /* And parse it */ - v = 0; - while (isdigit(c)) { - v = v * 10 + (c - '0'); - c = fgetc(fp); - } + /* And parse it */ + v = 0; + while (isdigit(c)) { + v = v * 10 + (c - '0'); + c = fgetc(fp); + } - if (c == EOF) - goto error; + if (c == EOF) + goto error; - switch (i++) { - case 0: - x = v; - break; - case 1: - y = v; - break; - case 2: - max = v; - break; - } + switch (i++) { + case 0: + x = v; + break; + case 1: + y = v; + break; + case 2: + max = v; + break; + } } im = ImagingNew(mode, x, y); if (!im) - return NULL; + return NULL; /* if (max != 255) ... FIXME: does anyone ever use this feature? */ if (strcmp(im->mode, "L") == 0) { - /* PPM "L" */ - for (y = 0; y < im->ysize; y++) - if (fread(im->image[y], im->xsize, 1, fp) != 1) - goto error; + /* PPM "L" */ + for (y = 0; y < im->ysize; y++) + if (fread(im->image[y], im->xsize, 1, fp) != 1) + goto error; } else { - /* PPM "RGB" or PyPPM mode */ - for (y = 0; y < im->ysize; y++) - for (x = i = 0; x < im->xsize; x++, i += im->pixelsize) - if (fread(im->image[y]+i, im->bands, 1, fp) != 1) - goto error; + /* PPM "RGB" or PyPPM mode */ + for (y = 0; y < im->ysize; y++) + for (x = i = 0; x < im->xsize; x++, i += im->pixelsize) + if (fread(im->image[y]+i, im->bands, 1, fp) != 1) + goto error; } fclose(fp); @@ -140,16 +140,16 @@ ImagingSaveRaw(Imaging im, FILE* fp) /* @PIL227: FIXME: for mode "1", map != 0 to 255 */ - /* PGM "L" */ - for (y = 0; y < im->ysize; y++) - fwrite(im->image[y], 1, im->xsize, fp); + /* PGM "L" */ + for (y = 0; y < im->ysize; y++) + fwrite(im->image[y], 1, im->xsize, fp); } else { - /* PPM "RGB" or other internal format */ - for (y = 0; y < im->ysize; y++) - for (x = i = 0; x < im->xsize; x++, i += im->pixelsize) - fwrite(im->image[y]+i, 1, im->bands, fp); + /* PPM "RGB" or other internal format */ + for (y = 0; y < im->ysize; y++) + for (x = i = 0; x < im->xsize; x++, i += im->pixelsize) + fwrite(im->image[y]+i, 1, im->bands, fp); } @@ -163,24 +163,24 @@ ImagingSavePPM(Imaging im, const char* outfile) FILE* fp; if (!im) { - (void) ImagingError_ValueError(NULL); - return 0; + (void) ImagingError_ValueError(NULL); + return 0; } fp = fopen(outfile, "wb"); if (!fp) { - (void) ImagingError_IOError(); - return 0; + (void) ImagingError_IOError(); + return 0; } if (strcmp(im->mode, "1") == 0 || strcmp(im->mode, "L") == 0) { - /* Write "PGM" */ - fprintf(fp, "P5\n%d %d\n255\n", im->xsize, im->ysize); + /* Write "PGM" */ + fprintf(fp, "P5\n%d %d\n255\n", im->xsize, im->ysize); } else if (strcmp(im->mode, "RGB") == 0) { /* Write "PPM" */ fprintf(fp, "P6\n%d %d\n255\n", im->xsize, im->ysize); } else { - (void) ImagingError_ModeError(); + (void) ImagingError_ModeError(); return 0; } From 1a43da7a8bda884a597f3a1623364f4719d21c14 Mon Sep 17 00:00:00 2001 From: wiredfool Date: Mon, 3 Oct 2016 03:09:54 -0700 Subject: [PATCH 5/9] Removed 'Debugging' open_ppm call that didn't check file provided parameters for sanity --- PIL/EpsImagePlugin.py | 5 +- PIL/IptcImagePlugin.py | 11 ++--- PIL/JpegImagePlugin.py | 4 +- PIL/PpmImagePlugin.py | 5 -- _imaging.c | 14 ------ libImaging/File.c | 110 ----------------------------------------- 6 files changed, 9 insertions(+), 140 deletions(-) diff --git a/PIL/EpsImagePlugin.py b/PIL/EpsImagePlugin.py index 9a668125a..77a7e7e1c 100644 --- a/PIL/EpsImagePlugin.py +++ b/PIL/EpsImagePlugin.py @@ -145,7 +145,8 @@ def Ghostscript(tile, size, fp, scale=1): status = gs.wait() if status: raise IOError("gs failed (status %d)" % status) - im = Image.core.open_ppm(outfile) + im = Image.open(outfile) + im.load() finally: try: os.unlink(outfile) @@ -154,7 +155,7 @@ def Ghostscript(tile, size, fp, scale=1): except OSError: pass - return im + return im.im.copy() class PSFile(object): diff --git a/PIL/IptcImagePlugin.py b/PIL/IptcImagePlugin.py index ed3bf81b1..1de17cbba 100644 --- a/PIL/IptcImagePlugin.py +++ b/PIL/IptcImagePlugin.py @@ -168,14 +168,9 @@ class IptcImageFile(ImageFile.ImageFile): o.close() try: - try: - # fast - self.im = Image.core.open_ppm(outfile) - except: - # slightly slower - im = Image.open(outfile) - im.load() - self.im = im.im + _im = Image.open(outfile) + _im.load() + self.im = _im.im finally: try: os.unlink(outfile) diff --git a/PIL/JpegImagePlugin.py b/PIL/JpegImagePlugin.py index e216fa06d..ef229e611 100644 --- a/PIL/JpegImagePlugin.py +++ b/PIL/JpegImagePlugin.py @@ -377,7 +377,9 @@ class JpegImageFile(ImageFile.ImageFile): raise ValueError("Invalid Filename") try: - self.im = Image.core.open_ppm(path) + _im = Image.open(path) + _im.load() + self.im = _im.im finally: try: os.unlink(path) diff --git a/PIL/PpmImagePlugin.py b/PIL/PpmImagePlugin.py index 68073cace..adaf8384c 100644 --- a/PIL/PpmImagePlugin.py +++ b/PIL/PpmImagePlugin.py @@ -123,11 +123,6 @@ class PpmImageFile(ImageFile.ImageFile): self.fp.tell(), (rawmode, 0, 1))] - # ALTERNATIVE: load via builtin debug function - # self.im = Image.core.open_ppm(self.filename) - # self.mode = self.im.mode - # self.size = self.im.size - # # -------------------------------------------------------------------- diff --git a/_imaging.c b/_imaging.c index ef5d346ba..47334f184 100644 --- a/_imaging.c +++ b/_imaging.c @@ -686,17 +686,6 @@ _radial_gradient(PyObject* self, PyObject* args) return PyImagingNew(ImagingFillRadialGradient(mode)); } -static PyObject* -_open_ppm(PyObject* self, PyObject* args) -{ - char* filename; - - if (!PyArg_ParseTuple(args, "s", &filename)) - return NULL; - - return PyImagingNew(ImagingOpenPPM(filename)); -} - static PyObject* _alpha_composite(ImagingObject* self, PyObject* args) { @@ -3424,9 +3413,6 @@ static PyMethodDef functions[] = { {"crc32", (PyCFunction)_crc32, 1}, {"getcodecstatus", (PyCFunction)_getcodecstatus, 1}, - /* Debugging stuff */ - {"open_ppm", (PyCFunction)_open_ppm, 1}, - /* Special effects (experimental) */ #ifdef WITH_EFFECTS {"effect_mandelbrot", (PyCFunction)_effect_mandelbrot, 1}, diff --git a/libImaging/File.c b/libImaging/File.c index b669ab665..d67bcabde 100644 --- a/libImaging/File.c +++ b/libImaging/File.c @@ -20,116 +20,6 @@ #include -Imaging -ImagingOpenPPM(const char* infile) -{ - FILE* fp; - int i, c, v; - char* mode; - int x, y, max; - Imaging im; - - if (!infile) - return ImagingError_ValueError(NULL); - - fp = fopen(infile, "rb"); - if (!fp) - return ImagingError_IOError(); - - /* PPM magic */ - if (fgetc(fp) != 'P') - goto error; - switch (fgetc(fp)) { - case '4': /* FIXME: 1-bit images are not yet supported */ - goto error; - case '5': - mode = "L"; - break; - case '6': - mode = "RGB"; - break; - default: - goto error; - } - - i = 0; - c = fgetc(fp); - - x = y = max = 0; - - while (i < 3) { - - /* Ignore optional comment fields */ - while (c == '\n') { - c = fgetc(fp); - if (c == '#') { - do { - c = fgetc(fp); - if (c == EOF) - goto error; - } while (c != '\n'); - c = fgetc(fp); - } - } - - /* Skip forward to next value */ - while (isspace(c)) - c = fgetc(fp); - - /* And parse it */ - v = 0; - while (isdigit(c)) { - v = v * 10 + (c - '0'); - c = fgetc(fp); - } - - if (c == EOF) - goto error; - - switch (i++) { - case 0: - x = v; - break; - case 1: - y = v; - break; - case 2: - max = v; - break; - } - } - - im = ImagingNew(mode, x, y); - if (!im) - return NULL; - - /* if (max != 255) ... FIXME: does anyone ever use this feature? */ - - if (strcmp(im->mode, "L") == 0) { - - /* PPM "L" */ - for (y = 0; y < im->ysize; y++) - if (fread(im->image[y], im->xsize, 1, fp) != 1) - goto error; - - } else { - - /* PPM "RGB" or PyPPM mode */ - for (y = 0; y < im->ysize; y++) - for (x = i = 0; x < im->xsize; x++, i += im->pixelsize) - if (fread(im->image[y]+i, im->bands, 1, fp) != 1) - goto error; - } - - fclose(fp); - - return im; - -error: - fclose(fp); - return ImagingError_IOError(); -} - int ImagingSaveRaw(Imaging im, FILE* fp) From 445451c0b9347b50e0f603db33f196e207de470d Mon Sep 17 00:00:00 2001 From: wiredfool Date: Mon, 3 Oct 2016 03:38:15 -0700 Subject: [PATCH 6/9] Added common check for size tuple errors --- PIL/Image.py | 22 ++++++++++++++++++++++ Tests/test_image.py | 12 ++++++++++++ 2 files changed, 34 insertions(+) diff --git a/PIL/Image.py b/PIL/Image.py index f20640c19..bbfee34b6 100644 --- a/PIL/Image.py +++ b/PIL/Image.py @@ -1985,6 +1985,22 @@ def _wedge(): return Image()._new(core.wedge("L")) +def _check_size(size): + """ + Common check to enforce type and sanity check on size tuples + + :param size: Should be a 2 tuple of (width, height) + :returns: True, or raises a ValueError + """ + + if not isinstance(size, tuple): + raise ValueError("Size must be a tuple") + if len(size) != 2: + raise ValueError("Size must be a tuple of length 2") + if size[0] <= 0 or size[1] <= 0: + raise ValueError("Width and Height must be > 0") + + return True def new(mode, size, color=0): """ @@ -2002,6 +2018,8 @@ def new(mode, size, color=0): :returns: An :py:class:`~PIL.Image.Image` object. """ + _check_size(size) + if color is None: # don't initialize return Image()._new(core.new(mode, size)) @@ -2039,6 +2057,8 @@ def frombytes(mode, size, data, decoder_name="raw", *args): :returns: An :py:class:`~PIL.Image.Image` object. """ + _check_size(size) + # may pass tuple instead of argument list if len(args) == 1 and isinstance(args[0], tuple): args = args[0] @@ -2091,6 +2111,8 @@ def frombuffer(mode, size, data, decoder_name="raw", *args): .. versionadded:: 1.1.4 """ + _check_size(size) + # may pass tuple instead of argument list if len(args) == 1 and isinstance(args[0], tuple): args = args[0] diff --git a/Tests/test_image.py b/Tests/test_image.py index b3fbd508d..1a67d79c8 100644 --- a/Tests/test_image.py +++ b/Tests/test_image.py @@ -237,5 +237,17 @@ class TestImage(PillowTestCase): im3 = Image.open('Tests/images/effect_spread.png') self.assert_image_similar(im2, im3, 110) + def test_check_size(self): + # Checking that the _check_size function throws value errors when we want it to. + with self.assertRaises(ValueError): + Image.new('RGB', 0) # not a tuple + with self.assertRaises(ValueError): + Image.new('RGB', (0,)) # Tuple too short + with self.assertRaises(ValueError): + Image.new('RGB', (0,0)) # w,h <= 0 + + self.assertTrue(Image.new('RGB', (1,1))) + + if __name__ == '__main__': unittest.main() From b3ad80a2bd81e98b3f16c2201749f3e45dd73426 Mon Sep 17 00:00:00 2001 From: wiredfool Date: Mon, 3 Oct 2016 03:50:25 -0700 Subject: [PATCH 7/9] Image.core.open_ppm has been removed. Test the Storage.c fix with an alternate method. Assert that the ordinary opener rejects the negative size in the PPM file --- Tests/test_file_ppm.py | 14 ++++++-------- Tests/test_image.py | 12 ++++++++++++ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/Tests/test_file_ppm.py b/Tests/test_file_ppm.py index 9284d422a..817a62393 100644 --- a/Tests/test_file_ppm.py +++ b/Tests/test_file_ppm.py @@ -45,15 +45,13 @@ class TestFilePpm(PillowTestCase): def test_neg_ppm(self): - """test_neg_ppm - - Storage.c accepted negative values for xsize, ysize. - open_ppm is a core debugging item that doesn't check any parameters for - sanity. - """ + # Storage.c accepted negative values for xsize, ysize. the + # internal open_ppm function didn't check for sanity but it + # has been removed. The default opener doesn't accept negative + # sizes. - with self.assertRaises(ValueError): - Image.core.open_ppm('Tests/images/negative_size.ppm') + with self.assertRaises(IOError): + Image.open('Tests/images/negative_size.ppm') if __name__ == '__main__': diff --git a/Tests/test_image.py b/Tests/test_image.py index 1a67d79c8..8657bd2e6 100644 --- a/Tests/test_image.py +++ b/Tests/test_image.py @@ -247,6 +247,18 @@ class TestImage(PillowTestCase): Image.new('RGB', (0,0)) # w,h <= 0 self.assertTrue(Image.new('RGB', (1,1))) + + def test_storage_neg(self): + # Storage.c accepted negative values for xsize, ysize. Was + # test_neg_ppm, but the core function for that has been + # removed Calling directly into core to test the error in + # Storage.c, rather than the size check above + + with self.assertRaises(ValueError): + Image.core.fill('RGB', (2,-2), (0,0,0)) + + + if __name__ == '__main__': From aa8cfce94c9ba054068baf870f82a0608a5395ae Mon Sep 17 00:00:00 2001 From: wiredfool Date: Mon, 3 Oct 2016 07:33:29 -0700 Subject: [PATCH 8/9] IOError is also a valid error here --- Tests/test_map.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/test_map.py b/Tests/test_map.py index 235bfadbc..d9b66e965 100644 --- a/Tests/test_map.py +++ b/Tests/test_map.py @@ -15,7 +15,7 @@ class TestMap(PillowTestCase): # This image hits the offset test. im = Image.open('Tests/images/l2rgb_read.bmp') - with self.assertRaises((ValueError, MemoryError)): + with self.assertRaises((ValueError, MemoryError, IOError)): im.load() Image.MAX_IMAGE_PIXELS = max_pixels From 22ff3f435879683b9d7896c0a2e15d053205a1ca Mon Sep 17 00:00:00 2001 From: wiredfool Date: Mon, 3 Oct 2016 07:42:48 -0700 Subject: [PATCH 9/9] Vulnerable map function is not called on windows --- PIL/ImageFile.py | 2 +- Tests/test_map.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/PIL/ImageFile.py b/PIL/ImageFile.py index a66452478..55cb701a1 100644 --- a/PIL/ImageFile.py +++ b/PIL/ImageFile.py @@ -154,7 +154,7 @@ class ImageFile(Image.Image): if d == "raw" and a[0] == self.mode and a[0] in Image._MAPMODES: try: if hasattr(Image.core, "map"): - # use built-in mapper + # use built-in mapper WIN32 only self.map = Image.core.map(self.filename) self.map.seek(o) self.im = self.map.readimage( diff --git a/Tests/test_map.py b/Tests/test_map.py index d9b66e965..14bd835a2 100644 --- a/Tests/test_map.py +++ b/Tests/test_map.py @@ -1,7 +1,10 @@ from helper import PillowTestCase, unittest +import sys from PIL import Image + +@unittest.skipIf(sys.platform.startswith('win32'), "Win32 does not call map_buffer") class TestMap(PillowTestCase): def test_overflow(self): # There is the potential to overflow comparisons in map.c