From 1c1e83c94235a0148503144f070155c4eba21cc9 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 02:53:26 +0200 Subject: [PATCH] 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 . --- .../fix-crash-on-ssl-hello.bugfix | 4 +++ src/lib/net/SecureSocket.cpp | 27 +++++++++++++++++++ src/lib/net/SecureSocket.h | 24 ++++++++++------- 3 files changed, 46 insertions(+), 9 deletions(-) create mode 100644 doc/newsfragments/fix-crash-on-ssl-hello.bugfix diff --git a/doc/newsfragments/fix-crash-on-ssl-hello.bugfix b/doc/newsfragments/fix-crash-on-ssl-hello.bugfix new file mode 100644 index 00000000..30bb0603 --- /dev/null +++ b/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. diff --git a/src/lib/net/SecureSocket.cpp b/src/lib/net/SecureSocket.cpp index 74feb47d..5a5ddde7 100644 --- a/src/lib/net/SecureSocket.cpp +++ b/src/lib/net/SecureSocket.cpp @@ -104,6 +104,8 @@ SecureSocket::close() void SecureSocket::freeSSLResources() { + std::lock_guard 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 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 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 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 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 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 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) { diff --git a/src/lib/net/SecureSocket.h b/src/lib/net/SecureSocket.h index c602e2da..c5348317 100644 --- a/src/lib/net/SecureSocket.h +++ b/src/lib/net/SecureSocket.h @@ -19,6 +19,7 @@ #include "net/TCPSocket.h" #include "net/XSocket.h" +#include 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;