Restrict topic hierarchy to 200 levels to prevent possible stack overflow.

Closes #1412. Thanks to Ryan Shaw.

ChangeLog.txt

@@ -1,3 +1,11 @@
+1.6.6 - 20190915
+================
+
+Broker:
+- Restrict topic hierarchy to 200 levels to prevent possible stack overflow.
+  Closes #1412.
+
+
 1.6.5 - 20190912
 ================
 

lib/util_topic.c

@@ -49,14 +49,25 @@ Contributors:
 int mosquitto_pub_topic_check(const char *str)
 {
 	int len = 0;
+#ifdef WITH_BROKER
+	int hier_count = 0;
+#endif
 	while(str && str[0]){
 		if(str[0] == '+' || str[0] == '#'){
 			return MOSQ_ERR_INVAL;
 		}
+#ifdef WITH_BROKER
+		else if(str[0] == '/'){
+			hier_count++;
+		}
+#endif
 		len++;
 		str = &str[1];
 	}
 	if(len > 65535) return MOSQ_ERR_INVAL;
+#ifdef WITH_BROKER
+	if(hier_count > TOPIC_HIERARCHY_LIMIT) return MOSQ_ERR_INVAL;
+#endif
 
 	return MOSQ_ERR_SUCCESS;
 }
@@ -64,6 +75,9 @@ int mosquitto_pub_topic_check(const char *str)
 int mosquitto_pub_topic_check2(const char *str, size_t len)
 {
 	size_t i;
+#ifdef WITH_BROKER
+	int hier_count = 0;
+#endif
 
 	if(len > 65535) return MOSQ_ERR_INVAL;
 
@@ -71,7 +85,15 @@ int mosquitto_pub_topic_check2(const char *str, size_t len)
 		if(str[i] == '+' || str[i] == '#'){
 			return MOSQ_ERR_INVAL;
 		}
+#ifdef WITH_BROKER
+		else if(str[i] == '/'){
+			hier_count++;
+		}
+#endif
 	}
+#ifdef WITH_BROKER
+	if(hier_count > TOPIC_HIERARCHY_LIMIT) return MOSQ_ERR_INVAL;
+#endif
 
 	return MOSQ_ERR_SUCCESS;
 }
@@ -87,6 +109,10 @@ int mosquitto_sub_topic_check(const char *str)
 {
 	char c = '\0';
 	int len = 0;
+#ifdef WITH_BROKER
+	int hier_count = 0;
+#endif
+
 	while(str && str[0]){
 		if(str[0] == '+'){
 			if((c != '\0' && c != '/') || (str[1] != '\0' && str[1] != '/')){
@@ -97,11 +123,19 @@ int mosquitto_sub_topic_check(const char *str)
 				return MOSQ_ERR_INVAL;
 			}
 		}
+#ifdef WITH_BROKER
+		else if(str[0] == '/'){
+			hier_count++;
+		}
+#endif
 		len++;
 		c = str[0];
 		str = &str[1];
 	}
 	if(len > 65535) return MOSQ_ERR_INVAL;
+#ifdef WITH_BROKER
+	if(hier_count > TOPIC_HIERARCHY_LIMIT) return MOSQ_ERR_INVAL;
+#endif
 
 	return MOSQ_ERR_SUCCESS;
 }
@@ -110,6 +144,9 @@ int mosquitto_sub_topic_check2(const char *str, size_t len)
 {
 	char c = '\0';
 	size_t i;
+#ifdef WITH_BROKER
+	int hier_count = 0;
+#endif
 
 	if(len > 65535) return MOSQ_ERR_INVAL;
 
@@ -123,8 +160,16 @@ int mosquitto_sub_topic_check2(const char *str, size_t len)
 				return MOSQ_ERR_INVAL;
 			}
 		}
+#ifdef WITH_BROKER
+		else if(str[i] == '/'){
+			hier_count++;
+		}
+#endif
 		c = str[i];
 	}
