From 93b2232bb9e8941d5c641976a507b4008f57dde3 Mon Sep 17 00:00:00 2001 From: Roger Light Date: Fri, 20 Aug 2021 14:38:54 +0100 Subject: [PATCH] Apply max_keepalive to MQTT v3.1.1 and v3.1 clients. --- ChangeLog.txt | 10 ++++++ lib/strings_mosq.c | 2 +- man/mosquitto.conf.5.xml | 9 +++++ mosquitto.conf | 6 ++++ src/handle_connect.c | 21 ++++++++---- test/broker/01-connect-max-keepalive.py | 44 +++++++++++++++++++++++++ test/broker/12-prop-server-keepalive.py | 6 ++-- test/broker/Makefile | 1 + test/broker/test.py | 1 + 9 files changed, 90 insertions(+), 10 deletions(-) create mode 100755 test/broker/01-connect-max-keepalive.py diff --git a/ChangeLog.txt b/ChangeLog.txt index 3cc753c9..e46f5379 100644 --- a/ChangeLog.txt +++ b/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" diff --git a/lib/strings_mosq.c b/lib/strings_mosq.c index 3688650d..ae9af1cf 100644 --- a/lib/strings_mosq.c +++ b/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: diff --git a/man/mosquitto.conf.5.xml b/man/mosquitto.conf.5.xml index f35e9062..99dd5d16 100644 --- a/man/mosquitto.conf.5.xml +++ b/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. + + 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. + + This option applies globally. Reloaded on reload signal. diff --git a/mosquitto.conf b/mosquitto.conf index 4e407417..5b1f785b 100644 --- a/mosquitto.conf +++ b/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 diff --git a/src/handle_connect.c b/src/handle_connect.c index c1a096b6..3643caea 100644 --- a/src/handle_connect.c +++ b/src/handle_connect.c @@ -249,18 +249,25 @@ 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)){ + 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(db.config->max_keepalive && - (context->keepalive > db.config->max_keepalive || context->keepalive == 0)){ + } - context->keepalive = db.config->max_keepalive; - if(mosquitto_property_add_int16(&connack_props, MQTT_PROP_SERVER_KEEP_ALIVE, context->keepalive)){ + 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; } diff --git a/test/broker/01-connect-max-keepalive.py b/test/broker/01-connect-max-keepalive.py new file mode 100755 index 00000000..ea9d6eb6 --- /dev/null +++ b/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) diff --git a/test/broker/12-prop-server-keepalive.py b/test/broker/12-prop-server-keepalive.py index 08c1b0d7..9dc07424 100755 --- a/test/broker/12-prop-server-keepalive.py +++ b/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) -connack_packet = mosq_test.gen_connack(rc=0, proto_ver=5, properties=props) +props = mqtt5_props.gen_uint16_prop(mqtt5_props.PROP_SERVER_KEEP_ALIVE, 60) \ + + 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) diff --git a/test/broker/Makefile b/test/broker/Makefile index 0bffe9a4..e0b817fb 100644 --- a/test/broker/Makefile +++ b/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 diff --git a/test/broker/test.py b/test/broker/test.py index 537f6b68..32ede2ea 100755 --- a/test/broker/test.py +++ b/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'),