diff --git a/include/wx/datstrm.h b/include/wx/datstrm.h index 53390c627a..36b4c3dd24 100644 --- a/include/wx/datstrm.h +++ b/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); }; diff --git a/src/common/datstrm.cpp b/src/common/datstrm.cpp index 870d3cb352..7d9ec69c2a 100644 --- a/src/common/datstrm.cpp +++ b/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) diff --git a/tests/streams/datastreamtest.cpp b/tests/streams/datastreamtest.cpp index 579730e2d6..181ef7c65c 100644 --- a/tests/streams/datastreamtest.cpp +++ b/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::ValueArray ValuesLL;