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

[Kees Cook]

Any comment on these patches? Can someone commit them if they're okay?

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

-- 
Kees Cook                                            @outflux.net