followup for libX11 XKB security fixes

Julien Cristau jcristau at debian.org
Thu May 23 10:26:50 PDT 2013


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


More information about the xorg-devel mailing list