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
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
Created attachment 36729 [details] [review] check for screen < 0 in validGlxScreen() Here's a (mostly untested) attempt at fixing a few of those things.
Created attachment 36730 [details] [review] use validGlxScreen in more places
Created attachment 36731 [details] [review] validate request lengths Something similar probably needs to be done for the glxcmdsswap.c?
Reviewed-by: Adam Jackson <ajax@redhat.com> for the above three patches.
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) {
Created attachment 38062 [details] [review] check request lengths before swapping
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.
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*
How can I reproduce the bug?
Created attachment 40181 [details] testcase for glXCreateContext attached trivial testcase crashes the server by passing a broken screen number to glxCreateContext
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.
(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.
Thank you a lot.
Fixed in master
fixed apparently by following mainline xorg commit ids in master: 402b329c3aa8ddbebaa1f593306a02d4cd6fed26 d9225b9602c85603ae616a7381c784f5cf5e811c 62319e8381ebd645ae36b25e5fc3c0e9b098387b 6c69235a9dfc52e4b4e47630ff4bab1a820eb543 ec9c97c6bf70b523bc500bd3adf62176f1bb33a4 3f0d3f4d97bce75c1828635c322b6560a45a037f (in git log order)
> --- 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) >
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.