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

[Alan Coopersmith]

On 06/ 6/13 10:40 AM, 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) now check "req" and fail if something has gone wrong.

An alternative would be to convert them to use Data instead of GetReqExtra()
as the requests like PutImage do that truly send arbitrarily large data to
the server, but I'm not sure it's worth it in these cases.

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

Style nit, please return MappingFailed instead of the raw value.  (I see
the comment above the function uses the raw values instead of the symbolic
names used in the man pages and server side code, which is unfortunate.)

> +    }
>
>       req->numKeyPerModifier = modifier_map->max_keypermod;

Since the protocol defines that as a single byte, we should probably reject
at the top of the function any modifier_map->max_keypermod > 255, as otherwise
we send a packet to the X server with more data than expected, which it will
reject - and that limit would also keep the mapSize within the normal Xlib
buffer size.   That doesn't need to be this patch though.

-- 
	-Alan Coopersmith-              alan.coopersmith at oracle.com
	 Oracle Solaris Engineering - http://blogs.oracle.com/alanc