IOT-1812: OICStrcpy is not safe with OICMalloc if the string can be empty

OICMalloc does not zero the memory allocated, and OICStrcpy does not
initialize the destination buffer if the source strlen is 0. Indeed
this seems to be by design, as the StringTests.StrcpyZeroSource
explicitly verifies that it does not initialize the destination buffer.

As a result, OICMalloc + OICStrcpy can result in uninitialized memory
(in a 1 byte buffer) that results in subsequently reading past the end
of the buffer, which can cause a crash.

Several security code paths are susceptible to this bug, which is
easy to reproduce with Application Verifier.

Change-Id: I6a3e2840c310d15a52656bf309ac9995de813683
Signed-off-by: Dave Thaler <dthaler@microsoft.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/17177
Tested-by: jenkins-iotivity <jenkins@iotivity.org>
Reviewed-by: Dan Mihai <Daniel.Mihai@microsoft.com>
Reviewed-by: Kevin Kane <kkane@microsoft.com>

diff --git a/resource/csdk/security/src/aclresource.c b/resource/csdk/security/src/aclresource.c
index c607291..83c0a51 100644 (file)
--- a/resource/csdk/security/src/aclresource.c
+++ b/resource/csdk/security/src/aclresource.c
@@ -179,17 +179,13 @@ OicSecAce_t* DuplicateACE(const OicSecAce_t* ace)
 
             //href is mandatory
             VERIFY_NOT_NULL(TAG, rsrc->href, ERROR);
-            allocateSize = strlen(rsrc->href) + 1;
-            newRsrc->href = (char*)OICMalloc(sizeof(char) * allocateSize);
+            newRsrc->href = (char*)OICStrdup(rsrc->href);
             VERIFY_NOT_NULL(TAG, newRsrc->href, ERROR);
-            OICStrcpy(newRsrc->href, allocateSize, rsrc->href);
 
             if(rsrc->rel)
             {
-                allocateSize = strlen(rsrc->rel) + 1;
-                newRsrc->rel = (char*)OICMalloc(sizeof(char) * allocateSize);
+                newRsrc->rel = OICStrdup(rsrc->rel);
                 VERIFY_NOT_NULL(TAG, newRsrc->rel, ERROR);
-                OICStrcpy(newRsrc->rel, allocateSize, rsrc->rel);
             }
 
             if(rsrc->types && 0 < rsrc->typeLen)
@@ -232,10 +228,8 @@ OicSecAce_t* DuplicateACE(const OicSecAce_t* ace)
 
                 if(validity->period)
                 {
-                    allocateSize = strlen(validity->period) + 1;
-                    newValidity->period = (char*)OICMalloc(sizeof(char) * allocateSize);
+                    newValidity->period = OICStrdup(validity->period);
                     VERIFY_NOT_NULL(TAG, newValidity->period, ERROR);
-                    OICStrcpy(newValidity->period, allocateSize, validity->period);
                 }
 
                 if(validity->recurrences && 0 < validity->recurrenceLen)
@@ -247,10 +241,8 @@ OicSecAce_t* DuplicateACE(const OicSecAce_t* ace)
 
                     for(size_t i = 0; i < validity->recurrenceLen; i++)
                     {
-                        allocateSize = strlen(validity->recurrences[i]) + 1;
-                        newValidity->recurrences[i] = (char*)OICMalloc(sizeof(char) * allocateSize);
+                        newValidity->recurrences[i] = OICStrdup(validity->recurrences[i]);
                         VERIFY_NOT_NULL(TAG, (newValidity->recurrences[i]), ERROR);
-                        OICStrcpy(newValidity->recurrences[i], allocateSize, validity->recurrences[i]);
                     }
                 }
             }

