Error found in sol

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.

[peter-pe]

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.