Send DISCONNECT with malformed-packet reason code on bad UNSUBSCRIBEs.

ChangeLog.txt

@@ -16,6 +16,8 @@ Broker:
   publish messages.
 - Add `mosquitto_client_protocol_version()` function which can be used by
   plugins to determine which version of MQTT a client has connected with.
+- Send DISCONNECT with `malformed-packet` reason code on invalid UNSUBSCRIBE
+  packets.
 
 Client library:
 - Client no longer generates random client ids for v3.1.1 clients, these are

src/handle_unsubscribe.c

@@ -46,15 +46,21 @@ int handle__unsubscribe(struct mosquitto_db *db, struct mosquitto *context)
 
 	if(context->protocol != mosq_p_mqtt31){
 		if((context->in_packet.command&0x0F) != 0x02){
-			return MOSQ_ERR_PROTOCOL;
+			return MOSQ_ERR_MALFORMED_PACKET;
 		}
 	}
-	if(packet__read_uint16(&context->in_packet, &mid)) return 1;
-	if(mid == 0) return MOSQ_ERR_PROTOCOL;
+	if(packet__read_uint16(&context->in_packet, &mid)) return MOSQ_ERR_MALFORMED_PACKET;
+	if(mid == 0) return MOSQ_ERR_MALFORMED_PACKET;
 
 	if(context->protocol == mosq_p_mqtt5){
 		rc = property__read_all(CMD_UNSUBSCRIBE, &context->in_packet, &properties);
-		if(rc) return rc;
+		if(rc){
+			if(rc == MOSQ_ERR_PROTOCOL){
+				return MOSQ_ERR_MALFORMED_PACKET;
+			}else{
+				return rc;
+			}
+		}
 		/* Immediately free, we don't do anything with User Property at the moment */
 		mosquitto_property_free_all(&properties);
 	}
@@ -62,7 +68,7 @@ int handle__unsubscribe(struct mosquitto_db *db, struct mosquitto *context)
 	if(context->protocol == mosq_p_mqtt311 || context->protocol == mosq_p_mqtt5){
 		if(context->in_packet.pos == context->in_packet.remaining_length){
 			/* No topic specified, protocol error. */
-			return MOSQ_ERR_PROTOCOL;
+			return MOSQ_ERR_MALFORMED_PACKET;
 		}
 	}
 
@@ -76,7 +82,7 @@ int handle__unsubscribe(struct mosquitto_db *db, struct mosquitto *context)
 		sub = NULL;
 		if(packet__read_string(&context->in_packet, &sub, &slen)){
 			mosquitto__free(reason_codes);
-			return 1;
+			return MOSQ_ERR_MALFORMED_PACKET;
 		}
 
 		if(!slen){
@@ -85,7 +91,7 @@ int handle__unsubscribe(struct mosquitto_db *db, struct mosquitto *context)
 					context->id);
 			mosquitto__free(sub);
 			mosquitto__free(reason_codes);
-			return 1;
+			return MOSQ_ERR_MALFORMED_PACKET;
 		}
 		if(mosquitto_sub_topic_check(sub)){
 			log__printf(NULL, MOSQ_LOG_INFO,
@@ -93,7 +99,7 @@ int handle__unsubscribe(struct mosquitto_db *db, struct mosquitto *context)
 					context->id);
 			mosquitto__free(sub);
 			mosquitto__free(reason_codes);
-			return 1;
+			return MOSQ_ERR_MALFORMED_PACKET;
 		}
 
 		log__printf(NULL, MOSQ_LOG_DEBUG, "\t%s", sub);

src/loop.c

