diff --git a/source/core_mqtt_serializer.c b/source/core_mqtt_serializer.c index 729817629..f549ced5f 100644 --- a/source/core_mqtt_serializer.c +++ b/source/core_mqtt_serializer.c @@ -1706,9 +1706,14 @@ MQTTStatus_t MQTT_GetConnectPacketSize( const MQTTConnectInfo_t * pConnectInfo, ( void * ) pPacketSize ) ); status = MQTTBadParameter; } - else if( ( pConnectInfo->clientIdentifierLength == 0U ) || ( pConnectInfo->pClientIdentifier == NULL ) ) + else if( ( pConnectInfo->clientIdentifierLength == 0U ) ^ ( ( pConnectInfo->pClientIdentifier == NULL ) || ( *( pConnectInfo->pClientIdentifier ) == '\0' ) ) ) { - LogError( ( "Mqtt_GetConnectPacketSize() client identifier must be set." ) ); + LogError( ( "Client ID length and value mismatch." ) ); + status = MQTTBadParameter; + } + else if( ( pConnectInfo->clientIdentifierLength == 0U ) && ( pConnectInfo->cleanSession == false ) ) + { + LogError( ( "Zero-length client identifier requires cleanSession=true per MQTT 3.1.1." ) ); status = MQTTBadParameter; } else if( ( pWillInfo != NULL ) && ( pWillInfo->payloadLength > ( size_t ) UINT16_MAX ) ) diff --git a/test/cbmc/proofs/MQTT_SerializeConnect/MQTT_SerializeConnect_harness.c b/test/cbmc/proofs/MQTT_SerializeConnect/MQTT_SerializeConnect_harness.c index 15f74cb0b..fea4e8bb1 100644 --- a/test/cbmc/proofs/MQTT_SerializeConnect/MQTT_SerializeConnect_harness.c +++ b/test/cbmc/proofs/MQTT_SerializeConnect/MQTT_SerializeConnect_harness.c @@ -39,7 +39,12 @@ void harness() MQTTStatus_t status = MQTTSuccess; pConnectInfo = allocateMqttConnectInfo( NULL ); - __CPROVER_assume( isValidMqttConnectInfo( pConnectInfo ) ); + + /* Avoid a malloc(0U) which causes dereference errors */ + if( ( pConnectInfo != NULL ) && ( pConnectInfo->clientIdentifierLength == 0U ) ) + { + pConnectInfo->pClientIdentifier = NULL; + } pWillInfo = allocateMqttPublishInfo( NULL ); __CPROVER_assume( isValidMqttPublishInfo( pWillInfo ) ); diff --git a/test/unit-test/core_mqtt_serializer_utest.c b/test/unit-test/core_mqtt_serializer_utest.c index c359e296c..8b5df8a25 100644 --- a/test/unit-test/core_mqtt_serializer_utest.c +++ b/test/unit-test/core_mqtt_serializer_utest.c @@ -463,11 +463,59 @@ void test_MQTT_GetConnectPacketSize( void ) status = MQTT_GetConnectPacketSize( &connectInfo, NULL, &remainingLength, &packetSize ); TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status ); + /* Verify NULL client identifier with provided non-zero length fails. */ connectInfo.pClientIdentifier = NULL; connectInfo.clientIdentifierLength = CLIENT_IDENTIFIER_LENGTH; + connectInfo.cleanSession = true; status = MQTT_GetConnectPacketSize( &connectInfo, NULL, &remainingLength, &packetSize ); TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status ); + /* Verify empty string client ID with provided non-zero length fails. */ + connectInfo.pClientIdentifier = ""; + connectInfo.clientIdentifierLength = 99U; + connectInfo.cleanSession = true; + status = MQTT_GetConnectPacketSize( &connectInfo, NULL, &remainingLength, &packetSize ); + TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status ); + + /* Verify zero-length client identifier must have clean session. */ + connectInfo.pClientIdentifier = NULL; + connectInfo.clientIdentifierLength = 0U; + connectInfo.cleanSession = false; + status = MQTT_GetConnectPacketSize( &connectInfo, NULL, &remainingLength, &packetSize ); + TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status ); + + /* Verify zero-length client identifier must have clean session. */ + connectInfo.pClientIdentifier = ""; + connectInfo.clientIdentifierLength = 0U; + connectInfo.cleanSession = false; + status = MQTT_GetConnectPacketSize( &connectInfo, NULL, &remainingLength, &packetSize ); + TEST_ASSERT_EQUAL_INT( MQTTBadParameter, status ); + + /* NULL client ID and client ID length of 0 is treated as zero-length identifier */ + connectInfo.pClientIdentifier = NULL; + connectInfo.clientIdentifierLength = 0U; + connectInfo.cleanSession = true; + connectInfo.pUserName = NULL; + connectInfo.pPassword = NULL; + status = MQTT_GetConnectPacketSize( &connectInfo, NULL, &remainingLength, &packetSize ); + TEST_ASSERT_EQUAL_INT( MQTTSuccess, status ); + /* Make sure remaining size returned is 12. */ + TEST_ASSERT_EQUAL_INT( 12, remainingLength ); + /* Make sure packet size is 14. */ + TEST_ASSERT_EQUAL_INT( 14, packetSize ); + + /* Empty string client ID and client ID length of 0 is treated as zero-length identifier */ + connectInfo.pClientIdentifier = ""; + connectInfo.clientIdentifierLength = 0U; + connectInfo.pUserName = NULL; + connectInfo.pPassword = NULL; + status = MQTT_GetConnectPacketSize( &connectInfo, NULL, &remainingLength, &packetSize ); + TEST_ASSERT_EQUAL_INT( MQTTSuccess, status ); + /* Make sure remaining size returned is 12. */ + TEST_ASSERT_EQUAL_INT( 12, remainingLength ); + /* Make sure packet size is 14. */ + TEST_ASSERT_EQUAL_INT( 14, packetSize ); + /* Test a will message payload length that is too large. */ memset( &connectInfo, 0x0, sizeof( connectInfo ) ); connectInfo.pClientIdentifier = CLIENT_IDENTIFIER;