followup for libX11 XKB security fixes

[Alan Coopersmith]

I'd originally coded all the checks for xkb->max_key_code, then in testing
found that because Xlib accesses the xkb->max_key_code'th entry of the array,
it needed to be xkb->max_key_code + 1, and I updated the checks - I guess I
missed those two.   Sorry about that.

Reviewed-by: Alan Coopersmith <alan.coopersmith at oracle.com>

On 05/23/13 10:26 AM, Julien Cristau wrote:
> Hi,
>
> I noticed some inconsistencies in the XKB security changes in Xlib.
> Resending to the public list now that the embargo is lifted.
>
> Here's another change I think is necessary:
>
> diff --git a/src/xkb/XKBGetMap.c b/src/xkb/XKBGetMap.c
> index 0875dfd..c73e655 100644
> --- a/src/xkb/XKBGetMap.c
> +++ b/src/xkb/XKBGetMap.c
> @@ -426,7 +426,7 @@ XkbServerMapPtr		srv;
>
>       if ( rep->totalVModMapKeys>0 ) {
>   	if (((int) rep->firstVModMapKey + rep->nVModMapKeys)
> -	     > xkb->max_key_code)
> +	     > xkb->max_key_code + 1)
>   	    return BadLength;
>   	if (((xkb->server==NULL)||(xkb->server->vmodmap==NULL))&&
>   	    (XkbAllocServerMap(xkb,XkbVirtualModMapMask,0)!=Success)) {
> diff --git a/src/xkb/XKBNames.c b/src/xkb/XKBNames.c
> index 6faef02..6df7406 100644
> --- a/src/xkb/XKBNames.c
> +++ b/src/xkb/XKBNames.c
> @@ -180,7 +180,7 @@ _XkbReadGetNamesReply(	Display *		dpy,
>   	    nKeys= xkb->max_key_code+1;
>   	    names->keys= _XkbTypedCalloc(nKeys,XkbKeyNameRec);
>   	}
> -	else if ( ((int)rep->firstKey + rep->nKeys) > xkb->max_key_code)
> +	if ( ((int)rep->firstKey + rep->nKeys) > xkb->max_key_code + 1)
>   	    goto BAILOUT;
>   	if (names->keys!=NULL) {
>   	    if (!_XkbCopyFromReadBuffer(&buf,
>
> This makes the checks against max_key_code consistent, and means the
> GetNames reply gets checked even when names->keys was just allocated.
> Running 'xset q' here under xtrace shows GetNames returning firstKey=8
> nKeys=248, which would trip the original check with maxKeyCode=255.
> GetMap seems to return firstVModMapKey=0 nVModMapKeys=0
> totalVModMapKeys=0 though, so I don't know how to trigger the first
> check.
>
> Cheers,
> Julien
>


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