fix security issue 2: imageop's poor validation of arguments could result in segfaults

patch by Victor Stinner
reviewed by myself and Brett

diff --git a/Lib/test/test_imageop.py b/Lib/test/test_imageop.py
index 8cd2dc11bd87339510109f74f95c08751e6a8ac3..6deaa348cb845aca801f38d84af91358b20ad9b1 100755 (executable)
--- a/Lib/test/test_imageop.py
+++ b/Lib/test/test_imageop.py
@@ -5,13 +5,74 @@
    Roger E. Masse
 """
 
-from test.test_support import verbose, unlink, import_module
+from test.test_support import verbose, unlink, import_module, run_unittest
 
 imageop = import_module('imageop', deprecated=True)
-import uu, os, imgfile
+import uu, os, unittest
+
+
+SIZES = (1, 2, 3, 4)
+_VALUES = (1, 2, 2**10, 2**15-1, 2**15, 2**15+1, 2**31-2, 2**31-1)
+VALUES = tuple( -x for x in reversed(_VALUES) ) + (0,) + _VALUES
+AAAAA = "A" * 1024
+
+
+class InputValidationTests(unittest.TestCase):
+
+    def _check(self, name, size=None, *extra):
+        func = getattr(imageop, name)
+        for height in VALUES:
+            for width in VALUES:
+                strlen = abs(width * height)
+                if size:
+                    strlen *= size
+                if strlen < 1024:
+                    data = "A" * strlen
+                else:
+                    data = AAAAA
+                if size:
+                    arguments = (data, size, width, height) + extra
+                else:
+                    arguments = (data, width, height) + extra
+                try:
+                    func(*arguments)
+                except (ValueError, imageop.error):
+                    pass
+
+    def check_size(self, name, *extra):
+        for size in SIZES:
+            self._check(name, size, *extra)
+
+    def check(self, name, *extra):
+        self._check(name, None, *extra)
+
+    def test_input_validation(self):
+        self.check_size("crop", 0, 0, 0, 0)
+        self.check_size("scale", 1, 0)
+        self.check_size("scale", -1, -1)
+        self.check_size("tovideo")
+        self.check("grey2mono", 128)
+        self.check("grey2grey4")
+        self.check("grey2grey2")
+        self.check("dither2mono")
+        self.check("dither2grey2")
+        self.check("mono2grey", 0, 0)
+        self.check("grey22grey")
+        self.check("rgb2rgb8") # nlen*4 == len
+        self.check("rgb82rgb")
+        self.check("rgb2grey")
+        self.check("grey2rgb")
+
 
 def test_main():
 
+    run_unittest(InputValidationTests)
+
+    try:
+        import imgfile
+    except ImportError:
+        return
+
     # Create binary test files
     uu.decode(get_qualified_path('testrgb'+os.extsep+'uue'), 'test'+os.extsep+'rgb')
 

diff --git a/Misc/NEWS b/Misc/NEWS
index bbbeb91da1128d30317da661da4850df813f7d57..1b08d76bd968832be40ce05a2a876d497ee57721 100644 (file)
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -54,6 +54,9 @@ Core and Builtins
 Extension Modules
 -----------------
 
+- Security Issue #2: imageop did not validate arguments correctly and could
+  segfault as a result.
+
 - Issue #3886: Possible integer overflows in the _hashopenssl module were
   closed.
 

diff --git a/Modules/imageop.c b/Modules/imageop.c
index a2b69b6cdf3220d7e64b76173df29843489f2d64..61a0c2f813775f7c358444abebe92384c0bb4119 100644 (file)
--- a/Modules/imageop.c
+++ b/Modules/imageop.c
@@ -26,6 +26,46 @@ typedef unsigned long Py_UInt32;
 static PyObject *ImageopError;
 static PyObject *ImageopDict;
 
+/**
+ * Check a coordonnate, make sure that (0 < value).
+ * Return 0 on error.
+ */
+static int
+check_coordonnate(int value, const char* name)
+{
+       if ( 0 < value)
+               return 1;
+       PyErr_Format(PyExc_ValueError, "%s value is negative or nul", name);
+       return 0;
+}
+
+/**
+ * Check integer overflow to make sure that product == x*y*size.
+ * Return 0 on error.
+ */
+static int
+check_multiply_size(int product, int x, const char* xname, int y, const char* yname, int size)
+{
+       if ( !check_coordonnate(x, xname) )
+               return 0;
+       if ( !check_coordonnate(y, yname) )
+               return 0;
+       if ( size == (product / y) / x )
+               return 1;
+       PyErr_SetString(ImageopError, "String has incorrect length");
+       return 0;
+}
+
+/**
+ * Check integer overflow to make sure that product == x*y.
+ * Return 0 on error.
+ */
+static int
+check_multiply(int product, int x, int y)
+{
+       return check_multiply_size(product, x, "x", y, "y", 1);
+}
+
 /* If this function returns true (the default if anything goes wrong), we're
    behaving in a backward-compatible way with respect to how multi-byte pixels
    are stored in the strings.  The code in this module was originally written
@@ -85,26 +125,21 @@ imageop_crop(PyObject *self, PyObject *args)
        if ( !PyArg_ParseTuple(args, "s#iiiiiii", &cp, &len, &size, &x, &y,
                          &newx1, &newy1, &newx2, &newy2) )
                return 0;
-    
+
        if ( size != 1 && size != 2 && size != 4 ) {
                PyErr_SetString(ImageopError, "Size should be 1, 2 or 4");
                return 0;
        }
-       if (( len != size*x*y ) ||
-            ( size != ((len / x) / y) )) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply_size(len, x, "x", y, "y", size) )
                return 0;
-       }
+
        xstep = (newx1 < newx2)? 1 : -1;
        ystep = (newy1 < newy2)? 1 : -1;
-    
+
         nlen = (abs(newx2-newx1)+1)*(abs(newy2-newy1)+1)*size;
-        if ( size != ((nlen / (abs(newx2-newx1)+1)) / (abs(newy2-newy1)+1)) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply_size(nlen, abs(newx2-newx1)+1, "abs(newx2-newx1)+1", abs(newy2-newy1)+1, "abs(newy2-newy1)+1", size) )
                return 0;
-       }
-       rv = PyString_FromStringAndSize(NULL,
-                            (abs(newx2-newx1)+1)*(abs(newy2-newy1)+1)*size);
+       rv = PyString_FromStringAndSize(NULL, nlen);
        if ( rv == 0 )
                return 0;
        ncp = (char *)PyString_AsString(rv);
@@ -131,7 +166,7 @@ imageop_crop(PyObject *self, PyObject *args)
        }
        return rv;
 }
- 
+
 static PyObject *
 imageop_scale(PyObject *self, PyObject *args)
 {
@@ -146,22 +181,17 @@ imageop_scale(PyObject *self, PyObject *args)
        if ( !PyArg_ParseTuple(args, "s#iiiii",
                          &cp, &len, &size, &x, &y, &newx, &newy) )
                return 0;
-    
+
        if ( size != 1 && size != 2 && size != 4 ) {
                PyErr_SetString(ImageopError, "Size should be 1, 2 or 4");
                return 0;
        }
-       if ( ( len != size*x*y ) ||
-             ( size != ((len / x) / y) ) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply_size(len, x, "x", y, "y", size) )
                return 0;
-       }
         nlen = newx*newy*size;
-       if ( size != ((nlen / newx) / newy) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply_size(nlen, newx, "newx", newy, "newy", size) )
                return 0;
-       }
-    
+
        rv = PyString_FromStringAndSize(NULL, nlen);
        if ( rv == 0 )
                return 0;
@@ -193,8 +223,8 @@ imageop_tovideo(PyObject *self, PyObject *args)
        unsigned char *cp, *ncp;
        int width;
        PyObject *rv;
-   
-    
+
+
        if ( !PyArg_ParseTuple(args, "s#iii", &cp, &len, &width, &maxx, &maxy) )
                return 0;
 
@@ -202,12 +232,9 @@ imageop_tovideo(PyObject *self, PyObject *args)
                PyErr_SetString(ImageopError, "Size should be 1 or 4");
                return 0;
        }
-       if ( ( maxx*maxy*width != len ) ||
-             ( maxx != ((len / maxy) / width) ) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply_size(len, maxx, "max", maxy, "maxy", width) )
                return 0;
-       }
-    
+
        rv = PyString_FromStringAndSize(NULL, len);
        if ( rv == 0 )
                return 0;
@@ -248,17 +275,14 @@ imageop_grey2mono(PyObject *self, PyObject *args)
        unsigned char ovalue;
        PyObject *rv;
        int i, bit;
-   
-    
+
+
        if ( !PyArg_ParseTuple(args, "s#iii", &cp, &len, &x, &y, &tres) )
                return 0;
 
-       if ( ( x*y != len ) ||
-             ( x != len / y ) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply(len, x, y) )
                return 0;
-       }
-    
+
        rv = PyString_FromStringAndSize(NULL, (len+7)/8);
        if ( rv == 0 )
                return 0;
@@ -290,17 +314,14 @@ imageop_grey2grey4(PyObject *self, PyObject *args)
        PyObject *rv;
        int i;
        int pos;
-   
-    
+
+
        if ( !PyArg_ParseTuple(args, "s#ii", &cp, &len, &x, &y) )
                return 0;
 
-       if ( ( x*y != len ) ||
-             ( x != len / y ) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply(len, x, y) )
                return 0;
-       }
-    
+
        rv = PyString_FromStringAndSize(NULL, (len+1)/2);
        if ( rv == 0 )
                return 0;
@@ -330,17 +351,14 @@ imageop_grey2grey2(PyObject *self, PyObject *args)
        PyObject *rv;
        int i;
        int pos;
-   
-    
+
+
        if ( !PyArg_ParseTuple(args, "s#ii", &cp, &len, &x, &y) )
                return 0;
 
-       if ( ( x*y != len ) ||
-             ( x != len / y ) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply(len, x, y) )
                return 0;
-       }
-    
+
        rv = PyString_FromStringAndSize(NULL, (len+3)/4);
        if ( rv == 0 )
                return 0;
@@ -369,17 +387,14 @@ imageop_dither2mono(PyObject *self, PyObject *args)
        unsigned char ovalue;
        PyObject *rv;
        int i, bit;
-   
-    
+
+
        if ( !PyArg_ParseTuple(args, "s#ii", &cp, &len, &x, &y) )
                return 0;
 
-       if ( ( x*y != len ) ||
-             ( x != len / y ) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply(len, x, y) )
                return 0;
-       }
-    
+
        rv = PyString_FromStringAndSize(NULL, (len+7)/8);
        if ( rv == 0 )
                return 0;
@@ -416,17 +431,14 @@ imageop_dither2grey2(PyObject *self, PyObject *args)
        int i;
        int pos;
        int sum = 0, nvalue;
-   
-    
+
+
        if ( !PyArg_ParseTuple(args, "s#ii", &cp, &len, &x, &y) )
                return 0;
 
-       if ( ( x*y != len ) ||
-             ( x != len / y ) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply(len, x, y) )
                return 0;
-       }
-    
+
        rv = PyString_FromStringAndSize(NULL, (len+3)/4);
        if ( rv == 0 )
                return 0;
@@ -457,20 +469,18 @@ imageop_mono2grey(PyObject *self, PyObject *args)
        unsigned char *cp, *ncp;
        PyObject *rv;
        int i, bit;
-    
+
        if ( !PyArg_ParseTuple(args, "s#iiii", &cp, &len, &x, &y, &v0, &v1) )
                return 0;
 
         nlen = x*y;
-       if ( x != (nlen / y) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply(nlen, x, y) )
                return 0;
-       }
        if ( (nlen+7)/8 != len ) {
                PyErr_SetString(ImageopError, "String has incorrect length");
                return 0;
        }
-    
+
        rv = PyString_FromStringAndSize(NULL, nlen);
        if ( rv == 0 )
                return 0;
@@ -498,20 +508,19 @@ imageop_grey22grey(PyObject *self, PyObject *args)
        unsigned char *cp, *ncp;
        PyObject *rv;
        int i, pos, value = 0, nvalue;
-    
+
        if ( !PyArg_ParseTuple(args, "s#ii", &cp, &len, &x, &y) )
                return 0;
 
        nlen = x*y;
-       if ( x != (nlen / y) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply(nlen, x, y) ) {
                return 0;
        }
        if ( (nlen+3)/4 != len ) {
                PyErr_SetString(ImageopError, "String has incorrect length");
                return 0;
        }
-    
+
        rv = PyString_FromStringAndSize(NULL, nlen);
        if ( rv == 0 )
                return 0;
@@ -538,20 +547,18 @@ imageop_grey42grey(PyObject *self, PyObject *args)
        unsigned char *cp, *ncp;
        PyObject *rv;
        int i, pos, value = 0, nvalue;
-    
+
        if ( !PyArg_ParseTuple(args, "s#ii", &cp, &len, &x, &y) )
                return 0;
 
        nlen = x*y;
-       if ( x != (nlen / y) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply(nlen, x, y) )
                return 0;
-       }
        if ( (nlen+1)/2 != len ) {
                PyErr_SetString(ImageopError, "String has incorrect length");
                return 0;
        }
-    
+
        rv = PyString_FromStringAndSize(NULL, nlen);
        if ( rv == 0 )
                return 0;
@@ -579,20 +586,16 @@ imageop_rgb2rgb8(PyObject *self, PyObject *args)
        PyObject *rv;
        int i, r, g, b;
        int backward_compatible = imageop_backward_compatible();
-    
+
        if ( !PyArg_ParseTuple(args, "s#ii", &cp, &len, &x, &y) )
                return 0;
 
-       nlen = x*y;
-       if ( x != (nlen / y) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply_size(len*4, x, "x", y, "y", 4) )
                return 0;
-       }
-       if ( nlen*4 != len ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       nlen = x*y;
+       if ( !check_multiply(nlen, x, y) )
                return 0;
-       }
-    
+
        rv = PyString_FromStringAndSize(NULL, nlen);
        if ( rv == 0 )
                return 0;
@@ -627,31 +630,22 @@ imageop_rgb82rgb(PyObject *self, PyObject *args)
        int i, r, g, b;
        unsigned char value;
        int backward_compatible = imageop_backward_compatible();
-    
+
        if ( !PyArg_ParseTuple(args, "s#ii", &cp, &len, &x, &y) )
                return 0;
 
-       nlen = x*y;
-       if ( x != (nlen / y) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
-               return 0;
-       }
-       if ( nlen != len ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply(len, x, y) )
                return 0;
-       }
-       
-       if ( nlen / x != y || nlen > INT_MAX / 4) {
-               PyErr_SetString(ImageopError, "Image is too large");
+       nlen = x*y*4;
+       if ( !check_multiply_size(nlen, x, "x", y, "y", 4) )
                return 0;
-       }
-    
-       rv = PyString_FromStringAndSize(NULL, nlen*4);
+
+       rv = PyString_FromStringAndSize(NULL, nlen);
        if ( rv == 0 )
                return 0;
        ncp = (unsigned char *)PyString_AsString(rv);
 
-       for ( i=0; i < nlen; i++ ) {
+       for ( i=0; i < len; i++ ) {
                /* Bits in source: RRRBBGGG
                ** Red and Green are multiplied by 36.5, Blue by 85
                */
@@ -686,20 +680,16 @@ imageop_rgb2grey(PyObject *self, PyObject *args)
        int i, r, g, b;
        int nvalue;
        int backward_compatible = imageop_backward_compatible();
-    
+
        if ( !PyArg_ParseTuple(args, "s#ii", &cp, &len, &x, &y) )
                return 0;
 
-       nlen = x*y;
-       if ( x != (nlen / y) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply_size(len, x, "x", y, "y", 4) )
                return 0;
-       }
-       if ( nlen*4 != len ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       nlen = x*y;
+       if ( !check_multiply(nlen, x, y) )
                return 0;
-       }
-    
+
        rv = PyString_FromStringAndSize(NULL, nlen);
        if ( rv == 0 )
                return 0;
@@ -735,31 +725,22 @@ imageop_grey2rgb(PyObject *self, PyObject *args)
        int i;
        unsigned char value;
        int backward_compatible = imageop_backward_compatible();
-    
+
        if ( !PyArg_ParseTuple(args, "s#ii", &cp, &len, &x, &y) )
                return 0;
 
-       nlen = x*y;
-       if ( x != (nlen / y) ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+       if ( !check_multiply(len, x, y) )
                return 0;
-       }
-       if ( nlen != len ) {
-               PyErr_SetString(ImageopError, "String has incorrect length");
+        nlen = x*y*4;
+       if ( !check_multiply_size(nlen, x, "x", y, "y", 4) )
                return 0;
-       }
-       
-       if ( nlen / x != y || nlen > INT_MAX / 4) {
-               PyErr_SetString(ImageopError, "Image is too large");
-               return 0;
-       }
-    
-       rv = PyString_FromStringAndSize(NULL, nlen*4);
+
+       rv = PyString_FromStringAndSize(NULL, nlen);
        if ( rv == 0 )
                return 0;
        ncp = (unsigned char *)PyString_AsString(rv);
 
-       for ( i=0; i < nlen; i++ ) {
+       for ( i=0; i < len; i++ ) {
                value = *cp++;
                if (backward_compatible) {
                        * (Py_UInt32 *) ncp = (Py_UInt32) value | ((Py_UInt32) value << 8 ) | ((Py_UInt32) value << 16);
@@ -774,39 +755,6 @@ imageop_grey2rgb(PyObject *self, PyObject *args)
        return rv;
 }
 
-/*
-static object *
-imageop_mul(object *self, object *args)
-{
-       char *cp, *ncp;
-       int len, size, x, y;
-       object *rv;
-       int i;
-
-       if ( !getargs(args, "(s#iii)", &cp, &len, &size, &x, &y) )
-               return 0;
-    
-       if ( size != 1 && size != 4 ) {
-               err_setstr(ImageopError, "Size should be 1 or 4");
-               return 0;
-       }
-       if ( len != size*x*y ) {
-               err_setstr(ImageopError, "String has incorrect length");
-               return 0;
-       }
-    
-       rv = newsizedstringobject(NULL, XXXX);
-       if ( rv == 0 )
-               return 0;
-       ncp = (char *)getstringvalue(rv);
-    
-    
-       for ( i=0; i < len; i += size ) {
-       }
-       return rv;
-}
-*/
-
 static PyMethodDef imageop_methods[] = {
        { "crop",               imageop_crop, METH_VARARGS },
        { "scale",              imageop_scale, METH_VARARGS },
@@ -831,11 +779,11 @@ PyMODINIT_FUNC
 initimageop(void)
 {
        PyObject *m;
-       
+
        if (PyErr_WarnPy3k("the imageop module has been removed in "
                           "Python 3.0", 2) < 0)
            return;
-       
+
        m = Py_InitModule("imageop", imageop_methods);
        if (m == NULL)
                return;