Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix potential buffer overflow in parseStringValue #152

Merged
merged 1 commit into from
Apr 6, 2018
Merged

Fix potential buffer overflow in parseStringValue #152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions include/aws_iot_json_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions include/aws_iot_shadow_json_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Expand Down
2 changes: 1 addition & 1 deletion samples/linux/jobs_sample/jobs_sample.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions samples/linux/shadow_sample/shadow_sample.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
1 change: 1 addition & 0 deletions samples/linux/shadow_sample_console_echo/shadow_console_echo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
25 changes: 15 additions & 10 deletions src/aws_iot_json_utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

*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.");
Expand All @@ -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;
}

Expand Down
24 changes: 12 additions & 12 deletions src/aws_iot_shadow_json.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A look up table could look better. Though it is a risky change.

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;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/src/aws_iot_test_jobs_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions tests/unit/src/aws_iot_tests_unit_json_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
44 changes: 34 additions & 10 deletions tests/unit/src/aws_iot_tests_unit_json_utils_helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should test the happy path too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like when the buffer is big enough? I think that's adequately tested by the integration tests (jobs integration test).

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++)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: You could do a mem compare

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that require keeping an additional block of all-zero memory around?

{
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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions tests/unit/src/aws_iot_tests_unit_shadow_action_helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
9 changes: 7 additions & 2 deletions tests/unit/src/aws_iot_tests_unit_shadow_delta_helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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;
Expand All @@ -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;

Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions tests/unit/src/aws_iot_tests_unit_shadow_json_builder_helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down