From 9bc3a4ca3ab20eef194d110fef294b6e0b88a20e Mon Sep 17 00:00:00 2001 From: Dave Thaler Date: Fri, 10 Feb 2017 10:51:23 -0800 Subject: [PATCH] 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 Reviewed-on: https://gerrit.iotivity.org/gerrit/17177 Tested-by: jenkins-iotivity Reviewed-by: Dan Mihai Reviewed-by: Kevin Kane --- resource/csdk/security/src/aclresource.c | 16 ++++------------ resource/csdk/security/src/dpairingresource.c | 9 ++------- resource/csdk/security/tool/json2cbor.c | 27 ++++++--------------------- 3 files changed, 12 insertions(+), 40 deletions(-) diff --git a/resource/csdk/security/src/aclresource.c b/resource/csdk/security/src/aclresource.c index c607291..83c0a51 100644 --- 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 --- 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 --- 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); -- 2.7.4