[PATCH] libX11: check size of GetReqExtra after XFlush

[Jamey Sharp]

I have a few comments, I guess.

- The indentation in the GetReqExtra definition didn't work out. Looks
  like it should be tabs and tabs only, there.

- This patch turns a buffer overrun into a null-pointer dereference if
  there's any caller that hasn't been updated. (While you've hit all the
  cases I can find in X.Org sources, keep in mind that there are
  closed-source Xlib extensions out there.) I think that's an
  improvement, but it's worth calling out in the commit message.

- I guess this new behavior should be documented in
  specs/libX11/AppC.xml.

- Your mention of 16K in the commit message confused me because I wasn't
  sure where you were getting that limit from. Note that Xlib's buffer
  size is actually runtime-configurable, down to a minimum of 2K. Could
  you explain it in terms of Xlib's output buffer size, please?

- Changing the implementation of GetReqExtra and the rest of Xlibint.h
  is only acceptable if code compiled using the old implementation is
  ABI compatible with code compiled using the new implementation. I
  believe you're good on that count, but I wanted to point it out.

On the whole, I believe this is an improvement. With the above
corrections, I'd be happy to commit this.

Jamey

On Mon, Jul 18, 2011 at 08:57:44AM -0700, Kees Cook wrote:
> 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 <kees.cook at canonical.com>
> > ---
> >  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg-devel/attachments/20110718/c7087c71/attachment.pgp>