From b6fe1a7af34ea620e002fc453f9c5eacf7db3969 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sat, 9 Mar 2013 13:48:28 -0800 Subject: integer overflow in DMXGetWindowAttributes() [CVE-2013-1992 2/3] If the server provided screenCount causes integer overflow when multiplied by the size of each array element, a smaller buffer would be allocated than the amount of data written to it. Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith --- src/dmx.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/src/dmx.c b/src/dmx.c index 15a6650..67434c8 100644 --- a/src/dmx.c +++ b/src/dmx.c @@ -524,6 +524,7 @@ Bool DMXGetWindowAttributes(Display *dpy, Window window, CARD32 *windows; /* Must match protocol size */ XRectangle *pos; /* Must match protocol size */ XRectangle *vis; /* Must match protocol size */ + Bool ret = False; DMXCheckExtension(dpy, info, False); @@ -538,11 +539,30 @@ Bool DMXGetWindowAttributes(Display *dpy, Window window, return False; } - /* FIXME: check for NULL? */ - screens = Xmalloc(rep.screenCount * sizeof(*screens)); - windows = Xmalloc(rep.screenCount * sizeof(*windows)); - pos = Xmalloc(rep.screenCount * sizeof(*pos)); - vis = Xmalloc(rep.screenCount * sizeof(*vis)); + /* + * rep.screenCount is a CARD32 so could be as large as 2^32 + * The X11 protocol limits the total screen size to 64k x 64k, + * and no screen can be smaller than a pixel. While technically + * that means we could theoretically reach 2^32 screens, and that's + * not even taking overlap into account, 64k seems far larger than + * any reasonable configuration, so we limit to that to prevent both + * integer overflow in the size calculations, and bad X server + * responses causing massive memory allocation. + */ + if (rep.screenCount < 65536) { + screens = Xmalloc(rep.screenCount * sizeof(*screens)); + windows = Xmalloc(rep.screenCount * sizeof(*windows)); + pos = Xmalloc(rep.screenCount * sizeof(*pos)); + vis = Xmalloc(rep.screenCount * sizeof(*vis)); + } else { + screens = windows = NULL; + pos = vis = NULL; + } + + if (!screens || !windows || !pos || !vis) { + _XEatDataWords(dpy, rep.length); + goto end; + } _XRead(dpy, (char *)screens, rep.screenCount * sizeof(*screens)); _XRead(dpy, (char *)windows, rep.screenCount * sizeof(*windows)); @@ -558,7 +578,9 @@ Bool DMXGetWindowAttributes(Display *dpy, Window window, inf->pos = pos[current]; inf->vis = vis[current]; } + ret = True; + end: Xfree(vis); Xfree(pos); Xfree(windows); @@ -566,7 +588,7 @@ Bool DMXGetWindowAttributes(Display *dpy, Window window, UnlockDisplay(dpy); SyncHandle(); - return True; + return ret; } /** If the DMXGetDesktopAttributes protocol request returns information -- cgit v1.2.1