diff --git a/ChangeLog.txt b/ChangeLog.txt index 3ad5ec14..51157a62 100644 --- a/ChangeLog.txt +++ b/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 diff --git a/src/handle_connect.c b/src/handle_connect.c index fab2e338..0aa7fd33 100644 --- a/src/handle_connect.c +++ b/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); } } } diff --git a/test/broker/09-plugin-acl-change.py b/test/broker/09-plugin-acl-change.py new file mode 100755 index 00000000..4c15c480 --- /dev/null +++ b/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) diff --git a/test/broker/Makefile b/test/broker/Makefile index 2f93ee55..04fe1314 100644 --- a/test/broker/Makefile +++ b/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 diff --git a/test/broker/c/Makefile b/test/broker/c/Makefile index 6a561ba1..babd396d 100644 --- a/test/broker/c/Makefile +++ b/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} diff --git a/test/broker/c/auth_plugin_acl_change.c b/test/broker/c/auth_plugin_acl_change.c new file mode 100644 index 00000000..7f7888a0 --- /dev/null +++ b/test/broker/c/auth_plugin_acl_change.c @@ -0,0 +1,52 @@ +#include +#include +#include +#include +#include + +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; +} diff --git a/test/broker/test.py b/test/broker/test.py index c022f887..c2cf5196 100755 --- a/test/broker/test.py +++ b/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'),