Merge pull request #1347 from p12tic/enforce-max-message-length

Enforce max message length [SECURITY VULNERABILITY CVE-2021-42076]

doc/newsfragments/enforce-maximum-message-length.bugfix

@@ -0,0 +1,6 @@
+SECURITY ISSUE
+
+Barrier will now enforce a maximum length of input messages (fixes CVE-2021-42076).
+
+Previously it was possible for a malicious client or server to send excessive length messages
+leading to denial of service by resource exhaustion.

src/lib/barrier/PacketStreamFilter.cpp

@@ -17,6 +17,7 @@
  */
 
 #include "barrier/PacketStreamFilter.h"
+#include "barrier/protocol_types.h"
 #include "base/IEventQueue.h"
 #include "mt/Lock.h"
 #include "base/TMethodEventJob.h"
@@ -133,8 +134,7 @@ PacketStreamFilter::isReadyNoLock() const
     return (m_size != 0 && m_buffer.getSize() >= m_size);
 }
 
-void
+bool PacketStreamFilter::readPacketSize()
-PacketStreamFilter::readPacketSize()
 {
     // note -- m_mutex must be locked on entry
 
@@ -146,7 +146,13 @@ PacketStreamFilter::readPacketSize()
                  ((UInt32)buffer[1] << 16) |
                  ((UInt32)buffer[2] <<  8) |
                   (UInt32)buffer[3];
+
+        if (m_size > PROTOCOL_MAX_MESSAGE_LENGTH) {
+            m_events->addEvent(Event(m_events->forIStream().inputFormatError(), getEventTarget()));
+            return false;
+        }
     }
+    return true;
 }
 
 bool
@@ -160,13 +166,17 @@ PacketStreamFilter::readMore()
     UInt32 n = getStream()->read(buffer, sizeof(buffer));
     while (n > 0) {
         m_buffer.write(buffer, n);
+
+        // if we don't yet have the next packet size then get it, if possible.
+        // Note that we can't wait for whole pending data to arrive because it may be huge in
+        // case of malicious or erroneous peer.
+        if (!readPacketSize()) {
+            break;
+        }
+
         n = getStream()->read(buffer, sizeof(buffer));
     }
 
-    // if we don't yet have the next packet size then get it,
-    // if possible.
-    readPacketSize();
-
     // note if we now have a whole packet
     bool isReady = isReadyNoLock();
 

src/lib/barrier/PacketStreamFilter.h

@@ -47,7 +47,9 @@ protected:
 
 private:
     bool                isReadyNoLock() const;
-    void                readPacketSize();
+
+    // returns false on erroneous packet size
+    bool readPacketSize();
     bool                readMore();
 
 private:

src/lib/barrier/ProtocolUtil.cpp

@@ -19,6 +19,8 @@
 #include "barrier/ProtocolUtil.h"
 #include "io/IStream.h"
 #include "base/Log.h"
+#include "barrier/protocol_types.h"
+#include "barrier/XBarrier.h"
 #include "common/stdvector.h"
 #include "base/String.h"
 
@@ -159,6 +161,10 @@ ProtocolUtil::vreadf(barrier::IStream* stream, const char* fmt, va_list args)
                            (static_cast<UInt32>(buffer[2]) <<  8) |
                             static_cast<UInt32>(buffer[3]);
 
