[PATCH 1/2] libX11: check size of GetReqExtra after XFlush

[Kees Cook]

Excellent! :) Thanks!

-Kees

On Mon, Jul 22, 2013 at 11:55:23PM -0700, Alan Coopersmith wrote:
> Sorry, was distracted long enough that all memory of these patches had been
> flushed from my cache, so I had to dig into them again to reunderstand them.
> 
> I think they look good enough now, so I've added
> Reviewed-by: Alan Coopersmith <alan.coopersmith at oracle.com>
> and pushed both to git master:
> To ssh://git.freedesktop.org/git/xorg/lib/libX11
>    24d3ee0..feb131b  master -> master
> 
> 	-alan-
> 
> 
> On 07/18/13 03:52 PM, Kees Cook wrote:
> >Re-re-ping. :) Can anyone commit these two patches please?
> >
> >Thanks!
> >
> >-Kees
> >
> >On Sun, Jun 09, 2013 at 11:13:42AM -0700, Kees Cook wrote:
> >>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) must check "req" and fail gracefully instead
> >>of crashing.
> >>
> >>Any 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.
> >>
> >>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/XlibInt.c         | 8 ++++++++
> >>  2 files changed, 11 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/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
> >
> 
> 
> -- 
> 	-Alan Coopersmith-              alan.coopersmith at oracle.com
> 	 Oracle Solaris Engineering - http://blogs.oracle.com/alanc

-- 
Kees Cook                                            @outflux.net