From 1a1ffde16ba4695fcfbd0b3f7ac15a0bf36f3ff7 Mon Sep 17 00:00:00 2001 From: "Roger A. Light" Date: Sun, 15 Sep 2019 22:35:08 +0100 Subject: [PATCH] Restrict topic hierarchy to 200 levels to prevent possible stack overflow. Closes #1412. Thanks to Ryan Shaw. --- ChangeLog.txt | 8 +++++ lib/util_topic.c | 45 ++++++++++++++++++++++++++ src/mosquitto_broker_internal.h | 3 ++ src/subs.c | 7 ++++ test/broker/02-subscribe-long-topic.py | 36 +++++++++++++++++++++ test/broker/03-publish-long-topic.py | 37 +++++++++++++++++++++ test/broker/Makefile | 2 ++ test/broker/test.py | 2 ++ test/mosq_test.py | 18 +++++++---- 9 files changed, 152 insertions(+), 6 deletions(-) create mode 100755 test/broker/02-subscribe-long-topic.py create mode 100755 test/broker/03-publish-long-topic.py diff --git a/ChangeLog.txt b/ChangeLog.txt index 8fcb17d3..e8e5ff98 100644 --- a/ChangeLog.txt +++ b/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 ================ diff --git a/lib/util_topic.c b/lib/util_topic.c index 67b78782..673cc6ca 100644 --- a/lib/util_topic.c +++ b/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; } diff --git a/src/mosquitto_broker_internal.h b/src/mosquitto_broker_internal.h index 25d4a5b0..322c6a8a 100644 --- a/src/mosquitto_broker_internal.h +++ b/src/mosquitto_broker_internal.h @@ -73,6 +73,9 @@ Contributors: #define WEBSOCKET_CLIENT -2 + +#define TOPIC_HIERARCHY_LIMIT 200 + /* ======================================== * UHPA data types * ======================================== */ diff --git a/src/subs.c b/src/subs.c index aae32666..c059874f 100644 --- a/src/subs.c +++ b/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 TOPIC_HIERARCHY_LIMIT){ + /* Set limit on hierarchy levels, to restrict stack usage. */ + goto cleanup; + } + return MOSQ_ERR_SUCCESS; cleanup: diff --git a/test/broker/02-subscribe-long-topic.py b/test/broker/02-subscribe-long-topic.py new file mode 100755 index 00000000..c2e7a107 --- /dev/null +++ b/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) + diff --git a/test/broker/03-publish-long-topic.py b/test/broker/03-publish-long-topic.py new file mode 100755 index 00000000..f57c33bb --- /dev/null +++ b/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) + diff --git a/test/broker/Makefile b/test/broker/Makefile index fb6cda1e..0b938042 100644 --- a/test/broker/Makefile +++ b/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 diff --git a/test/broker/test.py b/test/broker/test.py index 5aadbbee..2d010904 100755 --- a/test/broker/test.py +++ b/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'), diff --git a/test/mosq_test.py b/test/mosq_test.py index 2093e326..1669f566 100644 --- a/test/mosq_test.py +++ b/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: