From 8b937a4abd5425b6588c3e096f6daf16343674c4 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 | 28 +++++++++++++++++++ src/lib/net/SecureSocket.h | 24 +++++++++++----- 3 files changed, 49 insertions(+), 7 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 af5a795a..197ee832 100644 --- a/src/lib/net/SecureSocket.cpp +++ b/src/lib/net/SecureSocket.cpp @@ -101,6 +101,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); @@ -265,6 +267,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); @@ -291,6 +295,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)); @@ -324,6 +330,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; @@ -333,6 +341,8 @@ SecureSocket::initSsl(bool server) bool SecureSocket::load_certificates(const barrier::fs::path& path) { + std::lock_guard ssl_lock{ssl_mutex_}; + if (path.empty()) { showError("ssl certificate is not specified"); return false; @@ -374,6 +384,8 @@ static int cert_verify_ignore_callback(X509_STORE_CTX*, void*) void SecureSocket::initContext(bool server) { + // ssl_mutex_ is assumed to be acquired + SSL_library_init(); const SSL_METHOD* method; @@ -419,6 +431,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) { @@ -430,6 +444,8 @@ SecureSocket::createSSL() int SecureSocket::secureAccept(int socket) { + std::lock_guard ssl_lock{ssl_mutex_}; + createSSL(); // set connection socket to SSL state @@ -497,12 +513,15 @@ SecureSocket::secureAccept(int socket) int SecureSocket::secureConnect(int socket) { + // note that load_certificates acquires ssl_mutex_ if (!load_certificates(barrier::DataDirectories::ssl_certificate_path())) { LOG((CLOG_ERR "could not load client certificates")); // FIXME: this is fatal error, but we current don't disconnect because whole logic in this // function needs to be cleaned up } + std::lock_guard ssl_lock{ssl_mutex_}; + createSSL(); // attach the socket descriptor @@ -555,6 +574,7 @@ SecureSocket::secureConnect(int socket) bool SecureSocket::ensure_peer_certificate() { + // ssl_mutex_ is assumed to be acquired X509* cert; char* line; @@ -577,6 +597,8 @@ SecureSocket::ensure_peer_certificate() 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. @@ -692,6 +714,8 @@ SecureSocket::disconnect() bool SecureSocket::verify_cert_fingerprint(const barrier::fs::path& fingerprint_db_path) { + // ssl_mutex_ is assumed to be acquired + // calculate received certificate fingerprint barrier::FingerprintData fingerprint_sha1, fingerprint_sha256; try { @@ -818,6 +842,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) { @@ -860,6 +886,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 496a3656..c50ece11 100644 --- a/src/lib/net/SecureSocket.h +++ b/src/lib/net/SecureSocket.h @@ -21,6 +21,7 @@ #include "net/TCPSocket.h" #include "net/XSocket.h" #include "io/filesystem.h" +#include class IEventQueue; class SocketMultiplexer; @@ -61,29 +62,38 @@ 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 ensure_peer_certificate(); - void checkResult(int n, int& retry); + bool ensure_peer_certificate(); // 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 std::string& reason); std::string getError(); void disconnect(); + + // may only be called with ssl_mutex_ acquired bool verify_cert_fingerprint(const barrier::fs::path& fingerprint_db_path); 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;