Bug 28823 - Missing input sanitation in __glXDisp_CreateContext triggers SEGV
Missing input sanitation in __glXDisp_CreateContext triggers SEGV
Status: RESOLVED FIXED
Product: xorg
Classification: Unclassified
Component: Security
7.5 (2009.10)
x86 (IA32) Linux (All)
: medium major
Assigned To: X.Org Security
X.Org Security
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-29 14:10 UTC by halfdog
Modified: 2011-09-22 10:23 UTC (History)
2 users (show)

See Also:


Attachments
check for screen < 0 in validGlxScreen() (748 bytes, patch)
2010-07-03 12:21 UTC, Julien Cristau
no flags Details | Splinter Review
use validGlxScreen in more places (3.01 KB, patch)
2010-07-03 12:22 UTC, Julien Cristau
no flags Details | Splinter Review
validate request lengths (19.52 KB, patch)
2010-07-03 12:23 UTC, Julien Cristau
no flags Details | Splinter Review
validate request lengths (19.99 KB, patch)
2010-08-22 08:29 UTC, Julien Cristau
no flags Details | Splinter Review
check request lengths before swapping (19.29 KB, patch)
2010-08-22 08:32 UTC, Julien Cristau
no flags Details | Splinter Review
swap 2 * numAttribs ints, not just numAttribs (2.39 KB, patch)
2010-08-22 08:34 UTC, Julien Cristau
no flags Details | Splinter Review
testcase for glXCreateContext (861 bytes, text/plain)
2010-11-10 11:52 UTC, Julien Cristau
no flags Details
check that req->numattribs is not too big (4.59 KB, patch)
2010-11-10 14:51 UTC, Julien Cristau
no flags Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description halfdog 2010-06-29 14:10:51 UTC
During X server security testing, current lucid xserver was terminated on
invalid request:

ii  xserver-xorg                      1:7.5+5ubuntu1                           
      the X.Org X server
ii  xserver-xorg-core                 2:1.7.6-2ubuntu7.2                       
      Xorg X server - core server


These 2 packets (auth, __glXDisp_CreateContext) cause glx to read screen information from arbitrary memory location, but maybe only on 32 bit systems.

0000000: 6c00 0b00 0000 0000 0000 0000 9a03 0500  l...............
0000010: f857 3051 67e9 c5ba 735d 6edc b51c 95cf  .W0Qg...s]n.....


Problem is in glx/glxcmds.c validGlxScreen

static int
validGlxScreen(ClientPtr client, int screen, __GLXscreen **pGlxScreen, int *err)
{
    /*
    ** Check if screen exists.
    */
>>>>>> No error if screen number is negative
    if (screen >= screenInfo.numScreens) {
        client->errorValue = screen;
        *err = BadValue;
        return FALSE;
    }
>>>>>> Wrap around in 32bit mem few times
    *pGlxScreen = glxGetScreen(screenInfo.screens[screen]);

    return TRUE;
}


Backtrace:
0: /usr/bin/X (xorg_backtrace+0x3b) [0x80e937b]
1: /usr/bin/X (0x8048000+0x61c7d) [0x80a9c7d]
2: (vdso) (__kernel_rt_sigreturn+0x0) [0xa6c410]
3: /usr/lib/xorg/modules/extensions/libglx.so (0x3f6000+0x34a76) [0x42aa76]
4: /usr/lib/xorg/modules/extensions/libglx.so (0x3f6000+0x36c22) [0x42cc22]
5: /usr/bin/X (0x8048000+0x2a477) [0x8072477]
6: /usr/bin/X (0x8048000+0x1ed7a) [0x8066d7a]
7: /lib/tls/i686/cmov/libc.so.6 (__libc_start_main+0xe6) [0xac6bd6]
8: /usr/bin/X (0x8048000+0x1e961) [0x8066961]
Segmentation fault at address 0x79d95180

Consequence of error not clear, since invalid screen data pointer might produce nearly unpredictable side effects. Apart from that, function is called from various locations in code.


Credits to me@halfdog.net
Comment 1 halfdog 2010-06-30 13:31:32 UTC
The glx code is rather buggy, e.g.

Not even verify if screen is valid, just dereference (occurs more than once)

