Multiple unvalidated assumptions in XvMCGetDRInfo() [CVE-2013-1999]

The individual string sizes is assumed to not be more than the amount of
data read from the network, and could cause buffer overflow if they are.

The strings returned from the X server are assumed to be null terminated,
and could cause callers to read past the end of the buffer if they are not.

Also be sure to set the returned pointers to NULL, so callers don't try
accessing bad pointers on failure cases.

Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>

1 files changed, 19 insertions, 17 deletions

diff --git a/src/XvMC.c b/src/XvMC.c
index d8bc59d..cb42487 100644
--- a/src/XvMC.c
+++ b/src/XvMC.c
@@ -501,3 +501,2 @@ Status XvMCGetDRInfo(Display *dpy, XvPortID port,
     xvmcGetDRInfoReq  *req;
-    char *tmpBuf = NULL;
     CARD32 magic;
@@ -512,2 +511,5 @@ Status XvMCGetDRInfo(Display *dpy, XvPortID port,
 
+    *name = NULL;
+    *busID = NULL;
+
     XvMCCheckExtension (dpy, info, BadImplementation);
@@ -570,16 +572,11 @@ Status XvMCGetDRInfo(Display *dpy, XvPortID port,
     if (rep.length > 0) {
-
-        int realSize = rep.length << 2;
-
-	tmpBuf = (char *) Xmalloc(realSize);
-	if (tmpBuf) {
-	    *name = (char *) Xmalloc(rep.nameLen);
-	    if (*name) {
-		*busID = (char *) Xmalloc(rep.busIDLen);
-		if (! *busID) {
-		    XFree(*name);
-		    XFree(tmpBuf);
-		}
-	    } else {
-		XFree(tmpBuf);
+	unsigned long realSize = 0;
+	char *tmpBuf = NULL;
+
+	if (rep.length < (INT_MAX >> 2)) {
+	    realSize = rep.length << 2;
+	    if (realSize >= (rep.nameLen + rep.busIDLen)) {
+		tmpBuf = Xmalloc(realSize);
+		*name = Xmalloc(rep.nameLen);
+		*busID = Xmalloc(rep.busIDLen);
 	    }
@@ -588,9 +585,14 @@ Status XvMCGetDRInfo(Display *dpy, XvPortID port,
 	if (*name && *busID && tmpBuf) {
-
 	    _XRead(dpy, tmpBuf, realSize);
 	    strncpy(*name,tmpBuf,rep.nameLen);
+	    name[rep.nameLen - 1] = '\0';
 	    strncpy(*busID,tmpBuf+rep.nameLen,rep.busIDLen);
+	    busID[rep.busIDLen - 1] = '\0';
 	    XFree(tmpBuf);
-
 	} else {
+	    XFree(*name);
+	    *name = NULL;
+	    XFree(*busID);
+	    *name = NULL;
+	    XFree(tmpBuf);