From 1c7ad6773ce6be00dcd6e51e9be08f203abe5071 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Fri, 3 May 2013 23:29:22 -0700
Subject: [PATCH:libXrandr 1/6] 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]>
---
configure.ac | 6 ++++++
src/Xrandrint.h | 13 +++++++++++++
src/XrrCrtc.c | 6 +++---
src/XrrOutput.c | 2 +-
src/XrrProperty.c | 9 ++++-----
src/XrrProvider.c | 4 ++--
src/XrrProviderProperty.c | 9 ++++-----
src/XrrScreen.c | 2 +-
8 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/configure.ac b/configure.ac
index 3f28bef..8466999 100644
--- a/configure.ac
+++ b/configure.ac
@@ -55,6 +55,12 @@ AC_SUBST(RANDR_VERSION)
# Obtain compiler/linker options for depedencies
PKG_CHECK_MODULES(RANDR, x11 randrproto >= $RANDR_VERSION xext xextproto xrender renderproto)
+# Check for _XEatDataWords function that may be patched into older Xlib release
+SAVE_LIBS="$LIBS"
+LIBS="$RANDR_LIBS"
+AC_CHECK_FUNCS([_XEatDataWords])
+LIBS="$SAVE_LIBS"
+
AC_CONFIG_FILES([Makefile
src/Makefile
man/Makefile
diff --git a/src/Xrandrint.h b/src/Xrandrint.h
index aed10e4..1687c29 100644
--- a/src/Xrandrint.h
+++ b/src/Xrandrint.h
@@ -42,6 +42,19 @@ extern char XRRExtensionName[];
XExtDisplayInfo *XRRFindDisplay (Display *dpy);
+#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
/* deliberately opaque internal data structure; can be extended,
but not reordered */
diff --git a/src/XrrCrtc.c b/src/XrrCrtc.c
index 04087c5..a704a52 100644
--- a/src/XrrCrtc.c
+++ b/src/XrrCrtc.c
@@ -74,7 +74,7 @@ XRRGetCrtcInfo (Display *dpy, XRRScreenResources *resources, RRCrtc crtc)
xci = (XRRCrtcInfo *) Xmalloc(rbytes);
if (xci == NULL) {
- _XEatData (dpy, (unsigned long) nbytes);
+ _XEatDataWords (dpy, rep.length);
UnlockDisplay (dpy);
SyncHandle ();
return NULL;
@@ -203,7 +203,7 @@ XRRGetCrtcGamma (Display *dpy, RRCrtc crtc)
if (!crtc_gamma)
{
- _XEatData (dpy, (unsigned long) nbytes);
+ _XEatDataWords (dpy, rep.length);
goto out;
}
_XRead16 (dpy, crtc_gamma->red, rep.size * 2);
@@ -397,7 +397,7 @@ XRRGetCrtcTransform (Display *dpy,
int extraBytes = rep.length * 4 - CrtcTransformExtra;
extra = Xmalloc (extraBytes);
if (!extra) {
- _XEatData (dpy, extraBytes);
+ _XEatDataWords (dpy, rep.length - (CrtcTransformExtra >> 2));
UnlockDisplay (dpy);
SyncHandle ();
return False;
diff --git a/src/XrrOutput.c b/src/XrrOutput.c
index f13a932..4df894e 100644
--- a/src/XrrOutput.c
+++ b/src/XrrOutput.c
@@ -81,7 +81,7 @@ XRRGetOutputInfo (Display *dpy, XRRScreenResources *resources, RROutput output)
xoi = (XRROutputInfo *) Xmalloc(rbytes);
if (xoi == NULL) {
- _XEatData (dpy, (unsigned long) nbytes);
+ _XEatDataWords (dpy, rep.length - (OutputInfoExtra >> 2));
UnlockDisplay (dpy);
SyncHandle ();
return NULL;
diff --git a/src/XrrProperty.c b/src/XrrProperty.c
index 4c3fdb0..2b065b2 100644
--- a/src/XrrProperty.c
+++ b/src/XrrProperty.c
@@ -62,7 +62,7 @@ XRRListOutputProperties (Display *dpy, RROutput output, int *nprop)
props = (Atom *) Xmalloc (rbytes);
if (props == NULL) {
- _XEatData (dpy, nbytes);
+ _XEatDataWords (dpy, rep.length);
UnlockDisplay (dpy);
SyncHandle ();
*nprop = 0;
@@ -107,7 +107,7 @@ XRRQueryOutputProperty (Display *dpy, RROutput output, Atom property)
prop_info = (XRRPropertyInfo *) Xmalloc (rbytes);
if (prop_info == NULL) {
- _XEatData (dpy, nbytes);
+ _XEatDataWords(dpy, rep.length);
UnlockDisplay (dpy);
SyncHandle ();
return NULL;
@@ -313,14 +313,13 @@ XRRGetOutputProperty (Display *dpy, RROutput output,
* 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/XrrScreen.c b/src/XrrScreen.c
index f830913..08710b6 100644
--- a/src/XrrScreen.c
+++ b/src/XrrScreen.c
@@ -129,7 +129,7 @@ doGetScreenResources (Display *dpy, Window window, int poll)
if (xrsr == NULL || wire_names == NULL) {
if (xrsr) Xfree (xrsr);
if (wire_names) Xfree (wire_names);
- _XEatData (dpy, (unsigned long) nbytes);
+ _XEatDataWords (dpy, rep.length);
UnlockDisplay (dpy);
SyncHandle ();
return NULL;
--
1.7.9.2
From 0e79d96c36aef5889ae2e2a3fc2e96e93f30dc21 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Fri, 12 Apr 2013 21:44:59 -0700
Subject: [PATCH:libXrandr 2/6] integer overflow in XRRQueryOutputProperty()
[CVE-2013-1986 1/4]
rep.length is a CARD32, while rbytes was a signed int, so
rbytes = sizeof (XRRPropertyInfo) + rep.length * sizeof (long);
could result in integer overflow, leading to an undersized malloc
and reading data off the connection and writing it past the end of
the allocated buffer.
Reported-by: Ilja Van Sprundel <[email protected]>
Signed-off-by: Alan Coopersmith <[email protected]>
---
src/XrrProperty.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/XrrProperty.c b/src/XrrProperty.c
index 2b065b2..50382bf 100644
--- a/src/XrrProperty.c
+++ b/src/XrrProperty.c
@@ -31,6 +31,7 @@
#include <X11/extensions/render.h>
#include <X11/extensions/Xrender.h>
#include "Xrandrint.h"
+#include <limits.h>
Atom *
XRRListOutputProperties (Display *dpy, RROutput output, int *nprop)
@@ -84,7 +85,7 @@ XRRQueryOutputProperty (Display *dpy, RROutput output, Atom property)
XExtDisplayInfo *info = XRRFindDisplay(dpy);
xRRQueryOutputPropertyReply rep;
xRRQueryOutputPropertyReq *req;
- int rbytes, nbytes;
+ unsigned int rbytes, nbytes;
XRRPropertyInfo *prop_info;
RRCheckExtension (dpy, info, NULL);
@@ -102,10 +103,14 @@ XRRQueryOutputProperty (Display *dpy, RROutput output, Atom property)
return NULL;
}
- rbytes = sizeof (XRRPropertyInfo) + rep.length * sizeof (long);
- nbytes = rep.length << 2;
+ if (rep.length < ((INT_MAX / sizeof(long)) - sizeof (XRRPropertyInfo))) {
+ rbytes = sizeof (XRRPropertyInfo) + (rep.length * sizeof (long));
+ nbytes = rep.length << 2;
+
+ prop_info = Xmalloc (rbytes);
+ } else
+ prop_info = NULL;
- prop_info = (XRRPropertyInfo *) Xmalloc (rbytes);
if (prop_info == NULL) {
_XEatDataWords(dpy, rep.length);
UnlockDisplay (dpy);
--
1.7.9.2
From 289a1927949e6f278c18d115772e454837702e35 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Sat, 4 May 2013 21:37:49 -0700
Subject: [PATCH:libXrandr 4/6] integer overflow in XRRGetOutputProperty()
[CVE-2013-1986 3/4]
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.
(Same as reported against libX11 XGetWindowProperty by Ilja Van Sprundel)
Signed-off-by: Alan Coopersmith <[email protected]>
---
src/XrrProperty.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/XrrProperty.c b/src/XrrProperty.c
index 50382bf..707a28d 100644
--- a/src/XrrProperty.c
+++ b/src/XrrProperty.c
@@ -257,7 +257,7 @@ XRRGetOutputProperty (Display *dpy, RROutput output,
XExtDisplayInfo *info = XRRFindDisplay(dpy);
xRRGetOutputPropertyReply rep;
xRRGetOutputPropertyReq *req;
- long nbytes, rbytes;
+ unsigned long nbytes, rbytes;
RRCheckExtension (dpy, info, 1);
@@ -282,34 +282,40 @@ XRRGetOutputProperty (Display *dpy, RROutput output,
*prop = (unsigned char *) NULL;
if (rep.propertyType != None) {
+ int format = rep.format;
+
+ /*
+ * Protect against both integer overflow and just plain oversized
+ * memory allocation - no server should ever return this many props.
+ */
+ if (rep.nItems >= (INT_MAX >> 4))
+ format = -1; /* fall through to default error case */
+
/*
* One extra byte is malloced than is needed to contain the property
* 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.
*/
- switch (rep.format) {
+ switch (format) {
case 8:
nbytes = rep.nItems;
rbytes = rep.nItems + 1;
- if (rbytes > 0 &&
- (*prop = (unsigned char *) Xmalloc ((unsigned)rbytes)))
+ if (rbytes > 0 && (*prop = Xmalloc (rbytes)))
_XReadPad (dpy, (char *) *prop, nbytes);
break;
case 16:
nbytes = rep.nItems << 1;
rbytes = rep.nItems * sizeof (short) + 1;
- if (rbytes > 0 &&
- (*prop = (unsigned char *) Xmalloc ((unsigned)rbytes)))
+ if (rbytes > 0 && (*prop = Xmalloc (rbytes)))
_XRead16Pad (dpy, (short *) *prop, nbytes);
break;
case 32:
nbytes = rep.nItems << 2;
rbytes = rep.nItems * sizeof (long) + 1;
- if (rbytes > 0 &&
- (*prop = (unsigned char *) Xmalloc ((unsigned)rbytes)))
+ if (rbytes > 0 && (*prop = Xmalloc (rbytes)))
_XRead32 (dpy, (long *) *prop, nbytes);
break;
--
1.7.9.2
From c90f74497dbcb96854346435349c6e2207b530c5 Mon Sep 17 00:00:00 2001
From: Alan Coopersmith <[email protected]>
Date: Sat, 4 May 2013 21:47:50 -0700
Subject: [PATCH:libXrandr 6/6] Make XRRGet*Property() always initialize
returned values
Avoids memory corruption and other errors when callers access them
without checking to see if the calls returned an error value.
Callers are still required to check for errors, this just reduces the
damage when they don't.
(Same as reported against libX11 XGetWindowProperty by Ilja Van Sprundel)
Signed-off-by: Alan Coopersmith <[email protected]>
---
src/XrrProperty.c | 8 +++++++-
src/XrrProviderProperty.c | 8 +++++++-
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/src/XrrProperty.c b/src/XrrProperty.c
index 707a28d..2096c56 100644
--- a/src/XrrProperty.c
+++ b/src/XrrProperty.c
@@ -259,6 +259,13 @@ XRRGetOutputProperty (Display *dpy, RROutput output,
xRRGetOutputPropertyReq *req;
unsigned long nbytes, rbytes;
+ /* 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 = *bytes_after = 0L;
+ *prop = (unsigned char *) NULL;
+
RRCheckExtension (dpy, info, 1);
LockDisplay (dpy);
@@ -280,7 +287,6 @@ XRRGetOutputProperty (Display *dpy, RROutput output,
return ((xError *)&rep)->errorCode;
}
- *prop = (unsigned char *) NULL;
if (rep.propertyType != None) {
int format = rep.format;