MOSQ_OPT_SSL_CTX using SSL_CTX defaults is broken

[tim-nordell-nimbelink]

I'm pretty sure this is a regression (I haven't tested on older versions of libmosquitto but debugged for my use case) caused by d5aae3eca73cf8a57e7a4b7486ad386f1a7713b3. Basically in the baseline example for this functionality should be:

SSL_CTX *ssl_ctx = SSL_CTX_new(TLS_client_method());
mosquitto_void_option(ctx, MOSQ_OPT_SSL_CTX, ssl_ctx);

and libmosquitto should essentially still work for connecting to TLS. However, it errors out with an error message:

OpenSSL Error[0]: error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed

In net_mosq.c, from commit d5aae3eca73cf8a57e7a4b7486ad386f1a7713b3, the net__init_tls() call is only invoked if mosq->ssl_ctx hasn't been created yet. This is always hit in the case of MOSQ_OPT_SSL_CTX being specified. I think it should always be invoked (e.g. move up the invocation of net__init_tls() one line to just outside of the check for mosq->ssl_ctx already being instantiated) since net__init_tls() has an internal check if it's been already run already. This appears to fix the case of the example above where you instantiate the TLS instance outside of libmosquitto and otherwise leave it to defaults.

(It might need to be always invoked from net__init_ssl_ctx(...) to maintain prior behavior when MOSQ_OPT_SSL_CTX was created; I'm not sure of the interactions with users that aren't using MOSQ_OPT_SSL_CTX_WITH_DEFAULTS.)

Thanks, Tim

[ralight]

Thanks Tim, that's a good spot. It's now fixed and will be in 2.0.15 shortly.

[tim-nordell-nimbelink]

Hi @ralight -

The fix looks good. I'd note that the new test cases look like they cleanup the dynamic initialization of the mosquitto library, but not the dynamic initialization of SSL. It could potentially catch a double free (if supported in the allocation library used) in the test cases if libmosquitto accidentally free'd the SSL context created externally if the cleanup of the SSL contexts were added.

-- Tim

[ralight]

Nice idea, I've added that in to the tests.