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>.
This commit is contained in:
Povilas Kanapickas 2021-11-01 02:53:26 +02:00
parent caeebf6c36
commit 8b937a4abd
3 changed files with 49 additions and 7 deletions

View File

@ -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.

View File

@ -101,6 +101,8 @@ SecureSocket::close()
void SecureSocket::freeSSLResources() void SecureSocket::freeSSLResources()
{ {
std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
if (m_ssl->m_ssl != NULL) { if (m_ssl->m_ssl != NULL) {
SSL_shutdown(m_ssl->m_ssl); SSL_shutdown(m_ssl->m_ssl);
SSL_free(m_ssl->m_ssl); SSL_free(m_ssl->m_ssl);
@ -265,6 +267,8 @@ SecureSocket::doWrite()
int int
SecureSocket::secureRead(void* buffer, int size, int& read) SecureSocket::secureRead(void* buffer, int size, int& read)
{ {
std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
if (m_ssl->m_ssl != NULL) { if (m_ssl->m_ssl != NULL) {
LOG((CLOG_DEBUG2 "reading secure socket")); LOG((CLOG_DEBUG2 "reading secure socket"));
read = SSL_read(m_ssl->m_ssl, buffer, size); read = SSL_read(m_ssl->m_ssl, buffer, size);
@ -291,6 +295,8 @@ SecureSocket::secureRead(void* buffer, int size, int& read)
int int
SecureSocket::secureWrite(const void* buffer, int size, int& wrote) SecureSocket::secureWrite(const void* buffer, int size, int& wrote)
{ {
std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
if (m_ssl->m_ssl != NULL) { if (m_ssl->m_ssl != NULL) {
LOG((CLOG_DEBUG2 "writing secure socket:%p", this)); LOG((CLOG_DEBUG2 "writing secure socket:%p", this));
@ -324,6 +330,8 @@ SecureSocket::isSecureReady()
void void
SecureSocket::initSsl(bool server) SecureSocket::initSsl(bool server)
{ {
std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
m_ssl = new Ssl(); m_ssl = new Ssl();
m_ssl->m_context = NULL; m_ssl->m_context = NULL;
m_ssl->m_ssl = NULL; m_ssl->m_ssl = NULL;
@ -333,6 +341,8 @@ SecureSocket::initSsl(bool server)
bool SecureSocket::load_certificates(const barrier::fs::path& path) bool SecureSocket::load_certificates(const barrier::fs::path& path)
{ {
std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
if (path.empty()) { if (path.empty()) {
showError("ssl certificate is not specified"); showError("ssl certificate is not specified");
return false; return false;
@ -374,6 +384,8 @@ static int cert_verify_ignore_callback(X509_STORE_CTX*, void*)
void void
SecureSocket::initContext(bool server) SecureSocket::initContext(bool server)
{ {
// ssl_mutex_ is assumed to be acquired
SSL_library_init(); SSL_library_init();
const SSL_METHOD* method; const SSL_METHOD* method;
@ -419,6 +431,8 @@ SecureSocket::initContext(bool server)
void void
SecureSocket::createSSL() SecureSocket::createSSL()
{ {
// ssl_mutex_ is assumed to be acquired
// I assume just one instance is needed // I assume just one instance is needed
// get new SSL state with context // get new SSL state with context
if (m_ssl->m_ssl == NULL) { if (m_ssl->m_ssl == NULL) {
@ -430,6 +444,8 @@ SecureSocket::createSSL()
int int
SecureSocket::secureAccept(int socket) SecureSocket::secureAccept(int socket)
{ {
std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
createSSL(); createSSL();
// set connection socket to SSL state // set connection socket to SSL state
@ -497,12 +513,15 @@ SecureSocket::secureAccept(int socket)
int int
SecureSocket::secureConnect(int socket) SecureSocket::secureConnect(int socket)
{ {
// note that load_certificates acquires ssl_mutex_
if (!load_certificates(barrier::DataDirectories::ssl_certificate_path())) { if (!load_certificates(barrier::DataDirectories::ssl_certificate_path())) {
LOG((CLOG_ERR "could not load client certificates")); LOG((CLOG_ERR "could not load client certificates"));
// FIXME: this is fatal error, but we current don't disconnect because whole logic in this // FIXME: this is fatal error, but we current don't disconnect because whole logic in this
// function needs to be cleaned up // function needs to be cleaned up
} }
std::lock_guard<std::mutex> ssl_lock{ssl_mutex_};
createSSL(); createSSL();
// attach the socket descriptor // attach the socket descriptor
@ -555,6 +574,7 @@ SecureSocket::secureConnect(int socket)
bool bool
SecureSocket::ensure_peer_certificate() SecureSocket::ensure_peer_certificate()
{ {
// ssl_mutex_ is assumed to be acquired
X509* cert; X509* cert;
char* line; char* line;
@ -577,6 +597,8 @@ SecureSocket::ensure_peer_certificate()
void void
SecureSocket::checkResult(int status, int& retry) 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 // ssl errors are a little quirky. the "want" errors are normal and
// should result in a retry. // should result in a retry.
@ -692,6 +714,8 @@ SecureSocket::disconnect()
bool SecureSocket::verify_cert_fingerprint(const barrier::fs::path& fingerprint_db_path) bool SecureSocket::verify_cert_fingerprint(const barrier::fs::path& fingerprint_db_path)
{ {
// ssl_mutex_ is assumed to be acquired
// calculate received certificate fingerprint // calculate received certificate fingerprint
barrier::FingerprintData fingerprint_sha1, fingerprint_sha256; barrier::FingerprintData fingerprint_sha1, fingerprint_sha256;
try { try {
@ -818,6 +842,8 @@ showCipherStackDesc(STACK_OF(SSL_CIPHER) * stack) {
void void
SecureSocket::showSecureCipherInfo() SecureSocket::showSecureCipherInfo()
{ {
// ssl_mutex_ is assumed to be acquired
STACK_OF(SSL_CIPHER) * sStack = SSL_get_ciphers(m_ssl->m_ssl); STACK_OF(SSL_CIPHER) * sStack = SSL_get_ciphers(m_ssl->m_ssl);
if (sStack == NULL) { if (sStack == NULL) {
@ -860,6 +886,8 @@ SecureSocket::showSecureLibInfo()
void void
SecureSocket::showSecureConnectInfo() SecureSocket::showSecureConnectInfo()
{ {
// ssl_mutex_ is assumed to be acquired
const SSL_CIPHER* cipher = SSL_get_current_cipher(m_ssl->m_ssl); const SSL_CIPHER* cipher = SSL_get_current_cipher(m_ssl->m_ssl);
if (cipher != NULL) { if (cipher != NULL) {

View File

@ -21,6 +21,7 @@
#include "net/TCPSocket.h" #include "net/TCPSocket.h"
#include "net/XSocket.h" #include "net/XSocket.h"
#include "io/filesystem.h" #include "io/filesystem.h"
#include <mutex>
class IEventQueue; class IEventQueue;
class SocketMultiplexer; class SocketMultiplexer;
@ -61,29 +62,38 @@ public:
private: private:
// SSL // SSL
void initContext(bool server); void initContext(bool server); // may only be called with ssl_mutex_ acquired
void createSSL(); void createSSL(); // may only be called with ssl_mutex_ acquired.
int secureAccept(int s); int secureAccept(int s);
int secureConnect(int s); int secureConnect(int s);
bool ensure_peer_certificate(); bool ensure_peer_certificate(); // may only be called with ssl_mutex_ acquired
void checkResult(int n, int& retry);
void checkResult(int n, int& retry); // may only be called with m_ssl_mutex_ acquired.
void showError(const std::string& reason); void showError(const std::string& reason);
std::string getError(); std::string getError();
void disconnect(); void disconnect();
// may only be called with ssl_mutex_ acquired
bool verify_cert_fingerprint(const barrier::fs::path& fingerprint_db_path); bool verify_cert_fingerprint(const barrier::fs::path& fingerprint_db_path);
MultiplexerJobStatus serviceConnect(ISocketMultiplexerJob*, bool, bool, bool); MultiplexerJobStatus serviceConnect(ISocketMultiplexerJob*, bool, bool, bool);
MultiplexerJobStatus serviceAccept(ISocketMultiplexerJob*, bool, bool, bool); MultiplexerJobStatus serviceAccept(ISocketMultiplexerJob*, bool, bool, bool);
void showSecureConnectInfo(); void showSecureConnectInfo(); // may only be called with ssl_mutex_ acquired
void showSecureLibInfo(); void showSecureLibInfo();
void showSecureCipherInfo(); void showSecureCipherInfo(); // may only be called with ssl_mutex_ acquired
void handleTCPConnected(const Event& event, void*); void handleTCPConnected(const Event& event, void*);
void freeSSLResources(); void freeSSLResources();
private: 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; Ssl* m_ssl;
bool m_secureReady; bool m_secureReady;
bool m_fatal; bool m_fatal;