@@ -308,6 +308,9 @@ void do_disconnect(struct mosquitto_db *db, struct mosquitto *context, int reaso
 				switch(reason){
 					case MOSQ_ERR_SUCCESS:
 						break;
+					case MOSQ_ERR_MALFORMED_PACKET:
+						log__printf(NULL, MOSQ_LOG_NOTICE, "Client %s disconnected due to malformed packet.", id);
+						break;
 					case MOSQ_ERR_PROTOCOL:
 						log__printf(NULL, MOSQ_LOG_NOTICE, "Client %s disconnected due to protocol error.", id);
 						break;

src/read_handle.c

@@ -32,6 +32,8 @@ Contributors:
 
 int handle__packet(struct mosquitto_db *db, struct mosquitto *context)
 {
+	int rc = MOSQ_ERR_INVAL;
+
 	if(!context) return MOSQ_ERR_INVAL;
 
 	switch((context->in_packet.command)&0xF0){
@@ -56,7 +58,8 @@ int handle__packet(struct mosquitto_db *db, struct mosquitto *context)
 		case CMD_SUBSCRIBE:
 			return handle__subscribe(db, context);
 		case CMD_UNSUBSCRIBE:
-			return handle__unsubscribe(db, context);
+			rc = handle__unsubscribe(db, context);
+			break;
 #ifdef WITH_BRIDGE
 		case CMD_CONNACK:
 			return handle__connack(db, context);
@@ -68,8 +71,18 @@ int handle__packet(struct mosquitto_db *db, struct mosquitto *context)
 		case CMD_AUTH:
 			return handle__auth(db, context);
 		default:
-			/* If we don't recognise the command, return an error straight away. */
-			return MOSQ_ERR_PROTOCOL;
+			rc = MOSQ_ERR_PROTOCOL;
+	}
+
+	if(context->protocol == mosq_p_mqtt5){
+		if(rc == MOSQ_ERR_PROTOCOL){
+			send__disconnect(context, MQTT_RC_PROTOCOL_ERROR, NULL);
+		}else if(rc == MOSQ_ERR_MALFORMED_PACKET){
+			send__disconnect(context, MQTT_RC_MALFORMED_PACKET, NULL);
+		}else if(rc == MOSQ_ERR_UNKNOWN || rc == MOSQ_ERR_NOMEM){
+			send__disconnect(context, MQTT_RC_UNSPECIFIED, NULL);
+		}
 	}
+	return rc;
 }
 

test/broker/02-unsubscribe-invalid-no-topic.py

@@ -22,7 +22,11 @@ def do_test(proto_ver):
 
     try:
         sock = mosq_test.do_client_connect(connect_packet, connack_packet, port=port)
-        mosq_test.do_send_receive(sock, unsubscribe_packet, b"", "disconnect")
+        if proto_ver == 4:
+            mosq_test.do_send_receive(sock, unsubscribe_packet, b"", "disconnect")
+        else:
+            disconnect_packet = mosq_test.gen_disconnect(proto_ver=5, reason_code=mqtt5_rc.MQTT_RC_MALFORMED_PACKET)
+            mosq_test.do_send_receive(sock, unsubscribe_packet, disconnect_packet, "disconnect")
 
         rc = 0
 

test/broker/13-malformed-unsubscribe-v5.py

@@ -0,0 +1,69 @@
+#!/usr/bin/env python3
+
+# Test whether the broker handles malformed packets correctly - UNSUBSCRIBE
+# MQTTv5
+
+from mosq_test_helper import *
+
+rc = 1
+
+def do_test(unsubscribe_packet, reason_code, error_string):
+    global rc
+
+    rc = 1
+
+    keepalive = 10
+    connect_packet = mosq_test.gen_connect("test", proto_ver=5, keepalive=keepalive)
+    connack_packet = mosq_test.gen_connack(rc=0, proto_ver=5)
+
+    mid = 0
+    disconnect_packet = mosq_test.gen_disconnect(proto_ver=5, reason_code=reason_code)
+
+    sock = mosq_test.do_client_connect(connect_packet, connack_packet, port=port)
+    mosq_test.do_send_receive(sock, unsubscribe_packet, disconnect_packet, error_string=error_string)
+    rc = 0
+
+
+port = mosq_test.get_port()
+broker = mosq_test.start_broker(filename=os.path.basename(__file__), port=port)
+
+try:
+    # mid == 0
+    unsubscribe_packet = mosq_test.gen_unsubscribe(topic="test/topic", mid=0, proto_ver=5)
+    do_test(unsubscribe_packet, mqtt5_rc.MQTT_RC_MALFORMED_PACKET, "mid == 0")
+
+    # command flags != 0x02
+    unsubscribe_packet = mosq_test.gen_unsubscribe(topic="test/topic", mid=1, proto_ver=5, cmd=160)
+    do_test(unsubscribe_packet, mqtt5_rc.MQTT_RC_MALFORMED_PACKET, "command flags != 0x02")
+
+    # Truncated packet, no mid
+    unsubscribe_packet = struct.pack("!BB", 162, 0)
+    do_test(unsubscribe_packet, mqtt5_rc.MQTT_RC_MALFORMED_PACKET, "Truncated packet, no mid")
+
+    # Incorrect property
+    props = mqtt5_props.gen_uint32_prop(mqtt5_props.PROP_SESSION_EXPIRY_INTERVAL, 0)
+    unsubscribe_packet = mosq_test.gen_unsubscribe(topic="test/topic", mid=1, proto_ver=5, properties=props)
+    do_test(unsubscribe_packet, mqtt5_rc.MQTT_RC_MALFORMED_PACKET, "Incorrect property")
+
+    # Truncated packet, no topic
+    unsubscribe_packet = struct.pack("!BBH", 162, 2, 1)
+    do_test(unsubscribe_packet, mqtt5_rc.MQTT_RC_MALFORMED_PACKET, "Truncated packet, no topic")
+
+    # Truncated packet, empty topic
+    unsubscribe_packet = struct.pack("!BBHH", 162, 4, 1, 0)
+    do_test(unsubscribe_packet, mqtt5_rc.MQTT_RC_MALFORMED_PACKET, "Truncated packet, empty topic")
+
+    # Truncated packet, empty topic, with properties field
+    unsubscribe_packet = struct.pack("!BBHHH", 162, 5, 1, 0, 0)
+    do_test(unsubscribe_packet, mqtt5_rc.MQTT_RC_MALFORMED_PACKET, "Truncated packet, empty topic, with properties field")
+
+    # Bad topic
+    unsubscribe_packet = mosq_test.gen_unsubscribe(topic="#/test/topic", mid=1, proto_ver=5)
+    do_test(unsubscribe_packet, mqtt5_rc.MQTT_RC_MALFORMED_PACKET, "Bad topic")
+finally:
+    broker.terminate()
+    broker.wait()
+    (stdo, stde) = broker.communicate()
+    if rc:
+        print(stde.decode('utf-8'))
+        exit(rc)

test/broker/Makefile

@@ -17,7 +17,7 @@ test-compile :
 ptest : test-compile
 	./test.py
 
-test : test-compile 01 02 03 04 05 06 07 08 09 10 11 12
+test : test-compile 01 02 03 04 05 06 07 08 09 10 11 12 13
 
 01 :
 	./01-connect-anon-denied.py
@@ -216,3 +216,6 @@ endif
 	./12-prop-subpub-content-type.py
 	./12-prop-subpub-payload-format.py
 	./12-prop-topic-alias-invalid.py
+
+13 :
+	./13-malformed-unsubscribe-v5.py

test/broker/test.py

@@ -181,6 +181,8 @@ tests = [
     (1, './12-prop-subpub-content-type.py'),
     (1, './12-prop-subpub-payload-format.py'),
     (1, './12-prop-topic-alias-invalid.py'),
+
+    (1, './13-malformed-unsubscribe-v5.py'),
     ]
 
 ptest.run_tests(tests)

test/mosq_test.py

@@ -532,14 +532,23 @@ def gen_suback(mid, qos, proto_ver=4):
     else:
         return struct.pack('!BBHB', 144, 2+1, mid, qos)
 
-def gen_unsubscribe(mid, topic, proto_ver=4):
+def gen_unsubscribe(mid, topic, cmd=162, proto_ver=4, properties=b""):
     topic = topic.encode("utf-8")
     if proto_ver == 5:
-        pack_format = "!BBHBH"+str(len(topic))+"s"
-        return struct.pack(pack_format, 162, 2+2+len(topic)+1, mid, 0, len(topic), topic)
+        if properties == b"":
+            pack_format = "!BBHBH"+str(len(topic))+"s"
+            return struct.pack(pack_format, cmd, 2+2+len(topic)+1, mid, 0, len(topic), topic)
+        else:
+            properties = mqtt5_props.prop_finalise(properties)
+            packet = struct.pack("!B", cmd)
+            l = 2+2+len(topic)+1+len(properties)
+            packet += pack_remaining_length(l)
+            pack_format = "!HB"+str(len(properties))+"sH"+str(len(topic))+"s"
+            packet += struct.pack(pack_format, mid, len(properties), properties, len(topic), topic)
+            return packet
     else:
         pack_format = "!BBHH"+str(len(topic))+"s"
-        return struct.pack(pack_format, 162, 2+2+len(topic), mid, len(topic), topic)
+        return struct.pack(pack_format, cmd, 2+2+len(topic), mid, len(topic), topic)
 
 def gen_unsubscribe_multiple(mid, topics, proto_ver=4):
     packet = b""