Apply max_keepalive to MQTT v3.1.1 and v3.1 clients.

ChangeLog.txt

@@ -1,6 +1,12 @@
 2.0.12 - 2021-07-xx
 ===================
 
+Security:
+- 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
+  v3.1.1 protocol itself rather than an implementation, to be addressed.
+
 Broker:
 - Fix possible out of bounds memory reads when reading a corrupt/crafted
   configuration file. Unless your configuration file is writable by untrusted
@@ -9,6 +15,10 @@ Broker:
 - Fix TLS certificates and TLS-PSK not being able to be configured at the same
   time.
 - Disable TLS v1.3 when using TLS-PSK, because it isn't correctly configured.
+- 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
+  v3.1.1 protocol itself rather than an implementation, to be addressed.
 
 Client library:
 - If a client uses TLS-PSK then force the default cipher list to use "PSK"

lib/strings_mosq.c

@@ -42,7 +42,7 @@ const char *mosquitto_strerror(int mosq_errno)
 		case MOSQ_ERR_PROTOCOL:
 			return "A network protocol error occurred when communicating with the broker.";
 		case MOSQ_ERR_INVAL:
-			return "Invalid function arguments provided.";
+			return "Invalid arguments provided.";
 		case MOSQ_ERR_NO_CONN:
 			return "The client is not currently connected.";
 		case MOSQ_ERR_CONN_REFUSED:

man/mosquitto.conf.5.xml

@@ -624,6 +624,15 @@ log_timestamp_format %Y-%m-%dT%H:%M:%S
 						The maximum value allowable, and default value, is
 						65535. Do not set below 10 seconds.</para>
 
+					<para>
+						For MQTT v3.1.1 and v3.1 clients, there is no mechanism
+						to tell the client what keepalive value they should use.
+						If an MQTT v3.1.1 or v3.1 client specifies a keepalive
+						time greater than max_keepalive they will be sent a
+						CONNACK message with the "identifier rejected" reason
+						code, and disconnected.
+					</para>
+
 					<para>This option applies globally.</para>
 
 					<para>Reloaded on reload signal.</para>

mosquitto.conf

@@ -75,6 +75,12 @@
 # value, otherwise they will be sent a server keepalive telling them to use
 # max_keepalive. This only applies to MQTT v5 clients. The maximum value
 # allowable is 65535. Do not set below 10.
+#
+# For MQTT v3.1.1 and v3.1 clients, there is no mechanism to tell the client
+# what keepalive value they should use. If an MQTT v3.1.1 or v3.1 client
+# specifies a keepalive time greater than max_keepalive they will be sent a
+# CONNACK message with the "identifier rejected" reason code, and disconnected.
+#
 #max_keepalive 65535
 
 # For MQTT v5 clients, it is possible to have the server send a "maximum packet

src/handle_connect.c

