From 7a2f0f306676214a6eef55eb9ab4e1577ff7e1a6 Mon Sep 17 00:00:00 2001 From: "Roger A. Light" Date: Wed, 15 Apr 2026 12:31:28 +0100 Subject: [PATCH] Protocol error edge case test fixes and redundant check removal --- lib/handle_pubrel.c | 3 --- src/handle_disconnect.c | 16 +++++------- src/handle_subscribe.c | 2 +- test/broker/data/CONNECT.json | 33 ++++++++++++++++++++++++ test/broker/data/DISCONNECT.json | 8 +++--- test/broker/data/PUBLISH.json | 43 +++++-------------------------- test/broker/data/PUBREL.json | 8 +++--- test/broker/data/SUBSCRIBE.json | 23 +++++------------ test/broker/data/UNSUBSCRIBE.json | 5 ++++ test/broker/msg_sequence_test.py | 2 +- 10 files changed, 66 insertions(+), 77 deletions(-) diff --git a/lib/handle_pubrel.c b/lib/handle_pubrel.c index bc53177e..4d3a8deb 100644 --- a/lib/handle_pubrel.c +++ b/lib/handle_pubrel.c @@ -56,9 +56,6 @@ int handle__pubrel(struct mosquitto *mosq) #endif return MOSQ_ERR_PROTOCOL; } - if(mosq->protocol != mosq_p_mqtt31 && mosq->in_packet.command != (CMD_PUBREL|2)){ - return MOSQ_ERR_MALFORMED_PACKET; - } if(mosq->protocol != mosq_p_mqtt31){ if((mosq->in_packet.command&0x0F) != 0x02){ diff --git a/src/handle_disconnect.c b/src/handle_disconnect.c index fd135fab..95218950 100644 --- a/src/handle_disconnect.c +++ b/src/handle_disconnect.c @@ -37,8 +37,12 @@ int handle__disconnect(struct mosquitto *context) return MOSQ_ERR_INVAL; } - if(context->in_packet.command != CMD_DISCONNECT){ - return MOSQ_ERR_MALFORMED_PACKET; + if(context->protocol == mosq_p_mqtt311 || context->protocol == mosq_p_mqtt5){ + if((context->in_packet.command&0x0F) != 0x00){ + log__printf(NULL, MOSQ_LOG_INFO, "Protocol error from %s: DISCONNECT packet with incorrect flags %02X.", + context->id, context->in_packet.command); + return MOSQ_ERR_PROTOCOL; + } } if(context->protocol == mosq_p_mqtt5 && context->in_packet.remaining_length > 0){ @@ -68,14 +72,6 @@ int handle__disconnect(struct mosquitto *context) return MOSQ_ERR_PROTOCOL; } log__printf(NULL, MOSQ_LOG_DEBUG, "Received DISCONNECT from %s", context->id); - if(context->protocol == mosq_p_mqtt311 || context->protocol == mosq_p_mqtt5){ - if((context->in_packet.command&0x0F) != 0x00){ - log__printf(NULL, MOSQ_LOG_INFO, "Protocol error from %s: DISCONNECT packet with incorrect flags %02X.", - context->id, context->in_packet.command); - do_disconnect(context, MOSQ_ERR_PROTOCOL); - return MOSQ_ERR_PROTOCOL; - } - } if(reason_code == MQTT_RC_DISCONNECT_WITH_WILL_MSG){ mosquitto__set_state(context, mosq_cs_disconnect_with_will); }else{ diff --git a/src/handle_subscribe.c b/src/handle_subscribe.c index 032c63d1..5ef8b5b9 100644 --- a/src/handle_subscribe.c +++ b/src/handle_subscribe.c @@ -90,7 +90,7 @@ int handle__subscribe(struct mosquitto *context) /* If the identifier was force set to 0, this is an error */ if(subscription_identifier == 0){ mosquitto_property_free_all(&properties); - return MOSQ_ERR_MALFORMED_PACKET; + return MOSQ_ERR_PROTOCOL; } } diff --git a/test/broker/data/CONNECT.json b/test/broker/data/CONNECT.json index 63ce1d02..18628851 100644 --- a/test/broker/data/CONNECT.json +++ b/test/broker/data/CONNECT.json @@ -17,6 +17,10 @@ {"type":"send", "payload":"10 r15 s6 'MQIsdp' 02 00 k10 s1 'p'", "comment":"CONNECT"}, {"type":"recv", "payload":"20 r3 00 84 00", "comment": "CONNACK identifier rejected"} ]}, + { "name": "10 proto ver 4", "msgs":[ + {"type":"send", "payload":"10 r15 s6 'MQIsdp' 04 00 k10 s1 'p'", "comment":"CONNECT"}, + {"type":"recv", "payload":"20 r2 00 01", "comment": "CONNACK refused protocol version"} + ]}, { "name": "10 proto ver 6", "msgs":[ {"type":"send", "payload":"10 r15 s6 'MQIsdp' 06 00 k10 s1 'p'", "comment":"CONNECT"}, {"type":"recv", "payload":"20 r3 00 84 00", "comment": "CONNACK identifier rejected"} @@ -43,6 +47,31 @@ {"type":"send", "payload":"10 r13 s4 'MQTT' 04 02 k10 s1 'p'", "comment":"minimal valid CONNECT"}, {"type":"recv", "payload":"20 r2 00 00", "comment": "CONNACK"} ]}, + { "name": "10 missing protocol string length", "msgs":[ + {"type":"send", "payload":"10 r0", "comment": "CONNECT"} + ]}, + { "name": "10 incorrect protocol string length", "msgs":[ + {"type":"send", "payload":"10 r12 s3 'MQT' 04 02 k10 s1 'p'", "comment":"CONNECT"} + ]}, + { "name": "10 missing protocol string", "msgs":[ + {"type":"send", "payload":"10 r2 s4", "comment": "CONNECT"} + ]}, + { "name": "10 missing protocol version", "msgs":[ + {"type":"send", "payload":"10 r6 s4 'MQTT'", "comment": "CONNECT"} + ]}, + { "name": "10 incorrect protocol version ", "msgs":[ + {"type":"send", "payload":"10 r13 s4 'MQTT' 03 02 k10 s1 'p'", "comment":"CONNECT"}, + {"type":"recv", "payload":"20 r2 00 01", "comment": "CONNACK"} + ]}, + { "name": "10 incorrect command flags", "msgs":[ + {"type":"send", "payload":"1F r13 s4 'MQTT' 05 02 k10 s1 'p'", "comment":"CONNECT"} + ]}, + { "name": "10 missing keepalive", "msgs":[ + {"type":"send", "payload":"10 r8 s4 'MQTT' 05 02", "comment":"CONNECT"} + ]}, + { "name": "10 excess bytes ", "msgs":[ + {"type":"send", "payload":"10 r14 s4 'MQTT' 04 02 k10 s1 'p' 00", "comment":"CONNECT"} + ]}, { "name": "10 [MQTT-3.1.0-2]", "msgs":[ {"type":"send", "payload":"10 r13 s4 'MQTT' 04 02 k10 s1 'p'", "comment":"minimal valid CONNECT"}, {"type":"recv", "payload":"20 r2 00 00", "comment": "CONNACK"}, @@ -269,6 +298,10 @@ {"type":"send", "payload":"10 r15 s4 'MQTT' 05 02 k10 01 21 s1 'p'"}, {"type":"recv", "payload":"20 r3 00 81 00"} ]}, + { "name": "receive-maximum (two byte integer) == 0", "msgs": [ + {"type":"send", "payload":"10 r17 s4 'MQTT' 05 02 k10 03 21 0000 s1 'p'"}, + {"type":"recv", "payload":"20 r3 00 82 00"} + ]}, { "name": "maximum-packet-size (four byte integer)", "expect_disconnect":false, "msgs":[ {"type":"send", "payload":"10 r19 s4 'MQTT' 05 02 k10 05 27 10000001 s1 'p'"}, diff --git a/test/broker/data/DISCONNECT.json b/test/broker/data/DISCONNECT.json index 5d381f97..a9f81029 100644 --- a/test/broker/data/DISCONNECT.json +++ b/test/broker/data/DISCONNECT.json @@ -23,19 +23,19 @@ { "name": "E0 valid", "msgs": [{"type":"send", "payload":"E0 r0"}]}, { "name": "E1 [MQTT-3.14.1-1]", "msgs": [ {"type":"send", "payload":"E1 r0"}, - {"type":"recv", "payload":"E0 r1 81"} + {"type":"recv", "payload":"E0 r1 82"} ]}, { "name": "E2 [MQTT-3.14.1-1]", "msgs": [ {"type":"send", "payload":"E2 r0"}, - {"type":"recv", "payload":"E0 r1 81"} + {"type":"recv", "payload":"E0 r1 82"} ]}, { "name": "E4 [MQTT-3.14.1-1]", "msgs": [ {"type":"send", "payload":"E4 r0"}, - {"type":"recv", "payload":"E0 r1 81"} + {"type":"recv", "payload":"E0 r1 82"} ]}, { "name": "E8 [MQTT-3.14.1-1]", "msgs": [ {"type":"send", "payload":"E8 r0"}, - {"type":"recv", "payload":"E0 r1 81"} + {"type":"recv", "payload":"E0 r1 82"} ]}, { "name": "E0 RC=0x00 (normal disconnection)", "msgs": [{"type":"send", "payload":"E0 r1 00"}]}, diff --git a/test/broker/data/PUBLISH.json b/test/broker/data/PUBLISH.json index 1045778e..bd701e25 100644 --- a/test/broker/data/PUBLISH.json +++ b/test/broker/data/PUBLISH.json @@ -12,6 +12,7 @@ { "name": "38 QoS 0 Dup 1", "msgs": [{"type":"send", "payload":"38 r14 s5 'topic' 'payload'"}]}, { "name": "36 QoS 3 (no mid) [MQTT-3.3.1-4]", "msgs": [{"type":"send", "payload":"36 r14 s5 'topic' 'payload'"}]}, { "name": "36 QoS 3 (with mid) [MQTT-3.3.1-4]", "msgs": [{"type":"send", "payload":"36 r16 s5 'topic' m1234 'payload'"}]}, + { "name": "32 QoS 1 (no mid)", "msgs": [{"type":"send", "payload":"32 r7 s5 'topic'"}]}, { "name": "32 QoS 1 Mid 0", "msgs": [{"type":"send", "payload":"32 r16 s5 'topic' m0 'payload'"}]}, { "name": "34 QoS 2 Mid 0", "msgs": [{"type":"send", "payload":"34 r16 s5 'topic' m0 'payload'"}]}, { "name": "32 QoS 1 Dup 0", "expect_disconnect":false, "msgs": [ @@ -154,7 +155,7 @@ "tests": [ { "name": "maximum packet size", "connect":false, "expect_disconnect":false, "msgs":[ {"type":"send", "payload":"10 r19 s4 'MQTT' 05 02 k10 v5 2700000014 s1 'p'", "comment":"CONNECT with max-packet-size 20"}, - {"type":"recv", "payload":"20 r14 00000B 22 H10 27 L2000000 21 H20", "comment": "CONNACK"}, + {"type":"recv", "payload":"20 r14 00000b 22 H10 27 H30 84 80 21 00 14", "comment": "CONNACK"}, {"type":"send", "payload":"82 r11 m1234 v0 s5 'topic' 00", "comment":"SUBSCRIBE topic"}, {"type":"recv", "payload":"90 r4 m1234 v0 00", "comment":"SUBACK"}, {"type":"send", "payload":"30 r22 s5 'topic' v0 'payloadpayload'", "comment":"PUBLISH with size > 20"}, @@ -220,15 +221,15 @@ {"type":"send", "payload":"32 r18 s5 'topic' m1234 v1 23 'payload'"}, {"type":"recv", "payload":"E0 r1 81"} ]}, + { "name": "topic-alias (two byte integer) no topic", "msgs": [ + {"type":"send", "payload":"32 r15 s0 m1234 v3 23 H1 'payload'"}, + {"type":"recv", "payload":"E0 r1 82"} + ]}, { "name": "response-topic (UTF-8 string)", "expect_disconnect":false, "msgs": [ {"type":"send", "payload":"32 r21 s5 'topic' m1234 v4 08 s1 'p' 'payload'"}, {"type":"recv", "payload":"40 r3 m1234 10"} ]}, - { "name": "response-topic (UTF-8 string, with wildcard)", "ver":5, "msgs": [ - {"type":"send", "payload":"32 r21 s5 'topic' m1234 v4 08 s1 '#' 'payload'"}, - {"type":"recv", "payload":"E0 r1 82"} - ]}, { "name": "2*response-topic (UTF-8 string)", "msgs": [ {"type":"send", "payload":"32 r25 s5 'topic' m1234 v8 08 s1 'p' 08 s1 'p' 'payload'"}, {"type":"recv", "payload":"E0 r1 82"} @@ -237,10 +238,6 @@ {"type":"send", "payload":"32 r18 s5 'topic' m1234 v1 08 'payload'"}, {"type":"recv", "payload":"E0 r1 81"} ]}, - { "name": "response-topic (UTF-8 string) empty", "msgs": [ - {"type":"send", "payload":"32 r20 s5 'topic' m1234 v3 08 s0 'payload'"}, - {"type":"recv", "payload":"E0 r1 82"} - ]}, { "name": "correlation-data (binary data)", "expect_disconnect":false, "msgs": [ {"type":"send", "payload":"32 r21 s5 'topic' m1234 v4 09 s1 'p' 'payload'"}, @@ -313,38 +310,10 @@ {"type":"send", "payload":"32 r19 s5 'topic' m1234 v2 0B v0 'payload'"}, {"type":"recv", "payload":"E0 r1 82"} ]}, - { "name": "subscription-identifier=0x7F (variable byte integer)", "msgs": [ - {"type":"send", "payload":"32 r19 s5 'topic' m1234 v2 0B 7F 'payload'"}, - {"type":"recv", "payload":"E0 r1 82"} - ]}, { "name": "subscription-identifier=0x8000 (variable byte integer)", "msgs": [ {"type":"send", "payload":"32 r20 s5 'topic' m1234 v3 0B 8000 'payload'"}, {"type":"recv", "payload":"E0 r1 81"} ]}, - { "name": "subscription-identifier=0x8001 (variable byte integer)", "msgs": [ - {"type":"send", "payload":"32 r20 s5 'topic' m1234 v3 0B 8001 'payload'"}, - {"type":"recv", "payload":"E0 r1 82"} - ]}, - { "name": "subscription-identifier=0xFF7F (variable byte integer)", "msgs": [ - {"type":"send", "payload":"32 r20 s5 'topic' m1234 v3 0B FF7F 'payload'"}, - {"type":"recv", "payload":"E0 r1 82"} - ]}, - { "name": "subscription-identifier=0x808001 (variable byte integer)", "msgs": [ - {"type":"send", "payload":"32 r21 s5 'topic' m1234 v4 0B 808001 'payload'"}, - {"type":"recv", "payload":"E0 r1 82"} - ]}, - { "name": "subscription-identifier=0xFFFF7F (variable byte integer)", "msgs": [ - {"type":"send", "payload":"32 r21 s5 'topic' m1234 v4 0B FFFF7F 'payload'"}, - {"type":"recv", "payload":"E0 r1 82"} - ]}, - { "name": "subscription-identifier=0x80808001 (variable byte integer)", "msgs": [ - {"type":"send", "payload":"32 r22 s5 'topic' m1234 v5 0B 80808001 'payload'"}, - {"type":"recv", "payload":"E0 r1 82"} - ]}, - { "name": "subscription-identifier=0xFFFFFF7F (variable byte integer)", "msgs": [ - {"type":"send", "payload":"32 r22 s5 'topic' m1234 v5 0B FFFFFF7F 'payload'"}, - {"type":"recv", "payload":"E0 r1 82"} - ]}, { "name": "subscription-identifier=0x8080808001 (variable byte integer)", "msgs": [ {"type":"send", "payload":"32 r23 s5 'topic' m1234 v6 0B 8080808001 'payload'"}, {"type":"recv", "payload":"E0 r1 81"} diff --git a/test/broker/data/PUBREL.json b/test/broker/data/PUBREL.json index 7355e564..497edfad 100644 --- a/test/broker/data/PUBREL.json +++ b/test/broker/data/PUBREL.json @@ -72,19 +72,19 @@ ]}, { "name": "63 unsolicited", "msgs": [ {"type":"send", "payload":"63 r3 m1 00"}, - {"type":"recv", "payload":"E0 r1 81"} + {"type":"recv", "payload":"E0 r1 82"} ]}, { "name": "64 unsolicited", "msgs": [ {"type":"send", "payload":"64 r3 m1 00"}, - {"type":"recv", "payload":"E0 r1 81"} + {"type":"recv", "payload":"E0 r1 82"} ]}, { "name": "66 unsolicited", "msgs": [ {"type":"send", "payload":"66 r3 m1 00"}, - {"type":"recv", "payload":"E0 r1 81"} + {"type":"recv", "payload":"E0 r1 82"} ]}, { "name": "6A unsolicited", "msgs": [ {"type":"send", "payload":"6A r3 m1 00"}, - {"type":"recv", "payload":"E0 r1 81"} + {"type":"recv", "payload":"E0 r1 82"} ]} ] }, diff --git a/test/broker/data/SUBSCRIBE.json b/test/broker/data/SUBSCRIBE.json index 975bd0f2..7f9b3466 100644 --- a/test/broker/data/SUBSCRIBE.json +++ b/test/broker/data/SUBSCRIBE.json @@ -24,6 +24,7 @@ { "name": "82 topic with U+007F", "msgs": [{"type":"send", "payload":"82 r10 m1234 s5 746F707F70 00"} ] }, { "name": "82 topic with U+009F", "msgs": [{"type":"send", "payload":"82 r10 m1234 s5 746FC29F70 00"} ] }, { "name": "82 topic with U+FFFF", "msgs": [{"type":"send", "payload":"82 r10 m1234 s5 746FEDBFBF 00"} ] }, + { "name": "82 invalid topic", "msgs": [{"type":"send", "payload":"82 r10 m1234 s5 'ab/+e' 00"} ] }, { "name": "82 long", "msgs": [{"type":"send", "payload":"82 r7 m1234 s1 'p' 00 00"}]}, { "name": "82 short 5 [MQTT-3.8.3-3]", "msgs": [{"type":"send", "payload":"82 r5 m1234 s1 'p'"}]}, { "name": "82 short 4", "msgs": [{"type":"send", "payload":"82 r4 m1234 0000"}]}, @@ -163,6 +164,10 @@ {"type":"send", "payload":"82 r18 m1234 v0 s5 746FEDBFBF 'payload' 00"}, {"type":"recv", "payload":"E0 r1 81"} ]}, + { "name": "82 invalid topic", "msgs": [ + {"type":"send", "payload":"82 r10 m1234 s5 'ab/+e' 00"}, + {"type":"recv", "payload":"E0 r1 81"} + ]}, { "name": "82 long", "msgs": [ {"type":"send", "payload":"82 r8 m1234 v0 s1 'p' 00 00"}, {"type":"recv", "payload":"E0 r1 81"} @@ -231,22 +236,6 @@ {"type":"send", "payload":"82 r7 m0 00 s1 'p' 00"}, {"type":"recv", "payload":"E0 r1 81"} ]}, - { "name": "82 shared sub with + in sharename", "ver":5, "msgs": [ - {"type":"send", "payload":"82 r16 m1234 v0 s10 '$share/+/p' 00"}, - {"type":"recv", "payload":"E0 r1 82"} - ]}, - { "name": "82 shared sub with # in sharename", "ver":5, "msgs": [ - {"type":"send", "payload":"82 r16 m1234 v0 s10 '$share/#/p' 00"}, - {"type":"recv", "payload":"E0 r1 81"} - ]}, - { "name": "82 shared sub with no topic part 1", "ver":5, "msgs": [ - {"type":"send", "payload":"82 r15 m1234 v0 s9 '$share/p/' 00"}, - {"type":"recv", "payload":"E0 r1 82"} - ]}, - { "name": "82 shared sub with no topic part 2", "ver":5, "msgs": [ - {"type":"send", "payload":"82 r14 m1234 v0 s8 '$share/p' 00"}, - {"type":"recv", "payload":"E0 r1 82"} - ]}, { "name": "82 shared sub with no local set", "ver":5, "msgs": [ {"type":"send", "payload":"82 r16 m1234 v0 s10 '$share/p/p' 04"}, {"type":"recv", "payload":"E0 r1 82"} @@ -292,7 +281,7 @@ ]}, { "name": "82 with subscription-identifier=0 (variable byte integer)", "msgs": [ {"type":"send", "payload":"82 r9 m1 v2 0B v0 s1 'p' 00"}, - {"type":"recv", "payload":"E0 r1 81"} + {"type":"recv", "payload":"E0 r1 82"} ]}, { "name": "82 with subscription-identifier=1 (variable byte integer)", "expect_disconnect":false, "msgs": [ {"type":"send", "payload":"82 r9 m1 v2 0B v1 s1 'p' 00"}, diff --git a/test/broker/data/UNSUBSCRIBE.json b/test/broker/data/UNSUBSCRIBE.json index f6a609e0..37d34760 100644 --- a/test/broker/data/UNSUBSCRIBE.json +++ b/test/broker/data/UNSUBSCRIBE.json @@ -38,6 +38,7 @@ { "name": "A2 topic with U+007F", "msgs": [{"type":"send", "payload":"A2 r9 m1234 s5 746F707F70"}]}, { "name": "A2 topic with U+009F", "msgs": [{"type":"send", "payload":"A2 r9 m1234 s5 746FC29F70"}]}, { "name": "A2 topic with U+FFFF", "msgs": [{"type":"send", "payload":"A2 r9 m1234 s5 746FEDBFBF"}]}, + { "name": "A2 invalid topic", "msgs": [{"type":"send", "payload":"A2 r9 m1234 s5 'ab/+e'"}]}, { "name": "A2 zero mid", "msgs": [ {"type":"send", "payload":"A2 r8 0000 s1 'p' s1 'q'"}]} ] }, @@ -137,6 +138,10 @@ {"type":"send", "payload":"A2 r10 m1234 v0 s5 746FEDBFBF"}, {"type":"recv", "payload":"E0 r1 81"} ]}, + { "name": "A2 invalid topic", "msgs": [ + {"type":"send", "payload":"A2 r9 m1234 s5 'ab/+e'"}, + {"type":"recv", "payload":"E0 r1 81"} + ]}, { "name": "A2 multiple [MQTT-3.10.4-6]", "expect_disconnect":false, "msgs": [ {"type":"send", "payload":"A2 r9 m1234 v0 s1 'p' s1 'q'"}, {"type":"recv", "payload":"B0 r5 m1234 v0 11 11"} diff --git a/test/broker/msg_sequence_test.py b/test/broker/msg_sequence_test.py index 5f7e31ea..52e91673 100755 --- a/test/broker/msg_sequence_test.py +++ b/test/broker/msg_sequence_test.py @@ -89,7 +89,7 @@ class MsgSequence(object): def _recv_message(self, sock, msg): data = sock.recv(len(msg.message)) if data != msg.message: - raise ValueError("Receive message %s | rec:%s | exp:%s" % (msg.comment, data, msg.message)) + raise ValueError("Receive message %s | rec:%s | exp:%s" % (msg.comment, data.hex(), msg.message.hex())) def _disconnected_check(self, sock):