From 575fcdb78d5f052390309c1cfce6b009d783923b Mon Sep 17 00:00:00 2001 From: Muneeb Ahmed Date: Thu, 10 Dec 2020 23:28:50 -0800 Subject: [PATCH] Check for empty topic filters in subscribes --- source/core_mqtt_serializer.c | 34 ++++++++-------- test/unit-test/core_mqtt_serializer_utest.c | 44 ++++++++++++++++++++- 2 files changed, 58 insertions(+), 20 deletions(-) diff --git a/source/core_mqtt_serializer.c b/source/core_mqtt_serializer.c index c00a326c7..9bd870e89 100644 --- a/source/core_mqtt_serializer.c +++ b/source/core_mqtt_serializer.c @@ -201,7 +201,7 @@ static bool calculatePublishPacketSize( const MQTTPublishInfo_t * pPublishInfo, * @param[in] subscriptionType #MQTT_SUBSCRIBE or #MQTT_UNSUBSCRIBE. * * #MQTTBadParameter if the packet would exceed the size allowed by the - * MQTT spec; #MQTTSuccess otherwise. + * MQTT spec or a subscription is empty; #MQTTSuccess otherwise. */ static MQTTStatus_t calculateSubscriptionPacketSize( const MQTTSubscribeInfo_t * pSubscriptionList, size_t subscriptionCount, @@ -1032,6 +1032,17 @@ static MQTTStatus_t calculateSubscriptionPacketSize( const MQTTSubscribeInfo_t * { packetSize += 1U; } + + /* Validate each topic filter. */ + if( ( pSubscriptionList[ i ].topicFilterLength == 0U ) || + ( pSubscriptionList[ i ].pTopicFilter == NULL ) ) + { + status = MQTTBadParameter; + LogError( ( "Subscription #%lu in %sSUBSCRIBE packet cannot be empty.", + ( unsigned long ) i, + ( subscriptionType == MQTT_SUBSCRIBE ) ? "" : "UN" ) ); + /* It is not necessary to break as an error status has already been set. */ + } } /* At this point, the "Remaining length" has been calculated. Return error @@ -1045,7 +1056,8 @@ static MQTTStatus_t calculateSubscriptionPacketSize( const MQTTSubscribeInfo_t * MQTT_MAX_REMAINING_LENGTH ) ); status = MQTTBadParameter; } - else + + if( status == MQTTSuccess ) { *pRemainingLength = packetSize; @@ -1669,19 +1681,12 @@ MQTTStatus_t MQTT_GetSubscribePacketSize( const MQTTSubscribeInfo_t * pSubscript } else { - /* Calculate the MQTT UNSUBSCRIBE packet size. */ + /* Calculate the MQTT SUBSCRIBE packet size. */ status = calculateSubscriptionPacketSize( pSubscriptionList, subscriptionCount, pRemainingLength, pPacketSize, MQTT_SUBSCRIBE ); - - if( status == MQTTBadParameter ) - { - LogError( ( "SUBSCRIBE packet remaining length exceeds %lu, which is the " - "maximum size allowed by MQTT 3.1.1.", - MQTT_MAX_REMAINING_LENGTH ) ); - } } return status; @@ -1768,19 +1773,12 @@ MQTTStatus_t MQTT_GetUnsubscribePacketSize( const MQTTSubscribeInfo_t * pSubscri } else { - /* Calculate the MQTT SUBSCRIBE packet size. */ + /* Calculate the MQTT UNSUBSCRIBE packet size. */ status = calculateSubscriptionPacketSize( pSubscriptionList, subscriptionCount, pRemainingLength, pPacketSize, MQTT_UNSUBSCRIBE ); - - if( status == MQTTBadParameter ) - { - LogError( ( "UNSUBSCRIBE packet remaining length exceeds %lu, which is the " - "maximum size allowed by MQTT 3.1.1.", - MQTT_MAX_REMAINING_LENGTH ) ); - } } return status; diff --git a/test/unit-test/core_mqtt_serializer_utest.c b/test/unit-test/core_mqtt_serializer_utest.c index 597c010a2..35580c970 100644 --- a/test/unit-test/core_mqtt_serializer_utest.c +++ b/test/unit-test/core_mqtt_serializer_utest.c @@ -656,7 +656,7 @@ void test_MQTT_SerializeConnect( void ) void test_MQTT_GetSubscribePacketSize( void ) { MQTTSubscribeInfo_t subscriptionList; - size_t subscriptionCount = 0; + size_t subscriptionCount = 1; size_t remainingLength = 0; size_t packetSize = 0; MQTTStatus_t status = MQTTSuccess; @@ -693,6 +693,22 @@ void test_MQTT_GetSubscribePacketSize( void ) &packetSize ); TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status ); + /* Zero length topic filter. */ + subscriptionCount = 1; + status = MQTT_GetSubscribePacketSize( &subscriptionList, + subscriptionCount, + &remainingLength, + &packetSize ); + TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status ); + + /* NULL topic filter, nonzero length. */ + subscriptionList.topicFilterLength = 1; + status = MQTT_GetSubscribePacketSize( &subscriptionList, + subscriptionCount, + &remainingLength, + &packetSize ); + TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status ); + /* Verify packet size cannot exceed limit. Note the max remaining length of * an MQTT packet is 2^28-1 = 268435455, or 256MiB. Since the only way to increase * the subscribe packet size is with the topic filters of the subscriptions @@ -701,6 +717,10 @@ void test_MQTT_GetSubscribePacketSize( void ) for( i = 0; i < 4096; i++ ) { fourThousandSubscriptions[ i ].topicFilterLength = UINT16_MAX; + + /* We need to set this to avoid an early bad parameter, however we do + * not need a 65535 byte buffer as the packet will not be serialized. */ + fourThousandSubscriptions[ i ].pTopicFilter = ""; } subscriptionCount = sizeof( fourThousandSubscriptions ) / sizeof( fourThousandSubscriptions[ 0 ] ); @@ -730,7 +750,7 @@ void test_MQTT_GetSubscribePacketSize( void ) void test_MQTT_GetUnsubscribePacketSize( void ) { MQTTSubscribeInfo_t subscriptionList; - size_t subscriptionCount = 0; + size_t subscriptionCount = 1; size_t remainingLength = 0; size_t packetSize = 0; MQTTStatus_t status = MQTTSuccess; @@ -767,6 +787,22 @@ void test_MQTT_GetUnsubscribePacketSize( void ) &packetSize ); TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status ); + /* Zero length topic filter. */ + subscriptionCount = 1; + status = MQTT_GetUnsubscribePacketSize( &subscriptionList, + subscriptionCount, + &remainingLength, + &packetSize ); + TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status ); + + /* NULL topic filter, nonzero length. */ + subscriptionList.topicFilterLength = 1; + status = MQTT_GetUnsubscribePacketSize( &subscriptionList, + subscriptionCount, + &remainingLength, + &packetSize ); + TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status ); + /* Verify packet size cannot exceed limit. Note the max remaining length of * an MQTT packet is 2^28-1 = 268435455, or 256MiB. Since the only way to increase * the subscribe packet size is with the topic filters of the subscriptions @@ -775,6 +811,10 @@ void test_MQTT_GetUnsubscribePacketSize( void ) for( i = 0; i < 4096; i++ ) { fourThousandSubscriptions[ i ].topicFilterLength = UINT16_MAX; + + /* We need to set this to avoid an early bad parameter, however we do + * not need a 65535 byte buffer as the packet will not be serialized. */ + fourThousandSubscriptions[ i ].pTopicFilter = ""; } subscriptionCount = sizeof( fourThousandSubscriptions ) / sizeof( fourThousandSubscriptions[ 0 ] );