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>

3 files changed, 33 insertions, 24 deletions

diff --git a/include/os.h b/include/os.h
index b489211ab..823fe5d29 100644
--- a/include/os.h
+++ b/include/os.h
@@ -451,9 +451,10 @@ extern _X_EXPORT CallbackListPtr ReplyCallback;
 typedef struct {
     ClientPtr client;
     const void *replyData;
-    unsigned long dataLenBytes;
+    unsigned long dataLenBytes; /* actual bytes from replyData + pad bytes */
     unsigned long bytesRemaining;
     Bool startOfReply;
+    unsigned long padBytes;     /* pad bytes from zeroed array */
 } ReplyInfoRec;
 
 /* stuff for FlushCallback */
diff --git a/os/io.c b/os/io.c
index 068f5f028..955bf8b73 100644
--- a/os/io.c
+++ b/os/io.c
@@ -809,6 +809,7 @@ WriteToClient (ClientPtr who, int count, const void *__buf)
 	replyinfo.client = who;
 	replyinfo.replyData = buf;
 	replyinfo.dataLenBytes = count + padBytes;
+	replyinfo.padBytes = padBytes;
 	if (who->replyBytesRemaining)
 	{ /* still sending data of an earlier reply */
 	    who->replyBytesRemaining -= count + padBytes;
diff --git a/record/record.c b/record/record.c
index 68311ac8f..db77b64f5 100644
--- a/record/record.c
+++ b/record/record.c
@@ -269,8 +269,9 @@ RecordFlushReplyBuffer(
  *	  device events and EndOfData, pClient is NULL.
  *	category is the category of the protocol element, as defined
  *	  by the RECORD spec.
- *	data is a pointer to the protocol data, and datalen is its length
- *	  in bytes.
+ *	data is a pointer to the protocol data, and datalen - padlen
+ *	  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
  *	  calls to this function to complete this protocol element.  
  *	  In those subsequent calls, futurelen will be -1 to indicate
@@ -290,7 +291,7 @@ RecordFlushReplyBuffer(
  */
 static void
 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];
     int numElemHeaders = 0;
@@ -398,15 +399,20 @@ RecordAProtocolElement(RecordContextPtr pContext, ClientPtr pClient,
 	}
 	if (datalen)
 	{
+	    static char padBuffer[3]; /* as in FlushClient */
 	    memcpy(pContext->replyBuffer + pContext->numBufBytes,
-		   data, datalen);
-	    pContext->numBufBytes += datalen;
+		   data, datalen - padlen);
+	    pContext->numBufBytes += datalen - padlen;
+	    memcpy(pContext->replyBuffer + pContext->numBufBytes,
+		   padBuffer, padlen);
+	    pContext->numBufBytes += padlen;
 	}
     }
     else
+    {
 	RecordFlushReplyBuffer(pContext, (pointer)elemHeaderData,
-			       numElemHeaders, (pointer)data, datalen);
-
+			       numElemHeaders, (pointer)data, datalen - padlen);
+    }
 } /* RecordAProtocolElement */
 
 
@@ -483,19 +489,19 @@ RecordABigRequest(RecordContextPtr pContext, ClientPtr client, xReq *stuff)
     /* record the request header */
     bytesLeft = client->req_len << 2;
     RecordAProtocolElement(pContext, client, XRecordFromClient,
-			   (pointer)stuff, SIZEOF(xReq), bytesLeft);
+			   (pointer)stuff, SIZEOF(xReq), 0, bytesLeft);
 
     /* reinsert the extended length field that was squished out */
     bigLength = client->req_len + bytes_to_int32(sizeof(bigLength));
     if (client->swapped)
 	swapl(&bigLength);
     RecordAProtocolElement(pContext, client, XRecordFromClient,
-		(pointer)&bigLength, sizeof(bigLength), /* continuation */ -1);
+               (pointer)&bigLength, sizeof(bigLength), 0, /* continuation */ -1);
     bytesLeft -= sizeof(bigLength);
 
     /* record the rest of the request after the length */
     RecordAProtocolElement(pContext, client, XRecordFromClient,
-		(pointer)(stuff + 1), bytesLeft, /* continuation */ -1);
+               (pointer)(stuff + 1), bytesLeft, 0, /* continuation */ -1);
 } /* RecordABigRequest */
 
 
@@ -542,7 +548,7 @@ RecordARequest(ClientPtr client)
 		    RecordABigRequest(pContext, client, stuff);
 		else
 		    RecordAProtocolElement(pContext, client, XRecordFromClient,
-				(pointer)stuff, client->req_len << 2, 0);
+				(pointer)stuff, client->req_len << 2, 0, 0);
 	    }
 	    else /* extension, check minor opcode */
 	    {
@@ -566,7 +572,7 @@ RecordARequest(ClientPtr client)
 			else
 			    RecordAProtocolElement(pContext, client, 
 					XRecordFromClient, (pointer)stuff,
-					client->req_len << 2, 0);
+					client->req_len << 2, 0, 0);
 			break;
 		    }			    
 		} /* end for each minor op info */
@@ -619,7 +625,8 @@ RecordAReply(CallbackListPtr *pcbl, pointer nulldata, pointer calldata)
 	    if (pContext->continuedReply)
 	    {
 		RecordAProtocolElement(pContext, client, XRecordFromServer,
-		   (pointer)pri->replyData, pri->dataLenBytes, /* continuation */ -1);
+		   (pointer)pri->replyData, pri->dataLenBytes,
+			    pri->padBytes, /* continuation */ -1);
 		if (!pri->bytesRemaining)
 		    pContext->continuedReply = 0;
 	    }
@@ -629,7 +636,7 @@ RecordAReply(CallbackListPtr *pcbl, pointer nulldata, pointer calldata)
 		if (majorop <= 127)
 		{ /* core reply */
 		    RecordAProtocolElement(pContext, client, XRecordFromServer,
-		       (pointer)pri->replyData, pri->dataLenBytes, pri->bytesRemaining);
+		       (pointer)pri->replyData, pri->dataLenBytes, 0, pri->bytesRemaining);
 		    if (pri->bytesRemaining)
 			pContext->continuedReply = 1;
 		}
@@ -651,7 +658,7 @@ RecordAReply(CallbackListPtr *pcbl, pointer nulldata, pointer calldata)
 			{
 			    RecordAProtocolElement(pContext, client, 
 				XRecordFromServer, (pointer)pri->replyData,
-				pri->dataLenBytes, pri->bytesRemaining);
+				pri->dataLenBytes, 0, pri->bytesRemaining);
 			    if (pri->bytesRemaining)
 				pContext->continuedReply = 1;
 			    break;
@@ -723,7 +730,7 @@ RecordADeliveredEventOrError(CallbackListPtr *pcbl, pointer nulldata, pointer ca
 			
 		    }
 		    RecordAProtocolElement(pContext, pClient,
-			XRecordFromServer, pEvToRecord, SIZEOF(xEvent), 0);
+			XRecordFromServer, pEvToRecord, SIZEOF(xEvent), 0, 0);
 		}
 	    } /* end for each event */
 	} /* end this client is on this context */
@@ -774,7 +781,7 @@ RecordSendProtocolEvents(RecordClientsAndProtocolPtr pRCAP,
 	    }
 
 	    RecordAProtocolElement(pContext, NULL,
-		    XRecordFromServer,  pEvToRecord, SIZEOF(xEvent), 0);
+		    XRecordFromServer,  pEvToRecord, SIZEOF(xEvent), 0, 0);
 	    /* make sure device events get flushed in the absence
 	     * of other client activity
 	     */
@@ -2415,7 +2422,7 @@ ProcRecordEnableContext(ClientPtr client)
     assert(numEnabledContexts > 0);
 
     /* send StartOfData */
-    RecordAProtocolElement(pContext, NULL, XRecordStartOfData, NULL, 0, 0);
+    RecordAProtocolElement(pContext, NULL, XRecordStartOfData, NULL, 0, 0, 0);
     RecordFlushReplyBuffer(pContext, NULL, 0, NULL, 0);
     return Success;
 } /* ProcRecordEnableContext */
@@ -2446,7 +2453,7 @@ RecordDisableContext(RecordContextPtr pContext)
     if (!pContext->pRecordingClient) return;
     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);
 	/* Re-enable request processing on this connection. */
 	AttendClient(pContext->pRecordingClient);
@@ -2761,7 +2768,7 @@ RecordConnectionSetupInfo(RecordContextPtr pContext, NewClientInfoRec *pci)
 	SwapConnSetupPrefix(pci->prefix, (xConnSetupPrefix*)pConnSetup);
 	SwapConnSetupInfo((char*)pci->setup, (char*)(pConnSetup + prefixsize));
 	RecordAProtocolElement(pContext, pci->client, XRecordClientStarted,
-			       (pointer)pConnSetup, prefixsize + restsize, 0);
+			       (pointer)pConnSetup, prefixsize + restsize, 0, 0);
 	free(pConnSetup);
     }
     else
@@ -2770,9 +2777,9 @@ RecordConnectionSetupInfo(RecordContextPtr pContext, NewClientInfoRec *pci)
 	 * data in two pieces
 	 */
 	RecordAProtocolElement(pContext, pci->client, XRecordClientStarted,
-			(pointer)pci->prefix, prefixsize, restsize);
+			(pointer)pci->prefix, prefixsize, 0, restsize);
 	RecordAProtocolElement(pContext, pci->client, XRecordClientStarted,
-			(pointer)pci->setup, restsize, /* continuation */ -1);
+			(pointer)pci->setup, restsize, 0, /* continuation */ -1);
     }
 } /* RecordConnectionSetupInfo */
 
@@ -2849,7 +2856,7 @@ RecordAClientStateChange(CallbackListPtr *pcbl, pointer nulldata, pointer callda
 	    {
 		if (pContext->pRecordingClient && pRCAP->clientDied)
 		    RecordAProtocolElement(pContext, pClient,
-					   XRecordClientDied, NULL, 0, 0);
+					   XRecordClientDied, NULL, 0, 0, 0);
 		RecordDeleteClientFromRCAP(pRCAP, pos);
 	    }
 	}