From 7ab8e0101da31237adc7cc50dabe8a441a0ad91c Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 05:18:49 +0200 Subject: [PATCH 1/5] lib/server: Add a note about taking pointer to virtual member function --- src/lib/server/ClientProxy1_0.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/lib/server/ClientProxy1_0.cpp b/src/lib/server/ClientProxy1_0.cpp index 436e366e..ee7d32f0 100644 --- a/src/lib/server/ClientProxy1_0.cpp +++ b/src/lib/server/ClientProxy1_0.cpp @@ -173,6 +173,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())); From cc369820d45166a93972ecd0faf3aa1049a09b5a Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 05:18:50 +0200 Subject: [PATCH 2/5] lib/server: Remove unused code --- src/lib/server/ClientListener.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/lib/server/ClientListener.cpp b/src/lib/server/ClientListener.cpp index a9b3a700..f2fec8b8 100644 --- a/src/lib/server/ClientListener.cpp +++ b/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(unknownClient->getStream()); - IDataSocket* socket = NULL; - if (streamFileter != NULL) { - socket = dynamic_cast(streamFileter->getStream()); - } delete unknownClient; } From e33c81b835e14ca1534e3f375c17707f10a29152 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 05:18:51 +0200 Subject: [PATCH 3/5] lib: Enforce a maximum length of input messages This commit is the 1/3 part of the fix for the following security vulnerability: - CVE-2021-42076 DoS via excess length messages The issue has been reported by Matthias Gerstner . --- .../enforce-maximum-message-length.bugfix | 6 +++++ src/lib/barrier/ProtocolUtil.cpp | 10 +++++++ src/lib/barrier/protocol_types.h | 5 ++++ src/lib/client/ServerProxy.cpp | 27 +++++++++++++------ src/lib/server/ClientProxy1_0.cpp | 15 ++++++++--- 5 files changed, 52 insertions(+), 11 deletions(-) create mode 100644 doc/newsfragments/enforce-maximum-message-length.bugfix diff --git a/doc/newsfragments/enforce-maximum-message-length.bugfix b/doc/newsfragments/enforce-maximum-message-length.bugfix new file mode 100644 index 00000000..81ec2ba0 --- /dev/null +++ b/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. diff --git a/src/lib/barrier/ProtocolUtil.cpp b/src/lib/barrier/ProtocolUtil.cpp index 6e67b1b6..5a71010d 100644 --- a/src/lib/barrier/ProtocolUtil.cpp +++ b/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(buffer[2]) << 8) | static_cast(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(buffer[2]) << 8) | static_cast(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)); diff --git a/src/lib/barrier/protocol_types.h b/src/lib/barrier/protocol_types.h index ccc1c3ea..188aad4a 100644 --- a/src/lib/barrier/protocol_types.h +++ b/src/lib/barrier/protocol_types.h @@ -51,6 +51,11 @@ 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_LIST_LENGTH = 1024 * 1024; +static constexpr std::uint32_t PROTOCOL_MAX_STRING_LENGTH = 1024 * 1024; + // direction constants enum EDirection { kNoDirection, diff --git a/src/lib/client/ServerProxy.cpp b/src/lib/client/ServerProxy.cpp index ce65c72d..c6e35766 100644 --- a/src/lib/client/ServerProxy.cpp +++ b/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)) { - case kOkay: - break; + try { + switch ((this->*m_parser)(code)) { + case kOkay: + break; - case kUnknown: - LOG((CLOG_ERR "invalid message from server: %c%c%c%c", code[0], code[1], code[2], code[3])); + case kUnknown: + 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 diff --git a/src/lib/server/ClientProxy1_0.cpp b/src/lib/server/ClientProxy1_0.cpp index ee7d32f0..af927f9d 100644 --- a/src/lib/server/ClientProxy1_0.cpp +++ b/src/lib/server/ClientProxy1_0.cpp @@ -148,9 +148,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])); - 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])); + try { + LOG((CLOG_DEBUG2 "msg from \"%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; } From af90f39b4a00acf12def036b7ec4362b4c3404c7 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 05:18:52 +0200 Subject: [PATCH 4/5] lib/net: Limit the maximum size of TCP or SSL input buffers This commit is the 2/3 part of the fix for the following security vulnerability: - CVE-2021-42076 DoS via excess length messages The issue has been reported by Matthias Gerstner . --- src/lib/net/SecureSocket.cpp | 5 +++++ src/lib/net/TCPSocket.cpp | 8 +++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/lib/net/SecureSocket.cpp b/src/lib/net/SecureSocket.cpp index f096729d..af5a795a 100644 --- a/src/lib/net/SecureSocket.cpp +++ b/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; diff --git a/src/lib/net/TCPSocket.cpp b/src/lib/net/TCPSocket.cpp index e19786c8..002d4d67 100644 --- a/src/lib/net/TCPSocket.cpp +++ b/src/lib/net/TCPSocket.cpp @@ -33,9 +33,7 @@ #include #include -// -// TCPSocket -// +static const std::size_t MAX_INPUT_BUFFER_SIZE = 1024 * 1024; 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); From fd5295eb3199e7693c67b4b7116f47f50f876ef5 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 05:18:53 +0200 Subject: [PATCH 5/5] lib/barrier: Disconnect client on too long input packets This commit is the 3/3 part of the fix for the following security vulnerability: - CVE-2021-42076 DoS via excess length messages The issue has been reported by Matthias Gerstner . --- src/lib/barrier/PacketStreamFilter.cpp | 22 ++++++++++++++++------ src/lib/barrier/PacketStreamFilter.h | 4 +++- src/lib/barrier/protocol_types.h | 3 +++ src/lib/base/EventTypes.cpp | 1 + src/lib/base/EventTypes.h | 6 ++++++ src/lib/server/ClientProxy1_0.cpp | 6 ++++++ src/lib/server/ClientProxyUnknown.cpp | 6 ++++++ 7 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/lib/barrier/PacketStreamFilter.cpp b/src/lib/barrier/PacketStreamFilter.cpp index 85a85f22..5955b6c2 100644 --- a/src/lib/barrier/PacketStreamFilter.cpp +++ b/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 -PacketStreamFilter::readPacketSize() +bool 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(); diff --git a/src/lib/barrier/PacketStreamFilter.h b/src/lib/barrier/PacketStreamFilter.h index 3bccafe0..0c4bb04e 100644 --- a/src/lib/barrier/PacketStreamFilter.h +++ b/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: diff --git a/src/lib/barrier/protocol_types.h b/src/lib/barrier/protocol_types.h index 188aad4a..f7306067 100644 --- a/src/lib/barrier/protocol_types.h +++ b/src/lib/barrier/protocol_types.h @@ -20,6 +20,8 @@ #include "base/EventTypes.h" +#include + // protocol version number // 1.0: initial protocol // 1.1: adds KeyCode to key press, release, and repeat @@ -53,6 +55,7 @@ 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; diff --git a/src/lib/base/EventTypes.cpp b/src/lib/base/EventTypes.cpp index d54a2d50..173a0a97 100644 --- a/src/lib/base/EventTypes.cpp +++ b/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 diff --git a/src/lib/base/EventTypes.h b/src/lib/base/EventTypes.h index 8b02bef4..995490e9 100644 --- a/src/lib/base/EventTypes.h +++ b/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 { diff --git a/src/lib/server/ClientProxy1_0.cpp b/src/lib/server/ClientProxy1_0.cpp index af927f9d..33b0f159 100644 --- a/src/lib/server/ClientProxy1_0.cpp +++ b/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(this, &ClientProxy1_0::handleDisconnect, NULL)); + m_events->adoptHandler(m_events->forIStream().inputFormatError(), + stream->getEventTarget(), + new TMethodEventJob(this, + &ClientProxy1_0::handleDisconnect, NULL)); m_events->adoptHandler(m_events->forIStream().outputShutdown(), stream->getEventTarget(), new TMethodEventJob(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 diff --git a/src/lib/server/ClientProxyUnknown.cpp b/src/lib/server/ClientProxyUnknown.cpp index 50a6bdec..f9da361d 100644 --- a/src/lib/server/ClientProxyUnknown.cpp +++ b/src/lib/server/ClientProxyUnknown.cpp @@ -118,6 +118,10 @@ ClientProxyUnknown::addStreamHandlers() m_stream->getEventTarget(), new TMethodEventJob(this, &ClientProxyUnknown::handleDisconnect)); + m_events->adoptHandler(m_events->forIStream().inputFormatError(), + m_stream->getEventTarget(), + new TMethodEventJob(this, + &ClientProxyUnknown::handleDisconnect)); m_events->adoptHandler(m_events->forIStream().outputShutdown(), m_stream->getEventTarget(), new TMethodEventJob(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()); }