eclipse / mosquitto

Eclipse Mosquitto - An open source MQTT broker
https://mosquitto.org
Other
8.94k stars 2.37k forks source link

Leakage of User SSL Context in mosquitto_destroy #2992

Open CastleOnTheHill opened 7 months ago

CastleOnTheHill commented 7 months ago

Issue Description

Version: v2.0.18 Platform: Ubuntu 22

Analysis

When a user sets the user ssl ctx using mosquitto_opts_set(mosq, MOSQ_OPT_SSL_CTX, user_ctx), Mosquitto adds a context reference and stores it in mosq->user_ssl_ctx.

Only after a successful call to net__try_connect (indicating a successful TCP connection), do we pass the mosq->user_ssl_ctx to mosq->ssl_ctx during net__init_ssl_ctx.

In the mosquitto_destroy function, we currently only free the mosq->ssl_ctx.

However, if a user sets the user ssl ctx and mosquitto_connect fails due to network unavailability, calling mosquitto_destroy will leak the user ssl ctx.

Suggestions

When a user sets the user ssl ctx, we should free mosq->user_ssl_ctx instead of mosq->ssl_ctx in the mosquitto_destroy function.

--- a/lib/mosquitto.c
+++ b/lib/mosquitto.c
@@ -203,6 +203,9 @@ int mosquitto_reinitialise(struct mosquitto *mosq, const char *id, bool clean_st
    mosq->ssl = NULL;
    mosq->ssl_ctx = NULL;
    mosq->ssl_ctx_defaults = true;
+#ifndef WITH_BROKER
+   mosq->user_ssl_ctx = NULL;
+#endif
    mosq->tls_cert_reqs = SSL_VERIFY_PEER;
    mosq->tls_insecure = false;
    mosq->want_write = false;
@@ -268,9 +271,17 @@ void mosquitto__destroy(struct mosquitto *mosq)
    if(mosq->ssl){
        SSL_free(mosq->ssl);
    }
+#ifndef WITH_BROKER
+   if(mosq->user_ssl_ctx){
+       SSL_CTX_free(mosq->user_ssl_ctx);
+   }else if(mosq->ssl_ctx){
+       SSL_CTX_free(mosq->ssl_ctx);
+   }
+#else
    if(mosq->ssl_ctx){
        SSL_CTX_free(mosq->ssl_ctx);
    }
+#endif