From a0df3e1c7728205e5c7650b2e6dce684139254a6 Mon Sep 17 00:00:00 2001
From: Tobias Stoeckmann <tobias@stoeckmann.org>
Date: Sun, 25 Sep 2016 22:21:40 +0200
Subject: Avoid out of boundary accesses on illegal responses

The responses of the connected X server have to be properly checked
to avoid out of boundary accesses that could otherwise be triggered
by a malicious server.

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Reviewed-by: Matthieu Herrb <matthieu@herrb.eu>
---
 src/XrrConfig.c   | 32 +++++++++++++--------
 src/XrrCrtc.c     | 83 ++++++++++++++++++++++++++++++++++++++++++-------------
 src/XrrOutput.c   | 11 ++++++++
 src/XrrProvider.c | 28 ++++++++++++++++---
 src/XrrScreen.c   | 52 ++++++++++++++++++++++------------
 5 files changed, 154 insertions(+), 52 deletions(-)

diff --git a/src/XrrConfig.c b/src/XrrConfig.c
index 2f0282b..e68c45a 100644
--- a/src/XrrConfig.c
+++ b/src/XrrConfig.c
@@ -29,6 +29,7 @@
 #include <config.h>
 #endif
 
+#include <limits.h>
 #include <stdio.h>
 #include <X11/Xlib.h>
 /* we need to be able to manipulate the Display structure on events */
@@ -272,23 +273,30 @@ static XRRScreenConfiguration *_XRRGetScreenInfo (Display *dpy,
 	rep.rate = 0;
 	rep.nrateEnts = 0;
     }
+    if (rep.length < INT_MAX >> 2) {
+	nbytes = (long) rep.length << 2;
 
-    nbytes = (long) rep.length << 2;
+	nbytesRead = (long) (rep.nSizes * SIZEOF (xScreenSizes) +
+			    ((rep.nrateEnts + 1)& ~1) * 2 /* SIZEOF(CARD16) */);
 
-    nbytesRead = (long) (rep.nSizes * SIZEOF (xScreenSizes) +
-			 ((rep.nrateEnts + 1)& ~1) * 2 /* SIZEOF (CARD16) */);
+	/*
+	 * first we must compute how much space to allocate for
+	 * randr library's use; we'll allocate the structures in a single
+	 * allocation, on cleanlyness grounds.
+	 */
 
-    /*
-     * first we must compute how much space to allocate for
-     * randr library's use; we'll allocate the structures in a single
-     * allocation, on cleanlyness grounds.
-     */
+	rbytes = sizeof (XRRScreenConfiguration) +
+	  (rep.nSizes * sizeof (XRRScreenSize) +
+	   rep.nrateEnts * sizeof (int));
 
-    rbytes = sizeof (XRRScreenConfiguration) +
-      (rep.nSizes * sizeof (XRRScreenSize) +
-       rep.nrateEnts * sizeof (int));
+	scp = (struct _XRRScreenConfiguration *) Xmalloc(rbytes);
+    } else {
+	nbytes = 0;
+	nbytesRead = 0;
+	rbytes = 0;
+	scp = NULL;
+    }
 
-    scp = (struct _XRRScreenConfiguration *) Xmalloc(rbytes);
     if (scp == NULL) {
 	_XEatData (dpy, (unsigned long) nbytes);
 	return NULL;
diff --git a/src/XrrCrtc.c b/src/XrrCrtc.c
index 5ae35c5..6665092 100644
--- a/src/XrrCrtc.c
+++ b/src/XrrCrtc.c
@@ -24,6 +24,7 @@
 #include <config.h>
 #endif
 
+#include <limits.h>
 #include <stdio.h>
 #include <X11/Xlib.h>
 /* we need to be able to manipulate the Display structure on events */
@@ -57,22 +58,33 @@ XRRGetCrtcInfo (Display *dpy, XRRScreenResources *resources, RRCrtc crtc)
 	return NULL;
     }
 
