diff --git a/ChangeLog.txt b/ChangeLog.txt index cd84a8e1..ce4e26ac 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -1,12 +1,16 @@ 2.0.19 - 2024-09-xx =================== +Security: +- Fix mismatched subscribe/unsubscribe with normal/shared topics. + Broker: - Fix assert failure when loading a persistence file that contains subscriptions with no client id. - Fix local bridges being incorrectly expired when persistent_client_expiration is in use. - Fix use of CLOCK_BOOTTIME for getting time. Closes #3089. +- Fix mismatched subscribe/unsubscribe with normal/shared topics. Library: - Fix some error codes being converted to string as "unknown". Closes #2579. diff --git a/src/database.c b/src/database.c index 1a926f50..2698a95d 100644 --- a/src/database.c +++ b/src/database.c @@ -199,12 +199,16 @@ int db__open(struct mosquitto__config *config) /* Initialize the hashtable */ db.clientid_index_hash = NULL; - db.subs = NULL; + db.normal_subs = NULL; + db.shared_subs = NULL; - subhier = sub__add_hier_entry(NULL, &db.subs, "", 0); + subhier = sub__add_hier_entry(NULL, &db.shared_subs, "", 0); if(!subhier) return MOSQ_ERR_NOMEM; - subhier = sub__add_hier_entry(NULL, &db.subs, "$SYS", (uint16_t)strlen("$SYS")); + subhier = sub__add_hier_entry(NULL, &db.normal_subs, "", 0); + if(!subhier) return MOSQ_ERR_NOMEM; + + subhier = sub__add_hier_entry(NULL, &db.normal_subs, "$SYS", (uint16_t)strlen("$SYS")); if(!subhier) return MOSQ_ERR_NOMEM; retain__init(); @@ -240,7 +244,8 @@ static void subhier_clean(struct mosquitto__subhier **subhier) int db__close(void) { - subhier_clean(&db.subs); + subhier_clean(&db.normal_subs); + subhier_clean(&db.shared_subs); retain__clean(&db.retains); db__msg_store_clean(); diff --git a/src/loop.c b/src/loop.c index a5a63e77..ec953398 100644 --- a/src/loop.c +++ b/src/loop.c @@ -240,7 +240,8 @@ int mosquitto_main_loop(struct mosquitto__listener_sock *listensock, int listens flag_reload = false; } if(flag_tree_print){ - sub__tree_print(db.subs, 0); + sub__tree_print(db.normal_subs, 0); + sub__tree_print(db.shared_subs, 0); flag_tree_print = false; #ifdef WITH_XTREPORT xtreport(); diff --git a/src/mosquitto_broker_internal.h b/src/mosquitto_broker_internal.h index d91752eb..aff6e948 100644 --- a/src/mosquitto_broker_internal.h +++ b/src/mosquitto_broker_internal.h @@ -442,7 +442,8 @@ struct mosquitto_message_v5{ struct mosquitto_db{ dbid_t last_db_id; - struct mosquitto__subhier *subs; + struct mosquitto__subhier *normal_subs; + struct mosquitto__subhier *shared_subs; struct mosquitto__retainhier *retains; struct mosquitto *contexts_by_id; struct mosquitto *contexts_by_sock; diff --git a/src/persist_write.c b/src/persist_write.c index 954e36a4..33827544 100644 --- a/src/persist_write.c +++ b/src/persist_write.c @@ -266,7 +266,13 @@ static int persist__subs_save_all(FILE *db_fptr) { struct mosquitto__subhier *subhier, *subhier_tmp; - HASH_ITER(hh, db.subs, subhier, subhier_tmp){ + HASH_ITER(hh, db.normal_subs, subhier, subhier_tmp){ + if(subhier->children){ + persist__subs_save(db_fptr, subhier->children, "", 0); + } + } + + HASH_ITER(hh, db.shared_subs, subhier, subhier_tmp){ if(subhier->children){ persist__subs_save(db_fptr, subhier->children, "", 0); } diff --git a/src/subs.c b/src/subs.c index 0401bae8..992401fd 100644 --- a/src/subs.c +++ b/src/subs.c @@ -595,16 +595,29 @@ int sub__add(struct mosquitto *context, const char *sub, uint8_t qos, uint32_t i mosquitto__free(topics); return MOSQ_ERR_INVAL; } - HASH_FIND(hh, db.subs, topics[0], topiclen, subhier); - if(!subhier){ - subhier = sub__add_hier_entry(NULL, &db.subs, topics[0], (uint16_t)topiclen); - if(!subhier){ - mosquitto__free(local_sub); - mosquitto__free(topics); - log__printf(NULL, MOSQ_LOG_ERR, "Error: Out of memory."); - return MOSQ_ERR_NOMEM; - } + if(sharename){ + HASH_FIND(hh, db.shared_subs, topics[0], topiclen, subhier); + if(!subhier){ + subhier = sub__add_hier_entry(NULL, &db.shared_subs, topics[0], (uint16_t)topiclen); + if(!subhier){ + mosquitto__free(local_sub); + mosquitto__free(topics); + log__printf(NULL, MOSQ_LOG_ERR, "Error: Out of memory."); + return MOSQ_ERR_NOMEM; + } + } + }else{ + HASH_FIND(hh, db.normal_subs, topics[0], topiclen, subhier); + if(!subhier){ + subhier = sub__add_hier_entry(NULL, &db.normal_subs, topics[0], (uint16_t)topiclen); + if(!subhier){ + mosquitto__free(local_sub); + mosquitto__free(topics); + log__printf(NULL, MOSQ_LOG_ERR, "Error: Out of memory."); + return MOSQ_ERR_NOMEM; + } + } } rc = sub__add_context(context, sub, qos, identifier, options, subhier, topics, sharename); @@ -627,7 +640,11 @@ int sub__remove(struct mosquitto *context, const char *sub, uint8_t *reason) rc = sub__topic_tokenise(sub, &local_sub, &topics, &sharename); if(rc) return rc; - HASH_FIND(hh, db.subs, topics[0], strlen(topics[0]), subhier); + if(sharename){ + HASH_FIND(hh, db.shared_subs, topics[0], strlen(topics[0]), subhier); + }else{ + HASH_FIND(hh, db.normal_subs, topics[0], strlen(topics[0]), subhier); + } if(subhier){ *reason = MQTT_RC_NO_SUBSCRIPTION_EXISTED; rc = sub__remove_recurse(context, subhier, topics, reason, sharename); @@ -656,7 +673,12 @@ int sub__messages_queue(const char *source_id, const char *topic, uint8_t qos, i */ db__msg_store_ref_inc(*stored); - HASH_FIND(hh, db.subs, split_topics[0], strlen(split_topics[0]), subhier); + HASH_FIND(hh, db.normal_subs, split_topics[0], strlen(split_topics[0]), subhier); + if(subhier){ + rc = sub__search(subhier, split_topics, source_id, topic, qos, retain, *stored); + } + + HASH_FIND(hh, db.shared_subs, split_topics[0], strlen(split_topics[0]), subhier); if(subhier){ rc = sub__search(subhier, split_topics, source_id, topic, qos, retain, *stored); } diff --git a/test/broker/data/REGRESSION.json b/test/broker/data/REGRESSION.json index 4f21fa68..fe456869 100644 --- a/test/broker/data/REGRESSION.json +++ b/test/broker/data/REGRESSION.json @@ -14,5 +14,36 @@ {"type":"recv", "payload":"90 03 1234 00"} ], "comment": "https://github.com/eclipse/mosquitto/issues/2885"} ] + }, + { + "group": "REGRESSIONS", + "tests": [ + { + "name": "mismatched-shared-normal-subscribe-unsubscribe-leak", "ver":4, "expect_disconnect":false, "msgs": [ + {"type":"send", "payload":"82 1a 0001 0015 24 73 68 61 72 65 2f 73 68 61 72 65 6e 61 6d 65 2f 74 65 73 74 01"}, + {"type":"recv", "payload":"90 03 0001 01"}, + {"type":"send", "payload":"82 09 0002 0004 74 65 73 74 00"}, + {"type":"recv", "payload":"90 03 0002 00"}, + {"type":"send", "payload":"A2 08 0007 0004 74 65 73 74"}, + {"type":"recv", "payload":"B0 02 0007"} + ], + "comment": "Also part one of the next two tests" + }, + { + "name": "acl-check-uaf", "ver":4, "expect_disconnect":false, "msgs": [ + {"type":"send", "payload":"30 0D 0004 74657374 7061796C6F6164"} + ] + }, + { + "name": "shared-sub-uaf", "ver":4, "expect_disconnect":false, "msgs": [ + {"type":"send", "payload":"82 1a 0001 0015 24 73 68 61 72 65 2f 73 68 61 72 65 6e 61 6d 65 2f 74 65 73 74 01"}, + {"type":"recv", "payload":"90 03 0001 01"}, + {"type":"send", "payload":"82 09 0002 0004 74 65 73 74 00"}, + {"type":"recv", "payload":"90 03 0002 00"}, + {"type":"send", "payload":"A2 08 0007 0004 74 65 73 74"}, + {"type":"recv", "payload":"B0 02 0007"} + ] + } + ] } ] diff --git a/test/broker/msg_sequence_test.py b/test/broker/msg_sequence_test.py index 6bd3c9dc..61ed0986 100755 --- a/test/broker/msg_sequence_test.py +++ b/test/broker/msg_sequence_test.py @@ -218,6 +218,10 @@ finally: broker.terminate() broker.wait() (stdo, stde) = broker.communicate() + if broker.returncode != 0: + rc = broker.returncode + print(f"Broker exited with code {rc}. If there are no obvious errors this may be due to an ASAN build having leaks, which must be fixed.") + print("The easiest way to reproduce this is to run the broker with `mosquitto -p 1888`, rerun the test, then quit the broker.") if rc: #print(stde.decode('utf-8')) exit(rc) diff --git a/test/unit/subs_test.c b/test/unit/subs_test.c index 1402be20..45c07917 100644 --- a/test/unit/subs_test.c +++ b/test/unit/subs_test.c @@ -60,9 +60,10 @@ static void TEST_sub_add_single(void) rc = sub__add(&context, "a/b/c/d/e", 0, 0, 0); CU_ASSERT_EQUAL(rc, MOSQ_ERR_SUCCESS); - CU_ASSERT_PTR_NOT_NULL(db.subs); - if(db.subs){ - sub = db.subs; + CU_ASSERT_PTR_NOT_NULL(db.shared_subs); + CU_ASSERT_PTR_NOT_NULL(db.normal_subs); + if(db.normal_subs){ + sub = db.normal_subs; hier_quick_check(&sub, NULL, ""); hier_quick_check(&sub, NULL, "");