added mqtt protocal validation for duzzing

2020-10-23 15:14:03 -07:00 · 2020-10-23 15:14:03 -07:00 · 47f1948efd
--- 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)

--- 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
--- 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;
+                        }
                    }
                }
            }
--- 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)
 {