codepr
Error found in sol
Hi, we've identified several potential protocol violations in the Sol implementation. We have compiled our findings into a detailed report below, which we hope you will find helpful. Please let us know if you have any questions.
1.Code issues in response message when parsing errors are not followed
1.1 violation 1
According to the specification of MQTTv3.1.1: If the Server rejects any ClientId for any reason, the Server MUST respond with a CONNACK return code 0x02 (Identifier rejected) and close the Network Connection.
When the server needs to reject the client ID, the code does not implement the logic of sending the MQTT_IDENTIFIER_REJECTED (0x02) response code and closing the connection. Instead, for an empty client ID, the code automatically generates a random ID; there is no corresponding processing logic for other situations where the client ID may need to be rejected. When MQTT_NOT_AUTHORIZED or MQTT_BAD_USERNAME_OR_PASSWORD is returned, the code sends a CONNACK response but does not close the connection.
1.2 violation 2
According to the specification of MQTTv3.1.1: If a Client supplies a zero-byte ClientId with CleanSession set to 0, the Server MUST respond with a CONNACK return code 0x02 (Identifier rejected) and close the Network Connection [MQTT-3.1.3-8].
In the code, for the case of zero-byte ClientId and CleanSession is set to 0, the MQTT_NOT_AUTHORIZED (0x05) return code is sent instead of the MQTT_IDENTIFIER_REJECTED (0x02) required by the rule. In addition, after sending the CONNACK response, the server does not close the network connection, and only closes the connection when -ERRCLIENTDC is returned.
2.Unchecked valid value of protocol version
According to the specification of MQTTv3.1.1: The Server MUST validate that the CONNECT Packet conforms to section 3.1 and close the Network Connection without sending a CONNACK if the CONNECT Packet does not conform [MQTT-3.1.4-1].
The code does not fully verify that the CONNECT packet complies with the requirements of Section 3.1 of the MQTT v3.1.1 specification. In particular, the unpack_mqtt_connect function clearly comments that 'ignore the check of the protocol name and reserved bits', which violates the requirements of the rules. In addition, the code lacks verification of other aspects of the CONNECT packet, such as the protocol level, reserved flags, etc. Although the code will close the connection when an invalid opcode is found, no verification and corresponding handling are performed for other possible CONNECT packet format errors.
3.UserName,Password and Flag fields are not processed properly
3.1 violation 1
According to the specification of MQTTv3.1.1: If the Password Flag is set to 1, a password MUST be present in the payload [MQTT-3.1.2-21].
The code does not verify that the password is actually present in the payload when the Password flag is set to 1. In the unpack_mqtt_connect function, when the password flag is 1, the code attempts to read the password from the payload, but does not check whether the password is successfully read or verify that the password is actually present in the payload. If the password is not present in the payload but the flag is 1, the code may read incorrect data or a null pointer, leading to potential security issues.
3.2 violation 2
According to the specification of MQTTv3.1.1: If the User Name Flag is set to 0, the Password Flag MUST be set to 0 [MQTT-3.1.2-22].
The code does not implement the validation required by the MQTT v3.1.1 rule: when the username flag is set to 0, the password flag must be set to 0. In the unpack_mqtt_connect function, the code checks the username flag and password flag separately, but does not verify the relationship between them. Throughout the code base, there is no place to check whether the password flag is also 0 when the username flag is 0. This allows the client to send CONNECT packets that violate the MQTT v3.1.1 specification.
4.The program does not actively terminate and disconnect the existing ClientId client
According to the specification of MQTTv3.1.1: If the ClientId represents a Client already connected to the Server and the validation is successful, then the Server MUST disconnect the existing Client [MQTT-3.1.4-2].
The code does not check if a client with the same ClientId already exists before adding the new client to clients_map. Although the code comments indicate that it should 'kick him out accordingly to the MQTT v3.1.1 specs', this logic is not actually implemented. The HASH_ADD_STR macro will directly add a new client without replacing or disconnecting the existing client. You should first use HASH_FIND_STR to find an existing client, disconnect if found, and then add the new client, or use the HASH_REPLACE_STR macro.
5.The program does not check the UTF-8 encoding of SUBSCRIBE/PUBLISH TOPIC
5.1 violation 1
According to the specification of MQTTv3.1.1: The Topic Filters in a SUBSCRIBE packet payload MUST be UTF-8 encoded strings as defined in Section 1.5.3 [MQTT-3.8.3-1].
The code does not verify that the subject filters in the SUBSCRIBE packets are valid UTF-8 encoded strings when processing them. The unpack_string16 and unpack_bytes functions simply copy the bytes without any UTF-8 validation. The README.md file also clearly states that the code skips the UTF-8 string check for the subject.
5.2 violation 2
According to the specification of MQTTv3.1.1: Additionally, Topic Names and Topic Filters must be UTF-8 encoded strings that do not exceed 65535 bytes in length.
The code ensures that topic names and topic filters are no longer than 65535 bytes (by using the u16 type and the unpack_string16 function), but does not implement UTF-8 encoding validation at all. The README.md file explicitly admits that 'Sol skips some checks, such as UTF-8 strings for topics'. In the unpack_bytes function, the code simply allocates memory and copies the bytes, without any UTF-8 validation.
6.The program did not check whether the SUBSCRIBE TOPIC exists
According to the specification of MQTTv3.1.1: The payload of a SUBSCRIBE packet MUST contain at least one Topic Filter / QoS pair.
The code does not check whether it contains at least one topic filter/QoS pair when parsing and processing the SUBSCRIBE packet. In the unpack_mqtt_subscribe function, if len is 0 (that is, there is no topic filter/QoS pair), the loop will not execute and subscribe.tuples_len will be set to 0, indicating that there is no topic filter/QoS pair. In the subscribe_handler function, the code does not check whether s->tuples_len is greater than 0, but directly assumes that it is at least 1. Therefore, if a SUBSCRIBE packet without a topic filter/QoS pair is received, the code will process it normally without rejecting or reporting an error.
7.Problem with overlapping subscription topics
7.1 violation 1
According to the specification of MQTTv3.1.1: If a Server receives a SUBSCRIBE Packet containing a Topic Filter identical to an existing Subscription's Topic Filter, the Server MUST completely replace the existing Subscription with a new Subscription.
When the code processes the SUBSCRIBE packet, if an existing subscriber is found and clean_session is false, the existing subscription will not be replaced. In the subscribe_handler function, HASH_FIND_STR is used to find out whether there is a subscriber with the same client ID. If it is found (tmp is not NULL) and clean_session is false, it is skipped and not processed. In the topic_add_subscriber function, if an existing subscriber is found (tmp is not NULL), the function will not replace the existing subscriber and will not update the QoS. This violates the MQTT v3.1.1 rule, which requires that if the server receives a SUBSCRIBE packet containing a topic filter that is the same as the topic filter of an existing subscription, the server must completely replace the existing subscription with the new subscription.
7.2 violation 2
According to the specification of MQTTv3.1.1: If a Server receives a SUBSCRIBE Packet containing a Topic Filter identical to an existing Subscription's Topic Filter, the Topic Filter in the new Subscription will be identical to that in the previous Subscription, although its maximum QoS value could be different.
When the code processes a SUBSCRIBE packet, if an existing subscriber is found and clean_session is false, the QoS value is not updated. In the subscribe_handler function, HASH_FIND_STR is used to find out whether there is a subscriber with the same client ID. If it is found (tmp is not NULL) and clean_session is false, it is skipped and the QoS value is not updated. In the topic_add_subscriber function, if an existing subscriber is found (tmp is not NULL), the function does not replace the existing subscriber and does not update the QoS value. This violates the MQTT v3.1.1 rule, which requires that if the server receives a SUBSCRIBE packet containing a topic filter that is the same as the topic filter of an existing subscription, the topic filter in the new subscription will be the same as the topic filter in the previous subscription, although its maximum QoS value may be different.
8.The program does not follow the QoS degradation principle when delivering retained messages to subscribers
According to the specification of MQTTv3.1.1: If the subscribing Client has been granted maximum QoS 0, then an Application Message originally published as QoS 2 might get lost on the hop to the Client, but the Server should never send a duplicate of that Message.
The code does not properly downgrade the granted QoS to the client's maximum allowed QoS (0). The subscriber_new function directly sets granted_qos to the requested QoS (from unpack_mqtt_subscribe line 356) without enforcing the client's maximum QoS constraint. This could result in the server sending duplicate messages for QoS 2 publications when the client's subscription should only receive QoS 0, violating the MQTT 3.1.1 rule.
9.PUBLISH/SUBSCRIBE Topic wildcards and other related
9.1 violation 1
According to the specification of MQTTv3.1.1: The Topic Name in the PUBLISH Packet MUST NOT contain wildcard characters [MQTT-3.3.2-2].
The code does not check if the topic name contains wildcard characters (+ and #) when processing a PUBLISH packet. In the unpack_mqtt_publish function, the code extracts the topic name from the packet, but does not verify if the topic name contains wildcard characters. In the publish_handler function, the code receives the topic name and creates or gets the corresponding topic, again without checking if the topic name contains wildcard characters. Although the code has a match_subscription function to handle wildcard matching, it is not used to verify that the topic name does not contain wildcard characters when processing a PUBLISH packet. The checks required by the MQTT v3.1.1 rule, which explicitly states that topic names in PUBLISH packets must not contain wildcard characters, are not implemented throughout the code base.
9.2 violation 2
According to the specification of MQTTv3.1.1: The Server SHOULD prevent Clients from using Topic Names starting with a wildcard character to exchange messages with other Clients.
The code did not check if the topic name started with a wildcard character (+ and #) when processing a PUBLISH packet. In the unpack_mqtt_publish function, the code extracted the topic name from the packet, but did not verify if the topic name started with a wildcard character. In the publish_handler function, the code received the topic name and created or got the corresponding topic, also without checking if the topic name started with a wildcard character. Although the code had a match_subscription function to handle wildcard matching, it was not used to verify that the topic name did not start with a wildcard character when processing a PUBLISH packet. The checks required by the MQTT v3.1.1 rule, which clearly states that the server should prevent clients from exchanging messages with other clients using topic names that start with wildcard characters, were not implemented throughout the code base.
9.3 violation 3
According to the specification of MQTTv3.1.1: The multi-level wildcard character MUST be the last character specified in the Topic Filter when it is specified either on its own or following a topic level separator [MQTT-4.7.1-2].
The code does not implement validation that the multi-level wildcard '#' must be the last character in the topic filter. In the subscribe_handler function, it only checks whether the topic filter ends with '/#', but does not check whether '#' appears in other positions in the topic filter. If the client sends a topic filter containing '#' in the middle, the server will accept the subscription instead of rejecting it, which violates the MQTT v3.1.1 specification.
9.4 violation 4
According to the specification of MQTTv3.1.1: The Server MUST NOT match Topic Filters starting with a wildcard character (# or +) with Topic Names beginning with a wildcard character [MQTT-4.7.2-1].
There is no check implemented in the code to prevent a topic filter that starts with a wildcard character (# or +) from matching a topic name that starts with a wildcard character. In the match_subscription function, only the wildcard character in the topic filter is checked, but there is no check whether the topic name starts with a wildcard character. When processing a PUBLISH message, the server iterates over all wildcard subscriptions and calls the match_subscription function, but the function does not reject a match between a topic name that starts with a wildcard and a topic filter that starts with a wildcard character.
10.QoS under overlapping subscriptions
According to the specification of MQTTv3.1.1: When a published message matches multiple subscription filters due to overlapping Client subscriptions with wildcards, the Server MUST deliver the message to the Client using the maximum QoS level among all matching subscriptions [MQTT-3.3.5-1].
The code did not properly handle the case where a client had multiple overlapping subscriptions to the same topic. When a client subscribes to the same topic through multiple different subscriptions (perhaps using wildcards), the code should use the maximum QoS level among those subscriptions. However, the current implementation only stores one subscriber object per topic (indexed by client ID), which means that only the QoS level of the last subscription is saved, rather than using the maximum QoS level among all matching subscriptions. When handling wildcard subscriptions in the publish_handler function, the code did not check if there was already a subscriber with the same client ID, nor did it compare and select the maximum QoS level.
11.Keep messages relevant
11.1 violation 1
According to the specification of MQTTv3.1.1: If the Server receives a QoS 0 PUBLISH Packet with the RETAIN flag set to 1 from a Client, the Server MUST discard any message previously retained for that topic.
The code does not properly discard previously retained messages for the topic when processing a PUBLISH packet with QoS 0 and the RETAIN flag is set to 1. In the publish_handler function, when the RETAIN flag is set to 1, the code allocates memory for new retained messages and packages the messages, but does not check the QoS level of the message and does not free the previously allocated retained message memory. This causes a memory leak because the previously retained message memory is not freed. In addition, the code does not explicitly perform the operation of discarding the previously retained message, but simply overwrites the pointer.
11.2 violation 2
According to the specification of MQTTv3.1.1: A PUBLISH Packet with a zero-byte payload and RETAIN flag set to 1 MUST NOT be stored as a retained message on the Server [MQTT-3.3.1-11].
The code does not check if the payload length of the PUBLISH packet is zero. In the publish_handler function, when the RETAIN flag is set to 1, the code allocates memory for a new retained message and packages the message, but does not check if p->payloadlen is zero. According to the MQTT v3.1.1 specification, a PUBLISH packet with a zero-byte payload and the RETAIN flag set to 1 should not be stored as a retained message, but the previously stored retained message should be deleted. The current implementation will incorrectly store such a message as a retained message.
12.The program cannot modify the topic in a standardized manner
According to the specification of MQTTv3.1.1: When the Server performs subscription matching, it must not normalize Topic Names or Topic Filters, modify unrecognized characters, or substitute any characters during the process.
The code modifies the topic name and topic filter before performing subscription matching. In the subscribe_handler function, if the topic filter ends with '/#', the code removes the '#' character; if the topic filter does not end with '/', the code adds a '/' character to the end. In the publish_handler function, if the topic name does not end with '/', the code adds a '/' character to the end. These modifications violate the MQTT v3.1.1 rule, which requires that the server must not normalize the topic name or topic filter when performing subscription matching, must not modify unrecognized characters, and must not replace any characters in the process.
13.The program does not properly handle the relationship between persistent sessions and message publishing reconnection
According to the specification of MQTTv3.1.1: When a Client reconnects with CleanSession set to 0, both the Client and Server MUST re-send any unacknowledged PUBLISH Packets (where QoS > 0) and PUBREL Packets using their original Packet Identifiers [MQTT-4.4.0-1].
The code clears the session's outgoing messages list (list_clear) immediately after resending unacknowledged messages during reconnection with CleanSession=0. MQTT 3.1.1 requires the server to retain unacknowledged PUBLISH (QoS>0) and PUBREL packets until they are acknowledged, even across multiple disconnections. By removing the messages from the queue after the first resend, the server violates the rule, as it cannot retransmit them if the client disconnects again without acknowledging them.
14.The length of ClientID is not checked
According to the specification of MQTTv3.1.1: The Server MAY allow ClientId's that contain more than 23 encoded bytes.
The code enforces a maximum client ID length of 23 bytes. In connect_handler (handlers.c:394), the client ID is copied into a fixed-size buffer of MQTT_CLIENT_ID_LEN (24 bytes) using snprintf, which truncates the client ID to 23 characters (plus null terminator). This explicitly restricts client IDs to 23 bytes, violating the rule that servers MAY allow longer client IDs. While generate_random_id (util.c:139) generates a 23-byte ID, the critical violation occurs in connect_handler where externally provided client IDs are truncated.
15.Not following CONNECT errors and not allowing subsequent messages to be processed
According to the specification of MQTTv3.1.1: If the Server rejects the CONNECT Packet, the Server MUST NOT process any data sent by the Client after the CONNECT Packet [MQTT-3.1.4-5].
After the server rejects the CONNECT packet, it still processes subsequent data sent by the client. In the connect_handler function, when the connection is rejected (such as authentication failure or unauthorized), the server returns an error code (MQTT_BAD_USERNAME_OR_PASSWORD or MQTT_NOT_AUTHORIZED), but does not disconnect the client. In the process_message function, even if these error codes are returned, it only sends a CONNACK response and then continues to maintain the connection, allowing the client to send more packets. The server should disconnect immediately after rejecting CONNECT, rather than continuing to process data from this client.
16.There is a problem with the handling of wildcards
According to the specification of MQTTv3.1.1: If the Will Flag is set to 1, the Will Message MUST be removed from the stored Session state in the Server once the Will Message has been published or the Server has received a DISCONNECT packet from the Client [MQTT-3.1.2-10].
When the Will Flag is set to 1, the server does not remove the Will Message from the stored session state after publishing the Will Message or receiving a DISCONNECT packet from the client. In the server code, when the client unexpectedly disconnects, the Will Message is published (lines 811-816 of src/server.c), but the Will Message in the session is not cleared. Similarly, when the server receives a DISCONNECT packet (lines 496-499 of src/handlers.c), the Will Message is not cleared. The Will Message remains in the lwt_msg field in the client_session structure and is not cleared even after publishing or receiving a DISCONNECT.
17.There is a problem with the handling of wildcards
According to the specification of MQTTv3.1.1: Each non-wildcarded level in the Topic Filter must exactly match the corresponding level in the Topic Name, character for character, for the subscription match to succeed.
The match_subscription function has a problem in handling non-wildcard topic level matches. The function uses character-by-character comparison when comparing topic names and topic filters, but the processing logic after encountering the '+' wildcard is flawed. The function uses the index() function to skip the current topic level, but it does not correctly handle the case of multi-level topics. In particular, after processing a wildcard, the function uses ptopic = index(ptopic, '/') and ptopic = index(ptopic + 1, '/') to try to jump to the next topic level, but this method does not guarantee the correct alignment of the corresponding level in the topic name and the topic filter. This may cause non-wildcard level topic filters to be compared with non-corresponding levels in the topic name in some cases, thus violating the rule requirement of 'character-by-character exact match'.
18.Subscription wildcard does not work
According to the specification of MQTTv3.1.1: The Topic Filters in an UNSUBSCRIBE packet MUST be UTF-8 encoded strings as defined in Section 1.5.3, packed contiguously [MQTT-3.10.3-1].
The code does not validate UTF-8 encoding for UNSUBSCRIBE packet's Topic Filters. The function unpack_mqtt_unsubscribe() uses unpack_string16() to parse topic filters but lacks explicit UTF-8 validation checks, violating the MQTT-3.10.3-1 requirement that Topic Filters MUST be UTF-8 encoded.
19.The first packet is not CONNECT.
According to the specification of MQTTv3.1.1: After a Network Connection is established by a Client to a Server, the first Packet sent from the Client to the Server MUST be a CONNECT Packet [MQTT-3.1.0-1].
The server does not correctly verify whether the first packet sent by the client is a CONNECT packet. In the process_message function, after receiving the packet from the client, the server directly calls the handle_command function to process the packet without checking whether this is the first packet sent by the client. Although there is logic in the connect_handler function to check whether the client is connected (cc->connected == true), this only prevents the connected client from sending the second CONNECT packet, rather than ensuring that the first packet must be CONNECT. If the client first sends non-CONNECT packets (such as PUBLISH, SUBSCRIBE, etc.), the server will try to process these packets instead of rejecting and disconnecting, which violates the MQTT v3.1.0-1 rules.
20.Other
20.1 violation 1
According to the specification of MQTTv3.1.1: If a server receives a valid CONNECT packet but cannot process the request, the server must send a CONNACK packet with the corresponding non-zero Connect return code defined in the protocol specification and immediately close the network connection.
The code violates the MQTT 3.1.1 rule in two scenarios: 1) When a double CONNECT packet is received (lines 352-360), the server disconnects without sending a CONNACK. 2) In error cases (e.g., bad authentication or not authorized), the server sends a CONNACK with a non-zero return code but does not immediately close the connection, allowing further communication.
20.2 violation 2
According to the specification of MQTTv3.1.1: If a server receives a valid CONNECT packet but cannot process the request, the server must send a CONNACK packet with the corresponding non-zero Connect return code defined in the protocol specification and immediately close the network connection.
The code sends a CONNACK packet for authentication/authorization failures (e.g., MQTT_NOT_AUTHORIZED) via enqueue_event_write() but does not ensure the network connection is closed immediately afterward. The MQTT 3.1.1 rule mandates that the server MUST close the connection after sending a CONNACK with a non-zero return code. While the server enqueues the CONNACK response, the connection remains active until the write event completes, violating the requirement to terminate the connection upon failure.
20.3 violation 3
According to the specification of MQTTv3.1.1: If the Server chooses not to support topic filters that contain wildcard characters, the Server MUST reject any Subscription request whose filter contains wildcard characters [MQTT-3.8.3-2].
The server code does not provide any configuration options to disable wildcard support, and there is no check for wildcard support in the subscribe_handler function. Instead, the code explicitly handles subscription requests that contain wildcard characters, such as '#' and '+'. In the subscribe_handler function, the server detects a wildcard and adds it to the wildcards list, which is used when matching topics. If the server chooses not to support wildcards (although there is no such option in the current code), it should reject the subscription request when a wildcard is detected, but the existing code does not perform this check. In particular, when handling the '+' wildcard, the code uses the add_wildcard function to add it to the wildcards list instead of rejecting the subscription.
20.4 violation 4
According to the specification of MQTTv3.1.1: The wildcard characters can be used in Topic Filters, but these characters MUST NOT be used within a Topic Name to comply with protocol specifications [MQTT-4.7.1-1].
The code does not validate that Topic Names in PUBLISH packets exclude wildcard characters '+' and '#', violating [MQTT-4.7.1-1]. The unpack_mqtt_publish function extracts the topic from the packet without validation, and the publish_handler function processes and stores the topic without checking for wildcards. Both functions contribute to the violation by allowing unvalidated topic names to propagate through the system.
21.The relationship between Will Flag and Will QoS is not handled properly
According to the specification of MQTTv3.1.1: If the Will Flag is set to 1, the value of Will QoS can be 0 (0x00), 1 (0x01), or 2 (0x02). If the Will Flag is set to 0, then the Will QoS MUST be set to 0 (0x00) [MQTT-3.1.2-13]. If the Will Flag is set to 1, Will QoS MUST NOT be 3 (0x03) [MQTT-3.1.2-14].
Replay such packet:
echo 103000044D515454041E003C000E76696F6C61746F72436C69656E74000D62616477696C6C2F746F70696300056572726F72 | xxd -p -r | nc 127.0.0.1 1883
echo -n "101200044d515454040a00000006636c69656e7431" | xxd -r -p | nc 127.0.0.1 1883
The expected behavior should be that the server checks this logic and terminates the connection, but from the response it doesn't seem to do so.
22.The relationship between Will Flag, Will QoS, and Will Retain is not handled properly
According to the specification of MQTTv3.1.1: If the Will Flag is set to 0, the Will QoS and Will Retain fields in the Connect Flags MUST be set to zero and the Will Topic and Will Message fields MUST NOT be present in the payload [MQTT-3.1.2-11]. If the Will Flag is set to 0, then the Will Retain Flag MUST be set to 0 [MQTT-3.1.2-15].
Replay such packet:
echo 101500044D5154540422003C0009626164436C69656E74 | xxd -p -r | nc 127.0.0.1 1883
echo 101A00044D5154540422003C000E72657461696E56696F6C61746F72 | xxd -p -r | nc 127.0.0.1 1883
The expected behavior should be that the server checks this logic and terminates the connection, but from the response it doesn't seem to do so.
23.The Retain Flag of the Will message is not set to 1
According to the specification of MQTTv3.1.1: If the Will Flag is set to 1 and the Will Retain flag is set to 1, the Server MUST publish the Will Message as a retained message [MQTT-3.1.2-17].
Replay such packet:
echo -n 105100044D515454042E003C0012436C69656E74415F57696C6C5465737465720011746573742F636C69656E74412F77696C6C001E436C69656E74415F736179735F627974655F756E677261636566756C6C79 | xxd -p -r | nc 127.0.0.1 1883
The expected behavior should be that the server checks this logic and terminates the connection, but from the response it doesn't seem to do so.
24. The relationship between Will Flag and Will Topic && Will Message is not handled properly
According to the specification of MQTTv3.1.1: If the Will Flag is set to 1, the Will QoS and Will Retain fields in the Connect Flags will be used by the Server, and the Will Topic and Will Message fields MUST be present in the payload [MQTT-3.1.2-9].
Replay such packet:
echo 101800044D515454040E003C000C77696C6C56696F6C61746F72 | xxd -p -r | nc 127.0.0.1 1883
The expected behavior should be that the server checks this logic and terminates the connection, but from the response it doesn't seem to do so.