int __glXDisp_HyperpipeConfigSGIX(__GLXclientState *cl, GLbyte *pc)
{
    ClientPtr client = cl->client;
    xGLXHyperpipeConfigSGIXReq * req =
        (xGLXHyperpipeConfigSGIXReq *) pc;
    xGLXHyperpipeConfigSGIXReply reply;
    int screen = req->screen;
    void *rdata;

    int npipes=0, networkId;
    int hpId=-1;
    __GLXscreen *pGlxScreen;
>>>>>>>>>>>> untrusted screen var
    pGlxScreen = glxGetScreen(screenInfo.screens[screen]);


Some of the numattr loops do not check the client request size and the num attr value, large attr value (CARD32) triggers SEGV
Comment 2 Julien Cristau 2010-07-03 12:21:17 UTC
Created attachment 36729 [details] [review]
check for screen < 0 in validGlxScreen()

Here's a (mostly untested) attempt at fixing a few of those things.
Comment 3 Julien Cristau 2010-07-03 12:22:20 UTC
Created attachment 36730 [details] [review]
use validGlxScreen in more places
Comment 4 Julien Cristau 2010-07-03 12:23:34 UTC
Created attachment 36731 [details] [review]
validate request lengths

Something similar probably needs to be done for the glxcmdsswap.c?
Comment 5 ajax at nwnk dot net 2010-08-20 12:30:47 UTC
Reviewed-by: Adam Jackson <ajax@redhat.com>

for the above three patches.
Comment 6 Julien Cristau 2010-08-22 08:29:13 UTC
Created attachment 38061 [details] [review]
validate request lengths

While looking at glxcmdsswap.c I noticed a mistake and an omission in the previous patch.  Interdiff is:

diff --git a/glx/glxcmds.c b/glx/glxcmds.c
index 73c3b2a..70c766c 100644
--- a/glx/glxcmds.c
+++ b/glx/glxcmds.c
@@ -1511,7 +1511,7 @@ int __glXDisp_CreateWindow(__GLXclientState *cl, GLbyte *pc)
     DrawablePtr                 pDraw;
     int                         err;
 
-    REQUEST_SIZE_MATCH(xGLXCreateWindowReq);
+    REQUEST_FIXED_SIZE(xGLXCreateWindowReq, req->numAttribs << 3);
 
     LEGAL_NEW_RESOURCE(req->glxwindow, client);
 
diff --git a/glx/xfont.c b/glx/xfont.c
index b4081cf..eafac01 100644
--- a/glx/xfont.c
+++ b/glx/xfont.c
@@ -155,6 +155,8 @@ int __glXDisp_UseXFont(__GLXclientState *cl, GLbyte *pc)
     __GLXcontext *cx;
     int error;
 
+    REQUEST_SIZE_MATCH(xGLXUseXFontReq);
+
     req = (xGLXUseXFontReq *) pc;
     cx = __glXForceCurrent(cl, req->contextTag, &error);
     if (!cx) {
Comment 7 Julien Cristau 2010-08-22 08:32:18 UTC
Created attachment 38062 [details] [review]
check request lengths before swapping
Comment 8 Julien Cristau 2010-08-22 08:34:16 UTC
Created attachment 38063 [details] [review]
swap 2 * numAttribs ints, not just numAttribs

I noticed this while adding the length checks: some glx requests include a list of pairs of CARD32, with numAttribs elements (i.e. pairs).  From what I can tell we were only swapping numAttribs CARD32s.
Comment 9 Julien Cristau 2010-10-20 09:45:46 UTC
Review of attachment 38061 [details] [review]:

::: glx/glxcmds.c
@@ +1285,3 @@
     int err;
 
+    REQUEST_FIXED_SIZE(xGLXCreatePixmapReq, req->numAttribs << 3);

I suppose this should validate numAttribs first to make sure it doesn't overflow.  *sigh*
Comment 10 Thomas Biege 2010-10-21 02:50:38 UTC
How can I reproduce the bug?
Comment 11 Julien Cristau 2010-11-10 11:52:41 UTC
Created attachment 40181 [details]
testcase for glXCreateContext

attached trivial testcase crashes the server by passing a broken screen number to glxCreateContext
Comment 12 Julien Cristau 2010-11-10 14:51:11 UTC
Created attachment 40185 [details] [review]
check that req->numattribs is not too big

One more patch to check req->numAttribs before using it to compute the request length.
Comment 13 Julien Cristau 2010-11-10 14:52:23 UTC
(In reply to comment #3)
> Created an attachment (id=36730) [details]
> use validGlxScreen in more places

this one is actually patching dead code, so not strictly necessary.
Comment 14 Thomas Biege 2010-11-15 05:06:45 UTC
Thank you a lot.
Comment 15 Julien Cristau 2011-01-19 02:09:42 UTC
Fixed in master
Comment 16 Marcus Meissner 2011-09-22 09:22:05 UTC
fixed apparently by following mainline xorg commit ids in master:

 402b329c3aa8ddbebaa1f593306a02d4cd6fed26
 d9225b9602c85603ae616a7381c784f5cf5e811c
 62319e8381ebd645ae36b25e5fc3c0e9b098387b
 6c69235a9dfc52e4b4e47630ff4bab1a820eb543
 ec9c97c6bf70b523bc500bd3adf62176f1bb33a4
 3f0d3f4d97bce75c1828635c322b6560a45a037f

(in git log order)
Comment 17 Julien Cristau 2011-09-22 10:23:50 UTC
> --- Comment #16 from Marcus Meissner <meissner@suse.de> 2011-09-22 09:22:05 PDT ---
> fixed apparently by following mainline xorg commit ids in master:
> 
>  402b329c3aa8ddbebaa1f593306a02d4cd6fed26

1137c11be0f82049d28024eaf963c6f76e0d4334
a883cf1545abd89bb2cadfa659718884b56fd234

(fixed bugs in the initial series)

>  d9225b9602c85603ae616a7381c784f5cf5e811c
>  62319e8381ebd645ae36b25e5fc3c0e9b098387b
>  6c69235a9dfc52e4b4e47630ff4bab1a820eb543
>  ec9c97c6bf70b523bc500bd3adf62176f1bb33a4
>  3f0d3f4d97bce75c1828635c322b6560a45a037f
> 
> (in git log order)
>