Fix critical vulnerabilities and bugs with NetworkPacket

diff --git a/src/network/networkpacket.cpp b/src/network/networkpacket.cpp
index 1e4b6e23eec92e7d36e25727b4880eac12c05df1..0c56048729e3f60a22d8c95e8cec4dc304d7de9e 100644 (file)
--- a/src/network/networkpacket.cpp
+++ b/src/network/networkpacket.cpp
@@ -39,9 +39,9 @@ NetworkPacket::~NetworkPacket()
        m_data.clear();
 }
 
-void NetworkPacket::checkReadOffset(u32 from_offset)
+void NetworkPacket::checkReadOffset(u32 from_offset, u32 field_size)
 {
-       if (from_offset >= m_datasize) {
+       if (from_offset + field_size > m_datasize) {
                std::stringstream ss;
                ss << "Reading outside packet (offset: " <<
                                from_offset << ", packet size: " << getSize() << ")";
@@ -65,7 +65,7 @@ void NetworkPacket::putRawPacket(u8 *data, u32 datasize, u16 peer_id)
 
 char* NetworkPacket::getString(u32 from_offset)
 {
-       checkReadOffset(from_offset);
+       checkReadOffset(from_offset, 0);
 
        return (char*)&m_data[from_offset];
 }
@@ -83,6 +83,7 @@ void NetworkPacket::putRawString(const char* src, u32 len)
 
 NetworkPacket& NetworkPacket::operator>>(std::string& dst)
 {
+       checkReadOffset(m_read_offset, 2);
        u16 strLen = readU16(&m_data[m_read_offset]);
        m_read_offset += sizeof(u16);
 
@@ -92,6 +93,8 @@ NetworkPacket& NetworkPacket::operator>>(std::string& dst)
                return *this;
        }
 
+       checkReadOffset(m_read_offset, strLen);
+
        dst.reserve(strLen);
        dst.append((char*)&m_data[m_read_offset], strLen);
 
@@ -127,8 +130,9 @@ void NetworkPacket::putLongString(std::string src)
 
 NetworkPacket& NetworkPacket::operator>>(std::wstring& dst)
 {
+       checkReadOffset(m_read_offset, 2);
        u16 strLen = readU16(&m_data[m_read_offset]);
-       m_read_offset += sizeof(u16);
+       m_read_offset += 2;
 
        dst.clear();
 
@@ -136,6 +140,8 @@ NetworkPacket& NetworkPacket::operator>>(std::wstring& dst)
                return *this;
        }
 
+       checkReadOffset(m_read_offset, strLen * 2);
+
        dst.reserve(strLen);
        for(u16 i=0; i<strLen; i++) {
                wchar_t c16 = readU16(&m_data[m_read_offset]);
@@ -165,13 +171,16 @@ NetworkPacket& NetworkPacket::operator<<(std::wstring src)
 
 std::string NetworkPacket::readLongString()
 {
+       checkReadOffset(m_read_offset, 4);
        u32 strLen = readU32(&m_data[m_read_offset]);
-       m_read_offset += sizeof(u32);
+       m_read_offset += 4;
 
        if (strLen == 0) {
                return "";
        }
 
+       checkReadOffset(m_read_offset, strLen);
+
        std::string dst;
 
        dst.reserve(strLen);
@@ -184,114 +193,114 @@ std::string NetworkPacket::readLongString()
 
 NetworkPacket& NetworkPacket::operator>>(char& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 1);
 
        dst = readU8(&m_data[m_read_offset]);
 
-       incrOffset<char>();
+       m_read_offset += 1;
        return *this;
 }
 
 char NetworkPacket::getChar(u32 offset)
 {
-       checkReadOffset(offset);
+       checkReadOffset(offset, 1);
 
        return readU8(&m_data[offset]);
 }
 
 NetworkPacket& NetworkPacket::operator<<(char src)
 {
-       checkDataSize<u8>();
+       checkDataSize(1);
 
        writeU8(&m_data[m_read_offset], src);
 
-       incrOffset<char>();
+       m_read_offset += 1;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator<<(u8 src)
 {
-       checkDataSize<u8>();
+       checkDataSize(1);
 
        writeU8(&m_data[m_read_offset], src);
 
-       incrOffset<u8>();
+       m_read_offset += 1;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator<<(bool src)
 {
-       checkDataSize<u8>();
+       checkDataSize(1);
 
        writeU8(&m_data[m_read_offset], src);
 
-       incrOffset<u8>();
+       m_read_offset += 1;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator<<(u16 src)
 {
-       checkDataSize<u16>();
+       checkDataSize(2);
 
        writeU16(&m_data[m_read_offset], src);
 
-       incrOffset<u16>();
+       m_read_offset += 2;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator<<(u32 src)
 {
-       checkDataSize<u32>();
+       checkDataSize(4);
 
        writeU32(&m_data[m_read_offset], src);
 
-       incrOffset<u32>();
+       m_read_offset += 4;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator<<(u64 src)
 {
-       checkDataSize<u64>();
+       checkDataSize(8);
 
        writeU64(&m_data[m_read_offset], src);
 
-       incrOffset<u64>();
+       m_read_offset += 8;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator<<(float src)
 {
-       checkDataSize<float>();
+       checkDataSize(4);
 
        writeF1000(&m_data[m_read_offset], src);
 
-       incrOffset<float>();
+       m_read_offset += 4;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator>>(bool& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 1);
 
        dst = readU8(&m_data[m_read_offset]);
 
-       incrOffset<u8>();
+       m_read_offset += 1;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator>>(u8& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 1);
 
        dst = readU8(&m_data[m_read_offset]);
 
-       incrOffset<u8>();
+       m_read_offset += 1;
        return *this;
 }
 
 u8 NetworkPacket::getU8(u32 offset)
 {
-       checkReadOffset(offset);
+       checkReadOffset(offset, 1);
 
        return readU8(&m_data[offset]);
 }
@@ -302,85 +311,85 @@ u8* NetworkPacket::getU8Ptr(u32 from_offset)
                return NULL;
        }
 
-       checkReadOffset(from_offset);
+       checkReadOffset(from_offset, 1);
 
        return (u8*)&m_data[from_offset];
 }
 
 NetworkPacket& NetworkPacket::operator>>(u16& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 2);
 
        dst = readU16(&m_data[m_read_offset]);
 
-       incrOffset<u16>();
+       m_read_offset += 2;
        return *this;
 }
 
 u16 NetworkPacket::getU16(u32 from_offset)
 {
-       checkReadOffset(from_offset);
+       checkReadOffset(from_offset, 2);
 
        return readU16(&m_data[from_offset]);
 }
 
 NetworkPacket& NetworkPacket::operator>>(u32& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 4);
 
        dst = readU32(&m_data[m_read_offset]);
 
-       incrOffset<u32>();
+       m_read_offset += 4;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator>>(u64& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 8);
 
        dst = readU64(&m_data[m_read_offset]);
 
-       incrOffset<u64>();
+       m_read_offset += 8;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator>>(float& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 4);
 
        dst = readF1000(&m_data[m_read_offset]);
 
-       incrOffset<float>();
+       m_read_offset += 4;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator>>(v2f& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 8);
 
        dst = readV2F1000(&m_data[m_read_offset]);
 
-       incrOffset<v2f>();
+       m_read_offset += 8;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator>>(v3f& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 12);
 
        dst = readV3F1000(&m_data[m_read_offset]);
 
-       incrOffset<v3f>();
+       m_read_offset += 12;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator>>(s16& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 2);
 
        dst = readS16(&m_data[m_read_offset]);
 
-       incrOffset<s16>();
+       m_read_offset += 2;
        return *this;
 }
 
@@ -392,11 +401,11 @@ NetworkPacket& NetworkPacket::operator<<(s16 src)
 
 NetworkPacket& NetworkPacket::operator>>(s32& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 4);
 
        dst = readS32(&m_data[m_read_offset]);
 
-       incrOffset<s32>();
+       m_read_offset += 4;
        return *this;
 }
 
@@ -408,29 +417,31 @@ NetworkPacket& NetworkPacket::operator<<(s32 src)
 
 NetworkPacket& NetworkPacket::operator>>(v3s16& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 6);
 
        dst = readV3S16(&m_data[m_read_offset]);
 
-       incrOffset<v3s16>();
+       m_read_offset += 6;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator>>(v2s32& dst)
 {
+       checkReadOffset(m_read_offset, 8);
+
        dst = readV2S32(&m_data[m_read_offset]);
 
-       incrOffset<v2s32>();
+       m_read_offset += 8;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator>>(v3s32& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 12);
 
        dst = readV3S32(&m_data[m_read_offset]);
 
-       incrOffset<v3s32>();
+       m_read_offset += 12;
        return *this;
 }
 
@@ -474,21 +485,21 @@ NetworkPacket& NetworkPacket::operator<<(v3s32 src)
 
 NetworkPacket& NetworkPacket::operator>>(video::SColor& dst)
 {
-       checkReadOffset(m_read_offset);
+       checkReadOffset(m_read_offset, 4);
 
        dst = readARGB8(&m_data[m_read_offset]);
 
-       incrOffset<u32>();
+       m_read_offset += 4;
        return *this;
 }
 
 NetworkPacket& NetworkPacket::operator<<(video::SColor src)
 {
-       checkDataSize<u32>();
+       checkDataSize(4);
 
        writeU32(&m_data[m_read_offset], src.color);
 
-       incrOffset<u32>();
+       m_read_offset += 4;
        return *this;
 }
 

diff --git a/src/network/networkpacket.h b/src/network/networkpacket.h
index 0408b9cac14d415762e5ee268ba0ef1fec91a1b1..72f8cabe276e0c3e19af340d27c16eb104a00a89 100644 (file)
--- a/src/network/networkpacket.h
+++ b/src/network/networkpacket.h
@@ -111,21 +111,16 @@ public:
                // Temp, we remove SharedBuffer when migration finished
                Buffer<u8> oldForgePacket();
 private:
-               void checkReadOffset(u32 from_offset);
+               void checkReadOffset(u32 from_offset, u32 field_size);
 
-               template<typename T> void checkDataSize()
+               inline void checkDataSize(u32 field_size)
                {
-                       if (m_read_offset + sizeof(T) > m_datasize) {
-                               m_datasize = m_read_offset + sizeof(T);
+                       if (m_read_offset + field_size > m_datasize) {
+                               m_datasize = m_read_offset + field_size;
                                m_data.resize(m_datasize);
                        }
                }
 
-               template<typename T> void incrOffset()
-               {
-                       m_read_offset += sizeof(T);
-               }
-
                std::vector<u8> m_data;
                u32 m_datasize;
                u32 m_read_offset;