Make ZlibInStream more robust against failures

Move the checks around to avoid missing cases where we might access
memory that is no longer valid. Also avoid touching the underlying
stream implicitly (e.g. via the destructor) as it might also no
longer be valid.

A malicious server could theoretically use this for remote code
execution in the client.

Issue found by Pavel Cheremushkin from Kaspersky Lab

diff --git a/common/rdr/ZlibInStream.cxx b/common/rdr/ZlibInStream.cxx
index 4053bd1918ae2447d42b1da5491aefaea492c37a..a361010cf024cb047a08cfa4582ae0edd4e622e5 100644 (file)
--- a/common/rdr/ZlibInStream.cxx
+++ b/common/rdr/ZlibInStream.cxx
@@ -52,16 +52,16 @@ int ZlibInStream::pos()
   return offset + ptr - start;
 }
 
-void ZlibInStream::removeUnderlying()
+void ZlibInStream::flushUnderlying()
 {
   ptr = end = start;
-  if (!underlying) return;
 
   while (bytesIn > 0) {
     decompress(true);
     end = start; // throw away any data
   }
-  underlying = 0;
+
+  setUnderlying(NULL, 0);
 }
 
 void ZlibInStream::reset()
@@ -90,7 +90,7 @@ void ZlibInStream::init()
 void ZlibInStream::deinit()
 {
   assert(zs != NULL);
-  removeUnderlying();
+  setUnderlying(NULL, 0);
   inflateEnd(zs);
   delete zs;
   zs = NULL;
@@ -100,8 +100,6 @@ int ZlibInStream::overrun(int itemSize, int nItems, bool wait)
 {
   if (itemSize > bufSize)
     throw Exception("ZlibInStream overrun: max itemSize exceeded");
-  if (!underlying)
-    throw Exception("ZlibInStream overrun: no underlying stream");
 
   if (end - ptr != 0)
     memmove(start, ptr, end - ptr);
@@ -127,6 +125,9 @@ int ZlibInStream::overrun(int itemSize, int nItems, bool wait)
 
 bool ZlibInStream::decompress(bool wait)
 {
+  if (!underlying)
+    throw Exception("ZlibInStream overrun: no underlying stream");
+
   zs->next_out = (U8*)end;
   zs->avail_out = start + bufSize - end;
 

diff --git a/common/rdr/ZlibInStream.h b/common/rdr/ZlibInStream.h
index 6bd4da4c651fd9b2132f76396cf5e3b2dfe2fb5a..86ba1ff14e248707822df6ae4b33dad95c65b53a 100644 (file)
--- a/common/rdr/ZlibInStream.h
+++ b/common/rdr/ZlibInStream.h
@@ -38,7 +38,7 @@ namespace rdr {
     virtual ~ZlibInStream();
 
     void setUnderlying(InStream* is, int bytesIn);
-    void removeUnderlying();
+    void flushUnderlying();
     int pos();
     void reset();
 

diff --git a/common/rfb/CMsgReader.cxx b/common/rfb/CMsgReader.cxx
index a9e12d703626251655fd6af2e50af570b5e4d6b2..52d40ce7ae89f6e8c80357171ee1d8dbbb1bc3b1 100644 (file)
--- a/common/rfb/CMsgReader.cxx
+++ b/common/rfb/CMsgReader.cxx
@@ -242,7 +242,8 @@ void CMsgReader::readExtendedClipboard(rdr::S32 len)
       num++;
     }
 
-    zis.removeUnderlying();
+    zis.flushUnderlying();
+    zis.setUnderlying(NULL, 0);
 
     handler->handleClipboardProvide(flags, lengths, buffers);
 

diff --git a/common/rfb/SMsgReader.cxx b/common/rfb/SMsgReader.cxx
index ab42e59a66306042f581c96b4584a2f753957152..dc7ddea6b8d4558a595adab7d508e86d1b7b3842 100644 (file)
--- a/common/rfb/SMsgReader.cxx
+++ b/common/rfb/SMsgReader.cxx
@@ -293,7 +293,8 @@ void SMsgReader::readExtendedClipboard(rdr::S32 len)
       num++;
     }
 
-    zis.removeUnderlying();
+    zis.flushUnderlying();
+    zis.setUnderlying(NULL, 0);
 
     handler->handleClipboardProvide(flags, lengths, buffers);
 

diff --git a/common/rfb/TightDecoder.cxx b/common/rfb/TightDecoder.cxx
index 5b7c553d3c8d00dd5725c11272c1686b19bca94a..ebc98b06ec5219ce04bd96400d3b311b47c176a2 100644 (file)
--- a/common/rfb/TightDecoder.cxx
+++ b/common/rfb/TightDecoder.cxx
@@ -341,7 +341,8 @@ void TightDecoder::decodeRect(const Rect& r, const void* buffer,
 
     zis[streamId].readBytes(netbuf, dataSize);
 
-    zis[streamId].removeUnderlying();
+    zis[streamId].flushUnderlying();
+    zis[streamId].setUnderlying(NULL, 0);
     delete ms;
 
     bufptr = netbuf;

diff --git a/common/rfb/zrleDecode.h b/common/rfb/zrleDecode.h
index 32b5c92b20a19329552a0059abb0b7e27b12605e..f4325385c812ccca5bca9998fa76a49fffc127f8 100644 (file)
--- a/common/rfb/zrleDecode.h
+++ b/common/rfb/zrleDecode.h
@@ -174,7 +174,8 @@ void ZRLE_DECODE (const Rect& r, rdr::InStream* is,
     }
   }
 
-  zis->removeUnderlying();
+  zis->flushUnderlying();
+  zis->setUnderlying(NULL, 0);
 }
 
 #undef ZRLE_DECODE