integer overflow in XcupStoreColors() [CVE-2013-1982 2/6]

If the computed number of entries is large enough that it overflows when
multiplied by the size of a xColorItem struct, or is treated as negative
when compared to the size of the stack allocated buffer, then memory
corruption can occur when more bytes are read from the X server than the
size of the buffer we allocated to hold them.

The requirement to match the number of colors specified by the caller makes
this much harder to hit than the one in XcupGetReservedColormapEntries()

Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>

1 files changed, 11 insertions, 14 deletions

diff --git a/src/Xcup.c b/src/Xcup.c
index 670f356..cdc64c2 100644
--- a/src/Xcup.c
+++ b/src/Xcup.c
@@ -219,24 +219,21 @@ XcupStoreColors(
     }
 
     if (_XReply(dpy, (xReply *)&rep, 0, xFalse)) {
-	long nbytes;
+	unsigned long nbytes;
 	xColorItem* rbufp;
 	xColorItem* cs;
-	int nentries = rep.length / 3;
-
-	nbytes = nentries * SIZEOF (xColorItem);
+	unsigned int nentries = rep.length / 3;
 
-	if (nentries != ncolors) {
-	    _XEatDataWords(dpy, rep.length);
-	    UnlockDisplay (dpy);
-	    SyncHandle ();
-	    return False;
-	}
+	if ((nentries == ncolors) &&
+	    (nentries < (INT_MAX / SIZEOF (xColorItem)))) {
+	    nbytes = nentries * SIZEOF (xColorItem);
 
-	if (ncolors > 256)
-	    rbufp = (xColorItem*) Xmalloc (nbytes);
-	else
-	    rbufp = rbuf;
+	    if (ncolors > 256)
+		rbufp = Xmalloc (nbytes);
+	    else
+		rbufp = rbuf;
+	} else
+	    rbufp = NULL;
 
 	if (rbufp == NULL) {
 	    _XEatDataWords(dpy, rep.length);