1 x security it’s worse than it looks ilja van sprundel [email protected]
TRANSCRIPT
Who am I?
• IOActive• Director of Penetration Testing • Pentest • Code review • Break stuff for fun and profit
2
agenda
• Intro• Client• Server• Solutions ?
3
What this talk is about
• Implementation of X.org parsers• Client and server
• Identify some entrypoints • Show some bugs and trends of bugs that show up often• How bad is it really ?• What is it not about ?
• Network operational side• Yes, network X is bad (sniff, keylog, …) • Should tunnel over ssh if you want that
4
Intro
• I started this about a year ago • Looked at the client first, late last year (december)• Took about a week
• Grepping for entrypoints was easy • Reported ~80 bugs, all have been fixed (well, most)
• Been looking at the server on and off since may • A lot more tedious • Not finished (not by a long shot)• Reported ~120 bugs so far
5
6
Client
Some observations
• X is a client/server protocol to be used for the gui in most unices
• It’s networked (can be tcp/ip, ipc, …)• Binary protocol
• The client protocol parsers become interesting when suids use X
• You can make them connect to arbitrary X servers using the DISPLAY variable • Who says these have to be well behaving X servers ?
7
X network architecture
8
Some observations
9
Typical bugs
10
Status XAllocColorCells( register Display *dpy, Colormap cmap, Bool contig, unsigned long *masks, /* LISTofCARD32 */ /* RETURN */ unsigned int nplanes, /* CARD16 */ unsigned long *pixels, /* LISTofCARD32 */ /* RETURN */ unsigned int ncolors) /* CARD16 */{... xAllocColorCellsReply rep;... status = _XReply(dpy, (xReply *)&rep, 0, xFalse);
if (status) {_XRead32 (dpy, (long *) pixels, 4L * (long) (rep.nPixels)); _XRead32 (dpy, (long *) masks, 4L * (long) (rep.nMasks));
}...}
Typical bugs
11
XTimeCoord *XGetMotionEvents(register Display *dpy, Window w, Time start, Time stop, int *nEvents) /* RETURN */{ xGetMotionEventsReply rep;... if (!_XReply (dpy, (xReply *)&rep, 0, xFalse)) {…
}...
if (! (tc = (XTimeCoord *) Xmalloc( (unsigned)
(nbytes = (long) rep.nEvents * sizeof(XTimeCoord))))) { ...
}...
for (i = rep.nEvents, tcptr = tc; i > 0; i--, tcptr++) { _XRead (dpy, (char *) &xtc, nbytes); tcptr->time = xtc.time;
…}
...}
Typical bugs
12
_XkbReadGetDeviceInfoReply( Display * dpy,xkbGetDeviceInfoReply * rep, XkbDeviceInfoPtr devi)
{... if (rep->nBtnsWanted>0) {
act= &devi->btn_acts[rep->firstBtnWanted]; bzero((char *)act,(rep->nBtnsWanted*sizeof(XkbAction)));
}...
int size;act= &devi->btn_acts[rep->firstBtnRtrn];size= rep->nBtnsRtrn*SIZEOF(xkbActionWireDesc);if (!_XkbCopyFromReadBuffer(&buf,(char *)act,size)) goto BAILOUT;
...}
Typical bugs
13
Private Bool_XimXGetReadData( Xim im, char *buf, int buf_len, int *ret_len, XEvent *event){...
return_code = XGetWindowProperty(im->core.display,spec->lib_connect_wid, prop, 0L,(long)((length + 3)/ 4), True, AnyPropertyType,&type_ret, &format_ret, &nitems, &bytes_after_ret, &prop_ret);
...if (buf_len >= length) { (void)memcpy(buf, prop_ret, (int)nitems);
... }...}
Typical bugs
14
static char * ReadInFile(_Xconst char *filename){ register int fd, size;... if ( (fd = _XOpenFile (filename, O_RDONLY)) == -1 )
return (char *)NULL;...
if ( (fstat(fd, &status_buffer)) == -1 ) { close (fd); return (char *)NULL;} else size = status_buffer.st_size;
}
if (!(filebuf = Xmalloc(size + 1))) { /* leave room for '\0' */close(fd);return (char *)NULL;
} size = read (fd, filebuf, size); ...}
Typical bugs
15
static char*TransFileName(Xim im, char *name) { char *home = NULL, *lcCompose = NULL; char dir[XLC_BUFSIZE]; char *i = name, *ret, *j; int l = 0;
while (*i) { if (*i == '%') { i++; switch (*i) {
case 'H': home = getenv("HOME"); if (home) l += strlen(home); possible int overflow (long HOME and loads of %H's) break; case 'L': if (lcCompose == NULL) lcCompose = _XlcFileName(im->core.lcd, COMPOSE_FILE); if (lcCompose) l += strlen(lcCompose); possible integer overflow (long lcCompose and loads of %L's) break; case 'S': xlocaledir(dir, XLC_BUFSIZE); l += strlen(dir); possible integer overflow (long dir and loads of %S's) break; }
Typical bugs
16
j = ret = Xmalloc(l+1); integer overflow, alloc too short if any of the int overflows occured ... i = name; while (*i) { if (*i == '%') { i++; switch (*i) { case '%': *j++ = '%'; break; case 'H': if (home) { strcpy(j, home); buffer overflow if integer overflow occured j += strlen(home); } break; case 'L': if (lcCompose) { strcpy(j, lcCompose); buffer overflow if integer overflow occured j += strlen(lcCompose); } break; case 'S': strcpy(j, dir); buffer overflow if integer overflow occured j += strlen(dir); break; } ...
Typical bugs
17
XFixesCursorImage *XFixesGetCursorImage (Display *dpy) {... xXFixesGetCursorImageAndNameReply rep; int npixels;... if (!_XReply (dpy, (xReply *) &rep, 0, xFalse)) {... }... npixels = rep.width * rep.height; ushort * ushort... rlength = (sizeof (XFixesCursorImage) +
npixels * sizeof (unsigned long) + nbytes_name + 1);
... image = (XFixesCursorImage *) Xmalloc (rlength); ... image->x = rep.x;...}
Typical bugs
18
Bool DMXGetScreenAttributes(Display *dpy, int physical_screen, DMXScreenAttributes *attr){... xDMXGetScreenAttributesReply rep;... if (!_XReply(dpy, (xReply *)&rep, (SIZEOF(xDMXGetScreenAttributesReply) - 32) >> 2, xFalse)) { UnlockDisplay(dpy); SyncHandle(); return False; } attr->displayName = Xmalloc(rep.displayNameLength + 1 + 4 /* for pad */); _XReadPad(dpy, attr->displayName, rep.displayNameLength); attr->displayName[rep.displayNameLength] = '\0'; ...}
Typical bugs
19
intXGetDeviceButtonMapping( register Display *dpy, XDevice *device, unsigned char map[], unsigned int nmap){ int status = 0; unsigned char mapping[256]; /* known fixed size */... xGetDeviceButtonMappingReply rep;... status = _XReply(dpy, (xReply *) & rep, 0, xFalse); if (status == 1) {
nbytes = (long)rep.length << 2; _XRead(dpy, (char *)mapping, nbytes);
... }...}
Typical bugs
20
XEventClass *XGetDeviceDontPropagateList( register Display *dpy, Window window, int *count){... xGetDeviceDontPropagateListReply rep;... if (!_XReply(dpy, (xReply *) & rep, 0, xFalse)) {... }...
list = (XEventClass *) Xmalloc(rep.length * sizeof(XEventClass)); if (list) {
... for (i = 0; i < rep.length; i++) {
_XRead(dpy, (char *)(&ec), sizeof(CARD32));list[i] = (XEventClass) ec;
}}
...}
Typical bugs
21
static int_XIPassiveGrabDevice(Display* dpy, int deviceid, int grabtype, int detail, Window grab_window, Cursor cursor, int grab_mode, int paired_device_mode, Bool owner_events, XIEventMask *mask, int num_modifiers, XIGrabModifiers *modifiers_inout){... if (!_XReply(dpy, (xReply *)&reply, 0, xFalse)) {... }
failed_mods = calloc(reply.num_modifiers, sizeof(xXIGrabModifierInfo));... _XRead(dpy, (char*)failed_mods, reply.num_modifiers * sizeof(xXIGrabModifierInfo));
for (i = 0; i < reply.num_modifiers; i++) { modifiers_inout[i].status = failed_mods[i].status; … }...}
Typical bugs
22
XineramaScreenInfo *XineramaQueryScreens( Display *dpy, int *number){ XExtDisplayInfo *info = find_display (dpy); xXineramaQueryScreensReply rep;....
if((scrnInfo = Xmalloc(sizeof(XineramaScreenInfo) * rep.number))) { ... for(i = 0; i < rep.number; i++) {
_XRead(dpy, (char*)(&scratch), sz_XineramaScreenInfo);scrnInfo[i].screen_number = i;scrnInfo[i].x_org = scratch.x_org;scrnInfo[i].y_org = scratch.y_org;scrnInfo[i].width = scratch.width;scrnInfo[i].height = scratch.height;
}... }...}
Typical bugs
23
XvImageFormatValues * XvListImageFormats ( Display *dpy, XvPortID port, int *num){... xvListImageFormatsReply rep;... if (_XReply(dpy, (xReply *)&rep, 0, xFalse) == 0) {... }... int size = (rep.num_formats * sizeof(XvImageFormatValues));... if((ret = Xmalloc(size))) { ...
for(i = 0; i < rep.num_formats; i++) { ...
} }...}
Typical bugs
24
BoolXF86VidModeGetGammaRamp ( Display *dpy, int screen, int size, unsigned short *red, unsigned short *green, unsigned short *blue){... xXF86VidModeGetGammaRampReply rep;... if (!_XReply (dpy, (xReply *) &rep, 0, xFalse)) {... } if(rep.size) {
_XRead(dpy, (char*)red, rep.size << 1); _XRead(dpy, (char*)green, rep.size << 1); _XRead(dpy, (char*)blue, rep.size << 1);
}... }
Typical bugs
25
BooluniDRIOpenConnection(dpy, screen, hSAREA, busIdString) Display *dpy; int screen; drm_handle_t *hSAREA; char **busIdString;{... xXF86DRIOpenConnectionReply rep;...
if (!(*busIdString = (char *)Xcalloc(rep.busIdStringLength + 1, 1))) { ...
}_XReadPad(dpy, *busIdString, rep.busIdStringLength);
...}
Typical bugs
26
static XcursorFileHeader *_XcursorReadFileHeader (XcursorFile *file){... if (!_XcursorReadUInt (file, &head.ntoc)) unsigned 32bit ntoc var read from file
return NULL;... fileHeader = _XcursorFileHeaderCreate (head.ntoc); passed on to allocate buffer ... for (n = 0; n < fileHeader->ntoc; n++) {
if (!_XcursorReadUInt (file, &fileHeader->tocs[n].type)) break;if (!_XcursorReadUInt (file, &fileHeader->tocs[n].subtype)) break;if (!_XcursorReadUInt (file, &fileHeader->tocs[n].position)) break;
}...}
Typical bugs
27
static XcursorFileHeader *_XcursorFileHeaderCreate (int ntoc){ XcursorFileHeader *fileHeader;
if (ntoc > 0x10000)return NULL;
fileHeader = malloc (sizeof (XcursorFileHeader) + ntoc * sizeof (XcursorFileToc));
if (!fileHeader)return NULL;
fileHeader->magic = XCURSOR_MAGIC; fileHeader->header = XCURSOR_FILE_HEADER_LEN; fileHeader->version = XCURSOR_FILE_VERSION; fileHeader->ntoc = ntoc; fileHeader->tocs = (XcursorFileToc *) (fileHeader + 1); return fileHeader;}
Typical bugs
28
static void ReqCleanup( Widget widget, XtPointer closure, XEvent *ev, Boolean *cont){... char *value;... (void) XGetWindowProperty(event->display, XtWindow(widget),
event->atom, 0L, 1000000, True, AnyPropertyType, &target, &format, &length, &bytesafter, (unsigned char **) &value); should check return
value.XFree(value);
...}
More observations
• The X client libraries (Xlib)• All discovered X bugs are being fixed • The developer involved is actually quite good
• Amazing • Alan Coopersmith• Very deep understanding of X and the bugs involved • No pushback, no handholding• Worked tirelessly • 104 patches, with reviews and some tests in < 3 months
• And had some interesting comments 29
30
“I don't know how many setuid X clients still exist these days (is xterm still setuid on any platforms, or did they all get grantpt() or similar calls to avoid needing root?), but since we know there's more X clients than we can keep track of (especially once you get to home grown apps in various companies they've been using for decades), we
have to assume there still may be some. It would be good to put a reminder in the security advisory that best
practice is to separate out the parts of an application that require elevated privileges from the GUI to avoid such
issues - GTK requires this, but not all toolkits do.”
More observations
More observations
31
“Shoot me now. And then shoot daniels for not freeing us from XKB yet.And then shoot anyone who volunteers to try to fix XKB, before it's toolate for them too.”“Here's my initial analysis of the first part of the Xlib set, until I got so tired my head started spinning trying to figure them out”
“Really, if your window shape is anywhere near 2^32 rectangles, what are you doing?”
“Yes, these [bugs] all seem possible, and far more feasible now than when this code was written, back when disk sizes were still measured in megabytes.”
More observations
• Comment on LWN from one of the people that introduced some of these bugs in the ‘80’s
32
More observations
• Debian turn around on these bugs was ridiculously fast!
• 104 patches to merge in • 2 week embargo • Full releases and advisory on day embargo expired
• No one else managed to do this• I think Moritz Mühlenhoff deserves most of the
credit for that one
33
More observations
• However, Raw X is rarely used nowadays • There’s stuff build on top• And they use raw X • gtk+ • KDE (which uses QT which uses raw X)• Rare direct calls to Xlib code • other
34
More observations
• KDE / QT• Crappyness is about on par with Xlib• Trivial bugs!
35
More observations
• Several instances of this
36
Qkeymapper_x11.cpp void QKeyMapperPrivate::clearMappings(){... uchar *data = 0; if (XGetWindowProperty(X11->display, RootWindow(X11->display, 0), ATOM(_XKB_RULES_NAMES), 0, 1024, false, XA_STRING, &type, &format, &nitems, &bytesAfter, &data) == Success && type == XA_STRING && format == 8 && nitems > 2) { ... char *names[5] = { 0, 0, 0, 0, 0 }; char *p = reinterpret_cast<char *>(data), *end = p + nitems; int i = 0; do { names[i++] = p; p += qstrlen(p) + 1; } while (p < end);...}
More observations
• In QT init code (affects all QT applications)
37
Qapplication_x11.cppvoid qt_init(QApplicationPrivate *priv, int,
Display *display, Qt::HANDLE visual, Qt::HANDLE colormap){... } else if (arg == "-name") { if (++i < argc) appName = argv[i]; if it was previously new'ed, it isn't anymore. }...}
void qt_cleanup(){... if (X11->foreignDisplay) { delete [] (char *)appName; could delete [] a pointer that isn't new'ed and possibly corrupt memory appName = 0; }...}
More observations
• So I reported these bugs• They didn’t seem to think it was a security issue• Quote from X developer (previous slide) is dead on
• Suid remark • QT does not seem to agree with this
38
More observations
• "KDE has precisely one setuid application, kcheckpass, for this reason. I suspect that someone running an suid Qt application would fall into a huge number of problems, the most obvious one being a malicious style that would allow them to trivially execute arbitrary code“
• Wait, did we just get a free Code exec bug from the QT security team ?
39
More observations
• I respond back, saying there are more KDE suid binaries, and specifically mention kppp, and question him on the styles thing
40
More observations
41
“> I am aware of this, regardless, this is library code, as such, chances are, there are suid applications out there that will use it.
That would be a security hole in those applications rather than in Qt, there are many ways that people can abuse a library to create unsafe applications. > Do styles contain executable code ? Yes.”
42
“> also, does kppp no longer run suid ? kppp should not be installed setuid. Here's a quote from its FAQ: "There is no need for the setuid bit, if you know a bit of UNIX® systems administration. Simply create a modem group, add all users that you want to give access to the modem to that group and make the modem device read/writable for that group." I doubt any modern distro would install it suid, in fact most are extremely careful about what they allow to be suid and are actively working to minimise what is.”
More observations
More observations
• That kppp FAQ quote in incomplete, it goes on to say:
43
“… The KPPP team has lately done a lot of work to make KPPP setuid-safe. But it's up to you to decide if you install and how you install it.”
More observations
• In fact, distros do still have it suid. • E.g. Ubuntu • This is library code! They should not set policy for
the apps that use them. • They’re sitting on the fence, because it’s easy
• you don’t actually have to do anything• Either defend it, and shut up • Or do a suid check and exit()
44
More observations
• None of those bugs are fixed• Got the ok from QT security team to disclose:
45
“> Ok, since you guys don't consider this a security issue, you're ok with me talking about this publicly? Yes, that's fine”
More observations
• So loaders have LD_PRELOAD • And has been made setuid safe years ago • KDE/QT
• QT_PLUGIN_PATH • Gnome
• GTK_MODULES• Neither are setuid safe !
46
More observations
• The GTK+ people seem to be doing somewhat better.
• They do not allow suid GTK+ applications. • And clearly explain why on their webpage
47
More observations (http://www.gtk.org/setuid.html)
Talking Radios born from taco trucks
48
More observations
• This is beautiful, well though out and sane!• “Security of GTK+ requires the security of Xlib. The
GTK+ team is not prepared to make that guarantee”
• Or is it ?
49
More observations
50
Gtkmain.c/* This checks to see if the process is running suid or sgid * at the current time. If so, we don't allow GTK+ to be initialized. * This is meant to be a mild check - we only error out if we * can prove the programmer is doing something wrong, not if * they could be doing something wrong. For this reason, we * don't use issetugid() on BSD or prctl (PR_GET_DUMPABLE). */static gbooleancheck_setugid (void)
More observations
• What does that mean ? • Suid binaries can use GTK+, BUT …• … they must acquire the privileged resources they
want first • And then drop privileges • After that it’s ok to use GTK+• Want to have their cake and eat it too• Check should be stronger!
51
More observations
• games are a great example • They are suid• Share a highscore database
• Once aquired, privs are dropped • Only thing an attacker would have access to is that db • assuming a bug was found and exploited
• That db is considered trusted. • Any security bug in db parsing allows for further escalation
• Any user now playing any of those games gets pwned52
More observations
• By far the most common bug when using Xlib
• Check return values !• XLib defense in depth fixes. Now guarantees NULL
init of arguments on failure.53
void fn(){... SomeFormat *sf;... (void) XGetWindowProperty(dpy, w, property, 0L,
10000000, False, SomePropertyType, &type, &format, &length, &bytesafter, (unsigned char **) &sf);
... XFree((char*)sf);...}
More observations
• Developers using Xlib don’t seem to realize that most of the api’s they use parse potentially untrusted network data
• _XReply
• _XRead32
• _XRead
• _XGetAsyncReply
• XGetWindowProperty
• XNextEvent
• XPeekEventXIfEvent
• XCheckIfEvent
• XPeekIfEvent
• XCheckTypedWindowEvent
• XSetErrorHandler
• XQueryFont
54
More observations
• derived from XGetWindowProperty:• XFetchName
• XGetIconName
• XGetSizeHints
• XGetWMHints
• XGetWMSizeHints
• XGetIconSizes
• XGetTransientForHint
• XGetClassHint
• XGetRGBColormaps
• XGetTextProperty
• XGetWMName
• XGetWMIconName
• XGetWMClientMachine
• XGetCommand
• XGetWMColormapWindows
• XGetWMProtocols
55
• XScreenResourceString• XFetchBuffer• XFetchBytes• XkbRF_GetNamesProp
More observations
• Conceptually there’s a couple of X suid apps around that you’ll see: • Config tools (e.g. kppp) • Games (e.g. swell foop)• Screen locking utils (e.g. Xlock, Xlockmore, Xscreensaver, …)
• Virtually all of these apps do drop privileges • Still holding on to privileged resources in most cases
56
Client summary
• Xlib in suids is a bad idea • GTK+ kinda sorta still allowed in suids • Very common sloppy misuse of Xlib api’s
57
58
Server
server
• Reviewing the X server code has been ongoing work since may-june
• Not finished • a LOT of code• I’m lazy• GLX is a horrible demotivator!
59
Server architecture
• Remember the X architecture layout ?
60
X network architecture
61
server• This is a lie!• X hasn’t looked like this in a very long time • Should watch Daniel Stone’s “The real story behind
wayland and X” talk (http://www.youtube.com/watch?v=RIctzAQOe44)
• X hasn’t been network transparant in a long time • Shared memory • DRI / DRM
• Direct rendering
• MMIO
• Kernel drivers / ioctl’s
62
63
Server: core protocol
• Let’s start with the beginning • X server parses core X protocol • All straight c code • This is 30 years old
• If it was written in the early 90’s iso 80’s, it would’ve probably been written in C++.
• This code is actually pretty cool (from a security perspective)
• You can actually see where the bugs got patched over time (e.g. integer overflow checks)
64
Server: core protocol
• Dix/dispatch.c (dispatch())• This is where all connections for the X server come in• client calls
• initial connection (e.g. set up byte order) proc• Establish connection (e.g. authorization) proc
• Client now gets to send core X protocol requests to server
• Requests are limited to ~64k (~16mb with big requests enabled)
65
Server: core protocol
• Content of requests depends on the request (TLV)• All of them begin with a:
• CARD8 type; • CARD8 data;• CARD16 length;
• Anything < 4 bytes won’t make it through. • Type 0-127, core protocol • Type 128-255, extensions
• Extensions get to pick and choose how they want their (sub)type delivered • Will usually take data for (sub)type
66
Server: core protocol
• Procedures clients can call• Dix/tables.c
• ProcVector• SwappedProcVector (different byteorder)• Create/destroy/query
• Windows • Atoms • Properties• Image• Text• …
67
Server: Extensions
• This is how people have worked around X11s old age and brokenness (while leaving the core protocol in tact)
• Extensions for • Input• Output • Shared memory • OpenGL • DRI• …
68
Server: Extensions
• Extensions get registered by calling AddExtension()
• Will add MainProc to ProcVector (>128)• Will add SwappedMainProc to SwappedProcVector• MinorOpcodeProc decides how extension wants it’s
type delivered
69
Server: Extensions
• Extensions can also add new resources • Types of handles basically
• Done by calling CreateNewResourceType()• Takes two arguments
• Delete function • Name
• Delete logic can get very complicated • Possible source of bugs
70
Server: Extensions
• Rought list of Extensions• Core protocol
• ProcListExtensions proc• XListExtensions() xlib api• Shows list of loaded extension
71
$ grep AddExtension * -r | awk 'BEGIN{FS=":";} {print $1;}' | grep ".*\.c" | sort | uniq
composite/compext.cdamageext/damageext.c
dbe/dbe.cdix/extension.c
glx/glxext.chw/dmx/dmx.c
hw/dmx/glxProxy/glxext.chw/kdrive/ephyr/ephyrdriext.c
hw/xfree86/dixmods/extmod/xf86dga2.chw/xfree86/dixmods/extmod/xf86vmode.c
hw/xfree86/dri/xf86dri.chw/xfree86/dri2/dri2ext.c
hw/xquartz/applewm.chw/xquartz/pseudoramiX.chw/xquartz/xpr/appledri.chw/xwin/winwindowswm.c
randr/randr.crandr/rrxinerama.c
record/record.crender/render.c
Xext/bigreq.cXext/dpms.cXext/geext.c
Xext/panoramiX.cXext/saver.c
Xext/security.cXext/shape.c
Xext/shm.cXext/sync.c
Xext/xcmisc.cXext/xf86bigfont.c
Xext/xres.cXext/xselinux_ext.c
Xext/xtest.cXext/xvmain.c
Xext/xvmc.cxfixes/xfixes.cXi/exevents.c
Xi/extinit.cXi/selectev.c
xkb/xkb.c
int main() { Display *dpy = XOpenDisplay(NULL); int nextensions, i = 0; char **p, **extensionList = XListExtensions(dpy, &nextensions); for(p = extensionList; nextensions; nextensions--, p++){i++; printf("extension %d: %s\n", i, *p); }}
Server: Extensions
• Prime attack surface • Have reviewed most of these • Some are small, some are huge
• Some select few were HUMONGOUS• Varying level of quality
• Some good, some bad• GLX was sheer terror
72
Server: Request procedure
• So what does a general request Proc look like ?
• xTestReq is a structure describing the request • REQUEST() is a macro that defines a structure of type
xTestReq called stuff (and points it to the request buffer)
73
intProcTest(ClientPtr client) { REQUEST(xTestReq); REQUEST_SIZE_MATCH(xTestReq); useRequestData(stuff); return Success;}
Server: Request procedure
• REQUEST_SIZE_MATCH(type) • Indicates that the request has to be exactly as big as the
structure • REQUEST_AT_LEAST_SIZE(type)
• Indicates that the request has to be at least as big as the structure
• REQUEST_FIXED_SIZE(type, len)• Indicates that the request has to be exactly as big as the
structure + len • Has implicit integer overflow!
• Use of these macro’s prevent out of bound read/write 74
Server: Type of bugs
• There’s some typical bugs that keep showing up when looking at a bunch of these request Proc’s
75
Server: Type of bugs
• Memory allocation • Forgetting to check return values • In OOM cases, can return NULL • Would crash on NULL deref
76
Server: Type of bugs
• Integer overflows • when parsing large files
• > 4gig files • Read it all in one large buffer
• Buffer either dynamically grows (int overflow) • Loop over file to calculate size (int overflow)
• When parsing protocol data• out of bound reads (bypassing length checks) • Memory corruption (when doing buffer length calculations)
• Malloc wrappers • Implicit in REQUEST_FIXED_SIZE
77
Server: Type of bugs
• Cute realloc bugs • realloc() with size of 0 becomes free • Leads to use after free and double free bugs
78
Server: Type of bugs
• Unvalidated length fields / index (as part of the protocol)
• Leads to out of bound reads and writes
79
Server: Type of bugs
• Out of bound reads • No validation for this whatsoever when accessing data
in the request
80
Server: Type of bugs
• Byte order bugs when byte order swapping is • Basically need to create a swap wrapper before the
actual proc • This one needs to do proper calls to
• REQUEST_SIZE_MATCH• REQUEST_AT_LEAST_SIZE• REQUEST_FIXED_SIZE
• Often not written as careful as the actual proc • Dev forgot to use size macro’s • Or used them too late81
Server: Type of bugs
• Byte order bugs • Memory corruption If size macro’s are missing (or too
late)• Since in place byte swap happens
82
Server: Type of bugs
• Pretty standard memory leaks • Do alloc • Forgot to free in error corner cases
83
Server:…
• This is about as far as I’ve come • I had hoped to gotten further but
• There’s a ton of code really • GLX !@#$%^&^@#@!@!$$#$#• No seriously, GLX is horrible
• The rest of the slides will just be general entry points
84
Server: Drivers
• There’s a whole range of drivers that can be used in one way or the other
• font drivers • drawable drivers • gc rivers • Server display driver• opengl drivers • DRI drivers
85
Server: Server display Drivers
• Wanted to look at these • Haven’t gotten to it yet
86
Server: Font Drivers
• These get registered with RegisterFPEFunctions • Have actually looked at the one that contains the font
client code • This was horribly outdated code • Riddled with trivial bugs• Noone really uses font servers anymore
87
Server: OpenGL Drivers
• I was getting to these • GLX talks to OpenGL drivers • Figuring out the amount of control over data being
send back and forth is probably tedious work • Need to look at hundres of Proc functions • Drivers would call _glapi_set_dispatch
88
Server: Library dependancies
• pixman• mesa• SDL • DRMLib • FreeType• Gallium3d
89
Server: DRI
• Direct Rendering infrastructure • framework: allows for direct hardware access by
clients • Much more efficient • Requires DRI extensions in X server • Requires DRM drivers in kernel
• DRM: Direct Rendering Manager • Can talk to DRM and DRM drivers through IOCTL’s
90
Server: DRI
91
Server: DRM
• Looked at linux implementation of DRM • And DRM drivers • Drivers/gpu/drm/drm_drv.c• Exposes an ioctl interface • The drm itself has a number of ioctls • The drm drivers can expose a number of ioctls • Standard linux ioctl’s are just a number • BSD ioctl’s encode stuff in the ioctl number (in, out, len) • DRM ioctl’s are like BSD ioctls92
Server: DRM
• Also adds a bit more than just BSD ioctls• DRM Drivers expose their ioctls as follows
• 3rd argument are flags93
struct drm_ioctl_desc radeon_ioctls[] = { DRM_IOCTL_DEF_DRV(RADEON_CP_INIT, radeon_cp_init, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF_DRV(RADEON_CP_START, radeon_cp_start, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF_DRV(RADEON_CP_STOP, radeon_cp_stop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF_DRV(RADEON_CP_RESET, radeon_cp_reset, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF_DRV(RADEON_CP_IDLE, radeon_cp_idle, DRM_AUTH), DRM_IOCTL_DEF_DRV(RADEON_CP_RESUME, radeon_cp_resume, DRM_AUTH), DRM_IOCTL_DEF_DRV(RADEON_RESET, radeon_engine_reset, DRM_AUTH), DRM_IOCTL_DEF_DRV(RADEON_FULLSCREEN, radeon_fullscreen, DRM_AUTH), DRM_IOCTL_DEF_DRV(RADEON_SWAP, radeon_cp_swap, DRM_AUTH),…};
Server: DRM
• Flag meaning / validation:
94
} else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) || ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated) || ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) || (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL)) || (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) {
Server: SHM
• X allows for sharing of images between client and server over shared memory
• Much faster than ipc/tcp• X Extension • Procs
• Version• Attach• Detach• Putimg• Get img• Create pixmap
95
Server: SHM
• Fonts can use shared memory too (xf86bigfont)• Number of other places
96
Server: SHM
• Would find typical shared memory bugs ??• Incorrect permissions (e.g. world read/write) • Race conditions ?
97
Server: Security
• The whole X server runs as root • There has been some talk of implementing priv sep / priv drop• Afaik noone has done this for xorg • OpenBSD did it!
• Xorg guys should steal that code!
• X Access Control Extension (XaceHooks)• Influenced by LSM • Allows drivers to implement a security model • E.g. which actions is allowed for which connection• Does anyone really use this ? • Comes from the NSA …98
How bad is it? Can we do better?
• Trivial bugs in clientside fixed by now Yay!• Plenty of bugs left in the server / extensions / drivers • Maybe the load of bugs reported will scare people into
adopting wayland faster ?• Even with Wayland, Xorg is going to be around for a long
time• These bugs need to get fixed
• If clientside fix rate is any indication, this will happen fast.• Need driving force to go audit for more bugs
• Long and tedious work
• Should really implement priv sep / priv drop99
Questions ?
100