Treat truncated reads as errors in wxDataInputStream

Reading from a truncated stream returned values built from partially
or completely uninitialised buffers, e.g. ReadString() decoded the
unread tail of its temporary buffer and the fixed size readers returned
whatever happened to be on the stack. Check that all the requested bytes
were actually read and, if not, mark the input stream as being in error
and return an empty/zero value instead of silently wrong data.

Closes #26600.

include/wx/datstrm.h

@@ -112,6 +112,14 @@ public:
 protected:
     wxInputStream *m_input;
 
+private:
+    // Try to read exactly the given number of bytes into the provided buffer.
+    //
+    // Return true if all of them could be read or mark the input stream as
+    // being in error and return false if fewer bytes than requested are
+    // available, as this means the data is truncated and can't be used.
+    bool ReadBytes(void *buffer, size_t size);
+
     wxDECLARE_NO_COPY_CLASS(wxDataInputStream);
 };
 

src/common/datstrm.cpp

@@ -78,9 +78,21 @@ wxDataInputStream::wxDataInputStream(wxInputStream& s, const wxMBConv& conv)
 {
 }
 
+bool wxDataInputStream::ReadBytes(void *buffer, size_t size)
+{
+    if ( m_input->Read(buffer, size).LastRead() == size )
+        return true;
+
+    // We didn't get as many bytes as requested, so the stream is truncated and
+    // we can't return any meaningful data: mark it as being in error to let
+    // the caller know about it instead of silently returning wrong values.
+    m_input->Reset(wxSTREAM_READ_ERROR);
+    return false;
+}
+
 wxUint64 wxDataInputStream::Read64()
 {
-  wxUint64 tmp;
+  wxUint64 tmp = 0;
   Read64(&tmp, 1);
   return tmp;
 }
