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 <[email protected]> > --- > 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 > _______________________________________________ > [email protected]: 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 _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
