[PATCH] libX11: check size of GetReqExtra after XFlush

[Kees Cook]

Hi, any comments on this? Seems like kind of a nasty surprise bug...

Thanks,

-Kees

On Sat, Jul 09, 2011 at 12:42:57PM -0700, Kees Cook wrote:
> Two users of GetReqExtra pass arbitrarily sized allocations from the
> caller (ModMap and Host). Adjust the GetReqExtra macro to double-check
> the requested length and invalidate "req" when this happens. Users of
> potentially >16K lengths in GetReqExtra now check "req" and fail if
> something has gone wrong.
> 
> https://bugs.launchpad.net/ubuntu/+source/x11-xserver-utils/+bug/792628
> 
> Signed-off-by: Kees Cook <kees.cook at canonical.com>
> ---
>  include/X11/Xlibint.h |   28 ++++++++++++++++++----------
>  src/Host.c            |    8 ++++++++
>  src/ModMap.c          |    4 ++++
>  3 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/include/X11/Xlibint.h b/include/X11/Xlibint.h
> index 2ce356d..81f8cfd 100644
> --- a/include/X11/Xlibint.h
> +++ b/include/X11/Xlibint.h
> @@ -461,21 +461,29 @@ extern LockInfoPtr _Xglobal_lock;
>          WORD64ALIGN\
>  	if ((dpy->bufptr + SIZEOF(x##name##Req) + n) > dpy->bufmax)\
>  		_XFlush(dpy);\
> -	req = (x##name##Req *)(dpy->last_req = dpy->bufptr);\
> -	req->reqType = X_##name;\
> -	req->length = (SIZEOF(x##name##Req) + n)>>2;\
> -	dpy->bufptr += SIZEOF(x##name##Req) + n;\
> -	dpy->request++
> +	if ((dpy->bufptr + SIZEOF(x##name##Req) + n) <= dpy->bufmax) {\
> +	    req = (x##name##Req *)(dpy->last_req = dpy->bufptr);\
> +    	req->reqType = X_##name;\
> +    	req->length = (SIZEOF(x##name##Req) + n)>>2;\
> +    	dpy->bufptr += SIZEOF(x##name##Req) + n;\
> +    	dpy->request++;\
> +    } else {\
> +        req = NULL;\
> +    }
>  #else
>  #define GetReqExtra(name, n, req) \
>          WORD64ALIGN\
>  	if ((dpy->bufptr + SIZEOF(x/**/name/**/Req) + n) > dpy->bufmax)\
>  		_XFlush(dpy);\
> -	req = (x/**/name/**/Req *)(dpy->last_req = dpy->bufptr);\
> -	req->reqType = X_/**/name;\
> -	req->length = (SIZEOF(x/**/name/**/Req) + n)>>2;\
> -	dpy->bufptr += SIZEOF(x/**/name/**/Req) + n;\
> -	dpy->request++
> +	if ((dpy->bufptr + SIZEOF(x/**/name/**/Req) + n) <= dpy->bufmax) {\
> +    	req = (x/**/name/**/Req *)(dpy->last_req = dpy->bufptr);\
> +    	req->reqType = X_/**/name;\
> +    	req->length = (SIZEOF(x/**/name/**/Req) + n)>>2;\
> +    	dpy->bufptr += SIZEOF(x/**/name/**/Req) + n;\
> +    	dpy->request++;\
> +    } else {\
> +        req = NULL;\
> +    }
>  #endif
>  
>  
> diff --git a/src/Host.c b/src/Host.c
> index da9923a..3f87731 100644
> --- a/src/Host.c
> +++ b/src/Host.c
> @@ -83,6 +83,10 @@ XAddHost (
>  
>      LockDisplay(dpy);
>      GetReqExtra (ChangeHosts, length, req);
> +    if (!req) {
> +        UnlockDisplay(dpy);
> +        return 0;
> +    }
>      req->mode = HostInsert;
>      req->hostFamily = host->family;
>      req->hostLength = addrlen;
> @@ -118,6 +122,10 @@ XRemoveHost (
>  
>      LockDisplay(dpy);
>      GetReqExtra (ChangeHosts, length, req);
> +    if (!req) {
> +        UnlockDisplay(dpy);
> +        return 0;
> +    }
>      req->mode = HostDelete;
>      req->hostFamily = host->family;
>      req->hostLength = addrlen;
> diff --git a/src/ModMap.c b/src/ModMap.c
> index c99bfdd..5deb894 100644
> --- a/src/ModMap.c
> +++ b/src/ModMap.c
> @@ -75,6 +75,10 @@ XSetModifierMapping(
>  
>      LockDisplay(dpy);
>      GetReqExtra(SetModifierMapping, mapSize, req);
> +    if (!req) {
> +        UnlockDisplay(dpy);
> +        return 2;
> +    }
>  
>      req->numKeyPerModifier = modifier_map->max_keypermod;
>  
> -- 
> 1.7.4.1
> 
> 
> -- 
> Kees Cook
> Ubuntu Security Team
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
-- 
Kees Cook
Ubuntu Security Team