From e4f86a8934af3377e873c78fafdfc0b48171584f Mon Sep 17 00:00:00 2001 From: Adam Potolsky Date: Thu, 21 May 2015 15:22:39 -0700 Subject: [PATCH 1/2] Adding pass/fail retry logic to connection attempts #4697 #4650 --- src/lib/plugin/ns/SecureSocket.cpp | 94 ++++++++++++++++++------------ src/lib/plugin/ns/SecureSocket.h | 4 +- 2 files changed, 60 insertions(+), 38 deletions(-) diff --git a/src/lib/plugin/ns/SecureSocket.cpp b/src/lib/plugin/ns/SecureSocket.cpp index f182ee8a..6a34fce3 100644 --- a/src/lib/plugin/ns/SecureSocket.cpp +++ b/src/lib/plugin/ns/SecureSocket.cpp @@ -249,7 +249,7 @@ SecureSocket::createSSL() } } -bool +int SecureSocket::secureAccept(int socket) { createSSL(); @@ -269,24 +269,31 @@ SecureSocket::secureAccept(int socket) // tell user and sleep so the socket isn't hammered. LOG((CLOG_ERR "failed to accept secure socket")); LOG((CLOG_INFO "client connection may not be secure")); + m_secureReady = false; ARCH->sleep(1); + return -1; // Failed, error out } + // If not fatal and no retry, state is good if (retry == 0) { m_secureReady = true; - } - else { - m_secureReady = false; - } - - if (m_secureReady) { LOG((CLOG_INFO "accepted secure socket")); + return 1; } - return !m_secureReady; + // If not fatal and retry is set, not ready, and return retry + if (retry > 0) { + LOG((CLOG_DEBUG2 "retry accepting secure socket")); + m_secureReady = false; + return 0; + } + + // no good state exists here + LOG((CLOG_ERR "unexpected state attempting to accept connection")); + return -1; } -bool +int SecureSocket::secureConnect(int socket) { createSSL(); @@ -304,31 +311,32 @@ SecureSocket::secureConnect(int socket) if (fatal) { LOG((CLOG_ERR "failed to connect secure socket")); - LOG((CLOG_INFO "server connection may not be secure")); - return false; + return -1; } - if (retry == 0) { - m_secureReady = true; + // If we should retry, not ready and return 0 + if (retry > 0) { + LOG((CLOG_DEBUG2 "retry connect secure socket")); + m_secureReady = false; + return 0; + } + + // No error, set ready, process and return ok + m_secureReady = true; + if (verifyCertFingerprint()) { + LOG((CLOG_INFO "connected to secure socket")); + if (!showCertificate()) { + disconnect(); + return -1;// Cert fail, error + } } else { - m_secureReady = false; + LOG((CLOG_ERR "failed to verify server certificate fingerprint")); + disconnect(); + return -1; // Fingerprint failed, error } - - if (m_secureReady) { - if (verifyCertFingerprint()) { - LOG((CLOG_INFO "connected to secure socket")); - if (!showCertificate()) { - disconnect(); - } - } - else { - LOG((CLOG_ERR "failed to verify server certificate fingerprint")); - disconnect(); - } - } - - return !m_secureReady; + LOG((CLOG_DEBUG2 "connected secure socket")); + return 1; } bool @@ -545,14 +553,21 @@ SecureSocket::serviceConnect(ISocketMultiplexerJob* job, { Lock lock(&getMutex()); - bool retry = true; + int status = 0; #ifdef SYSAPI_WIN32 - retry = secureConnect(static_cast(getSocket()->m_socket)); + status = secureConnect(static_cast(getSocket()->m_socket)); #elif SYSAPI_UNIX retry = secureConnect(getSocket()->m_fd); #endif - return retry ? job : newJob(); + if (status > 0) { + return newJob(); + } + else if (status == 0) { + return job; + } + // If status < 0, error happened + return NULL; } ISocketMultiplexerJob* @@ -561,12 +576,19 @@ SecureSocket::serviceAccept(ISocketMultiplexerJob* job, { Lock lock(&getMutex()); - bool retry = true; + int status = 0; #ifdef SYSAPI_WIN32 - retry = secureAccept(static_cast(getSocket()->m_socket)); + status = secureAccept(static_cast(getSocket()->m_socket)); #elif SYSAPI_UNIX - retry = secureAccept(getSocket()->m_fd); + status = secureAccept(getSocket()->m_fd); #endif - return retry ? job : newJob(); + if (status > 0) { + return newJob(); + } + else if (status == 0) { + return job; + } + // If status < 0, error happened + return NULL; } diff --git a/src/lib/plugin/ns/SecureSocket.h b/src/lib/plugin/ns/SecureSocket.h index 754956ca..3ea13c59 100644 --- a/src/lib/plugin/ns/SecureSocket.h +++ b/src/lib/plugin/ns/SecureSocket.h @@ -57,8 +57,8 @@ private: // SSL void initContext(bool server); void createSSL(); - bool secureAccept(int s); - bool secureConnect(int s); + int secureAccept(int s); + int secureConnect(int s); bool showCertificate(); void checkResult(int n, bool& fatal, int& retry); void showError(const char* reason = NULL); From 5b3fa48902173dc46e1a8db91d37c3c95b8625cf Mon Sep 17 00:00:00 2001 From: Adam Potolsky Date: Fri, 22 May 2015 10:56:13 -0700 Subject: [PATCH 2/2] Made socket self-aware of when it is in a fatal state #4697 Added code to cleanly terminate connection on fatal socket state #4697 --- src/lib/net/IDataSocket.h | 1 + src/lib/net/TCPSocket.cpp | 17 +++++++ src/lib/net/TCPSocket.h | 1 + src/lib/plugin/ns/SecureSocket.cpp | 76 ++++++++++++++++++------------ src/lib/plugin/ns/SecureSocket.h | 5 +- src/lib/server/ClientListener.cpp | 24 ++++++---- 6 files changed, 84 insertions(+), 40 deletions(-) diff --git a/src/lib/net/IDataSocket.h b/src/lib/net/IDataSocket.h index ebd31049..08970657 100644 --- a/src/lib/net/IDataSocket.h +++ b/src/lib/net/IDataSocket.h @@ -68,5 +68,6 @@ public: virtual void shutdownInput() = 0; virtual void shutdownOutput() = 0; virtual bool isReady() const = 0; + virtual bool isFatal() const = 0; virtual UInt32 getSize() const = 0; }; diff --git a/src/lib/net/TCPSocket.cpp b/src/lib/net/TCPSocket.cpp index ad4fbd40..620e26ec 100644 --- a/src/lib/net/TCPSocket.cpp +++ b/src/lib/net/TCPSocket.cpp @@ -253,6 +253,14 @@ TCPSocket::isReady() const return (m_inputBuffer.getSize() > 0); } +bool +TCPSocket::isFatal() const +{ + // TCP sockets aren't ever left in a fatal state. + LOG((CLOG_ERR "isFatal() not valid for non-secure connections")); + return false; +} + UInt32 TCPSocket::getSize() const { @@ -481,6 +489,9 @@ TCPSocket::serviceConnected(ISocketMultiplexerJob* job, if (n > 0) { s_retryOutputBufferSize = 0; } + else if (n < 0) { + return NULL; + } } else { return job; @@ -537,6 +548,9 @@ TCPSocket::serviceConnected(ISocketMultiplexerJob* job, if (isSecure()) { if (isSecureReady()) { n = secureRead(buffer, sizeof(buffer)); + if (n < 0) { + return NULL; + } } else { return job; @@ -555,6 +569,9 @@ TCPSocket::serviceConnected(ISocketMultiplexerJob* job, if (isSecure() && isSecureReady()) { n = secureRead(buffer, sizeof(buffer)); + if (n < 0) { + return NULL; + } } else { n = ARCH->readSocket(m_socket, buffer, sizeof(buffer)); diff --git a/src/lib/net/TCPSocket.h b/src/lib/net/TCPSocket.h index 60866b3e..831d4729 100644 --- a/src/lib/net/TCPSocket.h +++ b/src/lib/net/TCPSocket.h @@ -52,6 +52,7 @@ public: virtual void shutdownInput(); virtual void shutdownOutput(); virtual bool isReady() const; + virtual bool isFatal() const; virtual UInt32 getSize() const; // IDataSocket overrides diff --git a/src/lib/plugin/ns/SecureSocket.cpp b/src/lib/plugin/ns/SecureSocket.cpp index 6a34fce3..d9e47d73 100644 --- a/src/lib/plugin/ns/SecureSocket.cpp +++ b/src/lib/plugin/ns/SecureSocket.cpp @@ -52,7 +52,8 @@ SecureSocket::SecureSocket( SocketMultiplexer* socketMultiplexer) : TCPSocket(events, socketMultiplexer), m_secureReady(false), - m_maxRetry(MAX_RETRY_COUNT) + m_maxRetry(MAX_RETRY_COUNT), + m_fatal(false) { } @@ -62,12 +63,14 @@ SecureSocket::SecureSocket( ArchSocket socket) : TCPSocket(events, socketMultiplexer, socket), m_secureReady(false), - m_maxRetry(MAX_RETRY_COUNT) + m_maxRetry(MAX_RETRY_COUNT), + m_fatal(false) { } SecureSocket::~SecureSocket() { + isFatal(true); if (m_ssl->m_ssl != NULL) { SSL_shutdown(m_ssl->m_ssl); @@ -78,13 +81,15 @@ SecureSocket::~SecureSocket() SSL_CTX_free(m_ssl->m_context); m_ssl->m_context = NULL; } - + ARCH->sleep(1); delete m_ssl; } void SecureSocket::close() { + isFatal(true); + SSL_shutdown(m_ssl->m_ssl); TCPSocket::close(); @@ -114,17 +119,23 @@ SecureSocket::secureRead(void* buffer, UInt32 n) LOG((CLOG_DEBUG2 "reading secure socket")); r = SSL_read(m_ssl->m_ssl, buffer, n); - bool fatal; static int retry; - checkResult(r, fatal, retry); + // Check result will cleanup the connection in the case of a fatal + checkResult(r, retry); if (retry) { - r = 0; + return 0; + } + + if (isFatal()) { + return -1; } } - - return r > 0 ? (UInt32)r : 0; + // According to SSL spec, r must not be negative and not have an error code + // from SSL_get_error(). If this happens, it is itself an error. Let the + // parent handle the case + return (UInt32)r; } UInt32 @@ -132,20 +143,27 @@ SecureSocket::secureWrite(const void* buffer, UInt32 n) { int r = 0; if (m_ssl->m_ssl != NULL) { - LOG((CLOG_DEBUG2 "writing secure socket")); + LOG((CLOG_DEBUG2 "writing secure socket:%p", this)); + r = SSL_write(m_ssl->m_ssl, buffer, n); - bool fatal; static int retry; - checkResult(r, fatal, retry); + // Check result will cleanup the connection in the case of a fatal + checkResult(r, retry); if (retry) { - r = 0; + return 0; + } + + if (isFatal()) { + return -1; } } - - return r > 0 ? (UInt32)r : 0; + // According to SSL spec, r must not be negative and not have an error code + // from SSL_get_error(). If this happens, it is itself an error. Let the + // parent handle the case + return (UInt32)r; } bool @@ -260,12 +278,11 @@ SecureSocket::secureAccept(int socket) LOG((CLOG_DEBUG2 "accepting secure socket")); int r = SSL_accept(m_ssl->m_ssl); - bool fatal; static int retry; - checkResult(r, fatal, retry); + checkResult(r, retry); - if (fatal) { + if (isFatal()) { // tell user and sleep so the socket isn't hammered. LOG((CLOG_ERR "failed to accept secure socket")); LOG((CLOG_INFO "client connection may not be secure")); @@ -304,12 +321,11 @@ SecureSocket::secureConnect(int socket) LOG((CLOG_DEBUG2 "connecting secure socket")); int r = SSL_connect(m_ssl->m_ssl); - bool fatal; static int retry; - checkResult(r, fatal, retry); + checkResult(r, retry); - if (fatal) { + if (isFatal()) { LOG((CLOG_ERR "failed to connect secure socket")); return -1; } @@ -362,13 +378,11 @@ SecureSocket::showCertificate() } void -SecureSocket::checkResult(int n, bool& fatal, int& retry) +SecureSocket::checkResult(int n, int& retry) { // ssl errors are a little quirky. the "want" errors are normal and // should result in a retry. - fatal = false; - int errorCode = SSL_get_error(m_ssl->m_ssl, n); switch (errorCode) { case SSL_ERROR_NONE: @@ -378,8 +392,8 @@ SecureSocket::checkResult(int n, bool& fatal, int& retry) case SSL_ERROR_ZERO_RETURN: // connection closed - retry = 0; - LOG((CLOG_DEBUG2 "SSL connection has been closed")); + isFatal(true); + LOG((CLOG_DEBUG "SSL connection has been closed")); break; case SSL_ERROR_WANT_READ: @@ -389,7 +403,7 @@ SecureSocket::checkResult(int n, bool& fatal, int& retry) retry += 1; // If there are a lot of retrys, it's worth warning about if ( retry % 5 == 0 ) { - LOG((CLOG_INFO "need to retry the same SSL function(%d) retry:%d", errorCode, retry)); + LOG((CLOG_DEBUG "need to retry the same SSL function(%d) retry:%d", errorCode, retry)); } else if ( retry == (maxRetry() / 2) ) { LOG((CLOG_WARN "need to retry the same SSL function(%d) retry:%d", errorCode, retry)); @@ -416,27 +430,27 @@ SecureSocket::checkResult(int n, bool& fatal, int& retry) } } - fatal = true; + isFatal(true); break; case SSL_ERROR_SSL: LOG((CLOG_ERR "a failure in the SSL library occurred")); - fatal = true; + isFatal(true); break; default: LOG((CLOG_ERR "unknown secure socket error")); - fatal = true; + isFatal(true); break; } // If the retry max would exceed the allowed, treat it as a fatal error if (retry > maxRetry()) { LOG((CLOG_ERR "Maximum retry count exceeded:%d",retry)); - fatal = true; + isFatal(true); } - if (fatal) { + if (isFatal()) { retry = 0; showError(); disconnect(); diff --git a/src/lib/plugin/ns/SecureSocket.h b/src/lib/plugin/ns/SecureSocket.h index 3ea13c59..81a6c7f3 100644 --- a/src/lib/plugin/ns/SecureSocket.h +++ b/src/lib/plugin/ns/SecureSocket.h @@ -44,6 +44,8 @@ public: void secureConnect(); void secureAccept(); bool isReady() const { return m_secureReady; } + bool isFatal() const { return m_fatal; } + void isFatal(bool b) { m_fatal = b; } bool isSecureReady(); bool isSecure() { return true; } UInt32 secureRead(void* buffer, UInt32 n); @@ -60,7 +62,7 @@ private: int secureAccept(int s); int secureConnect(int s); bool showCertificate(); - void checkResult(int n, bool& fatal, int& retry); + void checkResult(int n, int& retry); void showError(const char* reason = NULL); String getError(); void disconnect(); @@ -80,5 +82,6 @@ private: private: Ssl* m_ssl; bool m_secureReady; + bool m_fatal; int m_maxRetry; }; diff --git a/src/lib/server/ClientListener.cpp b/src/lib/server/ClientListener.cpp index 4383ac04..efcb736c 100644 --- a/src/lib/server/ClientListener.cpp +++ b/src/lib/server/ClientListener.cpp @@ -142,26 +142,34 @@ ClientListener::handleClientConnecting(const Event&, void*) { // accept client connection IDataSocket* socket = m_listen->accept(); - synergy::IStream* stream = socket; - if (stream == NULL) { + if (socket == NULL) { return; } LOG((CLOG_NOTE "accepted client connection")); + if (m_useSecureNetwork) { + LOG((CLOG_DEBUG2 "attempting sercure Connection")); + while(!socket->isReady()) { + if(socket->isFatal()) { + m_listen->deleteSocket(socket); + return; + } + LOG((CLOG_DEBUG2 "retrying sercure Connection")); + ARCH->sleep(.5f); + } + } + + LOG((CLOG_DEBUG2 "sercure Connection established:%d")); + + synergy::IStream* stream = socket; // filter socket messages, including a packetizing filter bool adopt = !m_useSecureNetwork; stream = new PacketStreamFilter(m_events, stream, adopt); assert(m_server != NULL); - if (m_useSecureNetwork) { - while(!socket->isReady()) { - ARCH->sleep(.5f); - } - } - // create proxy for unknown client ClientProxyUnknown* client = new ClientProxyUnknown(stream, 30.0, m_server, m_events); m_newClients.insert(client);