+                if (n > PROTOCOL_MAX_LIST_LENGTH) {
+                    throw XBadClient("Too long message received");
+                }
+
                 // convert it
                 void* v = va_arg(args, void*);
                 switch (len) {
@@ -211,6 +217,10 @@ ProtocolUtil::vreadf(barrier::IStream* stream, const char* fmt, va_list args)
                              (static_cast<UInt32>(buffer[2]) <<  8) |
                               static_cast<UInt32>(buffer[3]);
 
+                if (len > PROTOCOL_MAX_STRING_LENGTH) {
+                    throw XBadClient("Too long message received");
+                }
+
                 // use a fixed size buffer if its big enough
                 const bool useFixed = (len <= sizeof(buffer));
 

src/lib/barrier/protocol_types.h

@@ -20,6 +20,8 @@
 
 #include "base/EventTypes.h"
 
+#include <cstdint>
+
 // protocol version number
 // 1.0:  initial protocol
 // 1.1:  adds KeyCode to key press, release, and repeat
@@ -51,6 +53,12 @@ static const double        kKeepAlivesUntilDeath = 3.0;
 static const double        kHeartRate = -1.0;
 static const double        kHeartBeatsUntilDeath = 3.0;
 
+// Messages of very large size indicate a likely protocol error. We don't parse such messages and
+// drop connection instead. Note that e.g. the clipboard messages are already limited to 32kB.
+static constexpr std::uint32_t PROTOCOL_MAX_MESSAGE_LENGTH = 4 * 1024 * 1024;
+static constexpr std::uint32_t PROTOCOL_MAX_LIST_LENGTH = 1024 * 1024;
+static constexpr std::uint32_t PROTOCOL_MAX_STRING_LENGTH = 1024 * 1024;
+
 // direction constants
 enum EDirection {
     kNoDirection,

src/lib/base/EventTypes.cpp

@@ -56,6 +56,7 @@ REGISTER_EVENT(IStream, outputFlushed)
 REGISTER_EVENT(IStream, outputError)
 REGISTER_EVENT(IStream, inputShutdown)
 REGISTER_EVENT(IStream, outputShutdown)
+REGISTER_EVENT(IStream, inputFormatError)
 
 //
 // IpcClient

src/lib/base/EventTypes.h

@@ -133,6 +133,11 @@ public:
     */
     Event::Type        outputShutdown();
 
+    /** Get input format error event type
+
+        This is sent when a stream receives an irrecoverable input format error.
+    */
+    Event::Type inputFormatError();
     //@}
 
 private:
@@ -141,6 +146,7 @@ private:
     Event::Type        m_outputError;
     Event::Type        m_inputShutdown;
     Event::Type        m_outputShutdown;
+    Event::Type m_inputFormatError;
 };
 
 class IpcClientEvents : public EventTypes {

src/lib/client/ServerProxy.cpp

@@ -26,6 +26,7 @@
 #include "barrier/ProtocolUtil.h"
 #include "barrier/option_types.h"
 #include "barrier/protocol_types.h"
+#include "barrier/XBarrier.h"
 #include "io/IStream.h"
 #include "base/Log.h"
 #include "base/IEventQueue.h"
@@ -124,17 +125,27 @@ ServerProxy::handleData(const Event&, void*)
 
         // parse message
         LOG((CLOG_DEBUG2 "msg from server: %c%c%c%c", code[0], code[1], code[2], code[3]));
-        switch ((this->*m_parser)(code)) {
+        try {
-        case kOkay:
+            switch ((this->*m_parser)(code)) {
-            break;
+            case kOkay:
+                break;
 
-        case kUnknown:
+            case kUnknown:
-            LOG((CLOG_ERR "invalid message from server: %c%c%c%c", code[0], code[1], code[2], code[3]));
+                LOG((CLOG_ERR "invalid message from server: %c%c%c%c", code[0], code[1], code[2], code[3]));
+                m_client->disconnect("invalid message from server");
+                return;
+
+            case kDisconnect:
+                return;
+            }
+        } catch (const XBadClient& e) {
+            // TODO: disconnect handling is currently dispersed across both parseMessage() and
+            // handleData() functions, we should collect that to a single place
+
+            LOG((CLOG_ERR "protocol error from server: %s", e.what()));
+            ProtocolUtil::writef(m_stream, kMsgEBad);
             m_client->disconnect("invalid message from server");
             return;
-
-        case kDisconnect:
-            return;
         }
 
         // next message

src/lib/net/SecureSocket.cpp

@@ -43,6 +43,7 @@
 
 #define MAX_ERROR_SIZE 65535
 
+static const std::size_t MAX_INPUT_BUFFER_SIZE = 1024 * 1024;
 static const float s_retryDelay = 0.01f;
 
 enum {
@@ -178,6 +179,10 @@ SecureSocket::doRead()
         do {
             m_inputBuffer.write(buffer, bytesRead);
 
+            if (m_inputBuffer.getSize() > MAX_INPUT_BUFFER_SIZE) {
+                break;
+            }
+
             status = secureRead(buffer, sizeof(buffer), bytesRead);
             if (status < 0) {
                 return kBreak;

src/lib/net/TCPSocket.cpp

@@ -33,9 +33,7 @@
 #include <cstdlib>
 #include <memory>
 
-//
+static const std::size_t MAX_INPUT_BUFFER_SIZE = 1024 * 1024;
-// TCPSocket
-//
 
 TCPSocket::TCPSocket(IEventQueue* events, SocketMultiplexer* socketMultiplexer, IArchNetwork::EAddressFamily family) :
     IDataSocket(events),
@@ -345,6 +343,10 @@ TCPSocket::doRead()
         do {
             m_inputBuffer.write(buffer, (UInt32)bytesRead);
 
+            if (m_inputBuffer.getSize() > MAX_INPUT_BUFFER_SIZE) {
+                break;
+            }
+
             bytesRead = ARCH->readSocket(m_socket, buffer, sizeof(buffer));
         } while (bytesRead > 0);
 

src/lib/server/ClientListener.cpp

@@ -183,7 +183,6 @@ ClientListener::handleUnknownClient(const Event&, void* vclient)
 
     // get the real client proxy and install it
     ClientProxy* client = unknownClient->orphanClientProxy();
-    bool handshakeOk = true;
     if (client != NULL) {
         // handshake was successful
         m_waitingClients.push_back(client);
@@ -196,19 +195,11 @@ ClientListener::handleUnknownClient(const Event&, void* vclient)
                                 &ClientListener::handleClientDisconnected,
                                 client));
     }
-    else {
-        handshakeOk = false;
-    }
 
     // now finished with unknown client
     m_events->removeHandler(m_events->forClientProxyUnknown().success(), client);
     m_events->removeHandler(m_events->forClientProxyUnknown().failure(), client);
     m_newClients.erase(unknownClient);
-    PacketStreamFilter* streamFileter = dynamic_cast<PacketStreamFilter*>(unknownClient->getStream());
-    IDataSocket* socket = NULL;
-    if (streamFileter != NULL) {
-        socket = dynamic_cast<IDataSocket*>(streamFileter->getStream());
-    }
 
     delete unknownClient;
 }

src/lib/server/ClientProxy1_0.cpp

@@ -51,6 +51,10 @@ ClientProxy1_0::ClientProxy1_0(const std::string& name, barrier::IStream* stream
                             stream->getEventTarget(),
                             new TMethodEventJob<ClientProxy1_0>(this,
                                 &ClientProxy1_0::handleDisconnect, NULL));
+    m_events->adoptHandler(m_events->forIStream().inputFormatError(),
+                           stream->getEventTarget(),
+                           new TMethodEventJob<ClientProxy1_0>(this,
+                                &ClientProxy1_0::handleDisconnect, NULL));
     m_events->adoptHandler(m_events->forIStream().outputShutdown(),
                             stream->getEventTarget(),
                             new TMethodEventJob<ClientProxy1_0>(this,
@@ -90,6 +94,8 @@ ClientProxy1_0::removeHandlers()
                             getStream()->getEventTarget());
     m_events->removeHandler(m_events->forIStream().outputShutdown(),
                             getStream()->getEventTarget());
+    m_events->removeHandler(m_events->forIStream().inputFormatError(),
+                            getStream()->getEventTarget());
     m_events->removeHandler(Event::kTimer, this);
 
     // remove timer
@@ -148,9 +154,18 @@ ClientProxy1_0::handleData(const Event&, void*)
         }
 
         // parse message
-        LOG((CLOG_DEBUG2 "msg from \"%s\": %c%c%c%c", getName().c_str(), code[0], code[1], code[2], code[3]));
+        try {
-        if (!(this->*m_parser)(code)) {
+            LOG((CLOG_DEBUG2 "msg from \"%s\": %c%c%c%c", getName().c_str(), code[0], code[1], code[2], code[3]));
-            LOG((CLOG_ERR "invalid message from client \"%s\": %c%c%c%c", getName().c_str(), code[0], code[1], code[2], code[3]));
+            if (!(this->*m_parser)(code)) {
+                LOG((CLOG_ERR "invalid message from client \"%s\": %c%c%c%c", getName().c_str(), code[0], code[1], code[2], code[3]));
+                disconnect();
+                return;
+            }
+        } catch (const XBadClient& e) {
+            // TODO: disconnect handling is currently dispersed across both parseMessage() and
+            // handleData() functions, we should collect that to a single place
+
+            LOG((CLOG_ERR "protocol error from client: %s", e.what()));
             disconnect();
             return;
         }
@@ -173,6 +188,8 @@ ClientProxy1_0::parseHandshakeMessage(const UInt8* code)
     }
     else if (memcmp(code, kMsgDInfo, 4) == 0) {
         // future messages get parsed by parseMessage
+        // NOTE: we're taking address of virtual function here,
+        // not ClientProxy1_0 implementation of it.
         m_parser = &ClientProxy1_0::parseMessage;
         if (recvInfo()) {
             m_events->addEvent(Event(m_events->forClientProxy().ready(), getEventTarget()));

src/lib/server/ClientProxyUnknown.cpp

@@ -118,6 +118,10 @@ ClientProxyUnknown::addStreamHandlers()
                             m_stream->getEventTarget(),
                             new TMethodEventJob<ClientProxyUnknown>(this,
                                 &ClientProxyUnknown::handleDisconnect));
+    m_events->adoptHandler(m_events->forIStream().inputFormatError(),
+                           m_stream->getEventTarget(),
+                           new TMethodEventJob<ClientProxyUnknown>(this,
+                                &ClientProxyUnknown::handleDisconnect));
     m_events->adoptHandler(m_events->forIStream().outputShutdown(),
                             m_stream->getEventTarget(),
                             new TMethodEventJob<ClientProxyUnknown>(this,
@@ -149,6 +153,8 @@ ClientProxyUnknown::removeHandlers()
                             m_stream->getEventTarget());
         m_events->removeHandler(m_events->forIStream().inputShutdown(),
                             m_stream->getEventTarget());
+        m_events->removeHandler(m_events->forIStream().inputFormatError(),
+                                m_stream->getEventTarget());
         m_events->removeHandler(m_events->forIStream().outputShutdown(),
                             m_stream->getEventTarget());
     }