From bb82c72a1d69eaf60b7586570faf797df967f661 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Mon, 29 Apr 2013 18:39:34 -0700
Subject: [PATCH:libXi] 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 <[email protected]>
Signed-off-by: Peter Hutterer <[email protected]>
---
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.7.9.2
From 63841a81a340f1979cf5b0ffa1f7047dca988994 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Wed, 1 May 2013 23:58:39 -0700
Subject: [PATCH:libXi 01/13] 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 <[email protected]>
Reviewed-by: Peter Hutterer <[email protected]>
---
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 <X11/Xmd.h> /* for LONG64 on 64-bit platforms */
+#include <limits.h>
+
+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.7.9.2
From 0acd07e1c9583e2e7c26c13f8628bde992d9bce5 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Sat, 9 Mar 2013 22:26:52 -0800
Subject: [PATCH:libXi 02/13] 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 <[email protected]>
Signed-off-by: Alan Coopersmith <[email protected]>
Reviewed-by: Peter Hutterer <[email protected]>
---
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 <X11/extensions/XInput.h>
#include <X11/extensions/extutil.h>
#include "XIint.h"
+#include <limits.h>
#ifdef MIN /* some systems define this in <sys/param.h> */
#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.7.9.2
From 18855aa8c261863c00690b7df1e5ce4660687fb6 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Sat, 9 Mar 2013 23:37:23 -0800
Subject: [PATCH:libXi 03/13] 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 <[email protected]>
Reviewed-by: Peter Hutterer <[email protected]>
---
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.7.9.2
From a74690ba880036e01fc39f522b7567daf3746a31 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Fri, 26 Apr 2013 22:48:36 -0700
Subject: [PATCH:libXi 04/13] 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 <[email protected]>
Reviewed-by: Peter Hutterer <[email protected]>
---
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 <X11/extensions/XInput.h>
#include <X11/extensions/extutil.h>
#include "XIint.h"
+#include <limits.h>
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.7.9.2
From 38b563078ee050086526294f42f9f162be4bf97d Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Sat, 9 Mar 2013 22:55:23 -0800
Subject: [PATCH:libXi 05/13] 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 <[email protected]>
Signed-off-by: Alan Coopersmith <[email protected]>
Reviewed-by: Peter Hutterer <[email protected]>
---
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 <X11/extensions/XInput.h>
#include <X11/extensions/extutil.h>
#include "XIint.h"
+#include <limits.h>
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.7.9.2
From 9e39ad6e66a517c2324426c3aa32f4cfcc1cd4f5 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Sat, 9 Mar 2013 22:55:23 -0800
Subject: [PATCH:libXi 06/13] 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 <[email protected]>
Signed-off-by: Alan Coopersmith <[email protected]>
Reviewed-by: Peter Hutterer <[email protected]>
---
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 <X11/extensions/XInput.h>
#include <X11/extensions/extutil.h>
#include "XIint.h"
+#include <limits.h>
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.7.9.2
From 0bb14773ba37dfa8098b3e1784c80658019f9f21 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Sat, 9 Mar 2013 22:55:23 -0800
Subject: [PATCH:libXi 07/13] 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 <[email protected]>
Signed-off-by: Alan Coopersmith <[email protected]>
Reviewed-by: Peter Hutterer <[email protected]>
---
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 <X11/extensions/XInput.h>
#include <X11/extensions/extutil.h>
#include "XIint.h"
+#include <limits.h>
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.7.9.2
From 3d1fc7cd03e9a41b86cd83a8922df551a1b87179 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Sat, 9 Mar 2013 22:55:23 -0800
Subject: [PATCH:libXi 08/13] 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 <[email protected]>
Signed-off-by: Alan Coopersmith <[email protected]>
Reviewed-by: Peter Hutterer <[email protected]>
---
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 <X11/extensions/XInput.h>
#include <X11/extensions/extutil.h>
#include "XIint.h"
+#include <limits.h>
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.7.9.2
From 1202a8fc208581e184d327118a8a8ac3a27bbe64 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Sat, 9 Mar 2013 22:55:23 -0800
Subject: [PATCH:libXi 09/13] 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 <[email protected]>
Signed-off-by: Alan Coopersmith <[email protected]>
Reviewed-by: Peter Hutterer <[email protected]>
---
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 <X11/extensions/XInput2.h>
#include <X11/extensions/extutil.h>
#include "XIint.h"
+#include <limits.h>
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.7.9.2
From a3e76afd699b7afa590c8d3567663460c9870924 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Sat, 9 Mar 2013 22:55:23 -0800
Subject: [PATCH:libXi 10/13] 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 <[email protected]>
Signed-off-by: Alan Coopersmith <[email protected]>
Reviewed-by: Peter Hutterer <[email protected]>
---
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 <X11/extensions/ge.h>
#include <X11/extensions/geproto.h>
#include "XIint.h"
+#include <limits.h>
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.7.9.2
From b6246afd838842d28ac94d901507390624f31c7d Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Sun, 10 Mar 2013 13:30:55 -0700
Subject: [PATCH:libXi 11/13] 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 <[email protected]>
Reviewed-by: Peter Hutterer <[email protected]>
---
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 <X11/extensions/XInput.h>
#include <X11/extensions/extutil.h>
#include "XIint.h"
+#include <limits.h>
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.7.9.2
From 03f9746651d8d67c6d449737bd1681eb394b094c Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Sun, 10 Mar 2013 00:22:14 -0800
Subject: [PATCH:libXi 12/13] 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 <[email protected]>
Reviewed-by: Peter Hutterer <[email protected]>
---
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 <X11/extensions/XInput.h>
#include <X11/extensions/extutil.h>
#include "XIint.h"
+#include <limits.h>
/* 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.7.9.2
From 5019f7a8f06b6826cd197d8def6feb841f1490ec Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Sun, 10 Mar 2013 00:16:22 -0800
Subject: [PATCH:libXi 13/13] 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 <[email protected]>
Signed-off-by: Alan Coopersmith <[email protected]>
Reviewed-by: Peter Hutterer <[email protected]>
---
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.7.9.2