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 -- 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
