On 23 May 2013 19:33, Alan Coopersmith <[email protected]> wrote: > 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.
Seems right to me too. Acked-by: Daniel Stone <[email protected]> > Reviewed-by: Alan Coopersmith <[email protected]> > > > 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- [email protected] > Oracle Solaris Engineering - http://blogs.oracle.com/alanc > > _______________________________________________ > [email protected]: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
