diff --git a/inc/azure_umqtt_c/mqttconst.h b/inc/azure_umqtt_c/mqttconst.h index 58958d4..f3d6526 100644 --- a/inc/azure_umqtt_c/mqttconst.h +++ b/inc/azure_umqtt_c/mqttconst.h @@ -32,7 +32,9 @@ extern "C" { PINGRESP_TYPE, 0xD0, \ DISCONNECT_TYPE, 0xE0, \ PACKET_TYPE_ERROR, 0xE1, /* 0xE1 was assigned because ENUM_2 needs it */ \ - UNKNOWN_TYPE, 0xE2 /* 0xE2 was assigned because ENUM_2 needs it */ + UNKNOWN_TYPE, 0xE2, /* 0xE2 was assigned because ENUM_2 needs it */ \ + PACKET_INVALID1, 0x00, \ + PACKET_INVALID2, 0xF0 MU_DEFINE_ENUM_2(CONTROL_PACKET_TYPE, CONTROL_PACKET_TYPE_VALUES) diff --git a/src/mqtt_client.c b/src/mqtt_client.c index b320df5..88811af 100644 --- a/src/mqtt_client.c +++ b/src/mqtt_client.c @@ -769,7 +769,22 @@ static void recvCompleteCallback(void* context, CONTROL_PACKET_TYPE packet, int { /*Codes_SRS_MQTT_CLIENT_07_028: [If the actionResult parameter is of type CONNECT_ACK then the msgInfo value shall be a CONNECT_ACK structure.]*/ CONNECT_ACK connack = { 0 }; - connack.isSessionPresent = (byteutil_readByte(&iterator) == 0x1) ? true : false; + if (packetLength != 2) // CONNACK payload must be only 2 bytes + { + LogError("Invalid CONNACK packet."); + set_error_callback(mqtt_client, MQTT_CLIENT_COMMUNICATION_ERROR); + break; + } + + uint8_t connect_acknowledge_flags = byteutil_readByte(&iterator); + if (connect_acknowledge_flags & 0xFE) // bits 7-1 must be zero + { + LogError("Invalid CONNACK packet."); + set_error_callback(mqtt_client, MQTT_CLIENT_COMMUNICATION_ERROR); + break; + } + + connack.isSessionPresent = (connect_acknowledge_flags == 0x1) ? true : false; uint8_t rc = byteutil_readByte(&iterator); connack.returnCode = (rc < ((uint8_t)CONN_REFUSED_UNKNOWN)) ? @@ -801,6 +816,13 @@ static void recvCompleteCallback(void* context, CONTROL_PACKET_TYPE packet, int case PUBREL_TYPE: case PUBCOMP_TYPE: { + if (packetLength != 2) // CONNACK payload must be only 2 bytes + { + LogError("Invalid packet length."); + set_error_callback(mqtt_client, MQTT_CLIENT_COMMUNICATION_ERROR); + break; + } + /*Codes_SRS_MQTT_CLIENT_07_029: [If the actionResult parameter are of types PUBACK_TYPE, PUBREC_TYPE, PUBREL_TYPE or PUBCOMP_TYPE then the msgInfo value shall be a PUBLISH_ACK structure.]*/ MQTT_CLIENT_EVENT_RESULT action = (packet == PUBACK_TYPE) ? MQTT_CLIENT_ON_PUBLISH_ACK : (packet == PUBREC_TYPE) ? MQTT_CLIENT_ON_PUBLISH_RECV : @@ -875,6 +897,12 @@ static void recvCompleteCallback(void* context, CONTROL_PACKET_TYPE packet, int while (remainLen > 0) { uint8_t qosRet = byteutil_readByte(&iterator); + if (qosRet & 0x7C) // CONNACK payload must be only 2 bytes + { + LogError("Invalid SUBACK_TYPE packet."); + set_error_callback(mqtt_client, MQTT_CLIENT_COMMUNICATION_ERROR); + break; + } suback.qosReturn[suback.qosCount++] = (qosRet <= ((uint8_t)DELIVER_EXACTLY_ONCE)) ? (QOS_VALUE)qosRet : DELIVER_FAILURE; @@ -908,6 +936,13 @@ static void recvCompleteCallback(void* context, CONTROL_PACKET_TYPE packet, int { /*Codes_SRS_MQTT_CLIENT_07_031: [If the actionResult parameter is of type UNSUBACK_TYPE then the msgInfo value shall be a UNSUBSCRIBE_ACK structure.]*/ UNSUBSCRIBE_ACK unsuback = { 0 }; + if (packetLength != 2) // UNSUBACK_TYPE payload must be only 2 bytes + { + LogError("Invalid UNSUBACK packet length."); + set_error_callback(mqtt_client, MQTT_CLIENT_COMMUNICATION_ERROR); + break; + } + unsuback.packetId = byteutil_read_uint16(&iterator, packetLength); #ifndef NO_LOGGING diff --git a/src/mqtt_codec.c b/src/mqtt_codec.c index e8a1ca7..0d30fbc 100644 --- a/src/mqtt_codec.c +++ b/src/mqtt_codec.c @@ -568,6 +568,16 @@ static int prepareheaderDataInfo(MQTTCODEC_INSTANCE* codecData, uint8_t remainLe } } } + else if (codecData->remainLenIndex == (sizeof(codecData->storeRemainLen) / sizeof(codecData->storeRemainLen[0]))) + { + // The maximum number of bytes in the Remaining Length field is four + // This allows applications to send Control Packets of size up to 268,435,455 (256 MB). + // The representation of this number on the wire is: 0xFF, 0xFF, 0xFF, 0x7F. + + // The last byte has exceed the max value of 0x7F + LogError("MQTT packet len is invalid"); + result = MU_FAILURE; + } return result; } @@ -1066,6 +1076,46 @@ int mqtt_codec_bytesReceived(MQTTCODEC_HANDLE handle, const unsigned char* buffe if (codec_Data->currPacket == UNKNOWN_TYPE) { codec_Data->currPacket = processControlPacketType(iterator, &codec_Data->headerFlags); + + // validate packet type and invalid reserved header flags + switch (codec_Data->currPacket) + { + case PACKET_INVALID1: + case PACKET_INVALID2: + codec_Data->currPacket = PACKET_TYPE_ERROR; + result = MU_FAILURE; + break; + + case CONNECT_TYPE: + case CONNACK_TYPE: + case PUBACK_TYPE: + case PUBREC_TYPE: + case PUBCOMP_TYPE: + case SUBACK_TYPE: + case UNSUBACK_TYPE: + case PINGREQ_TYPE: + case PINGRESP_TYPE: + case DISCONNECT_TYPE: + if (codec_Data->headerFlags & 0x0F) // flags must be all zeros + { + codec_Data->currPacket = PACKET_TYPE_ERROR; + result = MU_FAILURE; + } + break; + + case PUBREL_TYPE: + case SUBSCRIBE_TYPE: + case UNSUBSCRIBE_TYPE: + if ((codec_Data->headerFlags & 0x0F) != 0x02) // only bit 1 must be set + { + codec_Data->currPacket = PACKET_TYPE_ERROR; + result = MU_FAILURE; + } + break; + + case PUBLISH_TYPE: + break; + } } else { @@ -1077,8 +1127,17 @@ int mqtt_codec_bytesReceived(MQTTCODEC_HANDLE handle, const unsigned char* buffe } else if (codec_Data->currPacket == PINGRESP_TYPE) { - /* Codes_SRS_MQTT_CODEC_07_034: [Upon a constructing a complete MQTT packet mqtt_codec_bytesReceived shall call the ON_PACKET_COMPLETE_CALLBACK function.] */ - completePacketData(codec_Data); + // PINGRESP must not have a payload + if (((int8_t*)buffer)[index] == 0) + { + /* Codes_SRS_MQTT_CODEC_07_034: [Upon a constructing a complete MQTT packet mqtt_codec_bytesReceived shall call the ON_PACKET_COMPLETE_CALLBACK function.] */ + completePacketData(codec_Data); + } + else + { + codec_Data->currPacket = PACKET_TYPE_ERROR; + result = MU_FAILURE; + } } } } diff --git a/tests/mqtt_codec_ut/mqtt_codec_ut.c b/tests/mqtt_codec_ut/mqtt_codec_ut.c index f6aa336..999bc11 100644 --- a/tests/mqtt_codec_ut/mqtt_codec_ut.c +++ b/tests/mqtt_codec_ut/mqtt_codec_ut.c @@ -1985,6 +1985,136 @@ TEST_FUNCTION(mqtt_codec_bytesReceived_unsuback_succeed) mqtt_codec_destroy(handle); } +TEST_FUNCTION(mqtt_codec_bytesReceived_pingresp_invalid_fails) +{ + // arrange + int result; + + unsigned char PINGRESP_RESP[] = { 0xD0, 0xFF }; + size_t length = sizeof(PINGRESP_RESP) / sizeof(PINGRESP_RESP[0]); + + TEST_COMPLETE_DATA_INSTANCE testData = { 0 }; + testData.dataHeader = PINGRESP_RESP + FIXED_HEADER_SIZE; + testData.Length = 2; + MQTTCODEC_HANDLE handle = mqtt_codec_create(TestOnCompleteCallback, &testData); + + umock_c_reset_all_calls(); + + // act + result = mqtt_codec_bytesReceived(handle, PINGRESP_RESP, length); + + // assert + ASSERT_ARE_NOT_EQUAL(int, result, 0); + ASSERT_ARE_EQUAL(char_ptr, umock_c_get_expected_calls(), umock_c_get_actual_calls()); + + // cleanup + mqtt_codec_destroy(handle); +} + +TEST_FUNCTION(mqtt_codec_bytesReceived_invalid_packet_id1_fails) +{ + // arrange + int result; + + unsigned char INVALID_PACKET_ID[] = { PACKET_INVALID1, 0x00 }; + size_t length = sizeof(INVALID_PACKET_ID) / sizeof(INVALID_PACKET_ID[0]); + + TEST_COMPLETE_DATA_INSTANCE testData = { 0 }; + testData.dataHeader = INVALID_PACKET_ID + FIXED_HEADER_SIZE; + testData.Length = 2; + MQTTCODEC_HANDLE handle = mqtt_codec_create(TestOnCompleteCallback, &testData); + + umock_c_reset_all_calls(); + + // act + result = mqtt_codec_bytesReceived(handle, INVALID_PACKET_ID, length); + + // assert + ASSERT_ARE_NOT_EQUAL(int, result, 0); + ASSERT_ARE_EQUAL(char_ptr, umock_c_get_expected_calls(), umock_c_get_actual_calls()); + + // cleanup + mqtt_codec_destroy(handle); +} + +TEST_FUNCTION(mqtt_codec_bytesReceived_invalid_packet_id2_fails) +{ + // arrange + int result; + + unsigned char INVALID_PACKET_ID[] = { PACKET_INVALID2, 0x00 }; + size_t length = sizeof(INVALID_PACKET_ID) / sizeof(INVALID_PACKET_ID[0]); + + TEST_COMPLETE_DATA_INSTANCE testData = { 0 }; + testData.dataHeader = INVALID_PACKET_ID + FIXED_HEADER_SIZE; + testData.Length = 2; + MQTTCODEC_HANDLE handle = mqtt_codec_create(TestOnCompleteCallback, &testData); + + umock_c_reset_all_calls(); + + // act + result = mqtt_codec_bytesReceived(handle, INVALID_PACKET_ID, length); + + // assert + ASSERT_ARE_NOT_EQUAL(int, result, 0); + ASSERT_ARE_EQUAL(char_ptr, umock_c_get_expected_calls(), umock_c_get_actual_calls()); + + // cleanup + mqtt_codec_destroy(handle); +} + +TEST_FUNCTION(mqtt_codec_bytesReceived_invalid_header_flags1_fails) +{ + // arrange + int result; + + unsigned char INVALID_CONNACK_HEADER_PACKET[] = { CONNACK_TYPE | 0x01, 0x00 }; + size_t length = sizeof(INVALID_CONNACK_HEADER_PACKET) / sizeof(INVALID_CONNACK_HEADER_PACKET[0]); + + TEST_COMPLETE_DATA_INSTANCE testData = { 0 }; + testData.dataHeader = INVALID_CONNACK_HEADER_PACKET + FIXED_HEADER_SIZE; + testData.Length = 2; + MQTTCODEC_HANDLE handle = mqtt_codec_create(TestOnCompleteCallback, &testData); + + umock_c_reset_all_calls(); + + // act + result = mqtt_codec_bytesReceived(handle, INVALID_CONNACK_HEADER_PACKET, length); + + // assert + ASSERT_ARE_NOT_EQUAL(int, result, 0); + ASSERT_ARE_EQUAL(char_ptr, umock_c_get_expected_calls(), umock_c_get_actual_calls()); + + // cleanup + mqtt_codec_destroy(handle); +} + +TEST_FUNCTION(mqtt_codec_bytesReceived_invalid_header_flags2_fails) +{ + // arrange + int result; + + unsigned char INVALID_CONNACK_HEADER_PACKET[] = { PUBREL_TYPE, 0x00 }; + size_t length = sizeof(INVALID_CONNACK_HEADER_PACKET) / sizeof(INVALID_CONNACK_HEADER_PACKET[0]); + + TEST_COMPLETE_DATA_INSTANCE testData = { 0 }; + testData.dataHeader = INVALID_CONNACK_HEADER_PACKET + FIXED_HEADER_SIZE; + testData.Length = 2; + MQTTCODEC_HANDLE handle = mqtt_codec_create(TestOnCompleteCallback, &testData); + + umock_c_reset_all_calls(); + + // act + result = mqtt_codec_bytesReceived(handle, INVALID_CONNACK_HEADER_PACKET, length); + + // assert + ASSERT_ARE_NOT_EQUAL(int, result, 0); + ASSERT_ARE_EQUAL(char_ptr, umock_c_get_expected_calls(), umock_c_get_actual_calls()); + + // cleanup + mqtt_codec_destroy(handle); +} + /* Tests_SRS_MQTT_CODEC_07_009: [mqtt_codec_connect shall construct a BUFFER_HANDLE that represents a MQTT CONNECT packet.] */ TEST_FUNCTION(mqtt_codec_connect_trace_succeeds) {