#5486 Fix potential unpackBinaryData buffer issues

6 files changed, 66 insertions, 27 deletions

diff --git a/indra/llmessage/lldatapacker.cpp b/indra/llmessage/lldatapacker.cpp
index e911150787..ecd0b4ee8d 100644
--- a/indra/llmessage/lldatapacker.cpp
+++ b/indra/llmessage/lldatapacker.cpp
@@ -289,32 +289,46 @@ bool LLDataPackerBinaryBuffer::packBinaryData(const U8 *value, S32 size, const c
 }
 
 
-bool LLDataPackerBinaryBuffer::unpackBinaryData(U8 *value, S32 &size, const char *name)
+bool LLDataPackerBinaryBuffer::unpackBinaryData(U8 *value, S32 value_size, S32 &out_size, const char *name)
 {
     if (!verifyLength(4, name))
     {
         LL_WARNS() << "LLDataPackerBinaryBuffer::unpackBinaryData would unpack invalid data, aborting!" << LL_ENDL;
+        out_size = 0;
         return false;
     }
 
-    htolememcpy(&size, mCurBufferp, MVT_S32, 4);
+    if (value_size < 0)
+    {
+        LL_WARNS() << "LLDataPackerBinaryBuffer::unpackBinaryData passed negative buffer size, aborting!" << LL_ENDL;
+        out_size = 0;
+        return false;
+    }
+
+    htolememcpy(&out_size, mCurBufferp, MVT_S32, 4);
 
-    if (size < 0)
+    if (out_size < 0)
     {
         LL_WARNS() << "LLDataPackerBinaryBuffer::unpackBinaryData unpacked invalid size, aborting!" << LL_ENDL;
+        out_size = 0;
         return false;
     }
 
     mCurBufferp += 4;
 
-    if (!verifyLength(size, name))
+    if (!verifyLength(out_size, name))
     {
         LL_WARNS() << "LLDataPackerBinaryBuffer::unpackBinaryData would unpack invalid data, aborting!" << LL_ENDL;
         return false;
     }
+    S32 copy_size = llmin(out_size, value_size);
+    htolememcpy(value, mCurBufferp, MVT_VARIABLE, copy_size);
+    mCurBufferp += out_size;
 
-    htolememcpy(value, mCurBufferp, MVT_VARIABLE, size);
-    mCurBufferp += size;
+    if (value_size < out_size)
+    {
+        LL_WARNS() << "LLDataPackerBinaryBuffer::unpackBinaryData buffer too small for data, truncating!" << LL_ENDL;
+    }
 
     return true;
 }
@@ -836,21 +850,34 @@ bool LLDataPackerAsciiBuffer::packBinaryData(const U8 *value, S32 size, const ch
 }
 
 
