Be defensive about overflows in stream objects

We use a lot of lengths given to us over the network, so be more
paranoid about them causing an overflow as otherwise an attacker
might trick us in to overwriting other memory.

This primarily affects the client which often gets lengths from the
server, but there are also some scenarios where the server might
theoretically be vulnerable.

Issue found by Pavel Cheremushkin from Kaspersky Lab.

diff --git a/common/rdr/FdInStream.cxx b/common/rdr/FdInStream.cxx
index 9e84ab7aaefc12e5c8b5d948d8ac1da996c9241c..1730d6d12eebc4eeac0360ff7179ff54815e467b 100644 (file)
--- a/common/rdr/FdInStream.cxx
+++ b/common/rdr/FdInStream.cxx
@@ -136,7 +136,7 @@ size_t FdInStream::overrun(size_t itemSize, size_t nItems, bool wait)
   ptr = start;
 
   size_t bytes_to_read;
-  while (end < start + itemSize) {
+  while ((size_t)(end - start) < itemSize) {
     bytes_to_read = start + bufSize - end;
     if (!timing) {
       // When not timing, we must be careful not to read too much
@@ -152,8 +152,10 @@ size_t FdInStream::overrun(size_t itemSize, size_t nItems, bool wait)
     end += n;
   }
 
-  if (itemSize * nItems > (size_t)(end - ptr))
-    nItems = (end - ptr) / itemSize;
+  size_t nAvail;
+  nAvail = (end - ptr) / itemSize;
+  if (nAvail < nItems)
+    return nAvail;
 
   return nItems;
 }

diff --git a/common/rdr/FdOutStream.cxx b/common/rdr/FdOutStream.cxx
index 1757dc35d0f420ba7e16406ed614385e84ff8918..f5d07e4bb6428793bbfac31c051bec83e5bfe497 100644 (file)
--- a/common/rdr/FdOutStream.cxx
+++ b/common/rdr/FdOutStream.cxx
@@ -149,9 +149,10 @@ size_t FdOutStream::overrun(size_t itemSize, size_t nItems)
     }
   }
 
-  // Can we fit all the items asked for?
-  if (itemSize * nItems > (size_t)(end - ptr))
-    nItems = (end - ptr) / itemSize;
+  size_t nAvail;
+  nAvail = (end - ptr) / itemSize;
+  if (nAvail < nItems)
+    return nAvail;
 
   return nItems;
 }

diff --git a/common/rdr/FileInStream.cxx b/common/rdr/FileInStream.cxx
index 94f5db882dd1e1c3063653613f6d83809f97533a..bdb05a3aae64834f1b9a924f6c359895ec79e99b 100644 (file)
--- a/common/rdr/FileInStream.cxx
+++ b/common/rdr/FileInStream.cxx
@@ -68,7 +68,7 @@ size_t FileInStream::overrun(size_t itemSize, size_t nItems, bool wait)
   ptr = b;
 
 
