Introduce RAII wrapper for SSL struct pointers

Under Wiebe's direct supervision, I implemented `FmqSsl`, as a wrapper
around OpenSSL's `SSL*` pointer.

Besides being safer in principle, this immediately allowed us to
remove all `SSL*` resource management from the `IOWrapper` destructor
(which was all the destructor did).

Author: Rowan Rodrik van der Molen

CMakeLists.shared

@@ -149,4 +149,5 @@ set(FLASHMQ_IMPLS
     ${RELPATH}fdmanaged.cpp
     ${RELPATH}http.cpp
     ${RELPATH}fmqsockaddr.cpp
+    ${RELPATH}fmqssl.cpp
     )

FlashMQTests/sharedsubscriptionstests.cpp

@@ -15,13 +15,13 @@ void MainTests::testSharedSubscribersUnit()
 
     ThreadGlobals::assignThreadData(t);
 
-    std::shared_ptr<Client> c1(new Client(ClientType::Normal, 0, t, nullptr, ConnectionProtocol::Mqtt, false, nullptr, settings, false));
+    std::shared_ptr<Client> c1(new Client(ClientType::Normal, 0, t, FmqSsl(), ConnectionProtocol::Mqtt, false, nullptr, settings, false));
     c1->setClientProperties(ProtocolVersion::Mqtt5, "clientid1", {}, "user1", true, 60);
 
-    std::shared_ptr<Client> c2(new Client(ClientType::Normal, 0, t, nullptr, ConnectionProtocol::Mqtt, false, nullptr, settings, false));
+    std::shared_ptr<Client> c2(new Client(ClientType::Normal, 0, t, FmqSsl(), ConnectionProtocol::Mqtt, false, nullptr, settings, false));
     c2->setClientProperties(ProtocolVersion::Mqtt5, "clientid2", {}, "user2", true, 60);
 
-    std::shared_ptr<Client> c3(new Client(ClientType::Normal, 0, t, nullptr, ConnectionProtocol::Mqtt, false, nullptr, settings, false));
+    std::shared_ptr<Client> c3(new Client(ClientType::Normal, 0, t, FmqSsl(), ConnectionProtocol::Mqtt, false, nullptr, settings, false));
     c3->setClientProperties(ProtocolVersion::Mqtt5, "clientid3", {}, "user3", true, 60);
 
     std::shared_ptr<Session> ses1 = std::make_shared<Session>(c1->getClientId(), c1->getUsername(), std::optional<std::string>());

FlashMQTests/tst_maintests.cpp

@@ -964,13 +964,13 @@ void MainTests::testSavingSessions()
         std::shared_ptr<SubscriptionStore> store(new SubscriptionStore());
         std::shared_ptr<ThreadData> t(new ThreadData(0, settings, pluginLoader));
 
-        std::shared_ptr<Client> c1(new Client(ClientType::Normal, 0, t, nullptr, ConnectionProtocol::Mqtt, false, nullptr, settings, false));
+        std::shared_ptr<Client> c1(new Client(ClientType::Normal, 0, t, FmqSsl(), ConnectionProtocol::Mqtt, false, nullptr, settings, false));
         c1->setClientProperties(ProtocolVersion::Mqtt5, "c1", {}, "user1", true, 60);
         store->registerClientAndKickExistingOne(c1, false, 512, 120);
         c1->getSession()->addIncomingQoS2MessageId(2);
         c1->getSession()->addIncomingQoS2MessageId(3);
 
-        std::shared_ptr<Client> c2(new Client(ClientType::Normal, 0, t, nullptr, ConnectionProtocol::Mqtt, false, nullptr, settings, false));
+        std::shared_ptr<Client> c2(new Client(ClientType::Normal, 0, t, FmqSsl(), ConnectionProtocol::Mqtt, false, nullptr, settings, false));
         c2->setClientProperties(ProtocolVersion::Mqtt5, "c2", {}, "user2", true, 60);
         store->registerClientAndKickExistingOne(c2, false, 512, 120);
         c2->getSession()->addOutgoingQoS2MessageId(55);
