Fix CONNECT performance with many user-properties.

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.

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

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;

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)

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

test/broker/test.py

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