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; }