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

Closes #1412. Thanks to Ryan Shaw.

lib/util_mosq.c

@@ -143,14 +143,25 @@ uint16_t mosquitto__mid_generate(struct mosquitto *mosq)
 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;
 }
@@ -158,6 +169,9 @@ int mosquitto_pub_topic_check(const char *str)
 int mosquitto_pub_topic_check2(const char *str, size_t len)
 {
 	int i;
+#ifdef WITH_BROKER
+	int hier_count = 0;
+#endif
 
 	if(len > 65535) return MOSQ_ERR_INVAL;
 
@@ -165,7 +179,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;
 }
@@ -181,6 +203,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] != '/')){
@@ -191,11 +217,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;
 }
@@ -204,6 +238,9 @@ int mosquitto_sub_topic_check2(const char *str, size_t len)
 {
 	char c = '\0';
 	int i;
+#ifdef WITH_BROKER
+	int hier_count = 0;
+#endif
 
 	if(len > 65535) return MOSQ_ERR_INVAL;
 
@@ -217,8 +254,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

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

src/subs.c

@@ -178,6 +178,7 @@ static int sub__topic_tokenise(const char *subtopic, struct sub__token **topics)
 	int start, stop, tlen;
 	int i;
 	mosquitto__topic_element_uhpa topic;
+	int count = 0;
 
 	assert(subtopic);
 	assert(topics);
@@ -200,6 +201,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;
 
@@ -219,6 +221,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,44 @@
+#!/usr/bin/env python
+
+# 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
+
+import time
+import inspect, os, sys
+# From http://stackoverflow.com/questions/279237/python-import-a-module-from-a-folder
+cmd_subfolder = os.path.realpath(os.path.abspath(os.path.join(os.path.split(inspect.getfile( inspect.currentframe() ))[0],"..")))
+if cmd_subfolder not in sys.path:
+    sys.path.insert(0, cmd_subfolder)
+
+import mosq_test
+
+
+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,45 @@
+#!/usr/bin/env python
+
+# 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
+
+
+import time
+import inspect, os, sys
+# From http://stackoverflow.com/questions/279237/python-import-a-module-from-a-folder
+cmd_subfolder = os.path.realpath(os.path.abspath(os.path.join(os.path.split(inspect.getfile( inspect.currentframe() ))[0],"..")))
+if cmd_subfolder not in sys.path:
+    sys.path.insert(0, cmd_subfolder)
+
+import mosq_test
+
+
+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

@@ -39,6 +39,7 @@ endif
 
 
 02 :
+	./02-subscribe-long-topic.py
 	./02-subscribe-qos0.py
 	./02-subscribe-qos1.py
 	./02-subscribe-qos2.py
@@ -67,6 +68,7 @@ endif
 	./03-pattern-matching.py
 	#./03-publish-qos1-queued-bytes.py
 	./03-publish-invalid-utf8.py
+	./03-publish-long-topic.py
 	./03-publish-dollar.py
 
 04 :

test/broker/ptest.py

@@ -25,6 +25,7 @@ tests = [
     (1, './02-subscribe-qos0.py'),
     (1, './02-subscribe-qos1.py'),
     (1, './02-subscribe-qos2.py'),
+    (1, './02-subscribe-long-topic.py'),
     (1, './02-subpub-qos0.py'),
     (1, './02-subpub-qos1.py'),
     (1, './02-subpub-qos2.py'),
@@ -46,6 +47,7 @@ tests = [
     (1, './03-publish-b2c-disconnect-qos1.py'),
     (1, './03-publish-c2b-disconnect-qos2.py'),
     (1, './03-publish-b2c-disconnect-qos2.py'),
+    (1, './03-publish-long-topic.py'),
     (1, './03-pattern-matching.py'),
     #(1, './03-publish-qos1-queued-bytes.py'),
     (1, './03-publish-invalid-utf8.py'),

test/mosq_test.py

@@ -395,9 +395,13 @@ def gen_pubrel(mid, dup=False):
 def gen_pubcomp(mid):
     return struct.pack('!BBH', 112, 2, mid)
 
+
 def gen_subscribe(mid, topic, qos):
-    pack_format = "!BBHH"+str(len(topic))+"sB"
+    packet = struct.pack("!B", 130)
-    return struct.pack(pack_format, 130, 2+2+len(topic)+1, mid, len(topic), topic, qos)
+    packet += bytes(pack_remaining_length(2+2+len(topic)+1))
+    pack_format = "!HH"+str(len(topic))+"sB"
+    return packet + struct.pack(pack_format, mid, len(topic), topic, qos)
+
 
 def gen_suback(mid, qos):
     return struct.pack('!BBHB', 144, 2+1, mid, qos)
@@ -419,7 +423,7 @@ def gen_disconnect():
     return struct.pack('!BB', 224, 0)
 
 def pack_remaining_length(remaining_length):
-    s = ""
+    s = bytes("".encode('utf-8'))
     while True:
         byte = remaining_length % 128
         remaining_length = remaining_length // 128