Merge pull request #2146 from wiredfool/map_vuln

Fixes for #2105

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)
 ------------------
 

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):

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]

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(

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)

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)

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
-
 
 #
 # --------------------------------------------------------------------

Tests/test_file_ppm.py

@@ -44,5 +44,15 @@ class TestFilePpm(PillowTestCase):
         self.assertRaises(ValueError, lambda: Image.open(path))
 
 
+    def test_neg_ppm(self):
+        # 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(IOError):
+            Image.open('Tests/images/negative_size.ppm')
+
+
 if __name__ == '__main__':
     unittest.main()

Tests/test_image.py

@@ -237,5 +237,29 @@ 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)))
+
+    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__':
     unittest.main()

Tests/test_map.py

@@ -0,0 +1,28 @@
+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
+        # if there are > 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, IOError)):
+            im.load()
+
+        Image.MAX_IMAGE_PIXELS = max_pixels
+
+
+if __name__ == '__main__':
+    unittest.main()

_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},

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.
+
+
+
+

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

libImaging/File.c

@@ -20,116 +20,6 @@
 
 #include <ctype.h>
 
-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)

libImaging/Storage.c

@@ -406,6 +406,10 @@ ImagingNew(const char* mode, int xsize, int ysize)
     } else
         bytes = strlen(mode); /* close enough */
 
+    if (xsize < 0 || ysize < 0) {
+        return (Imaging) ImagingError_ValueError("bad image size");
+    }
+
     if ((int64_t) xsize * (int64_t) ysize <= THRESHOLD / bytes) {
         im = ImagingNewBlock(mode, xsize, ysize);
         if (im)

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;