-    nbytes = (long) rep.length << 2;
+    if (rep.length < INT_MAX >> 2)
+    {
+	nbytes = (long) rep.length << 2;
 
-    nbytesRead = (long) (rep.nOutput * 4 +
-			 rep.nPossibleOutput * 4);
+	nbytesRead = (long) (rep.nOutput * 4 +
+			     rep.nPossibleOutput * 4);
 
-    /*
-     * first we must compute how much space to allocate for
-     * randr library's use; we'll allocate the structures in a single
-     * allocation, on cleanlyness grounds.
-     */
+	/*
+	 * first we must compute how much space to allocate for
+	 * randr library's use; we'll allocate the structures in a single
+	 * allocation, on cleanlyness grounds.
+	 */
 
-    rbytes = (sizeof (XRRCrtcInfo) +
-	      rep.nOutput * sizeof (RROutput) +
-	      rep.nPossibleOutput * sizeof (RROutput));
+	rbytes = (sizeof (XRRCrtcInfo) +
+		  rep.nOutput * sizeof (RROutput) +
+		  rep.nPossibleOutput * sizeof (RROutput));
+
+	xci = (XRRCrtcInfo *) Xmalloc(rbytes);
+    }
+    else
+    {
+	nbytes = 0;
+	nbytesRead = 0;
+	rbytes = 0;
+	xci = NULL;
+    }
 