@@ -249,21 +249,28 @@ int connect__on_authorised(struct mosquitto *context, void *auth_data_out, uint1
 #endif
 	context->max_qos = context->listener->max_qos;
 
-	if(context->protocol == mosq_p_mqtt5){
-		if(context->listener->max_topic_alias > 0){
-			if(mosquitto_property_add_int16(&connack_props, MQTT_PROP_TOPIC_ALIAS_MAXIMUM, context->listener->max_topic_alias)){
-				rc = MOSQ_ERR_NOMEM;
-				goto error;
-			}
-		}
 	if(db.config->max_keepalive &&
 			(context->keepalive > db.config->max_keepalive || context->keepalive == 0)){
 
 		context->keepalive = db.config->max_keepalive;
+		if(context->protocol == mosq_p_mqtt5){
 			if(mosquitto_property_add_int16(&connack_props, MQTT_PROP_SERVER_KEEP_ALIVE, context->keepalive)){
 				rc = MOSQ_ERR_NOMEM;
 				goto error;
 			}
+		}else{
+			send__connack(context, connect_ack, CONNACK_REFUSED_IDENTIFIER_REJECTED, NULL);
+			rc = MOSQ_ERR_INVAL;
+			goto error;
+		}
+	}
+
+	if(context->protocol == mosq_p_mqtt5){
+		if(context->listener->max_topic_alias > 0){
+			if(mosquitto_property_add_int16(&connack_props, MQTT_PROP_TOPIC_ALIAS_MAXIMUM, context->listener->max_topic_alias)){
+				rc = MOSQ_ERR_NOMEM;
+				goto error;
+			}
 		}
 		if(context->assigned_id){
 			if(mosquitto_property_add_string(&connack_props, MQTT_PROP_ASSIGNED_CLIENT_IDENTIFIER, context->id)){

test/broker/01-connect-max-keepalive.py

@@ -0,0 +1,44 @@
+#!/usr/bin/env python3
+
+# Test whether max_keepalive violations are rejected for MQTT < 5.0.
+
+from mosq_test_helper import *
+
+def write_config(filename, port):
+    with open(filename, 'w') as f:
+        f.write("listener %d\n" % (port))
+        f.write("allow_anonymous true\n")
+        f.write("max_keepalive 100\n")
+
+def do_test(proto_ver):
+    rc = 1
+
+    connect_packet = mosq_test.gen_connect("max-keepalive", keepalive=101, proto_ver=proto_ver)
+    connack_packet = mosq_test.gen_connack(rc=2, proto_ver=proto_ver)
+
+    port = mosq_test.get_port()
+    conf_file = os.path.basename(__file__).replace('.py', '.conf')
+    write_config(conf_file, port)
+    broker = mosq_test.start_broker(filename=os.path.basename(__file__), use_conf=True, port=port)
+
+    socks = []
+    try:
+        sock = mosq_test.do_client_connect(connect_packet, connack_packet, port=port)
+        sock.close()
+        rc = 0
+    except mosq_test.TestError:
+        pass
+    except Exception as err:
+        print(err)
+    finally:
+        os.remove(conf_file)
+        broker.terminate()
+        broker.wait()
+        (stdo, stde) = broker.communicate()
+        if rc:
+            print(stde.decode('utf-8'))
+            exit(rc)
+
+do_test(3)
+do_test(4)
+exit(0)

test/broker/12-prop-server-keepalive.py

@@ -22,8 +22,10 @@ rc = 1
 keepalive = 61
 connect_packet = mosq_test.gen_connect("test", proto_ver=5, keepalive=keepalive)
 
-props = mqtt5_props.gen_uint16_prop(mqtt5_props.PROP_SERVER_KEEP_ALIVE, 60)
+props = mqtt5_props.gen_uint16_prop(mqtt5_props.PROP_SERVER_KEEP_ALIVE, 60) \
-connack_packet = mosq_test.gen_connack(rc=0, proto_ver=5, properties=props)
+        + mqtt5_props.gen_uint16_prop(mqtt5_props.PROP_TOPIC_ALIAS_MAXIMUM, 10) \
+        + mqtt5_props.gen_uint16_prop(mqtt5_props.PROP_RECEIVE_MAXIMUM, 20)
+connack_packet = mosq_test.gen_connack(rc=0, proto_ver=5, properties=props, property_helper=False)
 
 broker = mosq_test.start_broker(filename=os.path.basename(__file__), port=port, use_conf=True)
 

test/broker/Makefile

@@ -31,6 +31,7 @@ test : test-compile 01 02 03 04 05 06 07 08 09 10 11 12 13 14
 	./01-connect-invalid-protonum.py
 	./01-connect-invalid-reserved.py
 	./01-connect-max-connections.py
+	./01-connect-max-keepalive.py
 	./01-connect-success.py
 	./01-connect-uname-invalid-utf8.py
 	./01-connect-uname-no-flag.py

test/broker/test.py

@@ -16,6 +16,7 @@ tests = [
     (1, './01-connect-invalid-protonum.py'),
     (1, './01-connect-invalid-reserved.py'),
     (1, './01-connect-max-connections.py'),
+    (1, './01-connect-max-keepalive.py'),
     (1, './01-connect-success.py'),
     (1, './01-connect-uname-invalid-utf8.py'),
     (1, './01-connect-uname-no-flag.py'),