From 9556ad67af3129ec4a7a4f4b54a0d59701beeae3 Mon Sep 17 00:00:00 2001

From: Tobias Stoeckmann <tobias@stoeckmann.org>

Date: Sun, 25 Sep 2016 21:37:01 +0200

Subject: [PATCH:libXtst] 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>

---

src/XRecord.c | 43 +++++++++++++++++++++++++++++++++++++++----

1 file 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) {

--

2.7.4