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; };