@@ -89,7 +101,8 @@ wxUint32 wxDataInputStream::Read32()
 {
   wxUint32 i32;
 
-  m_input->Read(&i32, 4);
+  if ( !ReadBytes(&i32, 4) )
+    return 0;
 
   if (m_be_order)
     return wxUINT32_SWAP_ON_LE(i32);
@@ -101,7 +114,8 @@ wxUint16 wxDataInputStream::Read16()
 {
   wxUint16 i16;
 
-  m_input->Read(&i16, 2);
+  if ( !ReadBytes(&i16, 2) )
+    return 0;
 
   if (m_be_order)
     return wxUINT16_SWAP_ON_LE(i16);
@@ -113,7 +127,9 @@ wxUint8 wxDataInputStream::Read8()
 {
   wxUint8 buf;
 
-  m_input->Read(&buf, 1);
+  if ( !ReadBytes(&buf, 1) )
+    return 0;
+
   return (wxUint8)buf;
 }
 
@@ -124,7 +140,9 @@ double wxDataInputStream::ReadDouble()
     {
         char buf[10];
 
-        m_input->Read(buf, 10);
+        if ( !ReadBytes(buf, 10) )
+            return 0.0;
+
         return wxConvertFromIeeeExtended((const wxInt8 *)buf);
     }
     else
@@ -174,8 +192,11 @@ wxString wxDataInputStream::ReadString()
         wxCharBuffer tmp(len);
         if ( tmp )
         {
-            m_input->Read(tmp.data(), len);
-            ret = m_conv->cMB2WC(tmp.data(), len, nullptr);
+            // Only decode the string if we could read all of its bytes: a
+            // shorter read means the stream is truncated and the rest of the
+            // buffer is uninitialised, so don't let it leak into the result.
+            if ( ReadBytes(tmp.data(), len) )
+                ret = m_conv->cMB2WC(tmp.data(), len, nullptr);
         }
     }
 
@@ -189,7 +210,13 @@ void DoReadLL(T *buffer, size_t size, wxInputStream *input, bool be_order)
     typedef T DataType;
     unsigned char *pchBuffer = new unsigned char[size * 8];
     // TODO: Check for overflow when size is of type uint and is > than 512m
-    input->Read(pchBuffer, size * 8);
+    if ( input->Read(pchBuffer, size * 8).LastRead() != size * 8 )
+    {
+        // Stream is truncated, don't use the partially read data.
+        input->Reset(wxSTREAM_READ_ERROR);
+        delete[] pchBuffer;
+        return;
+    }
     size_t idx_base = 0;
     if ( be_order )
     {
@@ -271,7 +298,12 @@ void DoReadI64(T *buffer, size_t size, wxInputStream *input, bool be_order)
     typedef T DataType;
     unsigned char *pchBuffer = (unsigned char*) buffer;
     // TODO: Check for overflow when size is of type uint and is > than 512m
-    input->Read(pchBuffer, size * 8);
+    if ( input->Read(pchBuffer, size * 8).LastRead() != size * 8 )
+    {
+        // Stream is truncated, don't use the partially read data.
+        input->Reset(wxSTREAM_READ_ERROR);
+        return;
+    }
     if ( be_order )
     {
         for ( wxUint32 i = 0; i < size; i++ )
@@ -348,14 +380,15 @@ void wxDataInputStream::ReadLL(wxLongLong *buffer, size_t size)
 
 wxLongLong wxDataInputStream::ReadLL(void)
 {
-    wxLongLong ll;
+    wxLongLong ll = 0;
     DoReadLL(&ll, (size_t)1, m_input, m_be_order);
     return ll;
 }
 
 void wxDataInputStream::Read32(wxUint32 *buffer, size_t size)
 {
-    m_input->Read(buffer, size * 4);
+    if ( !ReadBytes(buffer, size * 4) )
+        return;
 
     if (m_be_order)
     {
@@ -377,7 +410,8 @@ void wxDataInputStream::Read32(wxUint32 *buffer, size_t size)
 
 void wxDataInputStream::Read16(wxUint16 *buffer, size_t size)
 {
-  m_input->Read(buffer, size * 2);
+  if ( !ReadBytes(buffer, size * 2) )
+    return;
 
   if (m_be_order)
   {
@@ -399,7 +433,7 @@ void wxDataInputStream::Read16(wxUint16 *buffer, size_t size)
 
 void wxDataInputStream::Read8(wxUint8 *buffer, size_t size)
 {
-  m_input->Read(buffer, size);
+  ReadBytes(buffer, size);
 }
 
 void wxDataInputStream::ReadDouble(double *buffer, size_t size)

tests/streams/datastreamtest.cpp

@@ -21,6 +21,7 @@
 
 #include "wx/datstrm.h"
 #include "wx/wfstream.h"
+#include "wx/mstream.h"
 #include "wx/math.h"
 
 #include "testfile.h"
@@ -39,6 +40,8 @@ private:
         CPPUNIT_TEST( FloatRW );
         CPPUNIT_TEST( DoubleRW );
         CPPUNIT_TEST( StringRW );
+        CPPUNIT_TEST( ReadTruncatedString );
+        CPPUNIT_TEST( ReadTruncatedValue );
         CPPUNIT_TEST( LongLongRW );
         CPPUNIT_TEST( Int64RW );
         CPPUNIT_TEST( NaNRW );
@@ -63,6 +66,8 @@ private:
     void FloatRW();
     void DoubleRW();
     void StringRW();
+    void ReadTruncatedString();
+    void ReadTruncatedValue();
     void LongLongRW();
     void Int64RW();
     void NaNRW();
@@ -248,6 +253,39 @@ void DataStreamTestCase::StringRW()
     CPPUNIT_ASSERT_EQUAL( TestRW(s), s );
 }
 
+void DataStreamTestCase::ReadTruncatedString()
+{
+    // A string is stored as a 32 bit length followed by that many bytes. If the
+    // length is larger than the number of bytes actually present (a corrupt or
+    // malicious stream), ReadString() must not decode the uninitialised tail of
+    // its temporary buffer but return an empty string and put the stream into
+    // an error state.
+    const unsigned char data[] =
+    {
+        0x04, 0x00, 0x00, 0x00,     // little endian length: claims 4 bytes
+        'H', 'i'                    // but only 2 bytes follow
+    };
+
+    wxMemoryInputStream input(data, sizeof(data));
+    wxDataInputStream dis(input);
+
+    CPPUNIT_ASSERT_EQUAL( wxString(), dis.ReadString() );
+    CPPUNIT_ASSERT( !dis.IsOk() );
+}
+
+void DataStreamTestCase::ReadTruncatedValue()
+{
+    // Reading a fixed size value from a truncated stream must also fail instead
+    // of returning a value built from uninitialised memory.
+    const unsigned char data[] = { 0x12, 0x34 };  // only 2 of the 4 bytes
+
+    wxMemoryInputStream input(data, sizeof(data));
+    wxDataInputStream dis(input);
+
+    CPPUNIT_ASSERT_EQUAL( 0u, dis.Read32() );
+    CPPUNIT_ASSERT( !dis.IsOk() );
+}
+
 void DataStreamTestCase::LongLongRW()
 {
     TestMultiRW<wxLongLong>::ValueArray ValuesLL;