Avoid integer overflow in XGetDeviceProperties() [CVE-2013-1984 7/8]

If the number of items as reported by the Xserver is too large, it
could overflow the calculation for the size of the buffer to copy the
reply into, causing memory corruption.

Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>

1 files changed, 37 insertions, 24 deletions

diff --git a/src/XGetDProp.c b/src/XGetDProp.c
index f9e8f0c..3691122 100644
--- a/src/XGetDProp.c
+++ b/src/XGetDProp.c
@@ -38,6 +38,7 @@ in this Software without prior written authorization from the author.
 #include <X11/extensions/XInput.h>
 #include <X11/extensions/extutil.h>
 #include "XIint.h"
+#include <limits.h>
 
 int
 XGetDeviceProperty(Display* dpy, XDevice* dev,
@@ -48,7 +49,8 @@ XGetDeviceProperty(Display* dpy, XDevice* dev,
 {
     xGetDevicePropertyReq   *req;
     xGetDevicePropertyReply rep;
-    long                    nbytes, rbytes;
+    unsigned long           nbytes, rbytes;
+    int                     ret = Success;
 
     XExtDisplayInfo *info = XInput_find_display(dpy);
 
@@ -81,30 +83,43 @@ XGetDeviceProperty(Display* dpy, XDevice* dev,
 	 * data, but this last byte is null terminated and convenient for
 	 * returning string properties, so the client doesn't then have to
 	 * recopy the string to make it null terminated.
+	 *
+	 * Maximum item limits are set to both prevent integer overflow when
+	 * calculating the amount of memory to malloc, and to limit how much
+	 * memory will be used if a server provides an insanely high count.
 	 */
 	switch (rep.format) {
 	case 8:
-	    nbytes = rep.nItems;
-	    rbytes = rep.nItems + 1;
-	    if (rbytes > 0 &&
-		(*prop = (unsigned char *) Xmalloc ((unsigned)rbytes)))
-		_XReadPad (dpy, (char *) *prop, nbytes);
+	    if (rep.nItems < INT_MAX) {
+		nbytes = rep.nItems;
+		rbytes = rep.nItems + 1;
+		if ((*prop = Xmalloc (rbytes)))
+		    _XReadPad (dpy, (char *) *prop, nbytes);
+		else
+		    ret = BadAlloc;
+	    }
 	    break;
 
 	case 16:
-	    nbytes = rep.nItems << 1;
-	    rbytes = rep.nItems * sizeof (short) + 1;
-	    if (rbytes > 0 &&
-		(*prop = (unsigned char *) Xmalloc ((unsigned)rbytes)))
-		_XRead16Pad (dpy, (short *) *prop, nbytes);
+	    if (rep.nItems < (INT_MAX / sizeof (short))) {
+		nbytes = rep.nItems << 1;
+		rbytes = rep.nItems * sizeof (short) + 1;
+		if ((*prop = Xmalloc (rbytes)))
+		    _XRead16Pad (dpy, (short *) *prop, nbytes);
+		else
+		    ret = BadAlloc;
+	    }
 	    break;
 
 	case 32:
-	    nbytes = rep.nItems << 2;
-	    rbytes = rep.nItems * sizeof (long) + 1;
-	    if (rbytes > 0 &&
-		(*prop = (unsigned char *) Xmalloc ((unsigned)rbytes)))
-		_XRead32 (dpy, (long *) *prop, nbytes);
+	    if (rep.nItems < (INT_MAX / sizeof (long))) {
+		nbytes = rep.nItems << 2;
+		rbytes = rep.nItems * sizeof (long) + 1;
+		if ((*prop = Xmalloc (rbytes)))
+		    _XRead32 (dpy, (long *) *prop, nbytes);
+		else
+		    ret = BadAlloc;
+	    }
 	    break;
 
 	default:
@@ -112,16 +127,13 @@ XGetDeviceProperty(Display* dpy, XDevice* dev,
 	     * This part of the code should never be reached.  If it is,
 	     * the server sent back a property with an invalid format.
 	     */
-	    _XEatDataWords(dpy, rep.length);
-	    UnlockDisplay(dpy);
-	    SyncHandle();
-	    return(BadImplementation);
+	    ret = BadImplementation;
 	}
 	if (! *prop) {
 	    _XEatDataWords(dpy, rep.length);
-	    UnlockDisplay(dpy);
-	    SyncHandle();
-	    return(BadAlloc);
+	    if (ret == Success)
+		ret = BadAlloc;
+	    goto out;
 	}
 	(*prop)[rbytes - 1] = '\0';
     }
@@ -130,9 +142,10 @@ XGetDeviceProperty(Display* dpy, XDevice* dev,
     *actual_format = rep.format;
     *nitems = rep.nItems;
     *bytes_after = rep.bytesAfter;
+  out:
     UnlockDisplay (dpy);
     SyncHandle ();
 
-    return Success;
+    return ret;
 }