lib/net: Fix race conditions when closing SSL connections

This fixes the following security vulnerability:
- CVE-2021-42074 SIGSEGV on quick open/close sequence while sending
Hello message

The issue has been reported by Matthias Gerstner <mgerstner@suse.de>.

doc/newsfragments/fix-crash-on-ssl-hello.bugfix

@@ -0,0 +1,4 @@
+SECURITY ISSUE
+
+Fixed a bug which caused Barrier to crash when disconnecting a TCP session just after sending Hello message.
+This bug allowed an unauthenticated attacker to crash Barrier with only network access.

src/lib/net/SecureSocket.cpp

@@ -104,6 +104,8 @@ SecureSocket::close()
 
 void SecureSocket::freeSSLResources()
 {
+    std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
+
     if (m_ssl->m_ssl != NULL) {
         SSL_shutdown(m_ssl->m_ssl);
         SSL_free(m_ssl->m_ssl);
@@ -268,6 +270,8 @@ SecureSocket::doWrite()
 int
 SecureSocket::secureRead(void* buffer, int size, int& read)
 {
+    std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
+
     if (m_ssl->m_ssl != NULL) {
         LOG((CLOG_DEBUG2 "reading secure socket"));
         read = SSL_read(m_ssl->m_ssl, buffer, size);
@@ -294,6 +298,8 @@ SecureSocket::secureRead(void* buffer, int size, int& read)
 int
 SecureSocket::secureWrite(const void* buffer, int size, int& wrote)
 {
+    std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
+
     if (m_ssl->m_ssl != NULL) {
         LOG((CLOG_DEBUG2 "writing secure socket:%p", this));
 
@@ -327,6 +333,8 @@ SecureSocket::isSecureReady()
 void
 SecureSocket::initSsl(bool server)
 {
+    std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
+
     m_ssl = new Ssl();
     m_ssl->m_context = NULL;
     m_ssl->m_ssl = NULL;
@@ -336,6 +344,8 @@ SecureSocket::initSsl(bool server)
 
 bool SecureSocket::loadCertificates(std::string& filename)
 {
+    std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
+
     if (filename.empty()) {
         showError("ssl certificate is not specified");
         return false;
@@ -378,6 +388,8 @@ bool SecureSocket::loadCertificates(std::string& filename)
 void
 SecureSocket::initContext(bool server)
 {
+    // ssl_mutex_ is assumed to be acquired
+
     SSL_library_init();
 
     const SSL_METHOD* method;
@@ -415,6 +427,8 @@ SecureSocket::initContext(bool server)
 void
 SecureSocket::createSSL()
 {
+    // ssl_mutex_ is assumed to be acquired
+
     // I assume just one instance is needed
     // get new SSL state with context
     if (m_ssl->m_ssl == NULL) {
@@ -426,6 +440,8 @@ SecureSocket::createSSL()
 int
 SecureSocket::secureAccept(int socket)
 {
+    std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
+
     createSSL();
 
     // set connection socket to SSL state
@@ -475,6 +491,8 @@ SecureSocket::secureAccept(int socket)
 int
 SecureSocket::secureConnect(int socket)
 {
+    std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
+
     createSSL();
 
     // attach the socket descriptor
@@ -527,6 +545,7 @@ SecureSocket::secureConnect(int socket)
 bool
 SecureSocket::showCertificate()
 {
+    // ssl_mutex_ is assumed to be acquired
     X509* cert;
     char* line;
  
@@ -549,6 +568,8 @@ SecureSocket::showCertificate()
 void
 SecureSocket::checkResult(int status, int& retry)
 {
+    // ssl_mutex_ is assumed to be acquired
+
     // ssl errors are a little quirky. the "want" errors are normal and
     // should result in a retry.
 
@@ -685,6 +706,8 @@ void SecureSocket::formatFingerprint(std::string& fingerprint, bool hex, bool se
 bool
 SecureSocket::verifyCertFingerprint()
 {
+    // ssl_mutex_ is assumed to be acquired
+
     // calculate received certificate fingerprint
     X509 *cert = cert = SSL_get_peer_certificate(m_ssl->m_ssl);
     EVP_MD* tempDigest;
@@ -827,6 +850,8 @@ showCipherStackDesc(STACK_OF(SSL_CIPHER) * stack) {
 void
 SecureSocket::showSecureCipherInfo()
 {
+    // ssl_mutex_ is assumed to be acquired
+
     STACK_OF(SSL_CIPHER) * sStack = SSL_get_ciphers(m_ssl->m_ssl);
 
     if (sStack == NULL) {
@@ -869,6 +894,8 @@ SecureSocket::showSecureLibInfo()
 void
 SecureSocket::showSecureConnectInfo()
 {
+    // ssl_mutex_ is assumed to be acquired
+
     const SSL_CIPHER* cipher = SSL_get_current_cipher(m_ssl->m_ssl);
 
     if (cipher != NULL) {

src/lib/net/SecureSocket.h

@@ -19,6 +19,7 @@
 
 #include "net/TCPSocket.h"
 #include "net/XSocket.h"
+#include <mutex>
 
 class IEventQueue;
 class SocketMultiplexer;
@@ -59,30 +60,35 @@ public:
 
 private:
     // SSL
-    void                initContext(bool server);
-    void                createSSL();
+    void initContext(bool server); // may only be called with ssl_mutex_ acquired
+    void createSSL(); // may only be called with ssl_mutex_ acquired.
     int                    secureAccept(int s);
     int                    secureConnect(int s);
-    bool                showCertificate();
-    void                checkResult(int n, int& retry);
+    bool showCertificate(); // may only be called with ssl_mutex_ acquired
+    void checkResult(int n, int& retry); // may only be called with m_ssl_mutex_ acquired.
     void                showError(const char* reason = NULL);
     std::string getError();
     void                disconnect();
     void formatFingerprint(std::string& fingerprint, bool hex = true, bool separator = true);
-    bool                verifyCertFingerprint();
+    bool verifyCertFingerprint(); // may only be called with ssl_mutex_ acquired
 
     MultiplexerJobStatus serviceConnect(ISocketMultiplexerJob*, bool, bool, bool);
     MultiplexerJobStatus serviceAccept(ISocketMultiplexerJob*, bool, bool, bool);
 
-    void                showSecureConnectInfo();
-    void                showSecureLibInfo();
-    void                showSecureCipherInfo();
+    void showSecureConnectInfo(); // may only be called with ssl_mutex_ acquired
+    void showSecureLibInfo();
+    void showSecureCipherInfo(); // may only be called with ssl_mutex_ acquired
 
     void                handleTCPConnected(const Event& event, void*);
 
     void freeSSLResources();
 
 private:
+    // all accesses to m_ssl must be protected by this mutex. The only function that is called
+    // from outside SocketMultiplexer thread is close(), so we mostly care about things accessed
+    // by it.
+    std::mutex ssl_mutex_;
+
     Ssl*                m_ssl;
     bool                m_secureReady;
     bool                m_fatal;