+#ifdef WITH_BROKER
+	if(hier_count > TOPIC_HIERARCHY_LIMIT) return MOSQ_ERR_INVAL;
+#endif
 
 	return MOSQ_ERR_SUCCESS;
 }

src/mosquitto_broker_internal.h

@@ -73,6 +73,9 @@ Contributors:
 
 #define WEBSOCKET_CLIENT -2
 
+
+#define TOPIC_HIERARCHY_LIMIT 200
+
 /* ========================================
  * UHPA data types
  * ======================================== */

src/subs.c

@@ -220,6 +220,7 @@ static int sub__topic_tokenise(const char *subtopic, struct sub__token **topics)
 	int start, stop, tlen;
 	int i;
 	char *topic;
+	int count = 0;
 
 	assert(subtopic);
 	assert(topics);
@@ -242,6 +243,7 @@ static int sub__topic_tokenise(const char *subtopic, struct sub__token **topics)
 
 	stop = 0;
 	for(i=start; i<len+1; i++){
+		count++;
 		if(subtopic[i] == '/' || subtopic[i] == '\0'){
 			stop = i;
 
@@ -262,6 +264,11 @@ static int sub__topic_tokenise(const char *subtopic, struct sub__token **topics)
 		}
 	}
 
+	if(count > TOPIC_HIERARCHY_LIMIT){
+		/* Set limit on hierarchy levels, to restrict stack usage. */
+		goto cleanup;
+	}
+
 	return MOSQ_ERR_SUCCESS;
 
 cleanup:

test/broker/02-subscribe-long-topic.py

@@ -0,0 +1,36 @@
+#!/usr/bin/env python3
+
+# Test whether a SUBSCRIBE to a topic with 65535 hierarchy characters fails
+# This needs checking with MOSQ_USE_VALGRIND=1 to detect memory failures
+# https://github.com/eclipse/mosquitto/issues/1412
+
+from mosq_test_helper import *
+
+rc = 1
+mid = 1
+keepalive = 60
+connect_packet = mosq_test.gen_connect("subscribe-long-test", keepalive=keepalive)
+connack_packet = mosq_test.gen_connack(rc=0)
+
+subscribe_packet = mosq_test.gen_subscribe(mid, "/"*65535, 0)
+suback_packet = mosq_test.gen_suback(mid, 0)
+
+port = mosq_test.get_port()
+broker = mosq_test.start_broker(filename=os.path.basename(__file__), port=port)
+
+try:
+    sock = mosq_test.do_client_connect(connect_packet, connack_packet, port=port)
+    mosq_test.do_send_receive(sock, subscribe_packet, b"", "suback")
+
+    rc = 0
+
+    sock.close()
+finally:
+    broker.terminate()
+    broker.wait()
+    (stdo, stde) = broker.communicate()
+    if rc:
+        print(stde.decode('utf-8'))
+
+exit(rc)
+

test/broker/03-publish-long-topic.py

@@ -0,0 +1,37 @@
+#!/usr/bin/env python3
+
+# Test whether a PUBLISH to a topic with 65535 hierarchy characters fails
+# This needs checking with MOSQ_USE_VALGRIND=1 to detect memory failures
+# https://github.com/eclipse/mosquitto/issues/1412
+
+
+from mosq_test_helper import *
+
+rc = 1
+mid = 19
+keepalive = 60
+connect_packet = mosq_test.gen_connect("pub-qos1-test", keepalive=keepalive)
+connack_packet = mosq_test.gen_connack(rc=0)
+
+publish_packet = mosq_test.gen_publish("/"*65535, qos=1, mid=mid, payload="message")
+puback_packet = mosq_test.gen_puback(mid)
+
+port = mosq_test.get_port()
+broker = mosq_test.start_broker(filename=os.path.basename(__file__), port=port)
+
+try:
+    sock = mosq_test.do_client_connect(connect_packet, connack_packet, port=port)
+    mosq_test.do_send_receive(sock, publish_packet, b"", "puback")
+
+    rc = 0
+
+    sock.close()
+finally:
+    broker.terminate()
+    broker.wait()
+    (stdo, stde) = broker.communicate()
+    if rc:
+        print(stde.decode('utf-8'))
+
+exit(rc)
+

