From bb82c72a1d69eaf60b7586570faf797df967f661 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Mon, 29 Apr 2013 18:39:34 -0700 Subject: [PATCH 01/16] Expand comment on the memory vs. reply ordering in XIGetSelectedEvents() Unpacking from the wire involves un-interleaving the structs & masks, which wasn't obvious to me the first time I read it, so make notes before I forget again. Signed-off-by: Alan Coopersmith Signed-off-by: Peter Hutterer --- src/XISelEv.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/XISelEv.c b/src/XISelEv.c index fa7eb54..f871222 100644 --- a/src/XISelEv.c +++ b/src/XISelEv.c @@ -135,8 +135,14 @@ XIGetSelectedEvents(Display* dpy, Window win, int *num_masks_return) _XRead(dpy, (char*)mask_in, reply.length * 4); - /* Memory layout of the XIEventMask for a 3 mask reply: - * [struct a][struct b][struct c][masks a][masks b][masks c] + /* + * This function takes interleaved xXIEventMask structs & masks off + * the wire, such as this 3 mask reply: + * [struct a][masks a][struct b][masks b][struct c][masks c] + * And generates a memory buffer to be returned to callers in which + * they are not interleaved, so that callers can treat the returned + * pointer as a simple array of XIEventMask structs, such as: + * [struct a][struct b][struct c][masks a][masks b][masks c] */ len = reply.num_masks * sizeof(XIEventMask); -- 1.8.3.1 From 5d43d4914dcabb6de69859567061e99300e56ef4 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Fri, 17 May 2013 09:07:44 +1000 Subject: [PATCH 02/16] Copy the sequence number into the target event too (#64687) X.Org Bug 64687 Signed-off-by: Peter Hutterer Reviewed-by: Jasper St. Pierre --- src/XExtInt.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/XExtInt.c b/src/XExtInt.c index 507573b..8e19b97 100644 --- a/src/XExtInt.c +++ b/src/XExtInt.c @@ -915,6 +915,7 @@ static void xge_copy_to_cookie(xGenericEvent* ev, cookie->type = ev->type; cookie->evtype = ev->evtype; cookie->extension = ev->extension; + cookie->serial = ev->sequenceNumber; } static Bool @@ -1521,6 +1522,7 @@ wireToDeviceEvent(xXIDeviceEvent *in, XGenericEventCookie* cookie) out = next_block(&ptr_lib, sizeof(XIDeviceEvent)); out->display = cookie->display; out->type = in->type; + out->serial = in->sequenceNumber; out->extension = in->extension; out->evtype = in->evtype; out->send_event = ((in->type & 0x80) != 0); @@ -1793,6 +1795,7 @@ wireToDeviceChangedEvent(xXIDeviceChangedEvent *in, XGenericEventCookie *cookie) cookie->data = out = malloc(sizeof(XIDeviceChangedEvent) + len); out->type = in->type; + out->serial = in->sequenceNumber; out->display = cookie->display; out->extension = in->extension; out->evtype = in->evtype; @@ -1825,6 +1828,7 @@ wireToHierarchyChangedEvent(xXIHierarchyEvent *in, XGenericEventCookie *cookie) out->info = (XIHierarchyInfo*)&out[1]; out->display = cookie->display; out->type = in->type; + out->serial = in->sequenceNumber; out->extension = in->extension; out->evtype = in->evtype; out->send_event = ((in->type & 0x80) != 0); @@ -1865,6 +1869,7 @@ wireToRawEvent(XExtDisplayInfo *info, xXIRawEvent *in, XGenericEventCookie *cook out = next_block(&ptr, sizeof(XIRawEvent)); out->type = in->type; + out->serial = in->sequenceNumber; out->display = cookie->display; out->extension = in->extension; out->evtype = in->evtype; @@ -1915,6 +1920,7 @@ wireToEnterLeave(xXIEnterEvent *in, XGenericEventCookie *cookie) out->buttons.mask = (unsigned char*)&out[1]; out->type = in->type; + out->serial = in->sequenceNumber; out->display = cookie->display; out->extension = in->extension; out->evtype = in->evtype; @@ -1957,6 +1963,7 @@ wireToPropertyEvent(xXIPropertyEvent *in, XGenericEventCookie *cookie) cookie->data = out; out->type = in->type; + out->serial = in->sequenceNumber; out->extension = in->extension; out->evtype = in->evtype; out->send_event = ((in->type & 0x80) != 0); @@ -1977,6 +1984,7 @@ wireToTouchOwnershipEvent(xXITouchOwnershipEvent *in, cookie->data = out; out->type = in->type; + out->serial = in->sequenceNumber; out->display = cookie->display; out->extension = in->extension; out->evtype = in->evtype; @@ -2004,6 +2012,7 @@ wireToBarrierEvent(xXIBarrierEvent *in, XGenericEventCookie *cookie) out->display = cookie->display; out->type = in->type; + out->serial = in->sequenceNumber; out->extension = in->extension; out->evtype = in->evtype; out->send_event = ((in->type & 0x80) != 0); -- 1.8.3.1 From 59b8e1388a687f871831ac5a9e0ac11de75e2516 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Wed, 1 May 2013 23:58:39 -0700 Subject: [PATCH 03/16] Use _XEatDataWords to avoid overflow of rep.length bit shifting rep.length is a CARD32, so rep.length << 2 could overflow in 32-bit builds Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- configure.ac | 6 ++++++ src/XGMotion.c | 2 +- src/XGetDCtl.c | 2 +- src/XGetDProp.c | 5 ++--- src/XGetFCtl.c | 2 +- src/XGetKMap.c | 2 +- src/XGetMMap.c | 2 +- src/XGetProp.c | 4 +--- src/XGtSelect.c | 2 +- src/XIProperties.c | 7 +++---- src/XIint.h | 14 ++++++++++++++ src/XListDProp.c | 2 +- src/XListDev.c | 2 +- src/XOpenDev.c | 2 +- src/XQueryDv.c | 2 +- 15 files changed, 36 insertions(+), 20 deletions(-) diff --git a/configure.ac b/configure.ac index 8dbca38..f5ef1e2 100644 --- a/configure.ac +++ b/configure.ac @@ -31,6 +31,12 @@ PKG_CHECK_MODULES(XI, [xproto >= 7.0.13] [x11 >= 1.4.99.1] [xextproto >= 7.0.3] # CFLAGS only for PointerBarrier typedef PKG_CHECK_MODULES(XFIXES, [xfixes >= 5]) +# Check for _XEatDataWords function that may be patched into older Xlib releases +SAVE_LIBS="$LIBS" +LIBS="$XI_LIBS" +AC_CHECK_FUNCS([_XEatDataWords]) +LIBS="$SAVE_LIBS" + # Check for xmlto and asciidoc for man page conversion # (only needed by people building tarballs) if test "$have_xmlto" = yes && test "$have_asciidoc" = yes; then diff --git a/src/XGMotion.c b/src/XGMotion.c index 99b1c44..5feac85 100644 --- a/src/XGMotion.c +++ b/src/XGMotion.c @@ -112,7 +112,7 @@ XGetDeviceMotionEvents( Xfree(bufp); Xfree(savp); *nEvents = 0; - _XEatData(dpy, (unsigned long)size); + _XEatDataWords(dpy, rep.length); UnlockDisplay(dpy); SyncHandle(); return (NULL); diff --git a/src/XGetDCtl.c b/src/XGetDCtl.c index c66212d..f73a4e8 100644 --- a/src/XGetDCtl.c +++ b/src/XGetDCtl.c @@ -95,7 +95,7 @@ XGetDeviceControl( nbytes = (long)rep.length << 2; d = (xDeviceState *) Xmalloc((unsigned)nbytes); if (!d) { - _XEatData(dpy, (unsigned long)nbytes); + _XEatDataWords(dpy, rep.length); goto out; } sav = d; diff --git a/src/XGetDProp.c b/src/XGetDProp.c index 5d44f91..f9e8f0c 100644 --- a/src/XGetDProp.c +++ b/src/XGetDProp.c @@ -112,14 +112,13 @@ XGetDeviceProperty(Display* dpy, XDevice* dev, * This part of the code should never be reached. If it is, * the server sent back a property with an invalid format. */ - nbytes = rep.length << 2; - _XEatData(dpy, (unsigned long) nbytes); + _XEatDataWords(dpy, rep.length); UnlockDisplay(dpy); SyncHandle(); return(BadImplementation); } if (! *prop) { - _XEatData(dpy, (unsigned long) nbytes); + _XEatDataWords(dpy, rep.length); UnlockDisplay(dpy); SyncHandle(); return(BadAlloc); diff --git a/src/XGetFCtl.c b/src/XGetFCtl.c index 43afa00..28fab4d 100644 --- a/src/XGetFCtl.c +++ b/src/XGetFCtl.c @@ -95,7 +95,7 @@ XGetFeedbackControl( nbytes = (long)rep.length << 2; f = (xFeedbackState *) Xmalloc((unsigned)nbytes); if (!f) { - _XEatData(dpy, (unsigned long)nbytes); + _XEatDataWords(dpy, rep.length); goto out; } sav = f; diff --git a/src/XGetKMap.c b/src/XGetKMap.c index 9431fbb..00dde06 100644 --- a/src/XGetKMap.c +++ b/src/XGetKMap.c @@ -99,7 +99,7 @@ XGetDeviceKeyMapping(register Display * dpy, XDevice * dev, if (mapping) _XRead(dpy, (char *)mapping, nbytes); else - _XEatData(dpy, (unsigned long)nbytes); + _XEatDataWords(dpy, rep.length); } UnlockDisplay(dpy); diff --git a/src/XGetMMap.c b/src/XGetMMap.c index 8a1cdb2..ce10c2d 100644 --- a/src/XGetMMap.c +++ b/src/XGetMMap.c @@ -92,7 +92,7 @@ XGetDeviceModifierMapping( if (res->modifiermap) _XReadPad(dpy, (char *)res->modifiermap, nbytes); else - _XEatData(dpy, (unsigned long)nbytes); + _XEatDataWords(dpy, rep.length); res->max_keypermod = rep.numKeyPerModifier; } diff --git a/src/XGetProp.c b/src/XGetProp.c index c5d088b..34bc581 100644 --- a/src/XGetProp.c +++ b/src/XGetProp.c @@ -68,7 +68,6 @@ XGetDeviceDontPropagateList( int *count) { XEventClass *list = NULL; - int rlen; xGetDeviceDontPropagateListReq *req; xGetDeviceDontPropagateListReply rep; XExtDisplayInfo *info = XInput_find_display(dpy); @@ -90,7 +89,6 @@ XGetDeviceDontPropagateList( *count = rep.count; if (*count) { - rlen = rep.length << 2; list = (XEventClass *) Xmalloc(rep.length * sizeof(XEventClass)); if (list) { int i; @@ -105,7 +103,7 @@ XGetDeviceDontPropagateList( list[i] = (XEventClass) ec; } } else - _XEatData(dpy, (unsigned long)rlen); + _XEatDataWords(dpy, rep.length); } UnlockDisplay(dpy); diff --git a/src/XGtSelect.c b/src/XGtSelect.c index f890db7..5c0f812 100644 --- a/src/XGtSelect.c +++ b/src/XGtSelect.c @@ -104,7 +104,7 @@ XGetSelectedExtensionEvents( (XEventClass *) Xmalloc(*this_client_count * sizeof(XEventClass)); if (!*this_client_list) { - _XEatData(dpy, (unsigned long)tlen + alen); + _XEatDataWords(dpy, rep.length); UnlockDisplay(dpy); SyncHandle(); return (Success); diff --git a/src/XIProperties.c b/src/XIProperties.c index 83a7a68..5e58fb6 100644 --- a/src/XIProperties.c +++ b/src/XIProperties.c @@ -64,7 +64,7 @@ XIListProperties(Display* dpy, int deviceid, int *num_props_return) props = (Atom*)Xmalloc(rep.num_properties * sizeof(Atom)); if (!props) { - _XEatData(dpy, rep.num_properties << 2); + _XEatDataWords(dpy, rep.length); goto cleanup; } @@ -203,8 +203,7 @@ XIGetProperty(Display* dpy, int deviceid, Atom property, long offset, * This part of the code should never be reached. If it is, * the server sent back a property with an invalid format. */ - nbytes = rep.length << 2; - _XEatData(dpy, nbytes); + _XEatDataWords(dpy, rep.length); UnlockDisplay(dpy); SyncHandle(); return(BadImplementation); @@ -222,7 +221,7 @@ XIGetProperty(Display* dpy, int deviceid, Atom property, long offset, *data = Xmalloc(rbytes); if (!(*data)) { - _XEatData(dpy, nbytes); + _XEatDataWords(dpy, rep.length); UnlockDisplay(dpy); SyncHandle(); return(BadAlloc); diff --git a/src/XIint.h b/src/XIint.h index 571bb23..3ddc3c5 100644 --- a/src/XIint.h +++ b/src/XIint.h @@ -83,4 +83,18 @@ next_block(void **ptr, int size) { return ret; } +#ifndef HAVE__XEATDATAWORDS +#include /* for LONG64 on 64-bit platforms */ +#include + +static inline void _XEatDataWords(Display *dpy, unsigned long n) +{ +# ifndef LONG64 + if (n >= (ULONG_MAX >> 2)) + _XIOError(dpy); +# endif + _XEatData (dpy, n << 2); +} +#endif + #endif diff --git a/src/XListDProp.c b/src/XListDProp.c index 8667350..bde6cb5 100644 --- a/src/XListDProp.c +++ b/src/XListDProp.c @@ -65,7 +65,7 @@ XListDeviceProperties(Display* dpy, XDevice* dev, int *nprops_return) props = (Atom*)Xmalloc(rep.nAtoms * sizeof(Atom)); if (!props) { - _XEatData(dpy, rep.nAtoms << 2); + _XEatDataWords(dpy, rep.length); goto cleanup; } diff --git a/src/XListDev.c b/src/XListDev.c index bd6e70a..1fa4747 100644 --- a/src/XListDev.c +++ b/src/XListDev.c @@ -202,7 +202,7 @@ XListInputDevices( list = (xDeviceInfo *) Xmalloc(rlen); slist = list; if (!slist) { - _XEatData(dpy, (unsigned long)rlen); + _XEatDataWords(dpy, rep.length); UnlockDisplay(dpy); SyncHandle(); return (XDeviceInfo *) NULL; diff --git a/src/XOpenDev.c b/src/XOpenDev.c index 74f18ac..e784f8b 100644 --- a/src/XOpenDev.c +++ b/src/XOpenDev.c @@ -101,7 +101,7 @@ XOpenDevice( if (rlen - dlen > 0) _XEatData(dpy, (unsigned long)rlen - dlen); } else - _XEatData(dpy, (unsigned long)rlen); + _XEatDataWords(dpy, rep.length); UnlockDisplay(dpy); SyncHandle(); diff --git a/src/XQueryDv.c b/src/XQueryDv.c index 24d4e4e..69c285b 100644 --- a/src/XQueryDv.c +++ b/src/XQueryDv.c @@ -91,7 +91,7 @@ XQueryDeviceState( if (rlen > 0) { data = Xmalloc(rlen); if (!data) { - _XEatData(dpy, (unsigned long)rlen); + _XEatDataWords(dpy, rep.length); goto out; } _XRead(dpy, data, rlen); -- 1.8.3.1 From f3e08e4fbe40016484ba795feecf1a742170ffc1 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sat, 9 Mar 2013 22:26:52 -0800 Subject: [PATCH 04/16] Stack buffer overflow in XGetDeviceButtonMapping() [CVE-2013-1998 1/3] We copy the entire reply sent by the server into the fixed size mapping[] array on the stack, even if the server says it's a larger size than the mapping array can hold. HULK SMASH STACK! Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- src/XGetBMap.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/XGetBMap.c b/src/XGetBMap.c index 211c9ca..002daba 100644 --- a/src/XGetBMap.c +++ b/src/XGetBMap.c @@ -60,6 +60,7 @@ SOFTWARE. #include #include #include "XIint.h" +#include #ifdef MIN /* some systems define this in */ #undef MIN @@ -75,7 +76,6 @@ XGetDeviceButtonMapping( { int status = 0; unsigned char mapping[256]; /* known fixed size */ - long nbytes; XExtDisplayInfo *info = XInput_find_display(dpy); register xGetDeviceButtonMappingReq *req; @@ -92,13 +92,18 @@ XGetDeviceButtonMapping( status = _XReply(dpy, (xReply *) & rep, 0, xFalse); if (status == 1) { - nbytes = (long)rep.length << 2; - _XRead(dpy, (char *)mapping, nbytes); - - /* don't return more data than the user asked for. */ - if (rep.nElts) - memcpy((char *)map, (char *)mapping, MIN((int)rep.nElts, nmap)); - status = rep.nElts; + if (rep.length <= (sizeof(mapping) >> 2)) { + unsigned long nbytes = rep.length << 2; + _XRead(dpy, (char *)mapping, nbytes); + + /* don't return more data than the user asked for. */ + if (rep.nElts) + memcpy(map, mapping, MIN((int)rep.nElts, nmap)); + status = rep.nElts; + } else { + _XEatDataWords(dpy, rep.length); + status = 0; + } } else status = 0; UnlockDisplay(dpy); -- 1.8.3.1 From 91434737f592e8f5cc1762383882a582b55fc03a Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sat, 9 Mar 2013 23:37:23 -0800 Subject: [PATCH 05/16] memory corruption in _XIPassiveGrabDevice() [CVE-2013-1998 2/3] If the server returned more modifiers than the caller asked for, we'd just keep copying past the end of the array provided by the caller, writing over who-knows-what happened to be there. Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- src/XIPassiveGrab.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/XIPassiveGrab.c b/src/XIPassiveGrab.c index ac17c01..53b4084 100644 --- a/src/XIPassiveGrab.c +++ b/src/XIPassiveGrab.c @@ -88,7 +88,7 @@ _XIPassiveGrabDevice(Display* dpy, int deviceid, int grabtype, int detail, return -1; _XRead(dpy, (char*)failed_mods, reply.num_modifiers * sizeof(xXIGrabModifierInfo)); - for (i = 0; i < reply.num_modifiers; i++) + for (i = 0; i < reply.num_modifiers && i < num_modifiers; i++) { modifiers_inout[i].status = failed_mods[i].status; modifiers_inout[i].modifiers = failed_mods[i].modifiers; -- 1.8.3.1 From 5398ac0797f7516f2c9b8f2869a6c6d071437352 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Fri, 26 Apr 2013 22:48:36 -0700 Subject: [PATCH 06/16] unvalidated lengths in XQueryDeviceState() [CVE-2013-1998 3/3] If the lengths given for each class state in the reply add up to more than the rep.length, we could read past the end of the buffer allocated to hold the data read from the server. Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- src/XQueryDv.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/XQueryDv.c b/src/XQueryDv.c index 69c285b..3836777 100644 --- a/src/XQueryDv.c +++ b/src/XQueryDv.c @@ -59,6 +59,7 @@ SOFTWARE. #include #include #include "XIint.h" +#include XDeviceState * XQueryDeviceState( @@ -66,8 +67,8 @@ XQueryDeviceState( XDevice *dev) { int i, j; - int rlen; - int size = 0; + unsigned long rlen; + size_t size = 0; xQueryDeviceStateReq *req; xQueryDeviceStateReply rep; XDeviceState *state = NULL; @@ -87,9 +88,11 @@ XQueryDeviceState( if (!_XReply(dpy, (xReply *) & rep, 0, xFalse)) goto out; - rlen = rep.length << 2; - if (rlen > 0) { - data = Xmalloc(rlen); + if (rep.length > 0) { + if (rep.length < (INT_MAX >> 2)) { + rlen = (unsigned long) rep.length << 2; + data = Xmalloc(rlen); + } if (!data) { _XEatDataWords(dpy, rep.length); goto out; @@ -97,6 +100,10 @@ XQueryDeviceState( _XRead(dpy, data, rlen); for (i = 0, any = (XInputClass *) data; i < (int)rep.num_classes; i++) { + if (any->length > rlen) + goto out; + rlen -= any->length; + switch (any->class) { case KeyClass: size += sizeof(XKeyState); -- 1.8.3.1 From b0b13c12a8079a5a0e7f43b2b8983699057b2cec Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sat, 9 Mar 2013 22:55:23 -0800 Subject: [PATCH 07/16] integer overflow in XGetDeviceControl() [CVE-2013-1984 1/8] If the number of valuators reported by the server is large enough that it overflows when multiplied by the size of the appropriate struct, then memory corruption can occur when more bytes are copied from the X server reply than the size of the buffer we allocated to hold them. v2: check that reply size fits inside the data read from the server, so we don't read out of bounds either Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- src/XGetDCtl.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/src/XGetDCtl.c b/src/XGetDCtl.c index f73a4e8..51ed0ae 100644 --- a/src/XGetDCtl.c +++ b/src/XGetDCtl.c @@ -61,6 +61,7 @@ SOFTWARE. #include #include #include "XIint.h" +#include XDeviceControl * XGetDeviceControl( @@ -68,8 +69,6 @@ XGetDeviceControl( XDevice *dev, int control) { - int size = 0; - int nbytes, i; XDeviceControl *Device = NULL; XDeviceControl *Sav = NULL; xDeviceState *d = NULL; @@ -92,8 +91,12 @@ XGetDeviceControl( goto out; if (rep.length > 0) { - nbytes = (long)rep.length << 2; - d = (xDeviceState *) Xmalloc((unsigned)nbytes); + unsigned long nbytes; + size_t size = 0; + if (rep.length < (INT_MAX >> 2)) { + nbytes = (unsigned long) rep.length << 2; + d = Xmalloc(nbytes); + } if (!d) { _XEatDataWords(dpy, rep.length); goto out; @@ -111,33 +114,46 @@ XGetDeviceControl( case DEVICE_RESOLUTION: { xDeviceResolutionState *r; + size_t val_size; r = (xDeviceResolutionState *) d; - size += sizeof(XDeviceResolutionState) + - (3 * sizeof(int) * r->num_valuators); + if (r->num_valuators >= (INT_MAX / (3 * sizeof(int)))) + goto out; + val_size = 3 * sizeof(int) * r->num_valuators; + if ((sizeof(xDeviceResolutionState) + val_size) > nbytes) + goto out; + size += sizeof(XDeviceResolutionState) + val_size; break; } case DEVICE_ABS_CALIB: { + if (sizeof(xDeviceAbsCalibState) > nbytes) + goto out; size += sizeof(XDeviceAbsCalibState); break; } case DEVICE_ABS_AREA: { + if (sizeof(xDeviceAbsAreaState) > nbytes) + goto out; size += sizeof(XDeviceAbsAreaState); break; } case DEVICE_CORE: { + if (sizeof(xDeviceCoreState) > nbytes) + goto out; size += sizeof(XDeviceCoreState); break; } default: + if (d->length > nbytes) + goto out; size += d->length; break; } - Device = (XDeviceControl *) Xmalloc((unsigned)size); + Device = Xmalloc(size); if (!Device) goto out; @@ -150,6 +166,7 @@ XGetDeviceControl( int *iptr, *iptr2; xDeviceResolutionState *r; XDeviceResolutionState *R; + unsigned int i; r = (xDeviceResolutionState *) d; R = (XDeviceResolutionState *) Device; -- 1.8.3.1 From 322ee3576789380222d4403366e4fd12fb24cb6a Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sat, 9 Mar 2013 22:55:23 -0800 Subject: [PATCH 08/16] integer overflow in XGetFeedbackControl() [CVE-2013-1984 2/8] If the number of feedbacks reported by the server is large enough that it overflows when multiplied by the size of the appropriate struct, or if the total size of all the feedback structures overflows when added together, then memory corruption can occur when more bytes are copied from the X server reply than the size of the buffer we allocated to hold them. v2: check that reply size fits inside the data read from the server, so we don't read out of bounds either Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- src/XGetFCtl.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/src/XGetFCtl.c b/src/XGetFCtl.c index 28fab4d..bb50bf3 100644 --- a/src/XGetFCtl.c +++ b/src/XGetFCtl.c @@ -61,6 +61,7 @@ SOFTWARE. #include #include #include "XIint.h" +#include XFeedbackState * XGetFeedbackControl( @@ -68,8 +69,6 @@ XGetFeedbackControl( XDevice *dev, int *num_feedbacks) { - int size = 0; - int nbytes, i; XFeedbackState *Feedback = NULL; XFeedbackState *Sav = NULL; xFeedbackState *f = NULL; @@ -91,9 +90,16 @@ XGetFeedbackControl( goto out; if (rep.length > 0) { + unsigned long nbytes; + size_t size = 0; + int i; + *num_feedbacks = rep.num_feedbacks; - nbytes = (long)rep.length << 2; - f = (xFeedbackState *) Xmalloc((unsigned)nbytes); + + if (rep.length < (INT_MAX >> 2)) { + nbytes = rep.length << 2; + f = Xmalloc(nbytes); + } if (!f) { _XEatDataWords(dpy, rep.length); goto out; @@ -102,6 +108,10 @@ XGetFeedbackControl( _XRead(dpy, (char *)f, nbytes); for (i = 0; i < *num_feedbacks; i++) { + if (f->length > nbytes) + goto out; + nbytes -= f->length; + switch (f->class) { case KbdFeedbackClass: size += sizeof(XKbdFeedbackState); @@ -116,6 +126,8 @@ XGetFeedbackControl( { xStringFeedbackState *strf = (xStringFeedbackState *) f; + if (strf->num_syms_supported >= (INT_MAX / sizeof(KeySym))) + goto out; size += sizeof(XStringFeedbackState) + (strf->num_syms_supported * sizeof(KeySym)); } @@ -130,10 +142,12 @@ XGetFeedbackControl( size += f->length; break; } + if (size > INT_MAX) + goto out; f = (xFeedbackState *) ((char *)f + f->length); } - Feedback = (XFeedbackState *) Xmalloc((unsigned)size); + Feedback = Xmalloc(size); if (!Feedback) goto out; -- 1.8.3.1 From 6dd6dc51a2935c72774be81e5cc2ba2c30e9feff Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sat, 9 Mar 2013 22:55:23 -0800 Subject: [PATCH 09/16] integer overflow in XGetDeviceDontPropagateList() [CVE-2013-1984 3/8] If the number of event classes reported by the server is large enough that it overflows when multiplied by the size of the appropriate struct, then memory corruption can occur when more bytes are copied from the X server reply than the size of the buffer we allocated to hold them. V2: EatData if count is 0 but length is > 0 to avoid XIOErrors Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- src/XGetProp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/XGetProp.c b/src/XGetProp.c index 34bc581..b49328c 100644 --- a/src/XGetProp.c +++ b/src/XGetProp.c @@ -60,6 +60,7 @@ SOFTWARE. #include #include #include "XIint.h" +#include XEventClass * XGetDeviceDontPropagateList( @@ -88,10 +89,11 @@ XGetDeviceDontPropagateList( } *count = rep.count; - if (*count) { - list = (XEventClass *) Xmalloc(rep.length * sizeof(XEventClass)); + if (rep.length != 0) { + if ((rep.count != 0) && (rep.length < (INT_MAX / sizeof(XEventClass)))) + list = Xmalloc(rep.length * sizeof(XEventClass)); if (list) { - int i; + unsigned int i; CARD32 ec; /* read and assign each XEventClass separately because -- 1.8.3.1 From bb922ed4253b35590f0369f32a917ff89ade0830 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sat, 9 Mar 2013 22:55:23 -0800 Subject: [PATCH 10/16] integer overflow in XGetDeviceMotionEvents() [CVE-2013-1984 4/8] If the number of events or axes reported by the server is large enough that it overflows when multiplied by the size of the appropriate struct, then memory corruption can occur when more bytes are copied from the X server reply than the size of the buffer we allocated to hold them. Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- src/XGMotion.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/XGMotion.c b/src/XGMotion.c index 5feac85..a4c75b6 100644 --- a/src/XGMotion.c +++ b/src/XGMotion.c @@ -59,6 +59,7 @@ SOFTWARE. #include #include #include "XIint.h" +#include XDeviceTimeCoord * XGetDeviceMotionEvents( @@ -74,7 +75,7 @@ XGetDeviceMotionEvents( xGetDeviceMotionEventsReply rep; XDeviceTimeCoord *tc; int *data, *bufp, *readp, *savp; - long size, size2; + unsigned long size; int i, j; XExtDisplayInfo *info = XInput_find_display(dpy); @@ -104,10 +105,21 @@ XGetDeviceMotionEvents( SyncHandle(); return (NULL); } - size = rep.length << 2; - size2 = rep.nEvents * (sizeof(XDeviceTimeCoord) + (rep.axes * sizeof(int))); - savp = readp = (int *)Xmalloc(size); - bufp = (int *)Xmalloc(size2); + if (rep.length < (INT_MAX >> 2)) { + size = rep.length << 2; + savp = readp = Xmalloc(size); + } else { + size = 0; + savp = readp = NULL; + } + /* rep.axes is a CARD8, so assume max number of axes for bounds check */ + if (rep.nEvents < + (INT_MAX / (sizeof(XDeviceTimeCoord) + (UCHAR_MAX * sizeof(int))))) { + size_t bsize = rep.nEvents * + (sizeof(XDeviceTimeCoord) + (rep.axes * sizeof(int))); + bufp = Xmalloc(bsize); + } else + bufp = NULL; if (!bufp || !savp) { Xfree(bufp); Xfree(savp); -- 1.8.3.1 From 242f92b490a695fbab244af5bad11b71f897c732 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sat, 9 Mar 2013 22:55:23 -0800 Subject: [PATCH 11/16] integer overflow in XIGetProperty() [CVE-2013-1984 5/8] If the number of items reported by the server is large enough that it overflows when multiplied by the size of the appropriate item type, then memory corruption can occur when more bytes are copied from the X server reply than the size of the buffer we allocated to hold them. Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- src/XIProperties.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/XIProperties.c b/src/XIProperties.c index 5e58fb6..32436d1 100644 --- a/src/XIProperties.c +++ b/src/XIProperties.c @@ -38,6 +38,7 @@ #include #include #include "XIint.h" +#include Atom* XIListProperties(Display* dpy, int deviceid, int *num_props_return) @@ -170,7 +171,7 @@ XIGetProperty(Display* dpy, int deviceid, Atom property, long offset, { xXIGetPropertyReq *req; xXIGetPropertyReply rep; - long nbytes, rbytes; + unsigned long nbytes, rbytes; XExtDisplayInfo *info = XInput_find_display(dpy); @@ -216,9 +217,11 @@ XIGetProperty(Display* dpy, int deviceid, Atom property, long offset, * recopy the string to make it null terminated. */ - nbytes = rep.num_items * rep.format/8; - rbytes = nbytes + 1; - *data = Xmalloc(rbytes); + if (rep.num_items < (INT_MAX / (rep.format/8))) { + nbytes = rep.num_items * rep.format/8; + rbytes = nbytes + 1; + *data = Xmalloc(rbytes); + } if (!(*data)) { _XEatDataWords(dpy, rep.length); -- 1.8.3.1 From 528419b9ef437e7eeafb41bf45e8ff7d818bd845 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sat, 9 Mar 2013 22:55:23 -0800 Subject: [PATCH 12/16] integer overflow in XIGetSelectedEvents() [CVE-2013-1984 6/8] If the number of events or masks reported by the server is large enough that it overflows when multiplied by the size of the appropriate struct, or the sizes overflow as they are totaled up, then memory corruption can occur when more bytes are copied from the X server reply than the size of the buffer we allocated to hold them. v2: check that reply size fits inside the data read from the server, so that we don't read out of bounds either Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- src/XISelEv.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/XISelEv.c b/src/XISelEv.c index f871222..0471bef 100644 --- a/src/XISelEv.c +++ b/src/XISelEv.c @@ -42,6 +42,7 @@ in this Software without prior written authorization from the author. #include #include #include "XIint.h" +#include int XISelectEvents(Display* dpy, Window win, XIEventMask* masks, int num_masks) @@ -101,13 +102,14 @@ out: XIEventMask* XIGetSelectedEvents(Display* dpy, Window win, int *num_masks_return) { - int i, len = 0; + unsigned int i, len = 0; unsigned char *mask; XIEventMask *mask_out = NULL; xXIEventMask *mask_in = NULL, *mi; xXIGetSelectedEventsReq *req; xXIGetSelectedEventsReply reply; XExtDisplayInfo *info = XInput_find_display(dpy); + size_t rbytes; *num_masks_return = -1; LockDisplay(dpy); @@ -129,11 +131,16 @@ XIGetSelectedEvents(Display* dpy, Window win, int *num_masks_return) goto out; } - mask_in = Xmalloc(reply.length * 4); - if (!mask_in) + if (reply.length < (INT_MAX >> 2)) { + rbytes = (unsigned long) reply.length << 2; + mask_in = Xmalloc(rbytes); + } + if (!mask_in) { + _XEatDataWords(dpy, reply.length); goto out; + } - _XRead(dpy, (char*)mask_in, reply.length * 4); + _XRead(dpy, (char*)mask_in, rbytes); /* * This function takes interleaved xXIEventMask structs & masks off @@ -148,8 +155,14 @@ XIGetSelectedEvents(Display* dpy, Window win, int *num_masks_return) for (i = 0, mi = mask_in; i < reply.num_masks; i++) { - len += mi->mask_len * 4; - mi = (xXIEventMask*)((char*)mi + mi->mask_len * 4); + unsigned int mask_bytes = mi->mask_len * 4; + len += mask_bytes; + if (len > INT_MAX) + goto out; + if ((sizeof(xXIEventMask) + mask_bytes) > rbytes) + goto out; + rbytes -= (sizeof(xXIEventMask) + mask_bytes); + mi = (xXIEventMask*)((char*)mi + mask_bytes); mi++; } -- 1.8.3.1 From 17071c1c608247800b2ca03a35b1fcc9c4cabe6c Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sun, 10 Mar 2013 13:30:55 -0700 Subject: [PATCH 13/16] Avoid integer overflow in XGetDeviceProperties() [CVE-2013-1984 7/8] If the number of items as reported by the Xserver is too large, it could overflow the calculation for the size of the buffer to copy the reply into, causing memory corruption. Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- src/XGetDProp.c | 61 ++++++++++++++++++++++++++++++++++----------------------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/src/XGetDProp.c b/src/XGetDProp.c index f9e8f0c..3691122 100644 --- a/src/XGetDProp.c +++ b/src/XGetDProp.c @@ -38,6 +38,7 @@ in this Software without prior written authorization from the author. #include #include #include "XIint.h" +#include int XGetDeviceProperty(Display* dpy, XDevice* dev, @@ -48,7 +49,8 @@ XGetDeviceProperty(Display* dpy, XDevice* dev, { xGetDevicePropertyReq *req; xGetDevicePropertyReply rep; - long nbytes, rbytes; + unsigned long nbytes, rbytes; + int ret = Success; XExtDisplayInfo *info = XInput_find_display(dpy); @@ -81,30 +83,43 @@ XGetDeviceProperty(Display* dpy, XDevice* dev, * data, but this last byte is null terminated and convenient for * returning string properties, so the client doesn't then have to * recopy the string to make it null terminated. + * + * Maximum item limits are set to both prevent integer overflow when + * calculating the amount of memory to malloc, and to limit how much + * memory will be used if a server provides an insanely high count. */ switch (rep.format) { case 8: - nbytes = rep.nItems; - rbytes = rep.nItems + 1; - if (rbytes > 0 && - (*prop = (unsigned char *) Xmalloc ((unsigned)rbytes))) - _XReadPad (dpy, (char *) *prop, nbytes); + if (rep.nItems < INT_MAX) { + nbytes = rep.nItems; + rbytes = rep.nItems + 1; + if ((*prop = Xmalloc (rbytes))) + _XReadPad (dpy, (char *) *prop, nbytes); + else + ret = BadAlloc; + } break; case 16: - nbytes = rep.nItems << 1; - rbytes = rep.nItems * sizeof (short) + 1; - if (rbytes > 0 && - (*prop = (unsigned char *) Xmalloc ((unsigned)rbytes))) - _XRead16Pad (dpy, (short *) *prop, nbytes); + if (rep.nItems < (INT_MAX / sizeof (short))) { + nbytes = rep.nItems << 1; + rbytes = rep.nItems * sizeof (short) + 1; + if ((*prop = Xmalloc (rbytes))) + _XRead16Pad (dpy, (short *) *prop, nbytes); + else + ret = BadAlloc; + } break; case 32: - nbytes = rep.nItems << 2; - rbytes = rep.nItems * sizeof (long) + 1; - if (rbytes > 0 && - (*prop = (unsigned char *) Xmalloc ((unsigned)rbytes))) - _XRead32 (dpy, (long *) *prop, nbytes); + if (rep.nItems < (INT_MAX / sizeof (long))) { + nbytes = rep.nItems << 2; + rbytes = rep.nItems * sizeof (long) + 1; + if ((*prop = Xmalloc (rbytes))) + _XRead32 (dpy, (long *) *prop, nbytes); + else + ret = BadAlloc; + } break; default: @@ -112,16 +127,13 @@ XGetDeviceProperty(Display* dpy, XDevice* dev, * This part of the code should never be reached. If it is, * the server sent back a property with an invalid format. */ - _XEatDataWords(dpy, rep.length); - UnlockDisplay(dpy); - SyncHandle(); - return(BadImplementation); + ret = BadImplementation; } if (! *prop) { _XEatDataWords(dpy, rep.length); - UnlockDisplay(dpy); - SyncHandle(); - return(BadAlloc); + if (ret == Success) + ret = BadAlloc; + goto out; } (*prop)[rbytes - 1] = '\0'; } @@ -130,9 +142,10 @@ XGetDeviceProperty(Display* dpy, XDevice* dev, *actual_format = rep.format; *nitems = rep.nItems; *bytes_after = rep.bytesAfter; + out: UnlockDisplay (dpy); SyncHandle (); - return Success; + return ret; } -- 1.8.3.1 From ef82512288d8ca36ac0beeb289f158195b0a8cae Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sun, 10 Mar 2013 00:22:14 -0800 Subject: [PATCH 14/16] Avoid integer overflow in XListInputDevices() [CVE-2013-1984 8/8] If the length of the reply as reported by the Xserver is too long, it could overflow the calculation for the size of the buffer to copy the reply into, causing memory corruption. Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- src/XListDev.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/XListDev.c b/src/XListDev.c index 1fa4747..1c14b96 100644 --- a/src/XListDev.c +++ b/src/XListDev.c @@ -60,6 +60,7 @@ SOFTWARE. #include #include #include "XIint.h" +#include /* Calculate length field to a multiples of sizeof(XID). XIDs are typedefs * to ulong and thus may be 8 bytes on some platforms. This can trigger a @@ -179,7 +180,7 @@ XListInputDevices( XAnyClassPtr Any; char *nptr, *Nptr; int i; - long rlen; + unsigned long rlen; XExtDisplayInfo *info = XInput_find_display(dpy); LockDisplay(dpy); @@ -198,9 +199,10 @@ XListInputDevices( if ((*ndevices = rep.ndevices)) { /* at least 1 input device */ size = *ndevices * sizeof(XDeviceInfo); - rlen = rep.length << 2; /* multiply length by 4 */ - list = (xDeviceInfo *) Xmalloc(rlen); - slist = list; + if (rep.length < (INT_MAX >> 2)) { + rlen = rep.length << 2; /* multiply length by 4 */ + slist = list = Xmalloc(rlen); + } if (!slist) { _XEatDataWords(dpy, rep.length); UnlockDisplay(dpy); -- 1.8.3.1 From 81b4df8ac6aa1520c41c3526961014a6f115cc46 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sun, 10 Mar 2013 00:16:22 -0800 Subject: [PATCH 15/16] sign extension issue in XListInputDevices() [CVE-2013-1995] nptr is (signed) char, which can be negative, and will sign extend when added to the int size, which means size can be subtracted from, leading to allocating too small a buffer to hold the data being copied from the X server's reply. v2: check that string size fits inside the data read from the server, so that we don't read out of bounds either Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- src/XListDev.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/XListDev.c b/src/XListDev.c index 1c14b96..b85ff3c 100644 --- a/src/XListDev.c +++ b/src/XListDev.c @@ -73,7 +73,7 @@ static int pad_to_xid(int base_size) return ((base_size + padsize - 1)/padsize) * padsize; } -static int +static size_t SizeClassInfo(xAnyClassPtr *any, int num_classes) { int size = 0; @@ -170,7 +170,7 @@ XListInputDevices( register Display *dpy, int *ndevices) { - int size; + size_t size; xListInputDevicesReq *req; xListInputDevicesReply rep; xDeviceInfo *list, *slist = NULL; @@ -178,7 +178,7 @@ XListInputDevices( XDeviceInfo *clist = NULL; xAnyClassPtr any, sav_any; XAnyClassPtr Any; - char *nptr, *Nptr; + unsigned char *nptr, *Nptr; int i; unsigned long rlen; XExtDisplayInfo *info = XInput_find_display(dpy); @@ -217,9 +217,12 @@ XListInputDevices( size += SizeClassInfo(&any, (int)list->num_classes); } - for (i = 0, nptr = (char *)any; i < *ndevices; i++) { + Nptr = ((unsigned char *)list) + rlen + 1; + for (i = 0, nptr = (unsigned char *)any; i < *ndevices; i++) { size += *nptr + 1; nptr += (*nptr + 1); + if (nptr > Nptr) + goto out; } clist = (XDeviceInfoPtr) Xmalloc(size); @@ -245,8 +248,8 @@ XListInputDevices( } clist = sclist; - nptr = (char *)any; - Nptr = (char *)Any; + nptr = (unsigned char *)any; + Nptr = (unsigned char *)Any; for (i = 0; i < *ndevices; i++, clist++) { clist->name = (char *)Nptr; memcpy(Nptr, nptr + 1, *nptr); @@ -256,6 +259,7 @@ XListInputDevices( } } + out: XFree((char *)slist); UnlockDisplay(dpy); SyncHandle(); -- 1.8.3.1 From 661c45ca17c434dbd342a46fd3fb813852ae0ca9 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Tue, 21 May 2013 12:23:05 +1000 Subject: [PATCH 16/16] Don't overwrite the cookies serial number serial != sequenceNumber, see _XSetLastRequestRead() cookie->serial is already set at this point, setting it again directly from the sequenceNumber of the event causes a bunch of weird issues such as scrollbars and text drag-n-drop breaking. https://bugzilla.redhat.com/show_bug.cgi?id=965347 Signed-off-by: Peter Hutterer --- src/XExtInt.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/XExtInt.c b/src/XExtInt.c index 8e19b97..d3c6b7c 100644 --- a/src/XExtInt.c +++ b/src/XExtInt.c @@ -915,7 +915,6 @@ static void xge_copy_to_cookie(xGenericEvent* ev, cookie->type = ev->type; cookie->evtype = ev->evtype; cookie->extension = ev->extension; - cookie->serial = ev->sequenceNumber; } static Bool -- 1.8.3.1