frederik
/
xserver
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

record: Prevent out of bounds access when recording a reply. 

Any pad bytes in replies are written to the client from a zeroed
array. However, record extension tries to incorrectly access the pad
bytes from the end of reply data.

Signed-off-by: Rami Ylimäki <rami.ylimaki@vincit.fi>
Reviewed-by: Erkki Seppälä <erkki.seppala@vincit.fi>
This commit is contained in:
Rami Ylimäki 2011-10-04 12:25:26 +03:00 committed by Jeremy Huddleston
parent 1f5baa924a
commit c1bb8f43b9
3 changed files with 33 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
include/os.h
View File

@ -451,9 +451,10 @@ extern _X_EXPORT CallbackListPtr ReplyCallback;
typedef struct { typedef struct {
ClientPtr client; ClientPtr client;
const void *replyData; const void *replyData;
unsigned long dataLenBytes; unsigned long dataLenBytes; /* actual bytes from replyData + pad bytes */
unsigned long bytesRemaining; unsigned long bytesRemaining;
Bool startOfReply; Bool startOfReply;
unsigned long padBytes; /* pad bytes from zeroed array */
} ReplyInfoRec; } ReplyInfoRec;
/* stuff for FlushCallback */ /* stuff for FlushCallback */

1
os/io.c
View File