-  while (end < b + itemSize) {
+  while ((size_t)(end - b) < itemSize) {
     size_t n = fread((U8 *)end, b + sizeof(b) - end, 1, file);
     if (n == 0) {
       if (ferror(file))
@@ -80,8 +80,10 @@ size_t FileInStream::overrun(size_t itemSize, size_t nItems, bool wait)
     end += b + sizeof(b) - end;
   }
 
-  if (itemSize * nItems > (size_t)(end - ptr))
-    nItems = (end - ptr) / itemSize;
+  size_t nAvail;
+  nAvail = (end - ptr) / itemSize;
+  if (nAvail < nItems)
+    return nAvail;
 
   return nItems;
 }

diff --git a/common/rdr/HexInStream.cxx b/common/rdr/HexInStream.cxx
index 8f939889b6e53524399e95e654308f91ffa3be31..a6bc92cd198c2fe822f908a56fa7f3ccf6e681e4 100644 (file)
--- a/common/rdr/HexInStream.cxx
+++ b/common/rdr/HexInStream.cxx
@@ -91,7 +91,7 @@ size_t HexInStream::overrun(size_t itemSize, size_t nItems, bool wait) {
   offset += ptr - start;
   ptr = start;
 
-  while (end < ptr + itemSize) {
+  while ((size_t)(end - ptr) < itemSize) {
     size_t n = in_stream.check(2, 1, wait);
     if (n == 0) return 0;
     const U8* iptr = in_stream.getptr();
@@ -110,8 +110,10 @@ size_t HexInStream::overrun(size_t itemSize, size_t nItems, bool wait) {
     end += length;
   }
 
-  if (itemSize * nItems > (size_t)(end - ptr))
-    nItems = (end - ptr) / itemSize;
+  size_t nAvail;
+  nAvail = (end - ptr) / itemSize;
+  if (nAvail < nItems)
+    return nAvail;
 
   return nItems;
 }

diff --git a/common/rdr/HexOutStream.cxx b/common/rdr/HexOutStream.cxx
index 7232514c5995c3847e5050459d38678639a9b088..eac2eff803636eb18126bd5a235eca0700a39c5c 100644 (file)
--- a/common/rdr/HexOutStream.cxx
+++ b/common/rdr/HexOutStream.cxx
@@ -102,8 +102,10 @@ HexOutStream::overrun(size_t itemSize, size_t nItems) {
 
   writeBuffer();
 
-  if (itemSize * nItems > (size_t)(end - ptr))
-    nItems = (end - ptr) / itemSize;
+  size_t nAvail;
+  nAvail = (end - ptr) / itemSize;
+  if (nAvail < nItems)
+    return nAvail;
 
   return nItems;
 }

diff --git a/common/rdr/InStream.h b/common/rdr/InStream.h
index 14ecf0938b9eb0bc18129ee2a2df023a702f119a..f71a4d9e35a189deafdf849714c653b4586eee00 100644 (file)
--- a/common/rdr/InStream.h
+++ b/common/rdr/InStream.h
@@ -43,12 +43,15 @@ namespace rdr {
 
     inline size_t check(size_t itemSize, size_t nItems=1, bool wait=true)
     {
-      if (ptr + itemSize * nItems > end) {
-        if (ptr + itemSize > end)
-          return overrun(itemSize, nItems, wait);
+      size_t nAvail;
+
+      if (itemSize > (size_t)(end - ptr))
+        return overrun(itemSize, nItems, wait);
+
+      nAvail = (end - ptr) / itemSize;
+      if (nAvail < nItems)
+        return nAvail;
 
-        nItems = (end - ptr) / itemSize;
-      }
       return nItems;
     }
 
@@ -93,13 +96,12 @@ namespace rdr {
     // readBytes() reads an exact number of bytes.
 
     void readBytes(void* data, size_t length) {
-      U8* dataPtr = (U8*)data;
-      U8* dataEnd = dataPtr + length;
-      while (dataPtr < dataEnd) {
-        size_t n = check(1, dataEnd - dataPtr);
-        memcpy(dataPtr, ptr, n);
+      while (length > 0) {
+        size_t n = check(1, length);
+        memcpy(data, ptr, n);
         ptr += n;
-        dataPtr += n;
+        data = (U8*)data + n;
+        length -= n;
       }
     }
 

diff --git a/common/rdr/MemOutStream.h b/common/rdr/MemOutStream.h
index 4a815b300accde9b34bd1ef3b40cb8d6cd5626d9..b56bac3affd93efec72f6ff44f2732f114d0c371 100644 (file)
--- a/common/rdr/MemOutStream.h
+++ b/common/rdr/MemOutStream.h
@@ -23,6 +23,7 @@
 #ifndef __RDR_MEMOUTSTREAM_H__
 #define __RDR_MEMOUTSTREAM_H__
 
+#include <rdr/Exception.h>
 #include <rdr/OutStream.h>
 
 namespace rdr {
@@ -65,6 +66,9 @@ namespace rdr {
       if (len < (size_t)(end - start) * 2)
         len = (end - start) * 2;
 
+      if (len < (size_t)(end - start))
+        throw Exception("Overflow in MemOutStream::overrun()");
+
       U8* newStart = new U8[len];
       memcpy(newStart, start, ptr - start);
       ptr = newStart + (ptr - start);

diff --git a/common/rdr/OutStream.h b/common/rdr/OutStream.h
index 11aafd2d96077e9d378c54a6c157aa43f52acdb5..0f60ccc19cf7fd469eace5cd42395b7ae19d735c 100644 (file)
--- a/common/rdr/OutStream.h
+++ b/common/rdr/OutStream.h
@@ -46,12 +46,15 @@ namespace rdr {
 
     inline size_t check(size_t itemSize, size_t nItems=1)
     {
-      if (ptr + itemSize * nItems > end) {
-        if (ptr + itemSize > end)
-          return overrun(itemSize, nItems);
+      size_t nAvail;
+
+      if (itemSize > (size_t)(end - ptr))
+        return overrun(itemSize, nItems);
+
+      nAvail = (end - ptr) / itemSize;
+      if (nAvail < nItems)
+        return nAvail;
 
-        nItems = (end - ptr) / itemSize;
-      }
       return nItems;
     }
 
@@ -91,13 +94,12 @@ namespace rdr {
     // writeBytes() writes an exact number of bytes.
 
     void writeBytes(const void* data, size_t length) {
-      const U8* dataPtr = (const U8*)data;
-      const U8* dataEnd = dataPtr + length;
-      while (dataPtr < dataEnd) {
-        size_t n = check(1, dataEnd - dataPtr);
-        memcpy(ptr, dataPtr, n);
+      while (length > 0) {
+        size_t n = check(1, length);
+        memcpy(ptr, data, n);
         ptr += n;
-        dataPtr += n;
+        data = (U8*)data + n;
+        length -= n;
       }
     }
 

diff --git a/common/rdr/RandomStream.cxx b/common/rdr/RandomStream.cxx
index d5f1cc8539b15cd0dbc935616bdd2e0a636f60f7..1be9b251c395327f9128f8dac7434790e56ffdd6 100644 (file)
--- a/common/rdr/RandomStream.cxx
+++ b/common/rdr/RandomStream.cxx
@@ -126,8 +126,10 @@ size_t RandomStream::overrun(size_t itemSize, size_t nItems, bool wait) {
       *(U8*)end++ = (int) (256.0*rand()/(RAND_MAX+1.0));
   }
 
-  if (itemSize * nItems > (size_t)(end - ptr))
-    nItems = (end - ptr) / itemSize;
+  size_t nAvail;
+  nAvail = (end - ptr) / itemSize;
+  if (nAvail < nItems)
+    return nAvail;
 
   return nItems;
 }

diff --git a/common/rdr/TLSInStream.cxx b/common/rdr/TLSInStream.cxx
index d0f94263c2e1702df8ada33a918e9fda67d5a5ad..3e1172f18798a8de8f8e1082ecc6d2c06a1e68f5 100644 (file)
--- a/common/rdr/TLSInStream.cxx
+++ b/common/rdr/TLSInStream.cxx
@@ -43,7 +43,7 @@ ssize_t TLSInStream::pull(gnutls_transport_ptr_t str, void* data, size_t size)
       return -1;
     }
 
-    if (in->getend() - in->getptr() < (ptrdiff_t)size)
+    if ((size_t)(in->getend() - in->getptr()) < size)
       size = in->getend() - in->getptr();
   
     in->readBytes(data, size);
@@ -92,15 +92,17 @@ size_t TLSInStream::overrun(size_t itemSize, size_t nItems, bool wait)
   end -= ptr - start;
   ptr = start;
 
-  while (end < start + itemSize) {
+  while ((size_t)(end - start) < itemSize) {
     size_t n = readTLS((U8*) end, start + bufSize - end, wait);
     if (!wait && n == 0)
       return 0;
     end += n;
   }
 
-  if (itemSize * nItems > (size_t)(end - ptr))
-    nItems = (end - ptr) / itemSize;
+  size_t nAvail;
+  nAvail = (end - ptr) / itemSize;
+  if (nAvail < nItems)
+    return nAvail;
 
   return nItems;
 }

diff --git a/common/rdr/TLSOutStream.cxx b/common/rdr/TLSOutStream.cxx
index 30c456fe060f996963c1b6ce3869b526eda0be35..7d7c3b5648b11856eb1bad22376b6c063c2d67d9 100644 (file)
--- a/common/rdr/TLSOutStream.cxx
+++ b/common/rdr/TLSOutStream.cxx
@@ -100,8 +100,10 @@ size_t TLSOutStream::overrun(size_t itemSize, size_t nItems)
 
   flush();
 
-  if (itemSize * nItems > (size_t)(end - ptr))
-    nItems = (end - ptr) / itemSize;
+  size_t nAvail;
+  nAvail = (end - ptr) / itemSize;
+  if (nAvail < nItems)
+    return nAvail;
 
   return nItems;
 }

diff --git a/common/rdr/ZlibInStream.cxx b/common/rdr/ZlibInStream.cxx
index e2f971c7d6d05d1c649aee729b967de7a419d110..9fcfaf6b7bf6128180a129ad6726c5ee617bad66 100644 (file)
--- a/common/rdr/ZlibInStream.cxx
+++ b/common/rdr/ZlibInStream.cxx
@@ -113,8 +113,10 @@ size_t ZlibInStream::overrun(size_t itemSize, size_t nItems, bool wait)
       return 0;
   }
 
-  if (itemSize * nItems > (size_t)(end - ptr))
-    nItems = (end - ptr) / itemSize;
+  size_t nAvail;
+  nAvail = (end - ptr) / itemSize;
+  if (nAvail < nItems)
+    return nAvail;
 
   return nItems;
 }

diff --git a/common/rdr/ZlibOutStream.cxx b/common/rdr/ZlibOutStream.cxx
index 26a113159c499a96081031d49180893d85822ba4..7a0d692c02f00868b01b3b766adb96bbf80f9c14 100644 (file)
--- a/common/rdr/ZlibOutStream.cxx
+++ b/common/rdr/ZlibOutStream.cxx
@@ -130,8 +130,10 @@ size_t ZlibOutStream::overrun(size_t itemSize, size_t nItems)
     }
   }
 
-  if (itemSize * nItems > (size_t)(end - ptr))
-    nItems = (end - ptr) / itemSize;
+  size_t nAvail;
+  nAvail = (end - ptr) / itemSize;
+  if (nAvail < nItems)
+    return nAvail;
 
   return nItems;
 }