Fix for #575314.

Incoming QoS 2 messages that had not completed the QoS flow were not being
checked for ACL access when a clean session=False client was reconnecting.

Closes #575314.

ChangeLog.txt

@@ -17,6 +17,9 @@ Security:
 - If a plugin had granted ACL subscription access to a
   durable/non-clean-session client, then removed that access, the client would
   keep its existing subscription. This has been fixed.
+- Incoming QoS 2 messages that had not completed the QoS flow were not being
+  checked for ACL access when a clean session=False client was reconnecting.
+  This has been fixed.
 
 Broker:
 - Fix possible out of bounds memory reads when reading a corrupt/crafted

src/handle_connect.c

@@ -86,18 +86,22 @@ static char *client_id_gen(uint16_t *idlen, const char *auto_id_prefix, uint16_t
 static void connection_check_acl(struct mosquitto *context, struct mosquitto_client_msg **head)
 {
 	struct mosquitto_client_msg *msg_tail, *tmp;
+	int access;
 
 	DL_FOREACH_SAFE((*head), msg_tail, tmp){
 		if(msg_tail->direction == mosq_md_out){
-			if(mosquitto_acl_check(context, msg_tail->store->topic,
-								   msg_tail->store->payloadlen, msg_tail->store->payload,
-								   msg_tail->store->qos, msg_tail->store->retain, MOSQ_ACL_READ) != MOSQ_ERR_SUCCESS){
+			access = MOSQ_ACL_READ;
+		}else{
+			access = MOSQ_ACL_WRITE;
+		}
+		if(mosquitto_acl_check(context, msg_tail->store->topic,
+							   msg_tail->store->payloadlen, msg_tail->store->payload,
+							   msg_tail->store->qos, msg_tail->store->retain, access) != MOSQ_ERR_SUCCESS){
 
-				DL_DELETE((*head), msg_tail);
-				db__msg_store_ref_dec(&msg_tail->store);
-				mosquitto_property_free_all(&msg_tail->properties);
-				mosquitto__free(msg_tail);
-			}
+			DL_DELETE((*head), msg_tail);
+			db__msg_store_ref_dec(&msg_tail->store);
+			mosquitto_property_free_all(&msg_tail->properties);
+			mosquitto__free(msg_tail);
 		}
 	}
 }

test/broker/09-plugin-acl-change.py

@@ -0,0 +1,69 @@
+#!/usr/bin/env python3
+
+# A clean start=False client connects, and publishes to a topic it has access
+# to with QoS 2 - but does not send a PUBREL. It closes the connection. The
+# access to the topic is revoked, the client reconnects and it attempts to
+# complete the flow. Is the publish allowed? It should not be.
+
+from mosq_test_helper import *
+
+def write_config(filename, port, plugin_ver):
+    with open(filename, 'w') as f:
+        f.write("listener %d\n" % (port))
+        f.write("auth_plugin c/auth_plugin_acl_change.so\n")
+        f.write("allow_anonymous true\n")
+
+def do_test(plugin_ver):
+    port = mosq_test.get_port()
+    conf_file = os.path.basename(__file__).replace('.py', '.conf')
+    write_config(conf_file, port, plugin_ver)
+
+    rc = 1
+    connect1_packet = mosq_test.gen_connect("acl-change-test", clean_session=False)
+    connack1_packet = mosq_test.gen_connack(rc=0)
+
+    connect2_packet = mosq_test.gen_connect("acl-change-test", clean_session=False)
+    connack2_packet = mosq_test.gen_connack(rc=0,flags=1)
+
+    mid = 1
+    subscribe_packet = mosq_test.gen_subscribe(mid, "#", 0)
+    suback_packet = mosq_test.gen_suback(mid, 0)
+
+    mid = 2
+    publish1_packet = mosq_test.gen_publish("publish/topic", qos=2, mid=mid, payload="message")
+    pubrec1_packet = mosq_test.gen_pubrec(mid)
+    pubrel1_packet = mosq_test.gen_pubrel(mid)
+    pubcomp1_packet = mosq_test.gen_pubcomp(mid)
+
+    broker = mosq_test.start_broker(filename=os.path.basename(__file__), use_conf=True, port=port)
+
+    try:
+        sock = mosq_test.do_client_connect(connect1_packet, connack1_packet, timeout=20, port=port)
+
+        mosq_test.do_send_receive(sock, subscribe_packet, suback_packet, "suback 1")
+        mosq_test.do_send_receive(sock, publish1_packet, pubrec1_packet, "pubrec")
+        sock.close()
+
+        # ACL has changed
+        sock = mosq_test.do_client_connect(connect2_packet, connack2_packet, timeout=20, port=port)
+        mosq_test.do_send_receive(sock, subscribe_packet, suback_packet, "suback 2")
+        mosq_test.do_send_receive(sock, pubrel1_packet, pubcomp1_packet, "pubcomp")
+        mosq_test.do_ping(sock)
+
+        rc = 0
+
+        sock.close()
+    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(4)

test/broker/Makefile

@@ -166,6 +166,7 @@ endif
 	./09-extended-auth-multistep-reauth.py
 	./09-extended-auth-multistep.py
 	./09-extended-auth-single.py
+	./09-plugin-acl-change.py
 	./09-plugin-auth-acl-pub.py
 	./09-plugin-auth-acl-sub-denied.py
 	./09-plugin-auth-acl-sub.py

test/broker/c/Makefile

@@ -3,19 +3,20 @@
 CFLAGS=-I../../../include -Wall -Werror
 
 PLUGIN_SRC = \
-	auth_plugin_v4.c \
-	auth_plugin_v5.c \
-	auth_plugin_v5_handle_message.c \
-	auth_plugin_pwd.c \
 	auth_plugin_acl.c \
+	auth_plugin_acl_change.c \
 	auth_plugin_acl_sub_denied.c \
-	auth_plugin_v2.c \
 	auth_plugin_context_params.c \
-	auth_plugin_msg_params.c \
 	auth_plugin_extended_multiple.c \
 	auth_plugin_extended_single.c \
 	auth_plugin_extended_single2.c \
+	auth_plugin_msg_params.c \
 	auth_plugin_publish.c \
+	auth_plugin_pwd.c \
+	auth_plugin_v2.c \
+	auth_plugin_v4.c \
+	auth_plugin_v5.c \
+	auth_plugin_v5_handle_message.c \
 	plugin_control.c
 
 PLUGINS = ${PLUGIN_SRC:.c=.so}

test/broker/c/auth_plugin_acl_change.c

@@ -0,0 +1,52 @@
+#include <stdio.h>
+#include <string.h>
+#include <mosquitto.h>
+#include <mosquitto_broker.h>
+#include <mosquitto_plugin.h>
+
+int mosquitto_auth_acl_check_v5(int event, void *event_data, void *user_data);
+int mosquitto_auth_unpwd_check_v5(int event, void *event_data, void *user_data);
+
+static mosquitto_plugin_id_t *plg_id;
+
+static int login_count = 0;
+
+int mosquitto_plugin_version(int supported_version_count, const int *supported_versions)
+{
+	return 5;
+}
+
+int mosquitto_plugin_init(mosquitto_plugin_id_t *identifier, void **user_data, struct mosquitto_opt *auth_opts, int auth_opt_count)
+{
+	plg_id = identifier;
+
+	mosquitto_callback_register(plg_id, MOSQ_EVT_ACL_CHECK, mosquitto_auth_acl_check_v5, NULL, NULL);
+	mosquitto_callback_register(plg_id, MOSQ_EVT_BASIC_AUTH, mosquitto_auth_unpwd_check_v5, NULL, NULL);
+
+	return MOSQ_ERR_SUCCESS;
+}
+
+int mosquitto_plugin_cleanup(void *user_data, struct mosquitto_opt *auth_opts, int auth_opt_count)
+{
+	mosquitto_callback_unregister(plg_id, MOSQ_EVT_ACL_CHECK, mosquitto_auth_acl_check_v5, NULL);
+	mosquitto_callback_unregister(plg_id, MOSQ_EVT_BASIC_AUTH, mosquitto_auth_unpwd_check_v5, NULL);
+
+	return MOSQ_ERR_SUCCESS;
+}
+
+int mosquitto_auth_acl_check_v5(int event, void *event_data, void *user_data)
+{
+	struct mosquitto_evt_acl_check *ed = event_data;
+
+	if(login_count == 2 && ed->access == MOSQ_ACL_WRITE){
+		return MOSQ_ERR_ACL_DENIED;
+	}else{
+		return MOSQ_ERR_SUCCESS;
+	}
+}
+
+int mosquitto_auth_unpwd_check_v5(int event, void *event_data, void *user_data)
+{
+	login_count++;
+	return MOSQ_ERR_SUCCESS;
+}

test/broker/test.py

@@ -136,6 +136,7 @@ tests = [
     (1, './09-extended-auth-multistep-reauth.py'),
     (1, './09-extended-auth-multistep.py'),
     (1, './09-extended-auth-single.py'),
+    (1, './09-plugin-acl-change.py'),
     (1, './09-plugin-auth-acl-pub.py'),
     (1, './09-plugin-auth-acl-sub-denied.py'),
     (1, './09-plugin-auth-acl-sub.py'),