From 361d36770ba3ceef0272e53c59c169f16f16ecf6 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Fri, 8 Mar 2013 22:25:35 -0800 Subject: integer overflow in XGetWindowProperty() [CVE-2013-1981 10/13] If the reported number of properties is too large, the calculations to allocate memory for them may overflow, leaving us returning less memory to the caller than implied by the value written to *nitems. Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Reviewed-by: Matthieu Herrb Signed-off-by: Julien Cristau Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/GetProp.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) (limited to 'nx-X11/lib/X11/GetProp.c') diff --git a/nx-X11/lib/X11/GetProp.c b/nx-X11/lib/X11/GetProp.c index a80c19c50..a9c357d69 100644 --- a/nx-X11/lib/X11/GetProp.c +++ b/nx-X11/lib/X11/GetProp.c @@ -28,6 +28,7 @@ in this Software without prior written authorization from The Open Group. #include #endif #include "Xlibint.h" +#include int XGetWindowProperty( @@ -66,8 +67,17 @@ XGetWindowProperty( *prop = (unsigned char *) NULL; if (reply.propertyType != None) { - long nbytes, netbytes; - switch (reply.format) { + unsigned long nbytes, netbytes; + int format = reply.format; + + /* + * Protect against both integer overflow and just plain oversized + * memory allocation - no server should ever return this many props. + */ + if (reply.nItems >= (INT_MAX >> 4)) + format = -1; /* fall through to default error case */ + + switch (format) { /* * One extra byte is malloced than is needed to contain the property * data, but this last byte is null terminated and convenient for @@ -76,24 +86,21 @@ XGetWindowProperty( */ case 8: nbytes = netbytes = reply.nItems; - if (nbytes + 1 > 0 && - (*prop = (unsigned char *) Xmalloc ((unsigned)nbytes + 1))) + if (nbytes + 1 > 0 && (*prop = Xmalloc (nbytes + 1))) _XReadPad (dpy, (char *) *prop, netbytes); break; case 16: nbytes = reply.nItems * sizeof (short); netbytes = reply.nItems << 1; - if (nbytes + 1 > 0 && - (*prop = (unsigned char *) Xmalloc ((unsigned)nbytes + 1))) + if (nbytes + 1 > 0 && (*prop = Xmalloc (nbytes + 1))) _XRead16Pad (dpy, (short *) *prop, netbytes); break; case 32: nbytes = reply.nItems * sizeof (long); netbytes = reply.nItems << 2; - if (nbytes + 1 > 0 && - (*prop = (unsigned char *) Xmalloc ((unsigned)nbytes + 1))) + if (nbytes + 1 > 0 && (*prop = Xmalloc (nbytes + 1))) _XRead32 (dpy, (long *) *prop, netbytes); break; @@ -115,7 +122,7 @@ XGetWindowProperty( break; } if (! *prop) { - _XEatData(dpy, (unsigned long) netbytes); + _XEatDataWords(dpy, reply.length); UnlockDisplay(dpy); SyncHandle(); return(BadAlloc); /* not Success */ -- cgit v1.2.3 From e03f3922a3d97174916273e369b2e9a90f53a05a Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sat, 9 Mar 2013 11:04:37 -0800 Subject: Make XGetWindowProperty() always initialize returned values Avoids memory corruption and other errors when callers access them without checking to see if XGetWindowProperty() returned an error value. Callers are still required to check for errors, this just reduces the damage when they don't. Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Reviewed-by: Matthieu Herrb Signed-off-by: Julien Cristau Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/GetProp.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'nx-X11/lib/X11/GetProp.c') diff --git a/nx-X11/lib/X11/GetProp.c b/nx-X11/lib/X11/GetProp.c index a9c357d69..4149199f5 100644 --- a/nx-X11/lib/X11/GetProp.c +++ b/nx-X11/lib/X11/GetProp.c @@ -49,6 +49,13 @@ XGetWindowProperty( register xGetPropertyReq *req; xError error; + /* Always initialize return values, in case callers fail to initialize + them and fail to check the return code for an error. */ + *actual_type = None; + *actual_format = 0; + *nitems = *bytesafter = 0L; + *prop = (unsigned char *) NULL; + LockDisplay(dpy); GetReq (GetProperty, req); req->window = w; @@ -65,7 +72,6 @@ XGetWindowProperty( return (1); /* not Success */ } - *prop = (unsigned char *) NULL; if (reply.propertyType != None) { unsigned long nbytes, netbytes; int format = reply.format; -- cgit v1.2.3