unpackBinaryData() can overfill destination buffer

[akleshchev]

Environment

7.2.3.19375695301

Description

unpackBinaryData() can be passed a buffer of random size. A corrupted message, or message for a newer version of viewer can contain more data than viewer is ready for, which would result in unpackBinaryData() overfilling destination buffer, which would lead to a crash.

Reproduction steps

No known steps, was reported here #5483 (comment)

[opsec-ee]

Could it be the crash patterns you describe — valid pointers with corrupted data,
variable callstacks across builds — are consistent with a heap/stack
overflow in the network parsing layer that corrupts adjacent memory
non-deterministically depending on stack/heap layout.

The root cause is in lldatapacker.cpp line 292:

bool unpackBinaryData(U8 *value, S32 &size, const char *name)

The function reads 'size' from the source buffer (line 300), verifies
the SOURCE has enough bytes via verifyLength() (line 310), then copies
'size' bytes into 'value' (line 316). The destination capacity is never
checked because the function signature has no parameter for it.

verifyLength() (lldatapacker.h line 210) only validates mBufferp/mBufferSize.
It has no knowledge of the destination pointer.

Known vulnerable call sites:

lltextureanim.cpp:158 — 16-byte stack buffer, size from network
llviewerobject.cpp:1532 — 1024-byte stack buffer, size from network
llviewerobject.cpp:1862 — same pattern
llviewerobject.cpp:1789 — heap alloc 'size', copy 'sp_size' (mismatch)
llprimitive.cpp:1518 — 4096-byte stack buffer, size from network
post-copy check on line 1529 is too late
Look at the flow in llprimitive.cpp

if (!dp.unpackBinaryData(packed_buffer, size, "TextureEntry"))  // line 1518 — OVERFLOW HERE
{
    retval = TEM_INVALID;
    return retval;
}
...
else if (size >= MAX_TE_BUFFER)    // line 1529 — checking AFTER the copy
{
    size = MAX_TE_BUFFER - 1;      // line 1532 — truncating a variable, not undoing the write
}

By the time line 1529 runs, unpackBinaryData() has already returned. The 8192 bytes are already written into the 4096-byte buffer. Setting size = MAX_TE_BUFFER - 1 just changes the integer — it doesn't undo the overflow that already happened at line 316 in lldatapacker.cpp

the POC I have written show this
ASan fires inside unpackBinaryData(), not at line 1529.

The fix needs to happen at the function level — either add a
destination capacity parameter to unpackBinaryData() and validate
before the copy, or clamp 'size' at each call site before calling.

All line numbers verified against develop branch at commit 88c4cf7.

eager to help, hope it helps! [opsec.ee@pm.me]