It seems to me that the change to GetReqExtra should indeed be merged. I think now we're just debating what to do with any possibly-hazardous callers. Kees, perhaps you could split the patch? Regarding the rest, I like Alan's comments, and would add: On Jun 6, 2013 9:17 PM, "Alan Coopersmith" <<a href="mailto:alan.coopersmith@oracle.com">alan.coopersmith@oracle.com</a>> wrote: > 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. Since the original bug report was about xhost failing on long hostnames, maybe it actually is worthwhile to make the Host routines use Data? >> 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. I like this plan. Is MappingFailed the right return value for an out-of-range value? Jamey