diff --git a/resource/csdk/security/src/dpairingresource.c b/resource/csdk/security/src/dpairingresource.c
index f21d4b4..a87eef0 100644 (file)
--- a/resource/csdk/security/src/dpairingresource.c
+++ b/resource/csdk/security/src/dpairingresource.c
@@ -606,10 +606,8 @@ static OCEntityHandlerResult HandleDpairingPutRequest (const OCEntityHandlerRequ
 
                 if(pdAcl->periods && pdAcl->periods[0])
                 {
-                    size_t periodLen = strlen(pdAcl->periods[0]) + 1;
-                    validity->period = (char*)OICMalloc(periodLen * sizeof(char));
+                    validity->period = OICStrdup(pdAcl->periods[0]);
                     VERIFY_NOT_NULL(TAG, (validity->period), ERROR);
-                    OICStrcpy(validity->period, periodLen, pdAcl->periods[0]);
                 }
 
                 if(pdAcl->recurrences && 0 < pdAcl->prdRecrLen)
@@ -620,11 +618,8 @@ static OCEntityHandlerResult HandleDpairingPutRequest (const OCEntityHandlerRequ
 
                     for(size_t i = 0; i < pdAcl->prdRecrLen; i++)
                     {
-                        size_t recurrenceLen = strlen(pdAcl->recurrences[i]) + 1;
-                        validity->recurrences[i] = (char*)OICMalloc(recurrenceLen  * sizeof(char));
+                        validity->recurrences[i] = OICStrdup(pdAcl->recurrences[i]);
                         VERIFY_NOT_NULL(TAG, (validity->recurrences[i]), ERROR);
-
-                        OICStrcpy(validity->recurrences[i], recurrenceLen, pdAcl->recurrences[i]);
                     }
                 }
 

diff --git a/resource/csdk/security/tool/json2cbor.c b/resource/csdk/security/tool/json2cbor.c
index 22fa723..8431412 100644 (file)
--- a/resource/csdk/security/tool/json2cbor.c
+++ b/resource/csdk/security/tool/json2cbor.c
@@ -349,7 +349,6 @@ OicSecAcl_t* JSONToAclBin(const char * jsonStr)
             VERIFY_NOT_NULL(TAG, ace, ERROR);
             LL_APPEND(headAcl->aces, ace);
 
-            size_t jsonObjLen = 0;
             cJSON *jsonObj = NULL;
             jsonObj = cJSON_GetObjectItem(jsonAcl, OIC_JSON_SUBJECTID_NAME);
             VERIFY_NOT_NULL(TAG, jsonObj, ERROR);
@@ -382,24 +381,19 @@ OicSecAcl_t* JSONToAclBin(const char * jsonStr)
                 VERIFY_NOT_NULL(TAG, jsonRsrc, ERROR);
 
                 //href
-                size_t jsonRsrcObjLen = 0;
                 cJSON *jsonRsrcObj = cJSON_GetObjectItem(jsonRsrc, OIC_JSON_HREF_NAME);
                 VERIFY_NOT_NULL(TAG, jsonRsrcObj, ERROR);
                 VERIFY_SUCCESS(TAG, cJSON_String == jsonRsrcObj->type, ERROR);
 
-                jsonRsrcObjLen = strlen(jsonRsrcObj->valuestring) + 1;
-                rsrc->href = (char*)OICMalloc(jsonRsrcObjLen);
+                rsrc->href = OICStrdup(jsonRsrcObj->valuestring);
                 VERIFY_NOT_NULL(TAG, (rsrc->href), ERROR);
-                OICStrcpy(rsrc->href, jsonRsrcObjLen, jsonRsrcObj->valuestring);
 
                 //rel
                 jsonRsrcObj = cJSON_GetObjectItem(jsonRsrc, OIC_JSON_REL_NAME);
                 if(jsonRsrcObj)
                 {
-                    jsonRsrcObjLen = strlen(jsonRsrcObj->valuestring) + 1;
-                    rsrc->rel = (char*)OICMalloc(jsonRsrcObjLen);
+                    rsrc->rel = OICStrdup(jsonRsrcObj->valuestring);
                     VERIFY_NOT_NULL(TAG, (rsrc->rel), ERROR);
-                    OICStrcpy(rsrc->rel, jsonRsrcObjLen, jsonRsrcObj->valuestring);
                 }
 
                 //rt
@@ -476,10 +470,8 @@ OicSecAcl_t* JSONToAclBin(const char * jsonStr)
                     {
                         VERIFY_SUCCESS(TAG, (cJSON_String == jsonPeriod->type), ERROR);
 
-                        jsonObjLen = strlen(jsonPeriod->valuestring) + 1;
-                        validity->period = (char*)OICMalloc(jsonObjLen);
+                        validity->period = OICStrdup(jsonPeriod->valuestring);
                         VERIFY_NOT_NULL(TAG, validity->period, ERROR);
-                        OICStrcpy(validity->period, jsonObjLen, jsonPeriod->valuestring);
                     }
 
                     //Recurrence
@@ -500,10 +492,8 @@ OicSecAcl_t* JSONToAclBin(const char * jsonStr)
 #pragma warning(suppress : 4267)
                             jsonRecur = cJSON_GetArrayItem(jsonRecurObj, i);
                             VERIFY_NOT_NULL(TAG, jsonRecur, ERROR);
-                            jsonObjLen = strlen(jsonRecur->valuestring) + 1;
-                            validity->recurrences[i] = (char*)OICMalloc(jsonObjLen);
+                            validity->recurrences[i] = OICStrdup(jsonRecur->valuestring);
                             VERIFY_NOT_NULL(TAG, validity->recurrences[i], ERROR);
-                            OICStrcpy(validity->recurrences[i], jsonObjLen, jsonRecur->valuestring);
                         }
                     }
                 }
@@ -935,10 +925,8 @@ OicSecCred_t * JSONToCredBin(const char * jsonStr)
             jsonObj = cJSON_GetObjectItem(jsonCred, OIC_JSON_PERIOD_NAME);
             if(jsonObj && cJSON_String == jsonObj->type)
             {
-                jsonObjLen = strlen(jsonObj->valuestring) + 1;
-                cred->period = (char *)OICMalloc(jsonObjLen);
+                cred->period = OICStrdup(jsonObj->valuestring);
                 VERIFY_NOT_NULL(TAG, cred->period, ERROR);
-                strncpy(cred->period, jsonObj->valuestring, jsonObjLen);
             }
             cred->next = NULL;
         } while( ++idx < numCred);
@@ -1103,11 +1091,8 @@ static OicSecAmacl_t* JSONToAmaclBin(const char * jsonStr)
         VERIFY_NOT_NULL(TAG, jsonRsrcObj, ERROR);
         VERIFY_SUCCESS(TAG, cJSON_String == jsonRsrcObj->type, ERROR);
 
-        size_t jsonRsrcObjLen = 0;
-        jsonRsrcObjLen = strlen(jsonRsrcObj->valuestring) + 1;
-        headAmacl->resources[idxx] = (char*)OICMalloc(jsonRsrcObjLen);
+        headAmacl->resources[idxx] = OICStrdup(jsonRsrcObj->valuestring);
         VERIFY_NOT_NULL(TAG, (headAmacl->resources[idxx]), ERROR);
-        OICStrcpy(headAmacl->resources[idxx], jsonRsrcObjLen, jsonRsrcObj->valuestring);
 
     } while ( ++idxx < headAmacl->resourcesLen);