@ -809,6 +809,7 @@ WriteToClient (ClientPtr who, int count, const void *__buf)
replyinfo.client = who; replyinfo.client = who;
replyinfo.replyData = buf; replyinfo.replyData = buf;
replyinfo.dataLenBytes = count + padBytes; replyinfo.dataLenBytes = count + padBytes;
replyinfo.padBytes = padBytes;
if (who->replyBytesRemaining) if (who->replyBytesRemaining)
{ /* still sending data of an earlier reply */ { /* still sending data of an earlier reply */
who->replyBytesRemaining -= count + padBytes; who->replyBytesRemaining -= count + padBytes;

53
record/record.c
View File

@ -269,8 +269,9 @@ RecordFlushReplyBuffer(
* device events and EndOfData, pClient is NULL. * device events and EndOfData, pClient is NULL.
* category is the category of the protocol element, as defined * category is the category of the protocol element, as defined
* by the RECORD spec. * by the RECORD spec.
* data is a pointer to the protocol data, and datalen is its length * data is a pointer to the protocol data, and datalen - padlen
* in bytes. * is its length in bytes.
* padlen is the number of pad bytes from a zeroed array.
* futurelen is the number of bytes that will be sent in subsequent * futurelen is the number of bytes that will be sent in subsequent
* calls to this function to complete this protocol element. * calls to this function to complete this protocol element.
* In those subsequent calls, futurelen will be -1 to indicate * In those subsequent calls, futurelen will be -1 to indicate
@ -290,7 +291,7 @@ RecordFlushReplyBuffer(
*/ */
static void static void
RecordAProtocolElement(RecordContextPtr pContext, ClientPtr pClient, RecordAProtocolElement(RecordContextPtr pContext, ClientPtr pClient,
int category, pointer data, int datalen, int futurelen) int category, pointer data, int datalen, int padlen, int futurelen)
{ {
CARD32 elemHeaderData[2]; CARD32 elemHeaderData[2];
int numElemHeaders = 0; int numElemHeaders = 0;
@ -398,15 +399,20 @@ RecordAProtocolElement(RecordContextPtr pContext, ClientPtr pClient,
} }
if (datalen) if (datalen)
{ {
static char padBuffer[3]; /* as in FlushClient */
memcpy(pContext->replyBuffer + pContext->numBufBytes, memcpy(pContext->replyBuffer + pContext->numBufBytes,
data, datalen); data, datalen - padlen);
pContext->numBufBytes += datalen; pContext->numBufBytes += datalen - padlen;
memcpy(pContext->replyBuffer + pContext->numBufBytes,
padBuffer, padlen);
pContext->numBufBytes += padlen;
} }
} }
else else
{
RecordFlushReplyBuffer(pContext, (pointer)elemHeaderData, RecordFlushReplyBuffer(pContext, (pointer)elemHeaderData,
numElemHeaders, (pointer)data, datalen); numElemHeaders, (pointer)data, datalen - padlen);
}
} /* RecordAProtocolElement */ } /* RecordAProtocolElement */
@ -483,19 +489,19 @@ RecordABigRequest(RecordContextPtr pContext, ClientPtr client, xReq *stuff)
/* record the request header */ /* record the request header */
bytesLeft = client->req_len << 2; bytesLeft = client->req_len << 2;
RecordAProtocolElement(pContext, client, XRecordFromClient, RecordAProtocolElement(pContext, client, XRecordFromClient,
(pointer)stuff, SIZEOF(xReq), bytesLeft); (pointer)stuff, SIZEOF(xReq), 0, bytesLeft);
/* reinsert the extended length field that was squished out */ /* reinsert the extended length field that was squished out */
bigLength = client->req_len + bytes_to_int32(sizeof(bigLength)); bigLength = client->req_len + bytes_to_int32(sizeof(bigLength));
if (client->swapped) if (client->swapped)
swapl(&bigLength); swapl(&bigLength);
RecordAProtocolElement(pContext, client, XRecordFromClient, RecordAProtocolElement(pContext, client, XRecordFromClient,
(pointer)&bigLength, sizeof(bigLength), /* continuation */ -1); (pointer)&bigLength, sizeof(bigLength), 0, /* continuation */ -1);
bytesLeft -= sizeof(bigLength); bytesLeft -= sizeof(bigLength);
/* record the rest of the request after the length */ /* record the rest of the request after the length */
RecordAProtocolElement(pContext, client, XRecordFromClient, RecordAProtocolElement(pContext, client, XRecordFromClient,
(pointer)(stuff + 1), bytesLeft, /* continuation */ -1); (pointer)(stuff + 1), bytesLeft, 0, /* continuation */ -1);
} /* RecordABigRequest */ } /* RecordABigRequest */
@ -542,7 +548,7 @@ RecordARequest(ClientPtr client)
RecordABigRequest(pContext, client, stuff); RecordABigRequest(pContext, client, stuff);
else else
RecordAProtocolElement(pContext, client, XRecordFromClient, RecordAProtocolElement(pContext, client, XRecordFromClient,
(pointer)stuff, client->req_len << 2, 0); (pointer)stuff, client->req_len << 2, 0, 0);
} }
else /* extension, check minor opcode */ else /* extension, check minor opcode */
{ {
@ -566,7 +572,7 @@ RecordARequest(ClientPtr client)
else else
RecordAProtocolElement(pContext, client, RecordAProtocolElement(pContext, client,
XRecordFromClient, (pointer)stuff, XRecordFromClient, (pointer)stuff,
client->req_len << 2, 0); client->req_len << 2, 0, 0);
break; break;
} }
} /* end for each minor op info */ } /* end for each minor op info */
@ -619,7 +625,8 @@ RecordAReply(CallbackListPtr *pcbl, pointer nulldata, pointer calldata)
if (pContext->continuedReply) if (pContext->continuedReply)
{ {
RecordAProtocolElement(pContext, client, XRecordFromServer, RecordAProtocolElement(pContext, client, XRecordFromServer,
(pointer)pri->replyData, pri->dataLenBytes, /* continuation */ -1); (pointer)pri->replyData, pri->dataLenBytes,
pri->padBytes, /* continuation */ -1);
if (!pri->bytesRemaining) if (!pri->bytesRemaining)
pContext->continuedReply = 0; pContext->continuedReply = 0;
} }
@ -629,7 +636,7 @@ RecordAReply(CallbackListPtr *pcbl, pointer nulldata, pointer calldata)
if (majorop <= 127) if (majorop <= 127)
{ /* core reply */ { /* core reply */
RecordAProtocolElement(pContext, client, XRecordFromServer, RecordAProtocolElement(pContext, client, XRecordFromServer,
(pointer)pri->replyData, pri->dataLenBytes, pri->bytesRemaining); (pointer)pri->replyData, pri->dataLenBytes, 0, pri->bytesRemaining);
if (pri->bytesRemaining) if (pri->bytesRemaining)
pContext->continuedReply = 1; pContext->continuedReply = 1;
} }
@ -651,7 +658,7 @@ RecordAReply(CallbackListPtr *pcbl, pointer nulldata, pointer calldata)
{ {
RecordAProtocolElement(pContext, client, RecordAProtocolElement(pContext, client,
XRecordFromServer, (pointer)pri->replyData, XRecordFromServer, (pointer)pri->replyData,
pri->dataLenBytes, pri->bytesRemaining); pri->dataLenBytes, 0, pri->bytesRemaining);
if (pri->bytesRemaining) if (pri->bytesRemaining)
pContext->continuedReply = 1; pContext->continuedReply = 1;
break; break;
@ -723,7 +730,7 @@ RecordADeliveredEventOrError(CallbackListPtr *pcbl, pointer nulldata, pointer ca
} }
RecordAProtocolElement(pContext, pClient, RecordAProtocolElement(pContext, pClient,
XRecordFromServer, pEvToRecord, SIZEOF(xEvent), 0); XRecordFromServer, pEvToRecord, SIZEOF(xEvent), 0, 0);
} }
} /* end for each event */ } /* end for each event */
} /* end this client is on this context */ } /* end this client is on this context */
@ -774,7 +781,7 @@ RecordSendProtocolEvents(RecordClientsAndProtocolPtr pRCAP,
} }
RecordAProtocolElement(pContext, NULL, RecordAProtocolElement(pContext, NULL,
XRecordFromServer, pEvToRecord, SIZEOF(xEvent), 0); XRecordFromServer, pEvToRecord, SIZEOF(xEvent), 0, 0);
/* make sure device events get flushed in the absence /* make sure device events get flushed in the absence
* of other client activity * of other client activity
*/ */
@ -2415,7 +2422,7 @@ ProcRecordEnableContext(ClientPtr client)
assert(numEnabledContexts > 0); assert(numEnabledContexts > 0);
/* send StartOfData */ /* send StartOfData */
RecordAProtocolElement(pContext, NULL, XRecordStartOfData, NULL, 0, 0); RecordAProtocolElement(pContext, NULL, XRecordStartOfData, NULL, 0, 0, 0);
RecordFlushReplyBuffer(pContext, NULL, 0, NULL, 0); RecordFlushReplyBuffer(pContext, NULL, 0, NULL, 0);
return Success; return Success;
} /* ProcRecordEnableContext */ } /* ProcRecordEnableContext */
@ -2446,7 +2453,7 @@ RecordDisableContext(RecordContextPtr pContext)
if (!pContext->pRecordingClient) return; if (!pContext->pRecordingClient) return;
if (!pContext->pRecordingClient->clientGone) if (!pContext->pRecordingClient->clientGone)
{ {
RecordAProtocolElement(pContext, NULL, XRecordEndOfData, NULL, 0, 0); RecordAProtocolElement(pContext, NULL, XRecordEndOfData, NULL, 0, 0, 0);
RecordFlushReplyBuffer(pContext, NULL, 0, NULL, 0); RecordFlushReplyBuffer(pContext, NULL, 0, NULL, 0);
/* Re-enable request processing on this connection. */ /* Re-enable request processing on this connection. */
AttendClient(pContext->pRecordingClient); AttendClient(pContext->pRecordingClient);
@ -2761,7 +2768,7 @@ RecordConnectionSetupInfo(RecordContextPtr pContext, NewClientInfoRec *pci)
SwapConnSetupPrefix(pci->prefix, (xConnSetupPrefix*)pConnSetup); SwapConnSetupPrefix(pci->prefix, (xConnSetupPrefix*)pConnSetup);
SwapConnSetupInfo((char*)pci->setup, (char*)(pConnSetup + prefixsize)); SwapConnSetupInfo((char*)pci->setup, (char*)(pConnSetup + prefixsize));
RecordAProtocolElement(pContext, pci->client, XRecordClientStarted, RecordAProtocolElement(pContext, pci->client, XRecordClientStarted,
(pointer)pConnSetup, prefixsize + restsize, 0); (pointer)pConnSetup, prefixsize + restsize, 0, 0);
free(pConnSetup); free(pConnSetup);
} }
else else
@ -2770,9 +2777,9 @@ RecordConnectionSetupInfo(RecordContextPtr pContext, NewClientInfoRec *pci)
* data in two pieces * data in two pieces
*/ */
RecordAProtocolElement(pContext, pci->client, XRecordClientStarted, RecordAProtocolElement(pContext, pci->client, XRecordClientStarted,
(pointer)pci->prefix, prefixsize, restsize); (pointer)pci->prefix, prefixsize, 0, restsize);
RecordAProtocolElement(pContext, pci->client, XRecordClientStarted, RecordAProtocolElement(pContext, pci->client, XRecordClientStarted,
(pointer)pci->setup, restsize, /* continuation */ -1); (pointer)pci->setup, restsize, 0, /* continuation */ -1);
} }
} /* RecordConnectionSetupInfo */ } /* RecordConnectionSetupInfo */
@ -2849,7 +2856,7 @@ RecordAClientStateChange(CallbackListPtr *pcbl, pointer nulldata, pointer callda
{ {
if (pContext->pRecordingClient && pRCAP->clientDied) if (pContext->pRecordingClient && pRCAP->clientDied)
RecordAProtocolElement(pContext, pClient, RecordAProtocolElement(pContext, pClient,
XRecordClientDied, NULL, 0, 0); XRecordClientDied, NULL, 0, 0, 0);
RecordDeleteClientFromRCAP(pRCAP, pos); RecordDeleteClientFromRCAP(pRCAP, pos);
} }
} }