From f133c5acfb63bcee41efe125d53f40f1b1a2d6b6 Mon Sep 17 00:00:00 2001 From: Doug Hudson Date: Thu, 19 Mar 2015 18:02:22 -0400 Subject: [PATCH] Fix potential memcpy buffer overruns. Change-Id: Ia43054f29a3f3b3ac8118848847a06046822615e Signed-off-by: Doug Hudson Reviewed-on: https://gerrit.iotivity.org/gerrit/516 Tested-by: jenkins-iotivity Reviewed-by: Sachin Agrawal Reviewed-by: Erich Keane --- resource/csdk/connectivity/src/caprotocolmessage.c | 115 +++++++++++++++------ .../src/caprotocolmessage_singlethread.c | 106 +++++++++++++------ .../unittests/linux/ca_api_unittest.cpp | 24 +++-- 3 files changed, 174 insertions(+), 71 deletions(-) diff --git a/resource/csdk/connectivity/src/caprotocolmessage.c b/resource/csdk/connectivity/src/caprotocolmessage.c index 1622ee6..1520979 100644 --- a/resource/csdk/connectivity/src/caprotocolmessage.c +++ b/resource/csdk/connectivity/src/caprotocolmessage.c @@ -330,6 +330,13 @@ void CAParseHeadOption(const uint32_t code, const CAInfo_t info, coap_list_t **o coap_list_t *CACreateNewOptionNode(const uint16_t key, const uint32_t length, const uint8_t *data) { OIC_LOG(DEBUG, TAG, "CACreateNewOptionNode IN"); + + if (!data) + { + OIC_LOG(ERROR, TAG, "invalid pointer parameter"); + return NULL; + } + coap_option *option = coap_malloc(sizeof(coap_option) + length + 1); if (!option) { @@ -396,6 +403,12 @@ void CAGetInfoFromPDU(const coap_pdu_t *pdu, uint32_t *outCode, CAInfo_t *outInf { OIC_LOG(DEBUG, TAG, "CAGetInfoFromPDU IN"); + if (!pdu || !outCode || !outInfo || !outUri) + { + OIC_LOG(ERROR, TAG, "NULL pointer param"); + return; + } + coap_opt_iterator_t opt_iter; coap_option_iterator_init((coap_pdu_t *) pdu, &opt_iter, COAP_OPT_ALL); @@ -408,11 +421,6 @@ void CAGetInfoFromPDU(const coap_pdu_t *pdu, uint32_t *outCode, CAInfo_t *outInf // init HeaderOption list uint32_t count = CAGetOptionCount(opt_iter); - if (!outInfo) - { - OIC_LOG(ERROR, TAG, "outInfo is null"); - return; - } memset(outInfo, 0, sizeof(*outInfo)); outInfo->numOptions = count; // set type @@ -431,11 +439,9 @@ void CAGetInfoFromPDU(const coap_pdu_t *pdu, uint32_t *outCode, CAInfo_t *outInf } } - char buf[COAP_MAX_PDU_SIZE] = - { 0 }; + char buf[COAP_MAX_PDU_SIZE] = { 0 }; coap_opt_t *option; - char optionResult[CA_MAX_URI_LENGTH] = - { 0 }; + char optionResult[CA_MAX_URI_LENGTH] = { 0 }; uint32_t idx = 0; uint32_t optionLength = 0; bool isfirstsetflag = false; @@ -455,32 +461,72 @@ void CAGetInfoFromPDU(const coap_pdu_t *pdu, uint32_t *outCode, CAInfo_t *outInf isfirstsetflag = true; optionResult[optionLength] = '/'; optionLength++; - memcpy(optionResult + optionLength, buf, bufLength); - optionLength += bufLength; + // Make sure there is enough room in the optionResult buffer + if ((optionLength + bufLength) < sizeof(optionResult)) + { + memcpy(optionResult + optionLength, buf, bufLength); + optionLength += bufLength; + } + else + { + goto exit; + } } else { if (COAP_OPTION_URI_PATH == opt_iter.type) { - optionResult[optionLength] = '/'; - optionLength++; + // Make sure there is enough room in the optionResult buffer + if (optionLength < sizeof(optionResult)) + { + optionResult[optionLength] = '/'; + optionLength++; + } + else + { + goto exit; + } } else if (COAP_OPTION_URI_QUERY == opt_iter.type) { if(false == isQueryBeingProcessed) { - optionResult[optionLength] = '?'; - optionLength++; - isQueryBeingProcessed = true; + // Make sure there is enough room in the optionResult buffer + if (optionLength < sizeof(optionResult)) + { + optionResult[optionLength] = '?'; + optionLength++; + isQueryBeingProcessed = true; + } + else + { + goto exit; + } } else { - optionResult[optionLength] = '&'; - optionLength++; + // Make sure there is enough room in the optionResult buffer + if (optionLength < sizeof(optionResult)) + { + optionResult[optionLength] = '&'; + optionLength++; + } + else + { + goto exit; + } } } - memcpy(optionResult + optionLength, buf, bufLength); - optionLength += bufLength; + // Make sure there is enough room in the optionResult buffer + if ((optionLength + bufLength) < sizeof(optionResult)) + { + memcpy(optionResult + optionLength, buf, bufLength); + optionLength += bufLength; + } + else + { + goto exit; + } } } else @@ -541,7 +587,14 @@ void CAGetInfoFromPDU(const coap_pdu_t *pdu, uint32_t *outCode, CAInfo_t *outInf memcpy(outUri, optionResult, length); outUri[length] = '\0'; OIC_LOG_V(DEBUG, TAG, "made URL : %s, %s\n", optionResult, outUri); - }OIC_LOG(DEBUG, TAG, "CAGetInfoFromPDU OUT"); + } + OIC_LOG(DEBUG, TAG, "CAGetInfoFromPDU OUT"); + return; + +exit: + OIC_LOG(ERROR, TAG, "buffer too small"); + OICFree(outInfo->options); + return; } CAResult_t CAGenerateTokenInternal(CAToken_t *token) @@ -656,22 +709,16 @@ uint32_t CAGetOptionData(const uint8_t *data, uint32_t len, uint8_t *option, uin return 0; } - uint32_t cnt = 0; - while (len) + if (buflen <= len) { - if (cnt == buflen - 1) - { - break; - } - - *option++ = *data; - ++cnt; - ++data; - --len; + OIC_LOG(ERROR, TAG, "option buffer too small"); + return 0; } - *option = '\0'; - return cnt; + memcpy(option, data, len); + option[len] = '\0'; + + return len; } CAMessageType_t CAGetMessageTypeFromPduBinaryData(const void *pdu, uint32_t size) diff --git a/resource/csdk/connectivity/src/caprotocolmessage_singlethread.c b/resource/csdk/connectivity/src/caprotocolmessage_singlethread.c index 11113d1..5b41819 100644 --- a/resource/csdk/connectivity/src/caprotocolmessage_singlethread.c +++ b/resource/csdk/connectivity/src/caprotocolmessage_singlethread.c @@ -88,7 +88,7 @@ coap_pdu_t *CAGeneratePdu(const char *uri, uint32_t code, const CAInfo_t info) char *coapUri = (char *) OICCalloc(uriLength, sizeof(char)); if (NULL == coapUri) { - OIC_LOG(ERROR, TAG, "error"); + OIC_LOG(ERROR, TAG, "CAGeneratePdu, Memory allocation failed !"); return NULL; } @@ -317,6 +317,12 @@ coap_list_t *CACreateNewOptionNode(uint16_t key, uint32_t length, { OIC_LOG(DEBUG, TAG, "IN"); + if (!data) + { + OIC_LOG(ERROR, TAG, "invalid pointer parameter"); + return NULL; + } + coap_option *option = coap_malloc(sizeof(coap_option) + length + 1); if (!option) { @@ -383,6 +389,12 @@ void CAGetInfoFromPDU(const coap_pdu_t *pdu, uint32_t *outCode, CAInfo_t *outInf { OIC_LOG(DEBUG, TAG, "IN"); + if (!pdu || !outCode || !outInfo || !outUri) + { + OIC_LOG(ERROR, TAG, "NULL pointer param"); + return; + } + coap_opt_iterator_t opt_iter; coap_option_iterator_init((coap_pdu_t *) pdu, &opt_iter, COAP_OPT_ALL); @@ -395,11 +407,6 @@ void CAGetInfoFromPDU(const coap_pdu_t *pdu, uint32_t *outCode, CAInfo_t *outInf // init HeaderOption list uint32_t count = CAGetOptionCount(opt_iter); - if (!outInfo) - { - OIC_LOG(ERROR, TAG, "outInfo is NULL"); - return; - } memset(outInfo, 0, sizeof(*outInfo)); outInfo->numOptions = count; @@ -442,32 +449,72 @@ void CAGetInfoFromPDU(const coap_pdu_t *pdu, uint32_t *outCode, CAInfo_t *outInf isfirstsetflag = true; optionResult[optionLength] = '/'; optionLength++; - memcpy(optionResult + optionLength, buf, bufLength); - optionLength += bufLength; + // Make sure there is enough room in the optionResult buffer + if ((optionLength + bufLength) < sizeof(optionResult)) + { + memcpy(optionResult + optionLength, buf, bufLength); + optionLength += bufLength; + } + else + { + goto exit; + } } else { if (COAP_OPTION_URI_PATH == opt_iter.type) { - optionResult[optionLength] = '/'; - optionLength++; + // Make sure there is enough room in the optionResult buffer + if (optionLength < sizeof(optionResult)) + { + optionResult[optionLength] = '/'; + optionLength++; + } + else + { + goto exit; + } } else if (COAP_OPTION_URI_QUERY == opt_iter.type) { if(false == isQueryBeingProcessed) { - optionResult[optionLength] = '?'; - optionLength++; - isQueryBeingProcessed = true; + // Make sure there is enough room in the optionResult buffer + if (optionLength < sizeof(optionResult)) + { + optionResult[optionLength] = '?'; + optionLength++; + isQueryBeingProcessed = true; + } + else + { + goto exit; + } } else { - optionResult[optionLength] = '&'; - optionLength++; + // Make sure there is enough room in the optionResult buffer + if (optionLength < sizeof(optionResult)) + { + optionResult[optionLength] = '&'; + optionLength++; + } + else + { + goto exit; + } } } - memcpy(optionResult + optionLength, buf, bufLength); - optionLength += bufLength; + // Make sure there is enough room in the optionResult buffer + if ((optionLength + bufLength) < sizeof(optionResult)) + { + memcpy(optionResult + optionLength, buf, bufLength); + optionLength += bufLength; + } + else + { + goto exit; + } } } else @@ -530,6 +577,12 @@ void CAGetInfoFromPDU(const coap_pdu_t *pdu, uint32_t *outCode, CAInfo_t *outInf } OIC_LOG(DEBUG, TAG, "OUT"); + return; + +exit: + OIC_LOG(ERROR, TAG, "buffer too small"); + OICFree(outInfo->options); + return; } CAResult_t CAGenerateTokenInternal(CAToken_t *token) @@ -637,21 +690,16 @@ uint32_t CAGetOptionData(const uint8_t *data, uint32_t len, uint8_t *option, uin return 0; } - uint32_t cnt = 0; - while (len) + if (buflen <= len) { - if (cnt == buflen) - { - break; - } - *option++ = *data; - ++cnt; - ++data; - --len; + OIC_LOG(ERROR, TAG, "option buffer too small"); + return 0; } - *option = '\0'; - return cnt; + memcpy(option, data, len); + option[len] = '\0'; + + return len; } CAMessageType_t CAGetMessageTypeFromPduBinaryData(const void *pdu, uint32_t size) diff --git a/resource/csdk/connectivity/unittests/linux/ca_api_unittest.cpp b/resource/csdk/connectivity/unittests/linux/ca_api_unittest.cpp index 1f97b6c..1716d8f 100644 --- a/resource/csdk/connectivity/unittests/linux/ca_api_unittest.cpp +++ b/resource/csdk/connectivity/unittests/linux/ca_api_unittest.cpp @@ -481,14 +481,18 @@ TEST(AdvertiseResourceTest, TC_24_Positive_01) headerOpt = (CAHeaderOption_t *) calloc(1, optionNum * sizeof(CAHeaderOption_t)); char* tmpOptionData1 = (char *) "Hello"; + size_t tmpOptionDataLen = (strlen(tmpOptionData1) < CA_MAX_HEADER_OPTION_DATA_LENGTH) ? + strlen(tmpOptionData1) : CA_MAX_HEADER_OPTION_DATA_LENGTH - 1; headerOpt[0].optionID = 3000; - memcpy(headerOpt[0].optionData, tmpOptionData1, strlen(tmpOptionData1)); - headerOpt[0].optionLength = (uint16_t) strlen(tmpOptionData1); + memcpy(headerOpt[0].optionData, tmpOptionData1, tmpOptionDataLen); + headerOpt[0].optionLength = (uint16_t) tmpOptionDataLen; char* tmpOptionData2 = (char *) "World"; + tmpOptionDataLen = (strlen(tmpOptionData2) < CA_MAX_HEADER_OPTION_DATA_LENGTH) ? + strlen(tmpOptionData2) : CA_MAX_HEADER_OPTION_DATA_LENGTH - 1; headerOpt[1].optionID = 3001; - memcpy(headerOpt[1].optionData, tmpOptionData2, strlen(tmpOptionData2)); - headerOpt[1].optionLength = (uint16_t) strlen(tmpOptionData2); + memcpy(headerOpt[1].optionData, tmpOptionData2, tmpOptionDataLen); + headerOpt[1].optionLength = (uint16_t) tmpOptionDataLen; CAGenerateToken(&tempToken); @@ -507,14 +511,18 @@ TEST(AdvertiseResourceTest, TC_25_Negative_01) headerOpt = (CAHeaderOption_t *) calloc(1, optionNum * sizeof(CAHeaderOption_t)); char* tmpOptionData1 = (char *) "Hello"; + size_t tmpOptionDataLen = (strlen(tmpOptionData1) < CA_MAX_HEADER_OPTION_DATA_LENGTH) ? + strlen(tmpOptionData1) : CA_MAX_HEADER_OPTION_DATA_LENGTH - 1; headerOpt[0].optionID = 3000; - memcpy(headerOpt[0].optionData, tmpOptionData1, strlen(tmpOptionData1)); - headerOpt[0].optionLength = (uint16_t) strlen(tmpOptionData1); + memcpy(headerOpt[0].optionData, tmpOptionData1, tmpOptionDataLen); + headerOpt[0].optionLength = (uint16_t) tmpOptionDataLen; char* tmpOptionData2 = (char *) "World"; + tmpOptionDataLen = (strlen(tmpOptionData2) < CA_MAX_HEADER_OPTION_DATA_LENGTH) ? + strlen(tmpOptionData2) : CA_MAX_HEADER_OPTION_DATA_LENGTH - 1; headerOpt[1].optionID = 3001; - memcpy(headerOpt[1].optionData, tmpOptionData2, strlen(tmpOptionData2)); - headerOpt[1].optionLength = (uint16_t) strlen(tmpOptionData2); + memcpy(headerOpt[1].optionData, tmpOptionData2, tmpOptionDataLen); + headerOpt[1].optionLength = (uint16_t) tmpOptionDataLen; CAGenerateToken(&tempToken); -- 2.7.4