From bf3bdac082978ca32895a4b6a123016094905689 Mon Sep 17 00:00:00 2001 From: Michal Srb Date: Mon, 27 Mar 2017 13:37:11 +0300 Subject: Fix crash from integer overflow in SMsgReader::readClientCutText The length sent by client is U32, but is converted into int. If it was bigger than 0x7fffffff the resulting int is negative, it passes the check against maxCutText and later throws std::bad_alloc from CharArray which takes down the whole server. All the Streaming API deals with lengths in ints, so we can't tell it to skip that big amount of data. And it is not realistic to expect more than 2GB of clipboard data anyway. So lets just throw rdr::Exception that will disconnect this client and keep the server alive. --- common/rfb/SMsgReader.cxx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/rfb/SMsgReader.cxx b/common/rfb/SMsgReader.cxx index 89c9a8fd..3c08fd6f 100644 --- a/common/rfb/SMsgReader.cxx +++ b/common/rfb/SMsgReader.cxx @@ -200,6 +200,9 @@ void SMsgReader::readClientCutText() { is->skip(3); int len = is->readU32(); + if (len < 0) { + throw Exception("Cut text too long."); + } if (len > maxCutText) { is->skip(len); vlog.error("Cut text too long (%d bytes) - ignoring", len); -- cgit v1.2.3 From dccb5f7d776e93863ae10bbff56a45c523c6eeb0 Mon Sep 17 00:00:00 2001 From: Michal Srb Date: Mon, 27 Mar 2017 13:55:46 +0300 Subject: Prevent leak of SecurityServer and ClientServer. They are created in SConnection's and CConnection's constructors but never destroyed. There is no reason for the indirection, so lets make them direct members. --- common/rfb/CConnection.cxx | 5 ++--- common/rfb/CConnection.h | 4 ++-- common/rfb/SConnection.cxx | 12 +++++------- common/rfb/SConnection.h | 2 +- 4 files changed, 10 insertions(+), 13 deletions(-) diff --git a/common/rfb/CConnection.cxx b/common/rfb/CConnection.cxx index 20204181..88befd5e 100644 --- a/common/rfb/CConnection.cxx +++ b/common/rfb/CConnection.cxx @@ -44,7 +44,6 @@ CConnection::CConnection() state_(RFBSTATE_UNINITIALISED), useProtocol3_3(false), framebuffer(NULL), decoder(this) { - security = new SecurityClient(); } CConnection::~CConnection() @@ -167,7 +166,7 @@ void CConnection::processSecurityTypesMsg() int secType = secTypeInvalid; std::list secTypes; - secTypes = security->GetEnabledSecTypes(); + secTypes = security.GetEnabledSecTypes(); if (cp.isVersion(3,3)) { @@ -235,7 +234,7 @@ void CConnection::processSecurityTypesMsg() } state_ = RFBSTATE_SECURITY; - csecurity = security->GetCSecurity(secType); + csecurity = security.GetCSecurity(secType); processSecurityMsg(); } diff --git a/common/rfb/CConnection.h b/common/rfb/CConnection.h index 799a9c21..e0a000ff 100644 --- a/common/rfb/CConnection.h +++ b/common/rfb/CConnection.h @@ -26,6 +26,7 @@ #include #include +#include #include namespace rfb { @@ -34,7 +35,6 @@ namespace rfb { class CMsgWriter; class CSecurity; class IdentityVerifier; - class SecurityClient; class CConnection : public CMsgHandler { public: @@ -148,7 +148,7 @@ namespace rfb { stateEnum state() { return state_; } CSecurity *csecurity; - SecurityClient *security; + SecurityClient security; protected: void setState(stateEnum s) { state_ = s; } diff --git a/common/rfb/SConnection.cxx b/common/rfb/SConnection.cxx index 17ef4d90..85cc6e82 100644 --- a/common/rfb/SConnection.cxx +++ b/common/rfb/SConnection.cxx @@ -51,7 +51,7 @@ const SConnection::AccessRights SConnection::AccessFull = 0xffff; SConnection::SConnection() : readyForSetColourMapEntries(false), is(0), os(0), reader_(0), writer_(0), - security(0), ssecurity(0), state_(RFBSTATE_UNINITIALISED), + ssecurity(0), state_(RFBSTATE_UNINITIALISED), preferredEncoding(encodingRaw) { defaultMajorVersion = 3; @@ -60,8 +60,6 @@ SConnection::SConnection() defaultMinorVersion = 3; cp.setVersion(defaultMajorVersion, defaultMinorVersion); - - security = new SecurityServer(); } SConnection::~SConnection() @@ -142,7 +140,7 @@ void SConnection::processVersionMsg() std::list secTypes; std::list::iterator i; - secTypes = security->GetEnabledSecTypes(); + secTypes = security.GetEnabledSecTypes(); if (cp.isVersion(3,3)) { @@ -161,7 +159,7 @@ void SConnection::processVersionMsg() os->writeU32(*i); if (*i == secTypeNone) os->flush(); state_ = RFBSTATE_SECURITY; - ssecurity = security->GetSSecurity(*i); + ssecurity = security.GetSSecurity(*i); processSecurityMsg(); return; } @@ -193,7 +191,7 @@ void SConnection::processSecurityType(int secType) std::list secTypes; std::list::iterator i; - secTypes = security->GetEnabledSecTypes(); + secTypes = security.GetEnabledSecTypes(); for (i=secTypes.begin(); i!=secTypes.end(); i++) if (*i == secType) break; if (i == secTypes.end()) @@ -204,7 +202,7 @@ void SConnection::processSecurityType(int secType) try { state_ = RFBSTATE_SECURITY; - ssecurity = security->GetSSecurity(secType); + ssecurity = security.GetSSecurity(secType); } catch (rdr::Exception& e) { throwConnFailedException(e.str()); } diff --git a/common/rfb/SConnection.h b/common/rfb/SConnection.h index b43cf086..63dc3146 100644 --- a/common/rfb/SConnection.h +++ b/common/rfb/SConnection.h @@ -196,7 +196,7 @@ namespace rfb { rdr::OutStream* os; SMsgReader* reader_; SMsgWriter* writer_; - SecurityServer *security; + SecurityServer security; SSecurity* ssecurity; stateEnum state_; rdr::S32 preferredEncoding; -- cgit v1.2.3 From f3afa24da144409a3c3a0e35913112583d987671 Mon Sep 17 00:00:00 2001 From: Michal Srb Date: Mon, 27 Mar 2017 19:02:15 +0300 Subject: Prevent double free by crafted fences. If client sent fence with some data, followed by fence with no data (length 0), the original fence data were freed, but the pointer kept pointing at them. Sending one more fence would attempt to free them again. --- common/rfb/SMsgWriter.cxx | 4 +++- common/rfb/VNCSConnectionST.cxx | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/common/rfb/SMsgWriter.cxx b/common/rfb/SMsgWriter.cxx index cf3264e8..bc3f4398 100644 --- a/common/rfb/SMsgWriter.cxx +++ b/common/rfb/SMsgWriter.cxx @@ -101,7 +101,9 @@ void SMsgWriter::writeFence(rdr::U32 flags, unsigned len, const char data[]) os->writeU32(flags); os->writeU8(len); - os->writeBytes(data, len); + + if (len > 0) + os->writeBytes(data, len); endMsg(); } diff --git a/common/rfb/VNCSConnectionST.cxx b/common/rfb/VNCSConnectionST.cxx index 0a2ca334..d2206f9b 100644 --- a/common/rfb/VNCSConnectionST.cxx +++ b/common/rfb/VNCSConnectionST.cxx @@ -666,6 +666,7 @@ void VNCSConnectionST::fence(rdr::U32 flags, unsigned len, const char data[]) fenceFlags = flags & (fenceFlagBlockBefore | fenceFlagBlockAfter | fenceFlagSyncNext); fenceDataLen = len; delete [] fenceData; + fenceData = NULL; if (len > 0) { fenceData = new char[len]; memcpy(fenceData, data, len); -- cgit v1.2.3