test/broker/Makefile

@@ -75,6 +75,7 @@ endif
 	./02-subpub-qos2.py
 	./02-subscribe-dollar-v5.py
 	./02-subscribe-invalid-utf8.py
+	./02-subscribe-long-topic.py
 	./02-subscribe-persistence-flipflop.py
 	./02-subscribe-qos0.py
 	./02-subscribe-qos1.py
@@ -99,6 +100,7 @@ endif
 	./03-publish-dollar-v5.py
 	./03-publish-dollar.py
 	./03-publish-invalid-utf8.py
+	./03-publish-long-topic.py
 	./03-publish-qos1-no-subscribers-v5.py
 	./03-publish-qos1-retain-disabled.py
 	./03-publish-qos1.py

test/broker/test.py

@@ -54,6 +54,7 @@ tests = [
     (1, './02-subpub-qos2.py'),
     (1, './02-subscribe-dollar-v5.py'),
     (1, './02-subscribe-invalid-utf8.py'),
+    (1, './02-subscribe-long-topic.py'),
     (1, './02-subscribe-persistence-flipflop.py'),
     (1, './02-subscribe-qos0.py'),
     (1, './02-subscribe-qos1.py'),
@@ -77,6 +78,7 @@ tests = [
     (1, './03-publish-dollar-v5.py'),
     (1, './03-publish-dollar.py'),
     (1, './03-publish-invalid-utf8.py'),
+    (1, './03-publish-long-topic.py'),
     (1, './03-publish-qos1-no-subscribers-v5.py'),
     (1, './03-publish-qos1-retain-disabled.py'),
     (1, './03-publish-qos1.py'),

test/mosq_test.py

@@ -478,19 +478,25 @@ def gen_pubrel(mid, dup=False, proto_ver=4, reason_code=-1, properties=None):
 def gen_pubcomp(mid, proto_ver=4, reason_code=-1, properties=None):
     return _gen_command_with_mid(112, mid, proto_ver, reason_code, properties)
 
+
 def gen_subscribe(mid, topic, qos, proto_ver=4, properties=b""):
     topic = topic.encode("utf-8")
+    packet = struct.pack("!B", 130)
     if proto_ver == 5:
         if properties == b"":
-            pack_format = "!BBHBH"+str(len(topic))+"sB"
+            packet += pack_remaining_length(2+1+2+len(topic)+1)
-            return struct.pack(pack_format, 130, 2+1+2+len(topic)+1, mid, 0, len(topic), topic, qos)
+            pack_format = "!HBH"+str(len(topic))+"sB"
+            return packet + struct.pack(pack_format, mid, 0, len(topic), topic, qos)
         else:
             properties = mqtt5_props.prop_finalise(properties)
-            pack_format = "!BBH"+str(len(properties))+"s"+"H"+str(len(topic))+"sB"
+            packet += pack_remaining_length(2+1+2+len(topic)+len(properties))
-            return struct.pack(pack_format, 130, 2+1+2+len(topic)+len(properties), mid, properties, len(topic), topic, qos)
+            pack_format = "!H"+str(len(properties))+"s"+"H"+str(len(topic))+"sB"
+            return packet + struct.pack(pack_format, mid, properties, len(topic), topic, qos)
     else:
-        pack_format = "!BBHH"+str(len(topic))+"sB"
+        packet += pack_remaining_length(2+2+len(topic)+1)
-        return struct.pack(pack_format, 130, 2+2+len(topic)+1, mid, len(topic), topic, qos)
+        pack_format = "!HH"+str(len(topic))+"sB"
+        return packet + struct.pack(pack_format, mid, len(topic), topic, qos)
+
 
 def gen_suback(mid, qos, proto_ver=4):
     if proto_ver == 5: