From 2d1a002e0d1ba896fac136b58d0aa6b28f4c9173 Mon Sep 17 00:00:00 2001 From: Gordon Wang <36049150+gordonwang0@users.noreply.github.com> Date: Thu, 5 Apr 2018 17:04:56 -0700 Subject: [PATCH] Fix potential buffer overflow in parseStringValue. (#152) --- include/aws_iot_json_utils.h | 13 +++--- include/aws_iot_shadow_json_data.h | 1 + samples/linux/jobs_sample/jobs_sample.c | 2 +- samples/linux/shadow_sample/shadow_sample.c | 2 + .../shadow_console_echo.c | 1 + src/aws_iot_json_utils.c | 25 ++++++----- src/aws_iot_shadow_json.c | 24 +++++----- tests/integration/src/aws_iot_test_jobs_api.c | 2 +- .../src/aws_iot_tests_unit_json_utils.cpp | 1 + .../aws_iot_tests_unit_json_utils_helper.c | 44 ++++++++++++++----- .../aws_iot_tests_unit_shadow_action_helper.c | 3 ++ .../aws_iot_tests_unit_shadow_delta_helper.c | 9 +++- ...ot_tests_unit_shadow_json_builder_helper.c | 2 + 13 files changed, 87 insertions(+), 42 deletions(-) diff --git a/include/aws_iot_json_utils.h b/include/aws_iot_json_utils.h index 2c040cf9eb..af762d8a9f 100644 --- a/include/aws_iot_json_utils.h +++ b/include/aws_iot_json_utils.h @@ -181,14 +181,15 @@ IoT_Error_t parseBooleanValue(bool *b, const char *jsonString, jsmntok_t *token) * * Given a JSON node parse the string value from the value. * - * @param jsonString json string - * @param tok json token - pointer to JSON node - * @param s address of string to be updated + * @param buf address of string to be updated + * @param bufLen length of buf in bytes + * @param jsonString json string + * @param token json token - pointer to JSON node * - * @return SUCCESS - success - * @return JSON_PARSE_ERROR - error parsing value + * @return SUCCESS - success + * @return JSON_PARSE_ERROR - error parsing value */ -IoT_Error_t parseStringValue(char *buf, const char *jsonString, jsmntok_t *token); +IoT_Error_t parseStringValue(char *buf, size_t bufLen, const char *jsonString, jsmntok_t *token); /** * @brief Find the JSON node associated with the given key in the given object. diff --git a/include/aws_iot_shadow_json_data.h b/include/aws_iot_shadow_json_data.h index deaf9a177f..e2c61ac515 100644 --- a/include/aws_iot_shadow_json_data.h +++ b/include/aws_iot_shadow_json_data.h @@ -63,6 +63,7 @@ typedef enum { struct jsonStruct { const char *pKey; ///< JSON key void *pData; ///< pointer to the data (JSON value) + size_t dataLength; ///< Length (in bytes) of pData JsonPrimitiveType type; ///< type of JSON jsonStructCallback_t cb; ///< callback to be executed on receiving the Key value pair }; diff --git a/samples/linux/jobs_sample/jobs_sample.c b/samples/linux/jobs_sample/jobs_sample.c index 2f67551876..668ead3099 100644 --- a/samples/linux/jobs_sample/jobs_sample.c +++ b/samples/linux/jobs_sample/jobs_sample.c @@ -142,7 +142,7 @@ void iot_next_job_callback_handler(AWS_IoT_Client *pClient, char *topicName, uin char jobId[MAX_SIZE_OF_JOB_ID + 1]; AwsIotJobExecutionUpdateRequest updateRequest; - rc = parseStringValue(jobId, params->payload, tok); + rc = parseStringValue(jobId, MAX_SIZE_OF_JOB_ID + 1, params->payload, tok); if(SUCCESS != rc) { IOT_ERROR("parseStringValue returned error : %d ", rc); return; diff --git a/samples/linux/shadow_sample/shadow_sample.c b/samples/linux/shadow_sample/shadow_sample.c index 9a0b270bee..33d537e499 100644 --- a/samples/linux/shadow_sample/shadow_sample.c +++ b/samples/linux/shadow_sample/shadow_sample.c @@ -152,6 +152,7 @@ int main(int argc, char **argv) { jsonStruct_t windowActuator; windowActuator.cb = windowActuate_Callback; windowActuator.pData = &windowOpen; + windowActuator.dataLength = sizeof(bool); windowActuator.pKey = "windowOpen"; windowActuator.type = SHADOW_JSON_BOOL; @@ -159,6 +160,7 @@ int main(int argc, char **argv) { temperatureHandler.cb = NULL; temperatureHandler.pKey = "temperature"; temperatureHandler.pData = &temperature; + temperatureHandler.dataLength = sizeof(float); temperatureHandler.type = SHADOW_JSON_FLOAT; char rootCA[PATH_MAX + 1]; diff --git a/samples/linux/shadow_sample_console_echo/shadow_console_echo.c b/samples/linux/shadow_sample_console_echo/shadow_console_echo.c index 1c491fbd59..8938455b1e 100644 --- a/samples/linux/shadow_sample_console_echo/shadow_console_echo.c +++ b/samples/linux/shadow_sample_console_echo/shadow_console_echo.c @@ -139,6 +139,7 @@ int main(int argc, char** argv) { jsonStruct_t deltaObject; deltaObject.pData = stringToEchoDelta; + deltaObject.dataLength = SHADOW_MAX_SIZE_OF_RX_BUFFER; deltaObject.pKey = "state"; deltaObject.type = SHADOW_JSON_OBJECT; deltaObject.cb = DeltaCallback; diff --git a/src/aws_iot_json_utils.c b/src/aws_iot_json_utils.c index 16a26091a5..a733b89c41 100644 --- a/src/aws_iot_json_utils.c +++ b/src/aws_iot_json_utils.c @@ -170,12 +170,9 @@ IoT_Error_t parseBooleanValue(bool *b, const char *jsonString, jsmntok_t *token) IOT_WARN("Token was not a primitive."); return JSON_PARSE_ERROR; } - if(jsonString[token->start] == 't' && jsonString[token->start + 1] == 'r' && jsonString[token->start + 2] == 'u' - && jsonString[token->start + 3] == 'e') { + if(strncmp(jsonString + token->start, "true", 4) == 0) { *b = true; - } else if(jsonString[token->start] == 'f' && jsonString[token->start + 1] == 'a' - && jsonString[token->start + 2] == 'l' && jsonString[token->start + 3] == 's' - && jsonString[token->start + 4] == 'e') { + } else if(strncmp(jsonString + token->start, "false", 5) == 0) { *b = false; } else { IOT_WARN("Token was not a bool."); @@ -184,15 +181,23 @@ IoT_Error_t parseBooleanValue(bool *b, const char *jsonString, jsmntok_t *token) return SUCCESS; } -IoT_Error_t parseStringValue(char *buf, const char *jsonString, jsmntok_t *token) { - uint16_t size = 0; +IoT_Error_t parseStringValue(char *buf, size_t bufLen, const char *jsonString, jsmntok_t *token) { + /* This length does not include a null-terminator. */ + size_t stringLength = (size_t)(token->end - token->start); + if(token->type != JSMN_STRING) { IOT_WARN("Token was not a string."); return JSON_PARSE_ERROR; } - size = (uint16_t) (token->end - token->start); - memcpy(buf, jsonString + token->start, size); - buf[size] = '\0'; + + if (stringLength+1 > bufLen) { + IOT_WARN("Buffer too small to hold string value."); + return SHADOW_JSON_ERROR; + } + + strncpy(buf, jsonString + token->start, stringLength); + buf[stringLength] = '\0'; + return SUCCESS; } diff --git a/src/aws_iot_shadow_json.c b/src/aws_iot_shadow_json.c index 2ac49e2c76..cc4c106c23 100644 --- a/src/aws_iot_shadow_json.c +++ b/src/aws_iot_shadow_json.c @@ -354,28 +354,28 @@ bool isJsonValidAndParse(const char *pJsonDocument, void *pJsonHandler, int32_t } static IoT_Error_t UpdateValueIfNoObject(const char *pJsonString, jsonStruct_t *pDataStruct, jsmntok_t token) { - IoT_Error_t ret_val = SUCCESS; - if(pDataStruct->type == SHADOW_JSON_BOOL) { + IoT_Error_t ret_val = SHADOW_JSON_ERROR; + if(pDataStruct->type == SHADOW_JSON_BOOL && pDataStruct->dataLength >= sizeof(bool)) { ret_val = parseBooleanValue((bool *) pDataStruct->pData, pJsonString, &token); - } else if(pDataStruct->type == SHADOW_JSON_INT32) { + } else if(pDataStruct->type == SHADOW_JSON_INT32 && pDataStruct->dataLength >= sizeof(int32_t)) { ret_val = parseInteger32Value((int32_t *) pDataStruct->pData, pJsonString, &token); - } else if(pDataStruct->type == SHADOW_JSON_INT16) { + } else if(pDataStruct->type == SHADOW_JSON_INT16 && pDataStruct->dataLength >= sizeof(int16_t)) { ret_val = parseInteger16Value((int16_t *) pDataStruct->pData, pJsonString, &token); - } else if(pDataStruct->type == SHADOW_JSON_INT8) { + } else if(pDataStruct->type == SHADOW_JSON_INT8 && pDataStruct->dataLength >= sizeof(int8_t)) { ret_val = parseInteger8Value((int8_t *) pDataStruct->pData, pJsonString, &token); - } else if(pDataStruct->type == SHADOW_JSON_UINT32) { + } else if(pDataStruct->type == SHADOW_JSON_UINT32 && pDataStruct->dataLength >= sizeof(uint32_t)) { ret_val = parseUnsignedInteger32Value((uint32_t *) pDataStruct->pData, pJsonString, &token); - } else if(pDataStruct->type == SHADOW_JSON_UINT16) { + } else if(pDataStruct->type == SHADOW_JSON_UINT16 && pDataStruct->dataLength >= sizeof(uint16_t)) { ret_val = parseUnsignedInteger16Value((uint16_t *) pDataStruct->pData, pJsonString, &token); - } else if(pDataStruct->type == SHADOW_JSON_UINT8) { + } else if(pDataStruct->type == SHADOW_JSON_UINT8 && pDataStruct->dataLength >= sizeof(uint8_t)) { ret_val = parseUnsignedInteger8Value((uint8_t *) pDataStruct->pData, pJsonString, &token); - } else if(pDataStruct->type == SHADOW_JSON_FLOAT) { + } else if(pDataStruct->type == SHADOW_JSON_FLOAT && pDataStruct->dataLength >= sizeof(float)) { ret_val = parseFloatValue((float *) pDataStruct->pData, pJsonString, &token); - } else if(pDataStruct->type == SHADOW_JSON_DOUBLE) { + } else if(pDataStruct->type == SHADOW_JSON_DOUBLE && pDataStruct->dataLength >= sizeof(double)) { ret_val = parseDoubleValue((double *) pDataStruct->pData, pJsonString, &token); } else if(pDataStruct->type == SHADOW_JSON_STRING) { - ret_val = parseStringValue((char *) pDataStruct->pData, pJsonString, &token); - } + ret_val = parseStringValue((char *) pDataStruct->pData, pDataStruct->dataLength, pJsonString, &token); + } return ret_val; } diff --git a/tests/integration/src/aws_iot_test_jobs_api.c b/tests/integration/src/aws_iot_test_jobs_api.c index f8103deff9..df60484038 100644 --- a/tests/integration/src/aws_iot_test_jobs_api.c +++ b/tests/integration/src/aws_iot_test_jobs_api.c @@ -121,7 +121,7 @@ void iot_next_job_callback_handler(AWS_IoT_Client *pClient, char *topicName, uin char jobId[MAX_SIZE_OF_JOB_ID + 1]; AwsIotJobExecutionUpdateRequest updateRequest; - rc = parseStringValue(jobId, params->payload, tok); + rc = parseStringValue(jobId, MAX_SIZE_OF_JOB_ID + 1, params->payload, tok); if(SUCCESS != rc) { IOT_ERROR("parseStringValue returned error : %d ", rc); return; diff --git a/tests/unit/src/aws_iot_tests_unit_json_utils.cpp b/tests/unit/src/aws_iot_tests_unit_json_utils.cpp index 68c10921d9..f1fe2e33b3 100644 --- a/tests/unit/src/aws_iot_tests_unit_json_utils.cpp +++ b/tests/unit/src/aws_iot_tests_unit_json_utils.cpp @@ -28,6 +28,7 @@ TEST_GROUP_C(JsonUtils) { TEST_GROUP_C_WRAPPER(JsonUtils, ParseStringBasic) TEST_GROUP_C_WRAPPER(JsonUtils, ParseStringLongerStringIsValid) +TEST_GROUP_C_WRAPPER(JsonUtils, ParseStringWithBufferTooSmall) TEST_GROUP_C_WRAPPER(JsonUtils, ParseStringEmptyStringIsValid) TEST_GROUP_C_WRAPPER(JsonUtils, ParseStringErrorOnInteger) TEST_GROUP_C_WRAPPER(JsonUtils, ParseStringErrorOnBoolean) diff --git a/tests/unit/src/aws_iot_tests_unit_json_utils_helper.c b/tests/unit/src/aws_iot_tests_unit_json_utils_helper.c index c32c95794d..864d379d73 100644 --- a/tests/unit/src/aws_iot_tests_unit_json_utils_helper.c +++ b/tests/unit/src/aws_iot_tests_unit_json_utils_helper.c @@ -27,6 +27,8 @@ #include "aws_iot_tests_unit_helper_functions.h" #include "aws_iot_log.h" +#define STRING_BUFFER_LENGTH (50) + static IoT_Error_t rc = SUCCESS; static jsmn_parser test_parser; @@ -43,12 +45,12 @@ TEST_GROUP_C_TEARDOWN(JsonUtils) { TEST_C(JsonUtils, ParseStringBasic) { int r; const char *json = "{\"x\":\"test1\"}"; - char parsedString[50]; + char parsedString[STRING_BUFFER_LENGTH]; IOT_DEBUG("\n-->Running Json Utils Tests - Basic String Parsing \n"); r = jsmn_parse(&test_parser, json, strlen(json), t, sizeof(t) / sizeof(t[0])); - rc = parseStringValue(parsedString, json, t + 2); + rc = parseStringValue(parsedString, STRING_BUFFER_LENGTH, json, t + 2); CHECK_EQUAL_C_INT(3, r); CHECK_EQUAL_C_INT(SUCCESS, rc); @@ -58,26 +60,48 @@ TEST_C(JsonUtils, ParseStringBasic) { TEST_C(JsonUtils, ParseStringLongerStringIsValid) { int r; const char *json = "{\"x\":\"this is a longer string for test 2\"}"; - char parsedString[50]; + char parsedString[STRING_BUFFER_LENGTH]; IOT_DEBUG("\n-->Running Json Utils Tests - Parse long string \n"); r = jsmn_parse(&test_parser, json, strlen(json), t, sizeof(t) / sizeof(t[0])); - rc = parseStringValue(parsedString, json, t + 2); + rc = parseStringValue(parsedString, STRING_BUFFER_LENGTH, json, t + 2); CHECK_EQUAL_C_INT(3, r); CHECK_EQUAL_C_INT(SUCCESS, rc); CHECK_EQUAL_C_STRING("this is a longer string for test 2", parsedString); } +/* Test that parsing a string doesn't overflow the given buffer. */ +TEST_C(JsonUtils, ParseStringWithBufferTooSmall) { + static const char* const pJsonString = "{\"key\":\"This value is longer than JSON_BUFFER_LENGTH, which should be 50.\"}"; + char parsedString[STRING_BUFFER_LENGTH] = {0}; + int jsmnReturn, i; + + IOT_DEBUG("\n-->Running Json Utils Tests - String parsing with buffer too small. \n"); + + jsmnReturn = jsmn_parse(&test_parser, pJsonString, strlen(pJsonString), t, sizeof(t)/sizeof(t[0])); + CHECK_EQUAL_C_INT(3, jsmnReturn); + + rc = parseStringValue(parsedString, STRING_BUFFER_LENGTH, pJsonString, t + 2); + + CHECK_EQUAL_C_INT(SHADOW_JSON_ERROR, rc); + + /* Ensure there was no attempt to write to a buffer that's too small. */ + for(i = 0; i < STRING_BUFFER_LENGTH; i++) + { + CHECK_EQUAL_C_CHAR(0, parsedString[i]); + } +} + TEST_C(JsonUtils, ParseStringEmptyStringIsValid) { int r; const char *json = "{\"x\":\"\"}"; - char parsedString[50]; + char parsedString[STRING_BUFFER_LENGTH]; IOT_DEBUG("\n-->Running Json Utils Tests - Parse empty string \n"); r = jsmn_parse(&test_parser, json, strlen(json), t, sizeof(t) / sizeof(t[0])); - rc = parseStringValue(parsedString, json, t + 2); + rc = parseStringValue(parsedString, STRING_BUFFER_LENGTH, json, t + 2); CHECK_EQUAL_C_INT(3, r); CHECK_EQUAL_C_INT(SUCCESS, rc); @@ -87,12 +111,12 @@ TEST_C(JsonUtils, ParseStringEmptyStringIsValid) { int r; TEST_C(JsonUtils, ParseStringErrorOnInteger) { int r; const char *json = "{\"x\":3}"; - char parsedString[50]; + char parsedString[STRING_BUFFER_LENGTH]; IOT_DEBUG("\n-->Running Json Utils Tests - parse integer as string returns error \n"); r = jsmn_parse(&test_parser, json, strlen(json), t, sizeof(t) / sizeof(t[0])); - rc = parseStringValue(parsedString, json, t + 2); + rc = parseStringValue(parsedString, STRING_BUFFER_LENGTH, json, t + 2); CHECK_EQUAL_C_INT(3, r); CHECK_EQUAL_C_INT(JSON_PARSE_ERROR, rc); @@ -101,12 +125,12 @@ TEST_C(JsonUtils, ParseStringErrorOnInteger) { TEST_C(JsonUtils, ParseStringErrorOnBoolean) { int r; const char *json = "{\"x\":true}"; - char parsedString[50]; + char parsedString[STRING_BUFFER_LENGTH]; IOT_DEBUG("\n-->Running Json Utils Tests - parse boolean as string returns error \n"); r = jsmn_parse(&test_parser, json, strlen(json), t, sizeof(t) / sizeof(t[0])); - rc = parseStringValue(parsedString, json, t + 2); + rc = parseStringValue(parsedString, STRING_BUFFER_LENGTH, json, t + 2); CHECK_EQUAL_C_INT(3, r); CHECK_EQUAL_C_INT(JSON_PARSE_ERROR, rc); diff --git a/tests/unit/src/aws_iot_tests_unit_shadow_action_helper.c b/tests/unit/src/aws_iot_tests_unit_shadow_action_helper.c index 684603d43e..26d7b08a08 100644 --- a/tests/unit/src/aws_iot_tests_unit_shadow_action_helper.c +++ b/tests/unit/src/aws_iot_tests_unit_shadow_action_helper.c @@ -187,16 +187,19 @@ TEST_C(ShadowActionTests, UpdateTheJSONDocument) { dataFloatHandler.cb = NULL; dataFloatHandler.pData = &floatData; + dataFloatHandler.dataLength = sizeof(float); dataFloatHandler.pKey = "floatData"; dataFloatHandler.type = SHADOW_JSON_FLOAT; dataDoubleHandler.cb = NULL; dataDoubleHandler.pData = &doubleData; + dataDoubleHandler.dataLength = sizeof(double); dataDoubleHandler.pKey = "doubleData"; dataDoubleHandler.type = SHADOW_JSON_DOUBLE; dataBoolHandler.cb = NULL; dataBoolHandler.pData = &boolData; + dataBoolHandler.dataLength = sizeof(bool); dataBoolHandler.pKey = "boolData"; dataBoolHandler.type = SHADOW_JSON_BOOL; diff --git a/tests/unit/src/aws_iot_tests_unit_shadow_delta_helper.c b/tests/unit/src/aws_iot_tests_unit_shadow_delta_helper.c index 0ec18c0a13..1e6cc2a0ac 100644 --- a/tests/unit/src/aws_iot_tests_unit_shadow_delta_helper.c +++ b/tests/unit/src/aws_iot_tests_unit_shadow_delta_helper.c @@ -90,6 +90,7 @@ TEST_C(ShadowDeltaTest, registerDeltaSuccess) { windowHandler.pKey = "window"; windowHandler.type = SHADOW_JSON_BOOL; windowHandler.pData = &windowOpenData; + windowHandler.dataLength = sizeof(bool); params.payloadLen = strlen(deltaJSONString); params.payload = deltaJSONString; @@ -113,7 +114,7 @@ TEST_C(ShadowDeltaTest, registerDeltaSuccess) { TEST_C(ShadowDeltaTest, registerDeltaInt) { IoT_Error_t ret_val = SUCCESS; jsonStruct_t intHandler; - int intData = 0; + int32_t intData = 0; char deltaJSONString[] = "{\"state\":{\"delta\":{\"length\":23}},\"version\":1}"; IoT_Publish_Message_Params params; @@ -123,6 +124,7 @@ TEST_C(ShadowDeltaTest, registerDeltaInt) { intHandler.pKey = "length"; intHandler.type = SHADOW_JSON_INT32; intHandler.pData = &intData; + intHandler.dataLength = sizeof(int32_t); params.payloadLen = strlen(deltaJSONString); params.payload = deltaJSONString; @@ -143,7 +145,7 @@ TEST_C(ShadowDeltaTest, registerDeltaInt) { TEST_C(ShadowDeltaTest, registerDeltaIntNoCallback) { IoT_Error_t ret_val = SUCCESS; jsonStruct_t intHandler; - int intData = 0; + int32_t intData = 0; char deltaJSONString[] = "{\"state\":{\"delta\":{\"length_nocb\":23}},\"version\":1}"; IoT_Publish_Message_Params params; @@ -153,6 +155,7 @@ TEST_C(ShadowDeltaTest, registerDeltaIntNoCallback) { intHandler.pKey = "length_nocb"; intHandler.type = SHADOW_JSON_INT32; intHandler.pData = &intData; + intHandler.dataLength = sizeof(int32_t); params.payloadLen = strlen(deltaJSONString); params.payload = deltaJSONString; @@ -183,6 +186,7 @@ TEST_C(ShadowDeltaTest, DeltaNestedObject) { nestedObjectHandler.pKey = "sensors"; nestedObjectHandler.type = SHADOW_JSON_OBJECT; nestedObjectHandler.pData = &nestedObject; + nestedObjectHandler.dataLength = 100; snprintf(deltaJSONString, 100, "{\"state\":{\"delta\":{\"%s\":%s}},\"version\":1}", nestedObjectHandler.pKey, sentNestedObjectData); @@ -217,6 +221,7 @@ TEST_C(ShadowDeltaTest, DeltaVersionIgnoreOldVersion) { nestedObjectHandler.pKey = "sensors"; nestedObjectHandler.type = SHADOW_JSON_OBJECT; nestedObjectHandler.pData = &nestedObject; + nestedObjectHandler.dataLength = 100; snprintf(deltaJSONString, 100, "{\"state\":{\"delta\":{\"%s\":%s}},\"version\":1}", nestedObjectHandler.pKey, sentNestedObjectData); diff --git a/tests/unit/src/aws_iot_tests_unit_shadow_json_builder_helper.c b/tests/unit/src/aws_iot_tests_unit_shadow_json_builder_helper.c index 8cd9fc9a10..3541341b5e 100644 --- a/tests/unit/src/aws_iot_tests_unit_shadow_json_builder_helper.c +++ b/tests/unit/src/aws_iot_tests_unit_shadow_json_builder_helper.c @@ -60,11 +60,13 @@ TEST_GROUP_C_SETUP(ShadowJsonBuilderTests) { dataFloatHandler.pData = &floatData; dataFloatHandler.pKey = "floatData"; dataFloatHandler.type = SHADOW_JSON_FLOAT; + dataFloatHandler.dataLength = sizeof(float); dataDoubleHandler.cb = NULL; dataDoubleHandler.pData = &doubleData; dataDoubleHandler.pKey = "doubleData"; dataDoubleHandler.type = SHADOW_JSON_DOUBLE; + dataDoubleHandler.dataLength = sizeof(double); } TEST_GROUP_C_TEARDOWN(ShadowJsonBuilderTests) {