From d86106f2369ecf81155decaa360f9162c0c3cd53 Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Wed, 15 Aug 2018 21:21:09 +0200 Subject: 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 Reviewed-by: Matthieu Herrb --- src/SMlibint.h | 74 ------------- src/sm_process.c | 318 +++++++++++++++++++++++++++++++++++++++---------------- 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 @@ -183,80 +183,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 #endif #include +#include #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); -- cgit v1.2.1