[PATCH v2] libX11: check size of GetReqExtra after XFlush

[Kees Cook]

Two users of GetReqExtra pass arbitrarily sized allocations from the
caller (ModMap and Host). Adjust _XGetRequest() (called by the GetReqExtra
macro) to double-check the requested length and invalidate "req" when this
happens. Users of GetReqExtra passing lengths greater than the Xlib buffer
size (normally 16K) now check "req" and fail if something has gone wrong.

Any other callers of GetReqExtra that do not check "req" for NULL
will experience this change, in the pathological case, as a NULL
dereference instead of a buffer overflow. This is an improvement, but
the documentation for GetReqExtra has been updated to reflect the need
to check the value of "req" after the call.

Prior version of this patch:
http://lists.x.org/archives/xorg-devel/2011-July/023936.html

Bug that manifested the problem:
https://bugs.launchpad.net/ubuntu/+source/x11-xserver-utils/+bug/792628

Signed-off-by: Kees Cook <kees at outflux.net>
---
 specs/libX11/AppC.xml | 4 +++-
 src/Host.c            | 8 ++++++++
 src/ModMap.c          | 4 ++++
 src/XlibInt.c         | 8 ++++++++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/specs/libX11/AppC.xml b/specs/libX11/AppC.xml
index df25027..0b37048 100644
--- a/specs/libX11/AppC.xml
+++ b/specs/libX11/AppC.xml
@@ -2468,7 +2468,9 @@ which is the same as
 <function>GetReq</function>
 except that it takes an additional argument (the number of
 extra bytes to allocate in the output buffer after the request structure).  
-This number should always be a multiple of four.
+This number should always be a multiple of four. Note that it is possible
+for req to be set to NULL as a defensive measure if the requested length
+exceeds the Xlib's buffer size (normally 16K).
 </para>
 </sect2>
 <sect2 id="Variable_Length_Arguments">
diff --git a/src/Host.c b/src/Host.c
index da9923a..da5e2f7 100644
--- a/src/Host.c
+++ b/src/Host.c
@@ -83,6 +83,10 @@ XAddHost (
 
     LockDisplay(dpy);
     GetReqExtra (ChangeHosts, length, req);
+    if (!req) {
+	UnlockDisplay(dpy);
+	return 0;
+    }
     req->mode = HostInsert;
     req->hostFamily = host->family;
     req->hostLength = addrlen;
@@ -118,6 +122,10 @@ XRemoveHost (
 
     LockDisplay(dpy);
     GetReqExtra (ChangeHosts, length, req);
+    if (!req) {
+	UnlockDisplay(dpy);
+	return 0;
+    }
     req->mode = HostDelete;
     req->hostFamily = host->family;
     req->hostLength = addrlen;
diff --git a/src/ModMap.c b/src/ModMap.c
index 5c5b426..f66e559 100644
--- a/src/ModMap.c
+++ b/src/ModMap.c
@@ -80,6 +80,10 @@ XSetModifierMapping(
 
     LockDisplay(dpy);
     GetReqExtra(SetModifierMapping, mapSize, req);
+    if (!req) {
+	UnlockDisplay(dpy);
+	return 2;
+    }
 
     req->numKeyPerModifier = modifier_map->max_keypermod;
 
diff --git a/src/XlibInt.c b/src/XlibInt.c
index b06e57b..c3273a8 100644
--- a/src/XlibInt.c
+++ b/src/XlibInt.c
@@ -1733,6 +1733,14 @@ void *_XGetRequest(Display *dpy, CARD8 type, size_t len)
 
     if (dpy->bufptr + len > dpy->bufmax)
 	_XFlush(dpy);
+    /* Request still too large, so do not allow it to overflow. */
+    if (dpy->bufptr + len > dpy->bufmax) {
+	fprintf(stderr,
+		"Xlib: request %d length %zd would exceed buffer size.\n",
+		type, len);
+	/* Changes failure condition from overflow to NULL dereference. */
+	return NULL;
+    }
 
     if (len % 4)
 	fprintf(stderr,
-- 
1.8.1.2


-- 
Kees Cook                                            @outflux.net