@@ -1122,7 +1122,7 @@ void MainTests::testParsePacketHelper(const std::string &topic, uint8_t from_qos
     std::shared_ptr<PluginLoader> pluginLoader = std::make_shared<PluginLoader>();
     std::shared_ptr<ThreadData> t(new ThreadData(0, settings, pluginLoader));
 
-    std::shared_ptr<Client> dummyClient(new Client(ClientType::Normal, 0, t, nullptr, ConnectionProtocol::Mqtt, false, nullptr, settings, false));
+    std::shared_ptr<Client> dummyClient(new Client(ClientType::Normal, 0, t, FmqSsl(), ConnectionProtocol::Mqtt, false, nullptr, settings, false));
     dummyClient->setClientProperties(ProtocolVersion::Mqtt311, "qostestclient", {}, "user1", true, 60);
     store->registerClientAndKickExistingOne(dummyClient, false, 512, 120);
 
@@ -1192,7 +1192,7 @@ void MainTests::testbufferToMqttPacketsFuzz()
 
     settings.maxPacketSize = 32768;
 
-    std::shared_ptr<Client> dummyClient(new Client(ClientType::Normal, 0, t, nullptr, ConnectionProtocol::Mqtt, false, nullptr, settings, false));
+    std::shared_ptr<Client> dummyClient(new Client(ClientType::Normal, 0, t, FmqSsl(), ConnectionProtocol::Mqtt, false, nullptr, settings, false));
     dummyClient->setClientProperties(ProtocolVersion::Mqtt311, "dummy", {}, "user1", true, 60);
     store->registerClientAndKickExistingOne(dummyClient, false, 512, 120);
 
@@ -3061,7 +3061,7 @@ void MainTests::testTopicMatchingInSubscriptionTreeHelper(const std::string &sub
 
     std::shared_ptr<ThreadData> td;
     const Settings *settings = ThreadGlobals::getSettings();
-    std::shared_ptr<Client> client = std::make_shared<Client>(ClientType::Normal, 0, td, nullptr, ConnectionProtocol::Mqtt, false, nullptr, *settings, false);
+    std::shared_ptr<Client> client = std::make_shared<Client>(ClientType::Normal, 0, td, FmqSsl(), ConnectionProtocol::Mqtt, false, nullptr, *settings, false);
     client->setClientProperties(ProtocolVersion::Mqtt5, "mytestclient", {}, "myusername", true, 60);
     store.registerClientAndKickExistingOne(client);
 

FlashMQTests/websockettests.cpp

@@ -75,7 +75,7 @@ void MainTests::testWebsocketPing()
         int flags = fcntl(listen_socket, F_GETFL);
         check<std::runtime_error>(fcntl(client_socket, F_SETFL, flags | O_NONBLOCK ));
 
-        std::shared_ptr<Client> c1(new Client(ClientType::Normal, client_socket, t, nullptr, ConnectionProtocol::WebsocketMqtt, false, nullptr, settings, false));
+        std::shared_ptr<Client> c1(new Client(ClientType::Normal, client_socket, t, FmqSsl(), ConnectionProtocol::WebsocketMqtt, false, nullptr, settings, false));
         std::shared_ptr<Client> client = c1;
         t->giveClient(std::move(c1));
 
@@ -253,7 +253,7 @@ void MainTests::testWebsocketCorruptLengthFrame()
         int flags = fcntl(listen_socket, F_GETFL);
         check<std::runtime_error>(fcntl(client_socket, F_SETFL, flags | O_NONBLOCK ));
 
-        std::shared_ptr<Client> c1(new Client(ClientType::Normal, client_socket, t, nullptr, ConnectionProtocol::WebsocketMqtt, false, nullptr, settings, false));
+        std::shared_ptr<Client> c1(new Client(ClientType::Normal, client_socket, t, FmqSsl(), ConnectionProtocol::WebsocketMqtt, false, nullptr, settings, false));
         std::shared_ptr<Client> client = c1;
         t->giveClient(std::move(c1));
 
@@ -380,7 +380,7 @@ void MainTests::testWebsocketHugePing()
         int flags = fcntl(listen_socket, F_GETFL);
         check<std::runtime_error>(fcntl(client_socket, F_SETFL, flags | O_NONBLOCK ));
 
-        std::shared_ptr<Client> c1(new Client(ClientType::Normal, client_socket, t, nullptr, ConnectionProtocol::WebsocketMqtt, false, nullptr, settings, false));
+        std::shared_ptr<Client> c1(new Client(ClientType::Normal, client_socket, t, FmqSsl(), ConnectionProtocol::WebsocketMqtt, false, nullptr, settings, false));
         std::shared_ptr<Client> client = c1;
         t->giveClient(std::move(c1));
 
@@ -500,7 +500,7 @@ void MainTests::testWebsocketManyBigPingFrames()
         int flags = fcntl(listen_socket, F_GETFL);
         check<std::runtime_error>(fcntl(client_socket, F_SETFL, flags | O_NONBLOCK ));
 
-        std::shared_ptr<Client> c1(new Client(ClientType::Normal, client_socket, t, nullptr, ConnectionProtocol::WebsocketMqtt, false, nullptr, settings, false));
+        std::shared_ptr<Client> c1(new Client(ClientType::Normal, client_socket, t, FmqSsl(), ConnectionProtocol::WebsocketMqtt, false, nullptr, settings, false));
         std::shared_ptr<Client> client = c1;
         t->giveClient(std::move(c1));
 
@@ -646,7 +646,7 @@ void MainTests::testWebsocketClose()
         int flags = fcntl(listen_socket, F_GETFL);
         check<std::runtime_error>(fcntl(client_socket, F_SETFL, flags | O_NONBLOCK ));
 
-        std::shared_ptr<Client> c1(new Client(ClientType::Normal, client_socket, t, nullptr, ConnectionProtocol::WebsocketMqtt, false, nullptr, settings, false));
+        std::shared_ptr<Client> c1(new Client(ClientType::Normal, client_socket, t, FmqSsl(), ConnectionProtocol::WebsocketMqtt, false, nullptr, settings, false));
         std::shared_ptr<Client> client = c1;
         t->giveClient(std::move(c1));
 

client.cpp

@@ -61,14 +61,14 @@ Client::WriteBuf::WriteBuf(size_t size) :
  * @param fuzzMode
  */
 Client::Client(
-        ClientType type, int fd, std::shared_ptr<ThreadData> threadData, SSL *ssl, ConnectionProtocol connectionProtocol,
+        ClientType type, int fd, std::shared_ptr<ThreadData> threadData, FmqSsl &&ssl, ConnectionProtocol connectionProtocol,
         bool haproxy, const struct sockaddr *addr, const Settings &settings, bool fuzzMode) :
     fd(fd),
     fuzzMode(fuzzMode),
     maxOutgoingPacketSize(settings.maxPacketSize),
     maxIncomingPacketSize(settings.maxPacketSize),
     maxIncomingTopicAliasValue(settings.maxIncomingTopicAliasValue), // Retaining snapshot of current setting, to not confuse clients when the setting changes.
-    ioWrapper(ssl, connectionProtocol, settings.clientInitialBufferSize, this),
+    ioWrapper(std::move(ssl), connectionProtocol, settings.clientInitialBufferSize, this),
     readbuf(settings.clientInitialBufferSize),
     writebuf(settings.clientInitialBufferSize),
     clientType(type),

client.h

@@ -32,6 +32,7 @@ See LICENSE for license details.
 #include "enums.h"
 #include "fdmanaged.h"
 #include "mutexowned.h"
+#include "fmqssl.h"
 
 #include "publishcopyfactory.h"
 
@@ -161,7 +162,7 @@ class Client
     uint8_t preAuthPacketCounter = 0;
 
     Client(
-        ClientType type, int fd, std::shared_ptr<ThreadData> threadData, SSL *ssl, ConnectionProtocol connectionProtocol, bool haproxy,
+        ClientType type, int fd, std::shared_ptr<ThreadData> threadData, FmqSsl &&ssl, ConnectionProtocol connectionProtocol, bool haproxy,
         const struct sockaddr *addr, const Settings &settings, bool fuzzMode=false);
     Client(const Client &other) = delete;
     Client(Client &&other) = delete;

flashmqtestclient.cpp

@@ -127,7 +127,7 @@ void FlashMQTestClient::connectClient(ProtocolVersion protocolVersion, bool clea
     const std::string clientid = formatString("testclient_%d", clientCount++);
 
     std::shared_ptr<Client> client = std::make_shared<Client>(
-        ClientType::Normal, sockfd, testServerWorkerThreadData.getThreadData(), nullptr, ConnectionProtocol::Mqtt, false, reinterpret_cast<struct sockaddr*>(&servaddr), settings);
+        ClientType::Normal, sockfd, testServerWorkerThreadData.getThreadData(), FmqSsl(), ConnectionProtocol::Mqtt, false, reinterpret_cast<struct sockaddr*>(&servaddr), settings);
     this->client_weak = client;
     client->setClientProperties(protocolVersion, clientid, {}, "user", false, 60);
 

fmqssl.cpp

@@ -0,0 +1,47 @@
+#include <stdexcept>
+
+#include <openssl/ssl.h>
+
+#include "fmqssl.h"
+
+FmqSsl::~FmqSsl()
+{
+    if (d == nullptr) return;
+
+    /*
+     * We write the shutdown when we can, but don't take error conditions into account. If socket buffers are full, because
+     * clients disappear for instance, the socket is just closed. We don't care.
+     *
+     * Truncation attacks seem irrelevant. MQTT is frame based, so either end knows if the transmission is done or not. The
+     * close_notify is not used in determining whether to use or discard the received data.
+     */
+    SSL_shutdown(d);
+
+    SSL_free(d);
+    d = nullptr;
+}
+
+FmqSsl::FmqSsl(const SslCtxManager &ssl_ctx) :
+    d(SSL_new(ssl_ctx.get()))
+{
+}
+
+FmqSsl::FmqSsl(FmqSsl &&other) :
+    d(other.d)
+{
+    other.d = nullptr;
+}
+
+FmqSsl &FmqSsl::operator=(FmqSsl &&other)
+{
+    d = other.d;
+    other.d = nullptr;
+    return *this;
+}
+
+void FmqSsl::set_fd(int fd)
+{
+    if (!d) return;
+
+    SSL_set_fd(d, fd);
+}

fmqssl.h

@@ -0,0 +1,39 @@
+#ifndef FMQSSL_H
+#define FMQSSL_H
+
+#include <openssl/ssl.h>
+
+#include "sslctxmanager.h"
+
+class FmqSsl
+{
+    SSL* d = nullptr;
+
+public:
+    FmqSsl() = default;
+
+    FmqSsl(const SslCtxManager &ssl_ctx);
+
+    FmqSsl(const FmqSsl &other) = delete;
+
+    FmqSsl(FmqSsl &&other);
+
+    FmqSsl &operator=(const FmqSsl &other) = delete;
+
+    FmqSsl &operator=(FmqSsl &&other);
+
+    ~FmqSsl();
+
+    operator bool() const
+    {
+        return d != nullptr;
+    }
+    SSL* get() const
+    {
+        return d;
+    }
+
+    void set_fd(int fd);
+};
+
+#endif // FMQSSL_H

iowrapper.cpp

@@ -65,34 +65,16 @@ IncompleteWebsocketRead::IncompleteWebsocketRead()
     reset();
 }
 
-IoWrapper::IoWrapper(SSL *ssl, ConnectionProtocol connectionProtocol, const size_t initialBufferSize, Client *parent) :
+IoWrapper::IoWrapper(FmqSsl &&ssl, ConnectionProtocol connectionProtocol, const size_t initialBufferSize, Client *parent) :
     parentClient(parent),
-    ssl(ssl),
+    ssl(std::move(ssl)),
     connectionProtocol(connectionProtocol),
     websocketPendingBytes(connectionProtocol == ConnectionProtocol::WebsocketMqtt ? initialBufferSize : 0),
     websocketWriteRemainder(connectionProtocol == ConnectionProtocol::WebsocketMqtt ? initialBufferSize : 0)
 {
 
 }
 
-IoWrapper::~IoWrapper()
-{
-    if (ssl)
-    {
-        /*
-         * We write the shutdown when we can, but don't take error conditions into account. If socket buffers are full, because
-         * clients disappear for instance, the socket is just closed. We don't care.
-         *
-         * Truncation attacks seem irrelevant. MQTT is frame based, so either end knows if the transmission is done or not. The
-         * close_notify is not used in determining whether to use or discard the received data.
-         */
-        SSL_shutdown(ssl);
-
-        SSL_free(ssl);
-        ssl = nullptr;
-    }
-}
-
 void IoWrapper::startOrContinueSslHandshake()
 {
     if (parentClient->isOutgoingConnection())
@@ -105,10 +87,10 @@ void IoWrapper::startOrContinueSslConnect()
 {
     assert(ssl);
     ERR_clear_error();
-    int connected = SSL_connect(ssl);
+    int connected = SSL_connect(ssl.get());
     if (connected <= 0)
     {
-        int err = SSL_get_error(ssl, connected);
+        int err = SSL_get_error(ssl.get(), connected);
 
         if (err == SSL_ERROR_WANT_READ || err == SSL_ERROR_WANT_WRITE)
         {
@@ -134,10 +116,10 @@ void IoWrapper::startOrContinueSslConnect()
 void IoWrapper::startOrContinueSslAccept()
 {
     ERR_clear_error();
-    int accepted = SSL_accept(ssl);
+    int accepted = SSL_accept(ssl.get());
     if (accepted <= 0)
     {
-        int err = SSL_get_error(ssl, accepted);
+        int err = SSL_get_error(ssl.get(), accepted);
 
         if (err == SSL_ERROR_WANT_READ || err == SSL_ERROR_WANT_WRITE)
         {
@@ -176,7 +158,7 @@ bool IoWrapper::isSslAccepted() const
 
 bool IoWrapper::isSsl() const
 {
-    return this->ssl != nullptr;
+    return this->ssl;
 }
 
 static int verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
@@ -227,18 +209,18 @@ void IoWrapper::setSslVerify(int mode, const std::string &hostname)
     if (!ssl)
         return;
 
-    SSL_set_hostflags(ssl, X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS);
+    SSL_set_hostflags(ssl.get(), X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS);
 
     if (!hostname.empty())
     {
-        if (!SSL_set1_host(ssl, hostname.c_str()))
+        if (!SSL_set1_host(ssl.get(), hostname.c_str()))
             throw std::runtime_error("Failed setting hostname of SSL context.");
 
-        if (SSL_set_tlsext_host_name(ssl, hostname.c_str()) != 1)
+        if (SSL_set_tlsext_host_name(ssl.get(), hostname.c_str()) != 1)
             throw std::runtime_error("Failed setting SNI hostname of SSL context.");
     }
 
-    SSL_set_verify(ssl, mode, verify_callback);
+    SSL_set_verify(ssl.get(), mode, verify_callback);
 }
 
 bool IoWrapper::hasPendingWrite() const
@@ -259,7 +241,7 @@ bool IoWrapper::hasProcessedBufferedBytesToRead() const
     bool result = false;
 
     if (ssl)
-        result |= SSL_pending(ssl) > 0;
+        result |= SSL_pending(ssl.get()) > 0;
 
     /*
      * Note that this is tecnhically not 100% correct. If the only bytes are part of a header, doing a read will actually
@@ -278,13 +260,13 @@ WebsocketState IoWrapper::getWebsocketState() const
 
 X509Manager IoWrapper::getPeerCertificate() const
 {
-    X509Manager result(this->ssl);
+    X509Manager result(this->ssl.get());
     return result;
 }
 
 const char *IoWrapper::getSslVersion() const
 {
-    return SSL_get_version(ssl);
+    return SSL_get_version(ssl.get());
 }
 
 bool IoWrapper::needsHaProxyParsing() const
@@ -428,12 +410,12 @@ ssize_t IoWrapper::readOrSslRead(int fd, void *buf, size_t nbytes, IoWrapResult
     {
         this->sslReadWantsWrite = false;
         ERR_clear_error();
-        ssize_t n = SSL_read(ssl, buf, nbytes);
+        ssize_t n = SSL_read(ssl.get(), buf, nbytes);
 
         if (n > 0)
             return n;
 
-        int err = SSL_get_error(ssl, n);
+        int err = SSL_get_error(ssl.get(), n);
         unsigned long error_code = ERR_get_error();
 
         if (err == SSL_ERROR_WANT_READ || err == SSL_ERROR_WANT_WRITE)
@@ -539,11 +521,11 @@ ssize_t IoWrapper::writeOrSslWrite(int fd, const void *buf, size_t nbytes, IoWra
         this->incompleteSslWrite.reset();
 
         ERR_clear_error();
-        n = SSL_write(ssl, buf, nbytes_);
+        n = SSL_write(ssl.get(), buf, nbytes_);
 
         if (n <= 0)
         {
-            int err = SSL_get_error(ssl, n);
+            int err = SSL_get_error(ssl.get(), n);
             unsigned long error_code = ERR_get_error();
             if (err == SSL_ERROR_WANT_READ || err == SSL_ERROR_WANT_WRITE)
             {