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 <[email protected]> > 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 <[email protected]> > >>--- > >> 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- [email protected] > Oracle Solaris Engineering - http://blogs.oracle.com/alanc -- Kees Cook @outflux.net _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