-    xci = (XRRCrtcInfo *) Xmalloc(rbytes);
     if (xci == NULL) {
 	_XEatDataWords (dpy, rep.length);
 	UnlockDisplay (dpy);
@@ -194,12 +206,21 @@ XRRGetCrtcGamma (Display *dpy, RRCrtc crtc)
     if (!_XReply (dpy, (xReply *) &rep, 0, xFalse))
 	goto out;
 
-    nbytes = (long) rep.length << 2;
+    if (rep.length < INT_MAX >> 2)
+    {
+	nbytes = (long) rep.length << 2;
 
-    /* three channels of CARD16 data */
-    nbytesRead = (rep.size * 2 * 3);
+	/* three channels of CARD16 data */
+	nbytesRead = (rep.size * 2 * 3);
 
-    crtc_gamma = XRRAllocGamma (rep.size);
+	crtc_gamma = XRRAllocGamma (rep.size);
+    }
+    else
+    {
+	nbytes = 0;
+	nbytesRead = 0;
+	crtc_gamma = NULL;
+    }
 
     if (!crtc_gamma)
     {
@@ -357,7 +378,7 @@ XRRGetCrtcTransform (Display	*dpy,
     xRRGetCrtcTransformReq	*req;
     int				major_version, minor_version;
     XRRCrtcTransformAttributes	*attr;
-    char			*extra = NULL, *e;
+    char			*extra = NULL, *end = NULL, *e;
     int				p;
 
     *attributes = NULL;
@@ -395,9 +416,17 @@ XRRGetCrtcTransform (Display	*dpy,
 	else
 	{
 	    int extraBytes = rep.length * 4 - CrtcTransformExtra;
-	    extra = Xmalloc (extraBytes);
+	    if (rep.length < INT_MAX / 4 &&
+		rep.length * 4 >= CrtcTransformExtra) {
+		extra = Xmalloc (extraBytes);
+		end = extra + extraBytes;
+	    } else
+		extra = NULL;
 	    if (!extra) {
-		_XEatDataWords (dpy, rep.length - (CrtcTransformExtra >> 2));
+		if (rep.length > (CrtcTransformExtra >> 2))
+		    _XEatDataWords (dpy, rep.length - (CrtcTransformExtra >> 2));
+		else
+		    _XEatDataWords (dpy, rep.length);
 		UnlockDisplay (dpy);
 		SyncHandle ();
 		return False;
@@ -429,22 +458,38 @@ XRRGetCrtcTransform (Display	*dpy,
 
     e = extra;
 
+    if (e + rep.pendingNbytesFilter > end) {
+	XFree (extra);
+	return False;
+    }
     memcpy (attr->pendingFilter, e, rep.pendingNbytesFilter);
     attr->pendingFilter[rep.pendingNbytesFilter] = '\0';
     e += (rep.pendingNbytesFilter + 3) & ~3;
     for (p = 0; p < rep.pendingNparamsFilter; p++) {
 	INT32	f;
+	if (e + 4 > end) {
+	    XFree (extra);
+	    return False;
+	}
 	memcpy (&f, e, 4);
 	e += 4;
 	attr->pendingParams[p] = (XFixed) f;
     }
     attr->pendingNparams = rep.pendingNparamsFilter;
 
+    if (e + rep.currentNbytesFilter > end) {
+	XFree (extra);
+	return False;
+    }
     memcpy (attr->currentFilter, e, rep.currentNbytesFilter);
     attr->currentFilter[rep.currentNbytesFilter] = '\0';
     e += (rep.currentNbytesFilter + 3) & ~3;
     for (p = 0; p < rep.currentNparamsFilter; p++) {
 	INT32	f;
+	if (e + 4 > end) {
+	    XFree (extra);
+	    return False;
+	}
 	memcpy (&f, e, 4);
 	e += 4;
 	attr->currentParams[p] = (XFixed) f;
diff --git a/src/XrrOutput.c b/src/XrrOutput.c
index 85f0b6e..30f3d40 100644
--- a/src/XrrOutput.c
+++ b/src/XrrOutput.c
@@ -25,6 +25,7 @@
 #include <config.h>
 #endif
 
+#include <limits.h>
 #include <stdio.h>
 #include <X11/Xlib.h>
 /* we need to be able to manipulate the Display structure on events */
@@ -60,6 +61,16 @@ XRRGetOutputInfo (Display *dpy, XRRScreenResources *resources, RROutput output)
 	return NULL;
     }
 
+    if (rep.length > INT_MAX >> 2 || rep.length < (OutputInfoExtra >> 2))
+    {
+        if (rep.length > (OutputInfoExtra >> 2))
+	    _XEatDataWords (dpy, rep.length - (OutputInfoExtra >> 2));
+	else
+	    _XEatDataWords (dpy, rep.length);
+	UnlockDisplay (dpy);
+	SyncHandle ();
+	return NULL;
+    }
     nbytes = ((long) (rep.length) << 2) - OutputInfoExtra;
 
     nbytesRead = (long) (rep.nCrtcs * 4 +
diff --git a/src/XrrProvider.c b/src/XrrProvider.c
index 9e620c7..d796cd0 100644
--- a/src/XrrProvider.c
+++ b/src/XrrProvider.c
@@ -25,6 +25,7 @@
 #include <config.h>
 #endif
 
+#include <limits.h>
 #include <stdio.h>
 #include <X11/Xlib.h>
 /* we need to be able to manipulate the Display structure on events */
@@ -59,12 +60,20 @@ XRRGetProviderResources(Display *dpy, Window window)
       return NULL;
     }
 
-    nbytes = (long) rep.length << 2;
+    if (rep.length < INT_MAX >> 2) {
+	nbytes = (long) rep.length << 2;
 
-    nbytesRead = (long) (rep.nProviders * 4);
+	nbytesRead = (long) (rep.nProviders * 4);
 
-    rbytes = (sizeof(XRRProviderResources) + rep.nProviders * sizeof(RRProvider));
-    xrpr = (XRRProviderResources *) Xmalloc(rbytes);
+	rbytes = (sizeof(XRRProviderResources) + rep.nProviders *
+		  sizeof(RRProvider));
+	xrpr = (XRRProviderResources *) Xmalloc(rbytes);
+    } else {
+	nbytes = 0;
+	nbytesRead = 0;
+	rbytes = 0;
+	xrpr = NULL;
+    }
 
     if (xrpr == NULL) {
        _XEatDataWords (dpy, rep.length);
@@ -121,6 +130,17 @@ XRRGetProviderInfo(Display *dpy, XRRScreenResources *resources, RRProvider provi
 	return NULL;
     }
 
+    if (rep.length > INT_MAX >> 2 || rep.length < ProviderInfoExtra >> 2)
+    {
+	if (rep.length < ProviderInfoExtra >> 2)
+	    _XEatDataWords (dpy, rep.length);
+	else
+	    _XEatDataWords (dpy, rep.length - (ProviderInfoExtra >> 2));
+	UnlockDisplay (dpy);
+	SyncHandle ();
+	return NULL;
+    }
+
     nbytes = ((long) rep.length << 2) - ProviderInfoExtra;
 
     nbytesRead = (long)(rep.nCrtcs * 4 +
diff --git a/src/XrrScreen.c b/src/XrrScreen.c
index b8ce7e5..1f7ffe6 100644
--- a/src/XrrScreen.c
+++ b/src/XrrScreen.c
@@ -24,6 +24,7 @@
 #include <config.h>
 #endif
 
+#include <limits.h>
 #include <stdio.h>
 #include <X11/Xlib.h>
 /* we need to be able to manipulate the Display structure on events */
@@ -105,27 +106,36 @@ doGetScreenResources (Display *dpy, Window window, int poll)
 	xrri->has_rates = _XRRHasRates (xrri->minor_version, xrri->major_version);
     }
 
-    nbytes = (long) rep.length << 2;
+    if (rep.length < INT_MAX >> 2) {
+	nbytes = (long) rep.length << 2;
 
-    nbytesRead = (long) (rep.nCrtcs * 4 +
-			 rep.nOutputs * 4 +
-			 rep.nModes * SIZEOF (xRRModeInfo) +
-			 ((rep.nbytesNames + 3) & ~3));
+	nbytesRead = (long) (rep.nCrtcs * 4 +
+			     rep.nOutputs * 4 +
+			     rep.nModes * SIZEOF (xRRModeInfo) +
+			     ((rep.nbytesNames + 3) & ~3));
 
-    /*
-     * first we must compute how much space to allocate for
-     * randr library's use; we'll allocate the structures in a single
-     * allocation, on cleanlyness grounds.
-     */
+	/*
+	 * first we must compute how much space to allocate for
+	 * randr library's use; we'll allocate the structures in a single
+	 * allocation, on cleanlyness grounds.
+	 */
+
+	rbytes = (sizeof (XRRScreenResources) +
+		  rep.nCrtcs * sizeof (RRCrtc) +
+		  rep.nOutputs * sizeof (RROutput) +
+		  rep.nModes * sizeof (XRRModeInfo) +
+		  rep.nbytesNames + rep.nModes);    /* '\0' terminate names */
 
-    rbytes = (sizeof (XRRScreenResources) +
-	      rep.nCrtcs * sizeof (RRCrtc) +
-	      rep.nOutputs * sizeof (RROutput) +
-	      rep.nModes * sizeof (XRRModeInfo) +
-	      rep.nbytesNames + rep.nModes);	/* '\0' terminate names */
+	xrsr = (XRRScreenResources *) Xmalloc(rbytes);
+	wire_names = (char *) Xmalloc (rep.nbytesNames);
+    } else {
+	nbytes = 0;
+	nbytesRead = 0;
+	rbytes = 0;
+	xrsr = NULL;
+	wire_names = NULL;
+    }
 
-    xrsr = (XRRScreenResources *) Xmalloc(rbytes);
-    wire_names = (char *) Xmalloc (rep.nbytesNames);
     if (xrsr == NULL || wire_names == NULL) {
 	if (xrsr) Xfree (xrsr);
 	if (wire_names) Xfree (wire_names);
@@ -174,6 +184,14 @@ doGetScreenResources (Display *dpy, Window window, int poll)
     wire_name = wire_names;
     for (i = 0; i < rep.nModes; i++)  {
 	xrsr->modes[i].name = names;
+	if (xrsr->modes[i].nameLength > rep.nbytesNames) {
+	    Xfree (xrsr);
+	    Xfree (wire_names);
+	    UnlockDisplay (dpy);
+	    SyncHandle ();
+	    return NULL;
+	}
+	rep.nbytesNames -= xrsr->modes[i].nameLength;
 	memcpy (names, wire_name, xrsr->modes[i].nameLength);
 	names[xrsr->modes[i].nameLength] = '\0';
 	names += xrsr->modes[i].nameLength + 1;
-- 
cgit v1.1