From f546af4a8521cca7c60fbbf81a03ef95ae9cb089 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 05:18:51 +0200 Subject: [PATCH] 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 . (cherry picked from commit e33c81b835e14ca1534e3f375c17707f10a29152) --- .../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 e742687f..c1be0321 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 bc5e0377..5d6bd762 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 c067f132..1e5c339b 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 5cbaac2a..a688431e 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; }