From 8b937a4abd5425b6588c3e096f6daf16343674c4 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 02:53:26 +0200 Subject: [PATCH 1/2] 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; From f0efe043bbed01294c684c8d5c69cbfc77a3b050 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 02:53:27 +0200 Subject: [PATCH 2/2] lib/net: Fix incorrect sharing of data between different SSL sessions --- doc/newsfragments/ssl-corrupted-data.bugfix | 2 + src/lib/net/SecureSocket.cpp | 63 ++++++++------------- src/lib/net/SecureSocket.h | 12 ++++ 3 files changed, 39 insertions(+), 38 deletions(-) create mode 100644 doc/newsfragments/ssl-corrupted-data.bugfix diff --git a/doc/newsfragments/ssl-corrupted-data.bugfix b/doc/newsfragments/ssl-corrupted-data.bugfix new file mode 100644 index 00000000..db8bbf86 --- /dev/null +++ b/doc/newsfragments/ssl-corrupted-data.bugfix @@ -0,0 +1,2 @@ +Fixed a bug in SSL implementation that caused invalid data occasionally being sent to clients +under heavy load. diff --git a/src/lib/net/SecureSocket.cpp b/src/lib/net/SecureSocket.cpp index 197ee832..d6199f09 100644 --- a/src/lib/net/SecureSocket.cpp +++ b/src/lib/net/SecureSocket.cpp @@ -156,7 +156,7 @@ SecureSocket::secureAccept() TCPSocket::EJobResult SecureSocket::doRead() { - static UInt8 buffer[4096]; + UInt8 buffer[4096]; memset(buffer, 0, sizeof(buffer)); int bytesRead = 0; int status = 0; @@ -215,11 +215,6 @@ SecureSocket::doRead() TCPSocket::EJobResult SecureSocket::doWrite() { - static bool s_retry = false; - static int s_retrySize = 0; - static std::unique_ptr s_staticBuffer; - static std::size_t s_staticBufferSize = 0; - // write data int bufferSize = 0; int bytesWrote = 0; @@ -228,16 +223,16 @@ SecureSocket::doWrite() if (!isSecureReady()) return kRetry; - if (s_retry) { - bufferSize = s_retrySize; + if (do_write_retry_) { + bufferSize = do_write_retry_size_; } else { bufferSize = m_outputBuffer.getSize(); - if (bufferSize > s_staticBufferSize) { - s_staticBuffer.reset(new char[bufferSize]); - s_staticBufferSize = bufferSize; + if (bufferSize > do_write_retry_buffer_size_) { + do_write_retry_buffer_.reset(new char[bufferSize]); + do_write_retry_buffer_size_ = bufferSize; } if (bufferSize > 0) { - memcpy(s_staticBuffer.get(), m_outputBuffer.peek(bufferSize), bufferSize); + std::memcpy(do_write_retry_buffer_.get(), m_outputBuffer.peek(bufferSize), bufferSize); } } @@ -245,14 +240,14 @@ SecureSocket::doWrite() return kRetry; } - status = secureWrite(s_staticBuffer.get(), bufferSize, bytesWrote); + status = secureWrite(do_write_retry_buffer_.get(), bufferSize, bytesWrote); if (status > 0) { - s_retry = false; + do_write_retry_ = false; } else if (status < 0) { return kBreak; } else if (status == 0) { - s_retry = true; - s_retrySize = bufferSize; + do_write_retry_ = true; + do_write_retry_size_ = bufferSize; return kNew; } @@ -273,12 +268,10 @@ SecureSocket::secureRead(void* buffer, int size, int& read) LOG((CLOG_DEBUG2 "reading secure socket")); read = SSL_read(m_ssl->m_ssl, buffer, size); - static int retry; - // Check result will cleanup the connection in the case of a fatal - checkResult(read, retry); + checkResult(read, secure_read_retry_); - if (retry) { + if (secure_read_retry_) { return 0; } @@ -302,12 +295,10 @@ SecureSocket::secureWrite(const void* buffer, int size, int& wrote) wrote = SSL_write(m_ssl->m_ssl, buffer, size); - static int retry; - // Check result will cleanup the connection in the case of a fatal - checkResult(wrote, retry); + checkResult(wrote, secure_write_retry_); - if (retry) { + if (secure_write_retry_) { return 0; } @@ -454,9 +445,7 @@ SecureSocket::secureAccept(int socket) LOG((CLOG_DEBUG2 "accepting secure socket")); int r = SSL_accept(m_ssl->m_ssl); - static int retry; - - checkResult(r, retry); + checkResult(r, secure_accept_retry_); if (isFatal()) { // tell user and sleep so the socket isn't hammered. @@ -464,25 +453,25 @@ SecureSocket::secureAccept(int socket) LOG((CLOG_INFO "client connection may not be secure")); m_secureReady = false; ARCH->sleep(1); - retry = 0; + secure_accept_retry_ = 0; return -1; // Failed, error out } // If not fatal and no retry, state is good - if (retry == 0) { + if (secure_accept_retry_ == 0) { if (security_level_ == ConnectionSecurityLevel::ENCRYPTED_AUTHENTICATED) { if (verify_cert_fingerprint( barrier::DataDirectories::trusted_clients_ssl_fingerprints_path())) { LOG((CLOG_INFO "accepted secure socket")); if (!ensure_peer_certificate()) { - retry = 0; + secure_accept_retry_ = 0; disconnect(); return -1;// Cert fail, error } } else { LOG((CLOG_ERR "failed to verify server certificate fingerprint")); - retry = 0; + secure_accept_retry_ = 0; disconnect(); return -1; // Fingerprint failed, error } @@ -498,7 +487,7 @@ SecureSocket::secureAccept(int socket) } // If not fatal and retry is set, not ready, and return retry - if (retry > 0) { + if (secure_accept_retry_ > 0) { LOG((CLOG_DEBUG2 "retry accepting secure socket")); m_secureReady = false; ARCH->sleep(s_retryDelay); @@ -530,25 +519,23 @@ SecureSocket::secureConnect(int socket) LOG((CLOG_DEBUG2 "connecting secure socket")); int r = SSL_connect(m_ssl->m_ssl); - static int retry; - - checkResult(r, retry); + checkResult(r, secure_connect_retry_); if (isFatal()) { LOG((CLOG_ERR "failed to connect secure socket")); - retry = 0; + secure_connect_retry_ = 0; return -1; } // If we should retry, not ready and return 0 - if (retry > 0) { + if (secure_connect_retry_ > 0) { LOG((CLOG_DEBUG2 "retry connect secure socket")); m_secureReady = false; ARCH->sleep(s_retryDelay); return 0; } - retry = 0; + secure_connect_retry_ = 0; // No error, set ready, process and return ok m_secureReady = true; if (verify_cert_fingerprint(barrier::DataDirectories::trusted_servers_ssl_fingerprints_path())) { diff --git a/src/lib/net/SecureSocket.h b/src/lib/net/SecureSocket.h index c50ece11..be7dc0dd 100644 --- a/src/lib/net/SecureSocket.h +++ b/src/lib/net/SecureSocket.h @@ -98,4 +98,16 @@ private: bool m_secureReady; bool m_fatal; ConnectionSecurityLevel security_level_ = ConnectionSecurityLevel::ENCRYPTED; + + int secure_accept_retry_ = 0; // used only in secureAccept() + int secure_connect_retry_ = 0; // used only in secureConnect() + int secure_read_retry_ = 0; // used only in secureRead() + int secure_write_retry_ = 0; // used only in secureWrite() + + // The following are used only from doWrite() + // FIXME: using std::vector would simplify logic significantly. + bool do_write_retry_ = false; + int do_write_retry_size_ = 0; + std::unique_ptr do_write_retry_buffer_; + std::size_t do_write_retry_buffer_size_ = 0; };