From 45cd2a9f34272ed817fcb93c728b631261ac88cb Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 05:18:53 +0200 Subject: [PATCH] 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 . (cherry picked from commit fd5295eb3199e7693c67b4b7116f47f50f876ef5) --- 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 16f0fe76..b6befd66 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 bcbd604b..e6f1a37d 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 5d6bd762..6acee26f 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 2ba20778..7a41ded2 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 f81617e0..148fa2c8 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 a688431e..079d2283 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 dc79da7d..de6e233e 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()); }