Restrict topic hierarchy to 200 levels to prevent possible stack over…

…flow.

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,29 +49,51 @@ and the Eclipse Distribution License is available at
 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;
 }
 
 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;
 
  for(i=0; i<len; i++){
  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 @@ and the Eclipse Distribution License is available at
 
 #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, puback_packet, "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

test/broker/test.py

@@ -54,6 +54,7 @@
  (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'),

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"
- return struct.pack(pack_format, 130, 2+1+2+len(topic)+1, mid, 0, len(topic), topic, qos)
+ packet += pack_remaining_length(2+1+2+len(topic)+1)
+ 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"
- return struct.pack(pack_format, 130, 2+1+2+len(topic)+len(properties), mid, properties, len(topic), topic, qos)
+ packet += pack_remaining_length(2+1+2+len(topic)+len(properties))
+ 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"
- return struct.pack(pack_format, 130, 2+2+len(topic)+1, mid, len(topic), topic, qos)
+ packet += 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, proto_ver=4):
  if proto_ver == 5: