Fix potential memcpy buffer overruns.

Change-Id: Ia43054f29a3f3b3ac8118848847a06046822615e
Signed-off-by: Doug Hudson <douglas.hudson@intel.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/516
Tested-by: jenkins-iotivity <jenkins-iotivity@opendaylight.org>
Reviewed-by: Sachin Agrawal <sachin.agrawal@intel.com>
Reviewed-by: Erich Keane <erich.keane@intel.com>

diff --git a/resource/csdk/connectivity/src/caprotocolmessage.c b/resource/csdk/connectivity/src/caprotocolmessage.c
index 1622ee6..1520979 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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);