-bool LLDataPackerAsciiBuffer::unpackBinaryData(U8 *value, S32 &size, const char *name)
+bool LLDataPackerAsciiBuffer::unpackBinaryData(U8 *value, S32 value_size, S32 &out_size, const char *name)
 {
     bool success = true;
     char valuestr[DP_BUFSIZE];      /* Flawfinder: ignore */
     if (!getValueStr(name, valuestr, DP_BUFSIZE))
     {
+        out_size = 0;
+        return false;
+    }
+
+    if (value_size < 0)
+    {
+        LL_WARNS() << "LLDataPackerBinaryBuffer::unpackBinaryData passed negative buffer size, aborting!" << LL_ENDL;
+        out_size = 0;
         return false;
     }
 
     char *cur_pos = &valuestr[0];
-    sscanf(valuestr,"%010d", &size);
+    sscanf(valuestr,"%010d", &out_size);
     cur_pos += 11;
 
+    S32 max_bytes = llmin(out_size, value_size);
+    if (max_bytes != out_size)
+    {
+        LL_WARNS() << "LLDataPackerAsciiBuffer::unpackBinaryData: buffer too small for data, truncating!" << LL_ENDL;
+    }
     S32 i;
-    for (i = 0; i < size; i++)
+    for (i = 0; i < max_bytes; i++)
     {
         S32 val;
         sscanf(cur_pos,"%02x", &val);
@@ -1634,28 +1661,40 @@ bool LLDataPackerAsciiFile::packBinaryData(const U8 *value, S32 size, const char
 }
 
 
-bool LLDataPackerAsciiFile::unpackBinaryData(U8 *value, S32 &size, const char *name)
+bool LLDataPackerAsciiFile::unpackBinaryData(U8 *value, S32 value_size, S32 &out_size, const char *name)
 {
-    bool success = true;
     char valuestr[DP_BUFSIZE]; /*Flawfinder: ignore*/
     if (!getValueStr(name, valuestr, DP_BUFSIZE))
     {
+        out_size = 0;
+        return false;
+    }
+
+    if (value_size < 0)
+    {
+        LL_WARNS() << "LLDataPackerBinaryBuffer::unpackBinaryData passed negative buffer size, aborting!" << LL_ENDL;
+        out_size = 0;
         return false;
     }
 
     char *cur_pos = &valuestr[0];
-    sscanf(valuestr,"%010d", &size);
+    sscanf(valuestr,"%010d", &out_size);
     cur_pos += 11;
 
+    S32 max_bytes = llmin(out_size, value_size);
+    if (max_bytes != out_size)
+    {
+        LL_WARNS() << "LLDataPackerAsciiBuffer::unpackBinaryData: buffer too small for data, truncating!" << LL_ENDL;
+    }
     S32 i;
-    for (i = 0; i < size; i++)
+    for (i = 0; i < max_bytes; i++)
     {
         S32 val;
         sscanf(cur_pos,"%02x", &val);
         value[i] = val;
         cur_pos += 3;
     }
-    return success;
+    return true;
 }
 
 
diff --git a/indra/llmessage/lldatapacker.h b/indra/llmessage/lldatapacker.h
index 167c102b43..5ac45356cf 100644
--- a/indra/llmessage/lldatapacker.h
+++ b/indra/llmessage/lldatapacker.h
@@ -49,7 +49,7 @@ public:
     virtual bool        unpackString(std::string& value, const char *name) = 0;
 
     virtual bool        packBinaryData(const U8 *value, S32 size, const char *name) = 0;
-    virtual bool        unpackBinaryData(U8 *value, S32 &size, const char *name) = 0;
+    virtual bool        unpackBinaryData(U8 *value, S32 value_size, S32 &out_size, const char *name) = 0;
 
     // Constant size binary data packing
     virtual bool        packBinaryDataFixed(const U8 *value, S32 size, const char *name) = 0;
@@ -135,7 +135,7 @@ public:
     /*virtual*/ bool        unpackString(std::string& value, const char *name);
 
     /*virtual*/ bool        packBinaryData(const U8 *value, S32 size, const char *name);
-    /*virtual*/ bool        unpackBinaryData(U8 *value, S32 &size, const char *name);
+    /*virtual*/ bool        unpackBinaryData(U8 *value, S32 value_size, S32 &out_size, const char *name);
 
     // Constant size binary data packing
     /*virtual*/ bool        packBinaryDataFixed(const U8 *value, S32 size, const char *name);
@@ -246,7 +246,7 @@ public:
     /*virtual*/ bool        unpackString(std::string& value, const char *name);
 
     /*virtual*/ bool        packBinaryData(const U8 *value, S32 size, const char *name);
-    /*virtual*/ bool        unpackBinaryData(U8 *value, S32 &size, const char *name);
+    /*virtual*/ bool        unpackBinaryData(U8 *value, S32 value_size, S32 &out_size, const char *name);
 
     // Constant size binary data packing
     /*virtual*/ bool        packBinaryDataFixed(const U8 *value, S32 size, const char *name);
@@ -378,7 +378,7 @@ public:
     /*virtual*/ bool        unpackString(std::string& value, const char *name);
 
     /*virtual*/ bool        packBinaryData(const U8 *value, S32 size, const char *name);
-    /*virtual*/ bool        unpackBinaryData(U8 *value, S32 &size, const char *name);
+    /*virtual*/ bool        unpackBinaryData(U8 *value, S32 value_size, S32 &out_size, const char *name);
 
     /*virtual*/ bool        packBinaryDataFixed(const U8 *value, S32 size, const char *name);
     /*virtual*/ bool        unpackBinaryDataFixed(U8 *value, S32 size, const char *name);
diff --git a/indra/llprimitive/llprimitive.cpp b/indra/llprimitive/llprimitive.cpp
index c5d6076b98..7a638dd625 100644
--- a/indra/llprimitive/llprimitive.cpp
+++ b/indra/llprimitive/llprimitive.cpp
@@ -1515,7 +1515,7 @@ S32 LLPrimitive::unpackTEMessage(LLDataPacker &dp)
     S32 size;
     U32 face_count = 0;
 
-    if (!dp.unpackBinaryData(packed_buffer, size, "TextureEntry"))
+    if (!dp.unpackBinaryData(packed_buffer, MAX_TE_BUFFER, size, "TextureEntry"))
     {
         retval = TEM_INVALID;
         LL_WARNS() << "Bad texture entry block!  Abort!" << LL_ENDL;
diff --git a/indra/llprimitive/lltextureanim.cpp b/indra/llprimitive/lltextureanim.cpp
index 579538075a..963b7456b8 100644
--- a/indra/llprimitive/lltextureanim.cpp
+++ b/indra/llprimitive/lltextureanim.cpp
@@ -155,7 +155,7 @@ void LLTextureAnim::unpackTAMessage(LLDataPacker &dp)
 {
     S32 size;
     U8 data[TA_BLOCK_SIZE];
-    dp.unpackBinaryData(data, size, "TextureAnimation");
+    dp.unpackBinaryData(data, TA_BLOCK_SIZE, size, "TextureAnimation");
     if (size != TA_BLOCK_SIZE)
     {
         if (size)
diff --git a/indra/newview/llviewerobject.cpp b/indra/newview/llviewerobject.cpp
index db1ef54ffa..521c6731e0 100644
--- a/indra/newview/llviewerobject.cpp
+++ b/indra/newview/llviewerobject.cpp
@@ -1529,7 +1529,7 @@ U32 LLViewerObject::processUpdateMessage(LLMessageSystem *mesgsys,
                         U16 param_type;
                         S32 param_size;
                         dp.unpackU16(param_type, "param_type");
-                        dp.unpackBinaryData(param_block, param_size, "param_data");
+                        dp.unpackBinaryData(param_block, MAX_OBJECT_PARAMS_SIZE, param_size, "param_data");
                         //LL_INFOS() << "Param type: " << param_type << ", Size: " << param_size << LL_ENDL;
                         LLDataPackerBinaryBuffer dp2(param_block, param_size);
                         unpackParameterEntry(param_type, &dp2);
@@ -1786,7 +1786,7 @@ U32 LLViewerObject::processUpdateMessage(LLMessageSystem *mesgsys,
                     dp->unpackU32(size, "ScratchPadSize");
                     delete [] mData;
                     mData = new U8[size];
-                    dp->unpackBinaryData((U8 *)mData, sp_size, "PartData");
+                    dp->unpackBinaryData((U8 *)mData, size, sp_size, "PartData");
                 }
                 else
                 {
@@ -1859,7 +1859,7 @@ U32 LLViewerObject::processUpdateMessage(LLMessageSystem *mesgsys,
                     U16 param_type;
                     S32 param_size;
                     dp->unpackU16(param_type, "param_type");
-                    dp->unpackBinaryData(param_block, param_size, "param_data");
+                    dp->unpackBinaryData(param_block, MAX_OBJECT_PARAMS_SIZE, param_size, "param_data");
                     //LL_INFOS() << "Param type: " << param_type << ", Size: " << param_size << LL_ENDL;
                     LLDataPackerBinaryBuffer dp2(param_block, param_size);
                     unpackParameterEntry(param_type, &dp2);
diff --git a/indra/test/lldatapacker_tut.cpp b/indra/test/lldatapacker_tut.cpp
index 2e975ee637..5aa5233806 100644
--- a/indra/test/lldatapacker_tut.cpp
+++ b/indra/test/lldatapacker_tut.cpp
@@ -127,7 +127,7 @@ namespace tut
 
         LLDataPackerBinaryBuffer lldp1(packbuf, cur_size);
         lldp1.unpackString(unpkstr , "linden_lab_str");
-        lldp1.unpackBinaryData((U8*)unpkstrBinary, unpksizeBinary, "linden_lab_bd");
+        lldp1.unpackBinaryData((U8*)unpkstrBinary, 256, unpksizeBinary, "linden_lab_bd");
         lldp1.unpackBinaryDataFixed((U8*)unpkstrBinaryFixed, sizeBinaryFixed, "linden_lab_bdf");
         lldp1.unpackU8(unpkvalU8,"linden_lab_u8");
         lldp1.unpackU16(unpkvalU16,"linden_lab_u16");
@@ -286,7 +286,7 @@ namespace tut
 
         LLDataPackerAsciiBuffer lldp1(packbuf, cur_size);
         lldp1.unpackString(unpkstr , "linden_lab_str");
-        lldp1.unpackBinaryData((U8*)unpkstrBinary, unpksizeBinary, "linden_lab_bd");
+        lldp1.unpackBinaryData((U8*)unpkstrBinary, 256, unpksizeBinary, "linden_lab_bd");
         lldp1.unpackBinaryDataFixed((U8*)unpkstrBinaryFixed, sizeBinaryFixed, "linden_lab_bdf");
         lldp1.unpackU8(unpkvalU8,"linden_lab_u8");
         lldp1.unpackU16(unpkvalU16,"linden_lab_u16");
@@ -431,7 +431,7 @@ namespace tut
         LLDataPackerAsciiFile lldp1(fp,2);
 
         lldp1.unpackString(unpkstr , "linden_lab_str");
-        lldp1.unpackBinaryData((U8*)unpkstrBinary, unpksizeBinary, "linden_lab_bd");
+        lldp1.unpackBinaryData((U8*)unpkstrBinary, 256, unpksizeBinary, "linden_lab_bd");
         lldp1.unpackBinaryDataFixed((U8*)unpkstrBinaryFixed, sizeBinaryFixed, "linden_lab_bdf");
         lldp1.unpackU8(unpkvalU8,"linden_lab_u8");
         lldp1.unpackU16(unpkvalU16,"linden_lab_u16");
@@ -538,7 +538,7 @@ namespace tut
         LLDataPackerAsciiFile lldp1(istr,2);
 
         lldp1.unpackString(unpkstr , "linden_lab_str");
-        lldp1.unpackBinaryData((U8*)unpkstrBinary, unpksizeBinary, "linden_lab_bd");
+        lldp1.unpackBinaryData((U8*)unpkstrBinary, 256, unpksizeBinary, "linden_lab_bd");
         lldp1.unpackBinaryDataFixed((U8*)unpkstrBinaryFixed, sizeBinaryFixed, "linden_lab_bdf");
         lldp1.unpackU8(unpkvalU8,"linden_lab_u8");
         lldp1.unpackU16(unpkvalU16,"linden_lab_u16");