Re: [Dri-devel] Re: [Dri-devel][PATCH] Segfault in DRI Xserver extension

Felix,

Thanks for finding and fixing this vulnerability in the DRI extension. 
I'm copying the xpert list on this fix, because it looks like their may 
be other extensions with the same vulnerability.  For example:

   xc/programs/Xserver/Xext/panoramiX.c: line 991

Felix Kühling wrote:
> On Thu, 12 Dec 2002 22:26:23 +0000
> Keith Whitwell <[email protected]> wrote:
> 
> 
>>Felix Kühling wrote:
>>
>>>Hi,
>>>
>>>while I was messing around with my query programme I found this: if I
>>>specify an invalid screen as argument to XF86DRIGetClientDriverName the
>>>Xserver segfaults. I had a quick look at xc/xc/Xserver/GL/dri/xf86dri.c.
>>>stuff->screen is used as array index without checking. I'm not sure
>>>though, where would be the right place to fix it.
>>>
>>>Other functions in xf86dri.c must be affacted, too. They use
>>>stuff->screen in the same way.
>>
>>Those functions should validate any information they receive over the wire, as 
>>soon as is feasible.
> 
> 
> The problem is that the request records are all different. So we can't
> check stuff->screen in ProcXF86DRIDispatch. We have to do it in each
> request separately.
> 
> The attached patch does just that. While I grepped around to see how
> other extensions check this I found one more potential problem in
> programs/Xserver/GL/glx/glxcmds.c and two suspicious casts of screen
> from unsigned to int (screen is always unsigned in the request records).
> They are fixed in the attached patch as well.
> 
> Regards,
>    Felix
> 
>                __\|/__    ___     ___     ___
> __Tschüß_______\_6 6_/___/__ \___/__ \___/___\___You can do anything,___
> _____Felix_______\Ä/\ \_____\ \_____\ \______U___just not everything____
>   [email protected]    >o<__/   \___/   \___/        at the same time!
> 
> 
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Index: dri/xf86dri.c
> ===================================================================
> RCS file: /cvsroot/dri/xc/xc/programs/Xserver/GL/dri/xf86dri.c,v
> retrieving revision 1.10
> diff -u -r1.10 xf86dri.c
> --- dri/xf86dri.c	29 Oct 2002 20:28:57 -0000	1.10
> +++ dri/xf86dri.c	13 Dec 2002 15:51:57 -0000
> @@ -155,6 +155,11 @@
>  
>      REQUEST(xXF86DRIQueryDirectRenderingCapableReq);
>      REQUEST_SIZE_MATCH(xXF86DRIQueryDirectRenderingCapableReq);
> +    if (stuff->screen >= screenInfo.numScreens) {
> +	client->errorValue = stuff->screen;
> +	return BadValue;
> +    }
> +
>      rep.type = X_Reply;
>      rep.length = 0;
>      rep.sequenceNumber = client->sequence;
> @@ -184,6 +189,10 @@
>  
>      REQUEST(xXF86DRIOpenConnectionReq);
>      REQUEST_SIZE_MATCH(xXF86DRIOpenConnectionReq);
> +    if (stuff->screen >= screenInfo.numScreens) {
> +	client->errorValue = stuff->screen;
> +	return BadValue;
> +    }
>  
>      if (!DRIOpenConnection( screenInfo.screens[stuff->screen], 
>  			    &hSAREA,
> @@ -221,6 +230,10 @@
>      
>      REQUEST(xXF86DRIAuthConnectionReq);
>      REQUEST_SIZE_MATCH(xXF86DRIAuthConnectionReq);
> +    if (stuff->screen >= screenInfo.numScreens) {
> +	client->errorValue = stuff->screen;
> +	return BadValue;
> +    }
>  
>      rep.type = X_Reply;
>      rep.length = 0;
> @@ -242,6 +255,10 @@
>  {
>      REQUEST(xXF86DRICloseConnectionReq);
>      REQUEST_SIZE_MATCH(xXF86DRICloseConnectionReq);
> +    if (stuff->screen >= screenInfo.numScreens) {
> +	client->errorValue = stuff->screen;
> +	return BadValue;
> +    }
>  
>      DRICloseConnection( screenInfo.screens[stuff->screen]);
>  
> @@ -258,6 +275,10 @@
>  
>      REQUEST(xXF86DRIGetClientDriverNameReq);
>      REQUEST_SIZE_MATCH(xXF86DRIGetClientDriverNameReq);
> +    if (stuff->screen >= screenInfo.numScreens) {
> +	client->errorValue = stuff->screen;
> +	return BadValue;
> +    }
>  
>      DRIGetClientDriverName( screenInfo.screens[stuff->screen],
>  			    (int *)&rep.ddxDriverMajorVersion,
> @@ -295,6 +316,11 @@
>  
>      REQUEST(xXF86DRICreateContextReq);
>      REQUEST_SIZE_MATCH(xXF86DRICreateContextReq);
> +    if (stuff->screen >= screenInfo.numScreens) {
> +	client->errorValue = stuff->screen;
> +	return BadValue;
> +    }
> +
>      rep.type = X_Reply;
>      rep.length = 0;
>      rep.sequenceNumber = client->sequence;
> @@ -329,6 +355,10 @@
>  {
>      REQUEST(xXF86DRIDestroyContextReq);
>      REQUEST_SIZE_MATCH(xXF86DRIDestroyContextReq);
> +    if (stuff->screen >= screenInfo.numScreens) {
> +	client->errorValue = stuff->screen;
> +	return BadValue;
> +    }
>  
>      if (!DRIDestroyContext( screenInfo.screens[stuff->screen],
>  			    stuff->context)) {
> @@ -348,6 +378,11 @@
>  
>      REQUEST(xXF86DRICreateDrawableReq);
>      REQUEST_SIZE_MATCH(xXF86DRICreateDrawableReq);
> +    if (stuff->screen >= screenInfo.numScreens) {
> +	client->errorValue = stuff->screen;
> +	return BadValue;
> +    }
> +
>      rep.type = X_Reply;
>      rep.length = 0;
>      rep.sequenceNumber = client->sequence;
> @@ -378,6 +413,10 @@
>      REQUEST(xXF86DRIDestroyDrawableReq);
>      DrawablePtr pDrawable;
>      REQUEST_SIZE_MATCH(xXF86DRIDestroyDrawableReq);
> +    if (stuff->screen >= screenInfo.numScreens) {
> +	client->errorValue = stuff->screen;
> +	return BadValue;
> +    }
>  
>      if (!(pDrawable = (DrawablePtr)SecurityLookupDrawable(
>  						(Drawable)stuff->drawable,
> @@ -409,6 +448,11 @@
>  
>      REQUEST(xXF86DRIGetDrawableInfoReq);
>      REQUEST_SIZE_MATCH(xXF86DRIGetDrawableInfoReq);
> +    if (stuff->screen >= screenInfo.numScreens) {
> +	client->errorValue = stuff->screen;
> +	return BadValue;
> +    }
> +
>      rep.type = X_Reply;
>      rep.length = 0;
>      rep.sequenceNumber = client->sequence;
> @@ -483,6 +527,11 @@
>  
>      REQUEST(xXF86DRIGetDeviceInfoReq);
>      REQUEST_SIZE_MATCH(xXF86DRIGetDeviceInfoReq);
> +    if (stuff->screen >= screenInfo.numScreens) {
> +	client->errorValue = stuff->screen;
> +	return BadValue;
> +    }
> +
>      rep.type = X_Reply;
>      rep.length = 0;
>      rep.sequenceNumber = client->sequence;
> @@ -528,6 +577,11 @@
>      DrawablePtr                 pDrawable;
>  
>      REQUEST_SIZE_MATCH(xXF86DRIOpenFullScreenReq);
> +    if (stuff->screen >= screenInfo.numScreens) {
> +	client->errorValue = stuff->screen;
> +	return BadValue;
> +    }
> +
>      rep.type           = X_Reply;
>      rep.length         = 0;
>      rep.sequenceNumber = client->sequence;
> @@ -554,6 +608,11 @@
>      DrawablePtr                  pDrawable;
>  
>      REQUEST_SIZE_MATCH(xXF86DRICloseFullScreenReq);
> +    if (stuff->screen >= screenInfo.numScreens) {
> +	client->errorValue = stuff->screen;
> +	return BadValue;
> +    }
> +
>      rep.type           = X_Reply;
>      rep.length         = 0;
>      rep.sequenceNumber = client->sequence;
> Index: glx/glxcmds.c
> ===================================================================
> RCS file: /cvsroot/dri/xc/xc/programs/Xserver/GL/glx/glxcmds.c,v
> retrieving revision 1.8
> diff -u -r1.8 glxcmds.c
> --- glx/glxcmds.c	25 Nov 2002 19:58:38 -0000	1.8
> +++ glx/glxcmds.c	13 Dec 2002 15:52:01 -0000
> @@ -761,7 +761,7 @@
>      int i, p;
>  
>      screen = req->screen;
> -    if (screen > screenInfo.numScreens) {
> +    if (screen >= screenInfo.numScreens) {
>  	/* The client library must send a valid screen number. */
>  	client->errorValue = screen;
>  	return BadValue;
> @@ -1466,7 +1466,7 @@
>      ClientPtr client = cl->client;
>      xGLXQueryExtensionsStringReq *req = (xGLXQueryExtensionsStringReq *) pc;
>      xGLXQueryExtensionsStringReply reply;
> -    GLint screen;
> +    GLuint screen;
>      size_t n, length;
>      const char *ptr;
>      char *buf;
> @@ -1475,7 +1475,7 @@
>      /*
>      ** Check if screen exists.
>      */
> -    if ((screen < 0) || (screen >= screenInfo.numScreens)) {
> +    if (screen >= screenInfo.numScreens) {
>  	client->errorValue = screen;
>  	return BadValue;
>      }
> @@ -1511,7 +1511,7 @@
>      xGLXQueryServerStringReq *req = (xGLXQueryServerStringReq *) pc;
>      xGLXQueryServerStringReply reply;
>      int name;
> -    GLint screen;
> +    GLuint screen;
>      size_t n, length;
>      const char *ptr;
>      char *buf;
> @@ -1521,7 +1521,7 @@
>      /*
>      ** Check if screen exists.
>      */
> -    if ((screen < 0) || (screen >= screenInfo.numScreens)) {
> +    if (screen >= screenInfo.numScreens) {
>  	client->errorValue = screen;
>  	return BadValue;
>      }



-- 
                                /\
          Jens Owen            /  \/\ _
   [email protected]  /    \ \ \   Steamboat Springs, Colorado