From 4f104264e2ddafe7d767491a2fb453074eb9beaf Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Mon, 18 May 2020 18:57:26 +0200 Subject: [PATCH] Move auth failure delay to SConnection It's a generic feature that is better handled as part of SConnection's state machine. --- common/rfb/SConnection.cxx | 58 +++++++++++++++++++++++---------- common/rfb/SConnection.h | 16 ++++++--- common/rfb/VNCSConnectionST.cxx | 22 +------------ common/rfb/VNCSConnectionST.h | 4 --- 4 files changed, 54 insertions(+), 46 deletions(-) diff --git a/common/rfb/SConnection.cxx b/common/rfb/SConnection.cxx index 4869199a..e9dbd665 100644 --- a/common/rfb/SConnection.cxx +++ b/common/rfb/SConnection.cxx @@ -51,9 +51,9 @@ const SConnection::AccessRights SConnection::AccessFull = 0xffff; SConnection::SConnection() : readyForSetColourMapEntries(false), - is(0), os(0), reader_(0), writer_(0), - ssecurity(0), state_(RFBSTATE_UNINITIALISED), - preferredEncoding(encodingRaw), + is(0), os(0), reader_(0), writer_(0), ssecurity(0), + authFailureTimer(this, &SConnection::handleAuthFailureTimeout), + state_(RFBSTATE_UNINITIALISED), preferredEncoding(encodingRaw), clientClipboard(NULL), hasLocalClipboard(false) { defaultMajorVersion = 3; @@ -98,6 +98,7 @@ void SConnection::processMsg() case RFBSTATE_PROTOCOL_VERSION: processVersionMsg(); break; case RFBSTATE_SECURITY_TYPE: processSecurityTypeMsg(); break; case RFBSTATE_SECURITY: processSecurityMsg(); break; + case RFBSTATE_SECURITY_FAILURE: processSecurityFailure(); break; case RFBSTATE_INITIALISATION: processInitMsg(); break; case RFBSTATE_NORMAL: reader_->readMsg(); break; case RFBSTATE_QUERYING: @@ -240,16 +241,52 @@ void SConnection::processSecurityMsg() } catch (AuthFailureException& e) { vlog.error("AuthFailureException: %s", e.str()); state_ = RFBSTATE_SECURITY_FAILURE; - authFailure(e.str()); + // Introduce a slight delay of the authentication failure response + // to make it difficult to brute force a password + authFailureMsg.replaceBuf(strDup(e.str())); + authFailureTimer.start(100); } } +void SConnection::processSecurityFailure() +{ + // Silently drop any data if we are currently delaying an + // authentication failure response as otherwise we would close + // the connection on unexpected data, and an attacker could use + // that to detect our delayed state. + + while (is->checkNoWait(1)) + is->skip(1); +} + void SConnection::processInitMsg() { vlog.debug("reading client initialisation"); reader_->readClientInit(); } +bool SConnection::handleAuthFailureTimeout(Timer* t) +{ + if (state_ != RFBSTATE_SECURITY_FAILURE) { + close("SConnection::handleAuthFailureTimeout: invalid state"); + return false; + } + + try { + os->writeU32(secResultFailed); + if (!client.beforeVersion(3,8)) // 3.8 onwards have failure message + os->writeString(authFailureMsg.buf); + os->flush(); + } catch (rdr::Exception& e) { + close(e.str()); + return false; + } + + close(authFailureMsg.buf); + + return false; +} + void SConnection::throwConnFailedException(const char* format, ...) { va_list ap; @@ -378,19 +415,6 @@ void SConnection::authSuccess() { } -void SConnection::authFailure(const char* reason) -{ - if (state_ != RFBSTATE_SECURITY_FAILURE) - throw Exception("SConnection::authFailure: invalid state"); - - os->writeU32(secResultFailed); - if (!client.beforeVersion(3,8)) // 3.8 onwards have failure message - os->writeString(reason); - os->flush(); - - throw AuthFailureException(reason); -} - void SConnection::queryConnection(const char* userName) { approveConnection(true); diff --git a/common/rfb/SConnection.h b/common/rfb/SConnection.h index db3ab08c..a7c4e0a6 100644 --- a/common/rfb/SConnection.h +++ b/common/rfb/SConnection.h @@ -26,8 +26,10 @@ #include #include + #include #include +#include namespace rfb { @@ -102,10 +104,6 @@ namespace rfb { // authSuccess() is called when authentication has succeeded. virtual void authSuccess(); - // authFailure() is called when authentication has failed. The default - // implementation will inform the client and throw a AuthFailureException. - virtual void authFailure(const char* reason); - // queryConnection() is called when authentication has succeeded, but // before informing the client. It can be overridden to query a local user // to accept the incoming connection, for example. The userName argument @@ -240,15 +238,25 @@ namespace rfb { void processSecurityTypeMsg(); void processSecurityType(int secType); void processSecurityMsg(); + void processSecurityFailure(); void processInitMsg(); + bool handleAuthFailureTimeout(Timer* t); + int defaultMajorVersion, defaultMinorVersion; + rdr::InStream* is; rdr::OutStream* os; + SMsgReader* reader_; SMsgWriter* writer_; + SecurityServer security; SSecurity* ssecurity; + + MethodTimer authFailureTimer; + CharArray authFailureMsg; + stateEnum state_; rdr::S32 preferredEncoding; AccessRights accessRights; diff --git a/common/rfb/VNCSConnectionST.cxx b/common/rfb/VNCSConnectionST.cxx index 5d2d4b13..39efb6c6 100644 --- a/common/rfb/VNCSConnectionST.cxx +++ b/common/rfb/VNCSConnectionST.cxx @@ -52,8 +52,7 @@ VNCSConnectionST::VNCSConnectionST(VNCServerST* server_, network::Socket *s, losslessTimer(this), server(server_), updateRenderedCursor(false), removeRenderedCursor(false), continuousUpdates(false), encodeManager(this), idleTimer(this), - pointerEventTime(0), clientHasCursor(false), - authFailureTimer(this) + pointerEventTime(0), clientHasCursor(false) { setStreams(&sock->inStream(), &sock->outStream()); peerEndpoint.buf = sock->getPeerEndpoint(); @@ -152,15 +151,6 @@ void VNCSConnectionST::processMessages() sock->cork(true); while (getInStream()->checkNoWait(1)) { - // Silently drop any data if we are currently delaying an - // authentication failure response as otherwise we would close - // the connection on unexpected data, and an attacker could use - // that to detect our delayed state. - if (state() == RFBSTATE_SECURITY_FAILURE) { - getInStream()->skip(1); - continue; - } - if (pendingSyncFence) { syncFence = true; pendingSyncFence = false; @@ -437,14 +427,6 @@ void VNCSConnectionST::authSuccess() updates.add_changed(server->getPixelBuffer()->getRect()); } -void VNCSConnectionST::authFailure(const char* reason) -{ - // Introduce a slight delay of the authentication failure response - // to make it difficult to brute force a password - authFailureMsg.replaceBuf(strDup(reason)); - authFailureTimer.start(100); -} - void VNCSConnectionST::queryConnection(const char* userName) { server->queryConnection(this, userName); @@ -787,8 +769,6 @@ bool VNCSConnectionST::handleTimeout(Timer* t) if ((t == &congestionTimer) || (t == &losslessTimer)) writeFramebufferUpdate(); - else if (t == &authFailureTimer) - SConnection::authFailure(authFailureMsg.buf); } catch (rdr::Exception& e) { close(e.str()); } diff --git a/common/rfb/VNCSConnectionST.h b/common/rfb/VNCSConnectionST.h index c8f4c24f..7ec2d40b 100644 --- a/common/rfb/VNCSConnectionST.h +++ b/common/rfb/VNCSConnectionST.h @@ -111,7 +111,6 @@ namespace rfb { // These methods are invoked as callbacks from processMsg() virtual void authSuccess(); - virtual void authFailure(const char* reason); virtual void queryConnection(const char* userName); virtual void clientInit(bool shared); virtual void setPixelFormat(const PixelFormat& pf); @@ -189,9 +188,6 @@ namespace rfb { Point pointerEventPos; bool clientHasCursor; - Timer authFailureTimer; - CharArray authFailureMsg; - CharArray closeReason; }; } -- 2.39.5