backport r66689: imageop could segfault due to poor argument validation

Lib/test/test_imageop.py

@@ -5,9 +5,9 @@
    Roger E. Masse
 """
 
-from test.test_support import verbose, unlink
+from test.test_support import verbose, unlink, run_unittest
 
-import imageop, uu, os
+import imageop, uu, os, unittest
 
 import warnings
 warnings.filterwarnings("ignore",
@@ -15,7 +15,66 @@ warnings.filterwarnings("ignore",
                         DeprecationWarning,
                         ".*test_imageop")
 
-def main(use_rgbimg=1):
+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(use_rgbimg=True):
+    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')
@@ -171,7 +230,3 @@ def get_qualified_path(name):
         if os.path.exists(fullname):
             return fullname
     return name
-
-# rgbimg (unlike imgfile) is portable to platforms other than SGI.
-# So we prefer to use it.
-main(use_rgbimg=1)

Misc/NEWS

@@ -190,6 +190,9 @@ Library
 Extension Modules
 -----------------
 
+- Security Issue #2: imageop did not validate arguments correctly and could
+  segfault as a result.
+
 - Issue 3886: [CVE-2008-2316] Possible integer overflow in the _hashopenssl
   module was closed.
 

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
@@ -90,21 +130,16 @@ imageop_crop(PyObject *self, PyObject *args)
 		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);
@@ -151,16 +186,11 @@ imageop_scale(PyObject *self, PyObject *args)
 		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 )
@@ -202,11 +232,8 @@ 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 )
@@ -253,11 +280,8 @@ imageop_grey2mono(PyObject *self, PyObject *args)
 	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 )
@@ -295,11 +319,8 @@ imageop_grey2grey4(PyObject *self, PyObject *args)
 	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 )
@@ -335,11 +356,8 @@ imageop_grey2grey2(PyObject *self, PyObject *args)
 	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 )
@@ -374,11 +392,8 @@ imageop_dither2mono(PyObject *self, PyObject *args)
 	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 )
@@ -421,11 +436,8 @@ imageop_dither2grey2(PyObject *self, PyObject *args)
 	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 )
@@ -462,10 +474,8 @@ imageop_mono2grey(PyObject *self, PyObject *args)
 		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;
@@ -503,8 +513,7 @@ imageop_grey22grey(PyObject *self, PyObject *args)
 		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 ) {
@@ -543,10 +552,8 @@ imageop_grey42grey(PyObject *self, PyObject *args)
 		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;
@@ -583,15 +590,11 @@ imageop_rgb2rgb8(PyObject *self, PyObject *args)
 	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 )
@@ -631,27 +634,18 @@ imageop_rgb82rgb(PyObject *self, PyObject *args)
 	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++ ) {
 		/* Bits in source: RRRBBGGG
 		** Red and Green are multiplied by 36.5, Blue by 85
 		*/
@@ -690,15 +684,11 @@ imageop_rgb2grey(PyObject *self, PyObject *args)
 	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 )
@@ -739,27 +729,18 @@ imageop_grey2rgb(PyObject *self, PyObject *args)
 	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,6 +779,7 @@ PyMODINIT_FUNC
 initimageop(void)
 {
 	PyObject *m;
+
 	m = Py_InitModule("imageop", imageop_methods);
 	if (m == NULL)
 		return;