Fix potential buffer overflow in parseStringValue. (#152)

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.

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
 };

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;

samples/linux/shadow_sample/shadow_sample.c

@@ -152,13 +152,15 @@ 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;
 
 	jsonStruct_t temperatureHandler;
 	temperatureHandler.cb = NULL;
 	temperatureHandler.pKey = "temperature";
 	temperatureHandler.pData = &temperature;
+	temperatureHandler.dataLength = sizeof(float);
 	temperatureHandler.type = SHADOW_JSON_FLOAT;
 
 	char rootCA[PATH_MAX + 1];

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;

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;
 }
 

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;
 }

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;

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)

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);

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;
 

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);

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) {