Move auth failure delay to SConnection

It's a generic feature that is better handled as part of SConnection's
state machine.

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 <rdr/InStream.h>
 #include <rdr/OutStream.h>
+
 #include <rfb/SMsgHandler.h>
 #include <rfb/SecurityServer.h>
+#include <rfb/Timer.h>
 
 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<SConnection> 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;
   };
 }