Out of boundary access and endless loop in libXtst

A lack of range checks in libXtst allows out of boundary accesses.
The checks have to be done in-place here, because it cannot be done
without in-depth knowledge of the read data.

If XRecordStartOfData, XRecordEndOfData, or XRecordClientDied
without a client sequence have attached data, an endless loop would
occur. The do-while-loop continues until the current index reaches
the end. But in these cases, the current index would not be
incremented, leading to an endless processing.

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Reviewed-by: Matthieu Herrb <matthieu@herrb.eu>

1 files changed, 39 insertions, 4 deletions

diff --git a/src/XRecord.c b/src/XRecord.c
index 50420c0..fefd842 100644
--- a/src/XRecord.c
+++ b/src/XRecord.c
@@ -749,15 +749,23 @@ parse_reply_call_callback(
 	switch (rep->category) {
 	case XRecordFromServer:
 	    if (rep->elementHeader&XRecordFromServerTime) {
+		if (current_index + 4 > rep->length << 2)
+		    return Error;
 		EXTRACT_CARD32(rep->clientSwapped,
 			       reply->buf+current_index,
 			       data->server_time);
 		current_index += 4;
 	    }
+	    if (current_index + 1 > rep->length << 2)
+		return Error;
 	    switch (reply->buf[current_index]) {
 	    case X_Reply: /* reply */
+		if (current_index + 8 > rep->length << 2)
+		    return Error;
 		EXTRACT_CARD32(rep->clientSwapped,
 			       reply->buf+current_index+4, datum_bytes);
+		if (datum_bytes < 0 || datum_bytes > ((INT_MAX >> 2) - 8))
+		    return Error;
 		datum_bytes = (datum_bytes+8) << 2;
 		break;
 	    default: /* error or event */
@@ -766,52 +774,73 @@ parse_reply_call_callback(
 	    break;
 	case XRecordFromClient:
 	    if (rep->elementHeader&XRecordFromClientTime) {
+		if (current_index + 4 > rep->length << 2)
+		    return Error;
 		EXTRACT_CARD32(rep->clientSwapped,
 			       reply->buf+current_index,
 			       data->server_time);
 		current_index += 4;
 	    }
 	    if (rep->elementHeader&XRecordFromClientSequence) {
+		if (current_index + 4 > rep->length << 2)
+		    return Error;
 		EXTRACT_CARD32(rep->clientSwapped,
 			       reply->buf+current_index,
 			       data->client_seq);
 		current_index += 4;
 	    }
+	    if (current_index + 4 > rep->length<<2)
+		return Error;
 	    if (reply->buf[current_index+2] == 0
 		&& reply->buf[current_index+3] == 0) /* needn't swap 0 */
 	    {	/* BIG-REQUESTS */
+		if (current_index + 8 > rep->length << 2)
+		    return Error;
 		EXTRACT_CARD32(rep->clientSwapped,
 			       reply->buf+current_index+4, datum_bytes);
 	    } else {
 		EXTRACT_CARD16(rep->clientSwapped,
 			       reply->buf+current_index+2, datum_bytes);
 	    }
+	    if (datum_bytes < 0 || datum_bytes > INT_MAX >> 2)
+		return Error;
 	    datum_bytes <<= 2;
 	    break;
 	case XRecordClientStarted:
+	    if (current_index + 8 > rep->length << 2)
+		return Error;
 	    EXTRACT_CARD16(rep->clientSwapped,
 			   reply->buf+current_index+6, datum_bytes);
 	    datum_bytes = (datum_bytes+2) << 2;
 	    break;
 	case XRecordClientDied:
 	    if (rep->elementHeader&XRecordFromClientSequence) {
+		if (current_index + 4 > rep->length << 2)
+		    return Error;
 		EXTRACT_CARD32(rep->clientSwapped,
 			       reply->buf+current_index,
 			       data->client_seq);
 		current_index += 4;
-	    }
-	    /* fall through */
+	    } else if (current_index < rep->length << 2)
+		return Error;
+	    datum_bytes = 0;
+	    break;
 	case XRecordStartOfData:
 	case XRecordEndOfData:
+	    if (current_index < rep->length << 2)
+		return Error;
 	    datum_bytes = 0;
+	    break;
 	}
 
 	if (datum_bytes > 0) {
-	    if (current_index + datum_bytes > rep->length << 2)
+	    if (INT_MAX - datum_bytes < (rep->length << 2) - current_index) {
 		fprintf(stderr,
 			"XRecord: %lu-byte reply claims %d-byte element (seq %lu)\n",
-			(long)rep->length << 2, current_index + datum_bytes,
+			(unsigned long)rep->length << 2, current_index + datum_bytes,
 			dpy->last_request_read);
+		return Error;
+	    }
 	    /*
 	     * This assignment (and indeed the whole buffer sharing
 	     * scheme) assumes arbitrary 4-byte boundaries are
@@ -863,6 +892,12 @@ XRecordEnableContext(Display *dpy, XRecordContext context,
 	    return 0;
 	}
 
+	if (rep.length > INT_MAX >> 2) {
+	    UnlockDisplay(dpy);
+	    SyncHandle();
+	    return 0;
+	}
+
 	if (rep.length > 0) {
 	    reply = alloc_reply_buffer(info, rep.length<<2);
 	    if (!reply) {