diff --git a/ChangeLog.txt b/ChangeLog.txt index 47723dbe..3ad5ec14 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -2,6 +2,9 @@ =================== Security: +- An MQTT v5 client connecting with a large number of user-property properties + could cause excessive CPU usage, leading to a loss of performance and + possible denial of service. This has been fixed. - Fix `max_keepalive` not applying to MQTT v3.1.1 and v3.1 connections. These clients are now rejected if their keepalive value exceeds max_keepalive. This option allows CVE-2020-13849, which is for the MQTT diff --git a/lib/property_mosq.c b/lib/property_mosq.c index b4dc73dc..f8d15408 100644 --- a/lib/property_mosq.c +++ b/lib/property_mosq.c @@ -962,14 +962,14 @@ int mosquitto_property_check_all(int command, const mosquitto_property *properti if(rc) return rc; /* Check for duplicates */ - tail = p->next; - while(tail){ - if(p->identifier == tail->identifier - && p->identifier != MQTT_PROP_USER_PROPERTY){ - - return MOSQ_ERR_DUPLICATE_PROPERTY; + if(p->identifier != MQTT_PROP_USER_PROPERTY){ + tail = p->next; + while(tail){ + if(p->identifier == tail->identifier){ + return MOSQ_ERR_DUPLICATE_PROPERTY; + } + tail = tail->next; } - tail = tail->next; } p = p->next; diff --git a/test/broker/01-connect-575314.py b/test/broker/01-connect-575314.py new file mode 100755 index 00000000..4a8f314d --- /dev/null +++ b/test/broker/01-connect-575314.py @@ -0,0 +1,49 @@ +#!/usr/bin/env python3 + +# Check for performance of processing user-property on CONNECT + +from mosq_test_helper import * + +def do_test(): + rc = 1 + props = mqtt5_props.gen_string_pair_prop(mqtt5_props.PROP_USER_PROPERTY, "key", "value") + for i in range(0, 20000): + props += mqtt5_props.gen_string_pair_prop(mqtt5_props.PROP_USER_PROPERTY, "key", "value") + connect_packet_slow = mosq_test.gen_connect("connect-user-property", proto_ver=5, properties=props) + connect_packet_fast = mosq_test.gen_connect("a"*65000, proto_ver=5) + connack_packet = mosq_test.gen_connack(rc=0, proto_ver=5) + + port = mosq_test.get_port() + broker = mosq_test.start_broker(filename=os.path.basename(__file__), port=port) + + try: + t_start = time.monotonic() + sock = mosq_test.do_client_connect(connect_packet_slow, connack_packet, port=port) + t_stop = time.monotonic() + sock.close() + + t_diff_slow = t_stop - t_start + + t_start = time.monotonic() + sock = mosq_test.do_client_connect(connect_packet_fast, connack_packet, port=port) + t_stop = time.monotonic() + sock.close() + + t_diff_fast = t_stop - t_start + # 20 is chosen as a factor that works in plain mode and running under + # valgrind. The slow performance manifests as a factor of >100. Fast is <10. + if t_diff_slow / t_diff_fast < 20: + rc = 0 + except mosq_test.TestError: + pass + finally: + broker.terminate() + broker.wait() + (stdo, stde) = broker.communicate() + if rc: + print(stde.decode('utf-8')) + exit(rc) + + +do_test() +exit(0) diff --git a/test/broker/Makefile b/test/broker/Makefile index 51cb64b7..2f93ee55 100644 --- a/test/broker/Makefile +++ b/test/broker/Makefile @@ -23,6 +23,7 @@ msg_sequence_test: ./msg_sequence_test.py 01 : + ./01-connect-575314.py ./01-connect-allow-anonymous.py ./01-connect-disconnect-v5.py ./01-connect-max-connections.py diff --git a/test/broker/test.py b/test/broker/test.py index 08dd2f8c..c022f887 100755 --- a/test/broker/test.py +++ b/test/broker/test.py @@ -5,6 +5,7 @@ import ptest tests = [ #(ports required, 'path'), + (1, './01-connect-575314.py'), (1, './01-connect-allow-anonymous.py'), (1, './01-connect-disconnect-v5.py'), (1, './01-connect-max-connections.py'),