Fixed out ouf boundary accesses.

Out of boundary accesses can occur while processing messages. This
affects clients and the session server.

Generally, the code tries to prevent out of boundary accesses. It
initially "skips" over the memory areas by parsing supplied lengths.
Then, it checks if it skipped over the memory boundary. If not, then
data is actually read and memory allocated, etc.

The problem is that while initially skipping over the memory,
subsequent lengths are already parsed, i.e. accessed. This results in
out of boundary reads on hostile messages.

Lengths could also overflow on 32 bit systems, leading to out of
boundary writes if not enough bytes have been allocated.

Authentication is handled by libICE, which is not affected, because the
macros for skipping already take care about memory boundaries.

Therefore, this flaw can only be used by authenticated clients or by
hostile servers (which could simply accept every MIT cookie). Most
session managers only use Unix sockets, so in many cases it takes a
local authenticated user.

In order to fix this, I decided to move the macros from SMlibint.h to
its only callers in sm_process.c, turning them into functions for much
easier error handling and readability.

Instead of skipping over the memory, validation happens during actual
read and memory allocation operations, as it's rather unlikely to
encounter hostile code anyway, i.e. my code has more error cleanup
handling in it.

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

2 files changed, 229 insertions, 163 deletions

diff --git a/src/SMlibint.h b/src/SMlibint.h
index 7946920..94f13e9 100644
--- a/src/SMlibint.h
+++ b/src/SMlibint.h
@@ -184,80 +184,6 @@ in this Software without prior written authorization from The Open Group.
 
 
 /*
- * EXTRACT FOO
- */
-
-#define EXTRACT_ARRAY8(_pBuf, _swap, _len, _array8) \
-{ \
-    EXTRACT_CARD32 (_pBuf, _swap, _len); \
-    _array8 = malloc (_len + 1); \
-    memcpy (_array8, _pBuf, _len); \
-    _array8[_len] = '\0'; \
-    _pBuf += _len + PAD64 (4 + _len); \
-}
-
-#define EXTRACT_ARRAY8_AS_STRING(_pBuf, _swap, _string) \
-{ \
-    CARD32 _len; \
-    EXTRACT_CARD32 (_pBuf, _swap, _len); \
-    _string = malloc (_len + 1); \
-    memcpy (_string, _pBuf, _len); \
-    _string[_len] = '\0'; \
-    _pBuf += _len + PAD64 (4 + _len); \
-}
-
-#define EXTRACT_LISTOF_PROPERTY(_pBuf, _swap, _count, _props) \
-{ \
-    int _i, _j; \
-    EXTRACT_CARD32 (_pBuf, _swap, _count); \
-    _pBuf += 4; \
-    _props = malloc (_count * sizeof (SmProp *)); \
-    for (_i = 0; _i < _count; _i++) \
-    { \
-        _props[_i] = malloc (sizeof (SmProp)); \
-        EXTRACT_ARRAY8_AS_STRING (_pBuf, _swap, _props[_i]->name); \
-        EXTRACT_ARRAY8_AS_STRING (_pBuf, _swap, _props[_i]->type); \
-        EXTRACT_CARD32 (_pBuf, _swap, _props[_i]->num_vals); \
-        _pBuf += 4; \
-        _props[_i]->vals = malloc ( \
-	    _props[_i]->num_vals * sizeof (SmPropValue)); \
-        for (_j = 0; _j < _props[_i]->num_vals; _j++) \
-	{ \
-	    char *_temp; \
-            EXTRACT_ARRAY8 (_pBuf, _swap, _props[_i]->vals[_j].length, _temp);\
-	    _props[_i]->vals[_j].value = (SmPointer) _temp; \
-	} \
-    } \
-}
-
-
-#define SKIP_ARRAY8(_pBuf, _swap) \
-{ \
-    CARD32 _len; \
-    EXTRACT_CARD32 (_pBuf, _swap, _len); \
-    _pBuf += _len + PAD64 (4 + _len); \
-}
-
-#define SKIP_LISTOF_PROPERTY(_pBuf, _swap) \
-{ \
-    CARD32 _i, _j; \
-    CARD32 _count; \
-    EXTRACT_CARD32 (_pBuf, _swap, _count); \
-    _pBuf += 4; \
-    for (_i = 0; _i < _count; _i++) \
-    { \
-        CARD32 _numvals; \
-        SKIP_ARRAY8 (_pBuf, _swap); \
-        SKIP_ARRAY8 (_pBuf, _swap); \
-        EXTRACT_CARD32 (_pBuf, _swap, _numvals); \
-        _pBuf += 4; \
-        for (_j = 0; _j < _numvals; _j++) \
-            SKIP_ARRAY8 (_pBuf, _swap);\
-    } \
-}
-
-
-/*
  * Client replies not processed by callbacks (we block for them).
  */
 
diff --git a/src/sm_process.c b/src/sm_process.c
index 95883b9..ee38057 100644
--- a/src/sm_process.c
+++ b/src/sm_process.c
@@ -32,6 +32,7 @@ in this Software without prior written authorization from The Open Group.
 #include <config.h>
 #endif
 #include <X11/SM/SMlib.h>
+#include <limits.h>
 #include "SMlibint.h"
 
 
@@ -53,15 +54,120 @@ in this Software without prior written authorization from The Open Group.
        return; \
     }
 
-#define CHECK_COMPLETE_SIZE(_iceConn, _majorOp, _minorOp, _expected_len, _actual_len, _pStart, _severity) \
-    if (((unsigned long)(PADDED_BYTES64((_actual_len)) - SIZEOF (iceMsg)) >> 3)	\
-        != _expected_len) \
-    { \
-       _IceErrorBadLength (_iceConn, _majorOp, _minorOp, _severity); \
-       IceDisposeCompleteMessage (iceConn, _pStart); \
-       return; \
+
+static char *
+extractArray8(char **pBuf, char *pEnd, Bool swap, int *len)
+{
+    char	*p;
+    int		n;
+
+    if (pEnd - *pBuf < 4)
+	return NULL;
+    EXTRACT_CARD32 (*pBuf, swap, n);
+    if (n < 0 || n > INT_MAX - 7)
+	return NULL;
+
+    if ((p = malloc (n + 1)) == NULL)
+	return NULL;
+    memcpy(p, *pBuf, n);
+    p[n] = '\0';
+
+    *pBuf += n + PAD64 (4 + n);
+    if (len != NULL)
+	*len = n;
+
+    return p;
+}
+
+
+static SmProp **
+extractListofProperty(char *pBuf, char *pEnd, Bool swap, int *count)
+{
+    int		i, j, n;
+    SmProp	**props;
+
+    if (pEnd - pBuf < 4)
+	return NULL;
+    EXTRACT_CARD32 (pBuf, swap, n);
+    if (n < 0 || n > INT_MAX / sizeof (SmProp *))
+	return NULL;
+    pBuf += 4;
+
+    props = malloc (n * sizeof(SmProp *));
+    if (props == NULL)
+	return NULL;
+
+    for (i = 0; i < n; i++)
+    {
+	props[i] = calloc (1, sizeof (SmProp));
+	if (props[i] == NULL)
+	    goto fail;
+	if ((props[i]->name = extractArray8 (&pBuf, pEnd, swap, NULL)) == NULL)
+	    goto fail;
+	if ((props[i]->type = extractArray8 (&pBuf, pEnd, swap, NULL)) == NULL)
+	    goto fail;
+
+	if (pEnd - pBuf < 4)
+	    goto fail;
+	EXTRACT_CARD32 (pBuf, swap, props[i]->num_vals);
+	if (props[i]->num_vals < 0)
+	    goto fail;
+	pBuf += 4;
+	props[i]->vals = calloc (props[i]->num_vals, sizeof (SmPropValue));
+	if (props[i]->vals == NULL)
+	    goto fail;
+
+	for (j = 0; j < props[i]->num_vals; j++)
+	{
+	    props[i]->vals[j].value = extractArray8 (&pBuf, pEnd, swap,
+		&props[i]->vals[j].length);
+	    if (props[i]->vals[j].value == NULL)
+		goto fail;
+	}
+    }
+
+    *count = n;
+    return props;
+
+fail:
+    for (; i >= 0; i--)
+    {
+	if (props[i] != NULL)
+	{
+	    free (props[i]->name);
+	    free (props[i]->type);
+	    if (props[i]->vals != NULL)
+	    {
+		for (j = 0; j < props[i]->num_vals; j++)
+		    free (props[i]->vals[j].value);
+		free (props[i]->vals);
+	    }
+	    free (props[i]);
+	}
+    }
+    free (props);
+    return NULL;
+}
+
+
+static Bool
+validErrorMessage(char *pData, char *pEnd, int errorClass, Bool swap)
+{
+    if (errorClass == IceBadValue)
+    {
+	unsigned int length;
+
+	if (pEnd - pData < 8)
+	    return False;
+
+	pData += 4;
+	EXTRACT_CARD32 (pData, swap, length);
+	if (length > pEnd - pData)
+	    return False;
     }
 
+    return True;
+}
 
 
 void
@@ -88,7 +194,7 @@ _SmcProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
     case SM_Error:
     {
 	iceErrorMsg 	*pMsg;
-	char	    	*pData;
+	char	    	*pData, *pEnd;
 
 	CHECK_AT_LEAST_SIZE (iceConn, _SmcOpcode, opcode,
 	    length, SIZEOF (iceErrorMsg), IceFatalToProtocol);
@@ -108,6 +214,8 @@ _SmcProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
 	    pMsg->offendingSequenceNum = lswapl (pMsg->offendingSequenceNum);
 	}
 
+	pEnd = pData + (length << 3) - (SIZEOF (iceErrorMsg) - SIZEOF(iceMsg));
+
 	if (replyWait &&
 	    replyWait->minor_opcode_of_request == SM_RegisterClient &&
             pMsg->errorClass == IceBadValue &&
@@ -125,6 +233,13 @@ _SmcProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
 
 	    *replyReadyRet = True;
 	}
+	else if (!validErrorMessage(pData, pEnd, pMsg->errorClass, swap))
+	{
+	    _IceErrorBadLength (iceConn, _SmcOpcode, opcode,
+		IceFatalToProtocol);
+	    IceDisposeCompleteMessage (iceConn, pData);
+	    return;
+	}
 	else
 	{
 	    (*_SmcErrorHandler) (smcConn, swap,
@@ -151,14 +266,12 @@ _SmcProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
         else
 	{
 	    smRegisterClientReplyMsg 	*pMsg;
-	    char			*pData, *pStart;
+	    char			*pData, *pStart, *pEnd;
 	    _SmcRegisterClientReply 	*reply =
 	        (_SmcRegisterClientReply *) (replyWait->reply);
 
-#if 0 /* No-op */
 	    CHECK_AT_LEAST_SIZE (iceConn, _SmcOpcode, opcode,
 		length, SIZEOF (smRegisterClientReplyMsg), IceFatalToProtocol);
-#endif
 
 	    IceReadCompleteMessage (iceConn, SIZEOF (smRegisterClientReplyMsg),
 		smRegisterClientReplyMsg, pMsg, pStart);
@@ -170,16 +283,16 @@ _SmcProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
 	    }
 
 	    pData = pStart;
+	    pEnd = pStart + (length << 3) -
+		(SIZEOF (smRegisterClientReplyMsg) - SIZEOF (iceMsg));
 
-	    SKIP_ARRAY8 (pData, swap);		/* client id */
-
-	    CHECK_COMPLETE_SIZE (iceConn, _SmcOpcode, opcode,
-		length, pData - pStart + SIZEOF (smRegisterClientReplyMsg),
-		pStart, IceFatalToProtocol);
-
-	    pData = pStart;
-
-	    EXTRACT_ARRAY8_AS_STRING (pData, swap, reply->client_id);
+	    reply->client_id = extractArray8(&pData, pEnd, swap, NULL);
+	    if (reply->client_id == NULL) {
+		_IceErrorBadLength (iceConn, _SmcOpcode, opcode,
+		    IceFatalToProtocol);
+		IceDisposeCompleteMessage (iceConn, pStart);
+		return;
+	    }
 
 	    reply->status = 1;
 	    *replyReadyRet = True;
@@ -357,15 +470,13 @@ _SmcProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
         else
 	{
 	    smPropertiesReplyMsg 	*pMsg;
-	    char 			*pData, *pStart;
-	    int				numProps;
+	    char 			*pStart, *pEnd;
+	    int				numProps = 0;
 	    SmProp			**props = NULL;
 	    _SmcPropReplyWait 		*next;
 
-#if 0 /* No-op */
 	    CHECK_AT_LEAST_SIZE (iceConn, _SmcOpcode, opcode,
 		length, SIZEOF (smPropertiesReplyMsg), IceFatalToProtocol);
-#endif
 
 	    IceReadCompleteMessage (iceConn, SIZEOF (smPropertiesReplyMsg),
 		smPropertiesReplyMsg, pMsg, pStart);
@@ -376,17 +487,17 @@ _SmcProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
 		return;
 	    }
 
-	    pData = pStart;
-
-	    SKIP_LISTOF_PROPERTY (pData, swap);
-
-	    CHECK_COMPLETE_SIZE (iceConn, _SmcOpcode, opcode,
-	        length, pData - pStart + SIZEOF (smPropertiesReplyMsg),
-	        pStart, IceFatalToProtocol);
+	    pEnd = pStart + (length << 3) -
+		(SIZEOF (smPropertiesReplyMsg) - SIZEOF (iceMsg));
 
-	    pData = pStart;
-
-	    EXTRACT_LISTOF_PROPERTY (pData, swap, numProps, props);
+	    props = extractListofProperty(pStart, pEnd, swap, &numProps);
+	    if (props == NULL)
+	    {
+		_IceErrorBadLength (iceConn, _SmcOpcode, opcode,
+		    IceFatalToProtocol);
+		IceDisposeCompleteMessage (iceConn, pStart);
+		return;
+	    }
 
 	    next = smcConn->prop_reply_waits->next;
 
@@ -432,7 +543,7 @@ _SmsProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
     case SM_Error:
     {
 	iceErrorMsg 	*pMsg;
-	char	    	*pData;
+	char	    	*pData, *pEnd;
 
 	CHECK_AT_LEAST_SIZE (iceConn, _SmsOpcode, opcode,
 	    length, SIZEOF (iceErrorMsg), IceFatalToProtocol);
@@ -452,6 +563,16 @@ _SmsProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
 	    pMsg->offendingSequenceNum = lswapl (pMsg->offendingSequenceNum);
 	}
 
+	pEnd = pData + (length << 3) - (SIZEOF (iceErrorMsg) - SIZEOF (iceMsg));
+
+	if (!validErrorMessage(pData, pEnd, pMsg->errorClass, swap))
+	{
+	    _IceErrorBadLength (iceConn, _SmcOpcode, opcode,
+		IceFatalToProtocol);
+	    IceDisposeCompleteMessage (iceConn, pData);
+	    return;
+	}
+
 	(*_SmsErrorHandler) (smsConn, swap,
 	    pMsg->offendingMinorOpcode,
 	    pMsg->offendingSequenceNum,
@@ -465,14 +586,12 @@ _SmsProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
     case SM_RegisterClient:
     {
 	smRegisterClientMsg 	*pMsg;
-	char 			*pData, *pStart;
+	char 			*pData, *pStart, *pEnd;
 	char 			*previousId;
 	int                      idLen;
 
-#if 0 /* No-op */
 	CHECK_AT_LEAST_SIZE (iceConn, _SmsOpcode, opcode,
 	    length, SIZEOF (smRegisterClientMsg), IceFatalToProtocol);
-#endif
 
 	IceReadCompleteMessage (iceConn, SIZEOF (smRegisterClientMsg),
 	    smRegisterClientMsg, pMsg, pStart);
@@ -484,16 +603,17 @@ _SmsProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
 	}
 
 	pData = pStart;
+	pEnd = pStart + (length << 3) -
+	    (SIZEOF (smRegisterClientMsg) - SIZEOF (iceMsg));
 
-	SKIP_ARRAY8 (pData, swap);	/* previous id */
-
-	CHECK_COMPLETE_SIZE (iceConn, _SmsOpcode, opcode,
-	   length, pData - pStart + SIZEOF (smRegisterClientMsg),
-	   pStart, IceFatalToProtocol);
-
-	pData = pStart;
-
-	EXTRACT_ARRAY8 (pData, swap, idLen, previousId);
+	previousId = extractArray8(&pData, pEnd, swap, &idLen);
+	if (previousId == NULL)
+	{
+	    _IceErrorBadLength (iceConn, _SmcOpcode, opcode,
+		IceFatalToProtocol);
+	    IceDisposeCompleteMessage (iceConn, pStart);
+	    return;
+	}
 
 	if (*previousId == '\0')
 	{
@@ -720,14 +840,12 @@ _SmsProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
     case SM_CloseConnection:
     {
 	smCloseConnectionMsg 	*pMsg;
-	char 			*pData, *pStart;
+	char 			*pData, *pStart, *pEnd;
 	int 			count, i;
 	char 			**reasonMsgs = NULL;
 
-#if 0 /* No-op */
 	CHECK_AT_LEAST_SIZE (iceConn, _SmsOpcode, opcode,
-	    length, SIZEOF (smCloseConnectionMsg), IceFatalToProtocol);
-#endif
+	    length, SIZEOF (smCloseConnectionMsg) + 8, IceFatalToProtocol);
 
 	IceReadCompleteMessage (iceConn, SIZEOF (smCloseConnectionMsg),
 	    smCloseConnectionMsg, pMsg, pStart);
@@ -739,22 +857,35 @@ _SmsProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
 	}
 
 	pData = pStart;
+	pEnd = pStart + (length << 3) -
+	    (SIZEOF (smCloseConnectionMsg) - SIZEOF (iceMsg));
 
 	EXTRACT_CARD32 (pData, swap, count);
 	pData += 4;
 
-	for (i = 0; i < count; i++)
-	    SKIP_ARRAY8 (pData, swap);
-
-	CHECK_COMPLETE_SIZE (iceConn, _SmsOpcode, opcode,
-	   length, pData - pStart + SIZEOF (smCloseConnectionMsg),
-	   pStart, IceFatalToProtocol);
-
-	pData = pStart + 8;
+	if (count < 0 || count > INT_MAX / sizeof (char *) ||
+	    (reasonMsgs = malloc (count * sizeof (char *))) == NULL)
+	{
+	    _IceErrorBadLength (iceConn, _SmcOpcode, opcode, IceFatalToProtocol);
+	    IceDisposeCompleteMessage (iceConn, pStart);
+	    return;
+	}
 
-	reasonMsgs = malloc (count * sizeof (char *));
 	for (i = 0; i < count; i++)
-	    EXTRACT_ARRAY8_AS_STRING (pData, swap, reasonMsgs[i]);
+	{
+	    reasonMsgs[i] = extractArray8(&pData, pEnd, swap, NULL);
+	    if (reasonMsgs[i] == NULL)
+		break;
+	}
+	if (i != count) {
+	    while (i-- > 0)
+		free (reasonMsgs[i]);
+	    free (reasonMsgs);
+	    _IceErrorBadLength (iceConn, _SmcOpcode, opcode,
+		IceFatalToProtocol);
+	    IceDisposeCompleteMessage (iceConn, pStart);
+	    return;
+	}
 
 	IceDisposeCompleteMessage (iceConn, pStart);
 
@@ -767,14 +898,12 @@ _SmsProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
     case SM_SetProperties:
     {
 	smSetPropertiesMsg 	*pMsg;
-	char 			*pData, *pStart;
+	char 			*pStart, *pEnd;
 	SmProp			**props = NULL;
-	int 			numProps;
+	int 			numProps = 0;
 
-#if 0 /* No-op */
 	CHECK_AT_LEAST_SIZE (iceConn, _SmsOpcode, opcode,
 	    length, SIZEOF (smSetPropertiesMsg), IceFatalToProtocol);
-#endif
 
 	IceReadCompleteMessage (iceConn, SIZEOF (smSetPropertiesMsg),
 	    smSetPropertiesMsg, pMsg, pStart);
@@ -785,17 +914,17 @@ _SmsProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
 	    return;
 	}
 
-	pData = pStart;
-
-	SKIP_LISTOF_PROPERTY (pData, swap);
-
-	CHECK_COMPLETE_SIZE (iceConn, _SmsOpcode, opcode,
-	   length, pData - pStart + SIZEOF (smSetPropertiesMsg),
-	   pStart, IceFatalToProtocol);
+	pEnd = pStart + (length << 3) -
+	    (SIZEOF (smSetPropertiesMsg) - SIZEOF (iceMsg));
 
-	pData = pStart;
-
-	EXTRACT_LISTOF_PROPERTY (pData, swap, numProps, props);
+	props = extractListofProperty(pStart, pEnd, swap, &numProps);
+	if (props == NULL)
+	{
+	    _IceErrorBadLength (iceConn, _SmcOpcode, opcode,
+		IceFatalToProtocol);
+	    IceDisposeCompleteMessage (iceConn, pStart);
+	    return;
+	}
 
 	(*smsConn->callbacks.set_properties.callback) (smsConn,
 	    smsConn->callbacks.set_properties.manager_data, numProps, props);
@@ -807,14 +936,12 @@ _SmsProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
     case SM_DeleteProperties:
     {
 	smDeletePropertiesMsg 	*pMsg;
-	char 			*pData, *pStart;
+	char 			*pData, *pStart, *pEnd;
 	int 			count, i;
 	char 			**propNames = NULL;
 
-#if 0 /* No-op */
 	CHECK_AT_LEAST_SIZE (iceConn, _SmsOpcode, opcode,
-	    length, SIZEOF (smDeletePropertiesMsg), IceFatalToProtocol);
-#endif
+	    length, SIZEOF (smDeletePropertiesMsg) + 8, IceFatalToProtocol);
 
 	IceReadCompleteMessage (iceConn, SIZEOF (smDeletePropertiesMsg),
 	    smDeletePropertiesMsg, pMsg, pStart);
@@ -826,22 +953,35 @@ _SmsProcessMessage(IceConn iceConn, IcePointer clientData, int opcode,
 	}
 
 	pData = pStart;
+	pEnd = pStart + (length << 3) -
+	    (SIZEOF (smDeletePropertiesMsg) - SIZEOF (iceMsg));
 
 	EXTRACT_CARD32 (pData, swap, count);
 	pData += 4;
 
-	for (i = 0; i < count; i++)
-	    SKIP_ARRAY8 (pData, swap);	/* prop names */
-
-	CHECK_COMPLETE_SIZE (iceConn, _SmsOpcode, opcode,
-	   length, pData - pStart + SIZEOF (smDeletePropertiesMsg),
-	   pStart, IceFatalToProtocol);
-
-	pData = pStart + 8;
+	if (count < 0 || count > INT_MAX / sizeof (char *) ||
+	    (propNames = malloc (count * sizeof (char *))) == NULL)
+	{
+	    IceDisposeCompleteMessage (iceConn, pStart);
+	    return;
+	}
 
-	propNames = malloc (count * sizeof (char *));
 	for (i = 0; i < count; i++)
-	    EXTRACT_ARRAY8_AS_STRING (pData, swap, propNames[i]);
+	{
+	    propNames[i] = extractArray8(&pData, pEnd, swap, NULL);
+	    if (propNames[i] == NULL)
+		break;
+	}
+	if (i != count)
+	{
+	    while (i-- > 0)
+		free (propNames[i]);
+	    free (propNames);
+	    _IceErrorBadLength (iceConn, _SmcOpcode, opcode,
+		IceFatalToProtocol);
+	    IceDisposeCompleteMessage (iceConn, pStart);
+	    return;
+	}
 
 	IceDisposeCompleteMessage (iceConn, pStart);