Harden filamesh parser and LinearImage against malformed inputs#9905
Harden filamesh parser and LinearImage against malformed inputs#9905sharadboni wants to merge 3 commits intogoogle:mainfrom
Conversation
…and LinearImage MeshReader (libs/filameshio/src/MeshReader.cpp): - loadMeshFromFile: check open() return value to prevent passing fd=-1 to fileSize/read, which causes undefined behavior (SEGV on lseek with invalid fd). - loadMeshFromBuffer: validate header->parts against SIZE_MAX/sizeof(Part) to prevent integer overflow in pointer arithmetic. Validate combined payload sizes (vertexSize + indexSize + parts*sizeof(Part)) do not overflow. Add sanity check on materialCount. Validate nameLength to prevent unbounded reads from attacker-controlled material name lengths. - Compressed index/vertex buffer paths: check indexSize*indexCount and vertexSize*vertexCount for size_t overflow before malloc, preventing undersized allocation and subsequent heap buffer overflow. LinearImage (libs/image/src/LinearImage.cpp): - Use uint64_t for width*height*channels multiplication instead of uint32_t to prevent silent integer overflow (e.g. 65536*65536*3 wraps to a small value in uint32_t). Validate against SIZE_MAX/sizeof(float) before allocation to prevent undersized buffer and heap corruption.
|
@poweifeng Could you review this security fix? It adds bounds validation to the filamesh parser, fixes a uint32 overflow in LinearImage dimensions, and adds overflow checks in compressed mesh decompression. |
|
See #9910 for |
MeshReader: - Add a new loadMeshFromBuffer overload that takes a dataSize and validates every read against the buffer end. The old overload is kept as a backward-compatible thin wrapper (dataSize = SIZE_MAX). - loadMeshFromFile forwards the known file size through the new path. - Use memcpy to read Header, CompressionHeader and Part instances instead of casting into the buffer; avoids strict-aliasing and alignment UB on unaligned caller buffers. - Fix the materialCount / nameLength reads: the original code did "(uint32_t) *p" which dereferences a single byte, then advanced by sizeof(uint32_t). Replaced with memcpy so the full 4-byte value is read (the writer always emits 4 bytes). - Promote nameLength to size_t before adding 1 so that nameLength == UINT32_MAX cannot wrap to zero and bypass the material-name bounds check. - Reject unknown header.version values before interpreting fields. - Check malloc returns on the compressed index/vertex paths and free the scratch buffer when decoding fails. LinearImage: - Throw std::runtime_error (matching libs/imageio) instead of std::overflow_error. - Saturate the width * height * channels computation in two stages so it cannot overflow uint64_t before the SIZE_MAX check. test_filamesh: exercise the bounds-validated overload.
sharadboni
changed the title
|
Thanks @romainguy — I took a look at #9910 and agree the I also took the opportunity to address three latent issues that neither PR was fixing, which I think make this version strictly stronger than #9910:
Plus: a If you would prefer I land the |
|
|
||
| // Promote to size_t before adding 1 so that nameLength == UINT32_MAX | ||
| // cannot wrap to 0 and bypass the bounds check. | ||
| const size_t nameSpan = size_t(nameLength) + 1; |
There was a problem hiding this comment.
On 32bit platforms, size_t is 32-bit. So this will overflow to 0 if nameLength is UINT32_MAX.
There was a problem hiding this comment.
Good catch. The size_t cast was a no-op on 32-bit targets, so UINT32_MAX + 1 still wraps there. Switched to if (nameLength >= dataSize - consumed) return {}; before doing the + 1, so the add only ever runs on a bounded value.
| utils::slog.e << "Invalid material name length." << utils::io::endl; | ||
| return {}; | ||
| } | ||
| partsMaterial[i] = (const char*) (base + consumed); |
There was a problem hiding this comment.
You just validated nameSpan to ensure it fits in the buffer and then used this assignment that ignores the value. This will call strlen, which will incur a security hole if the string is not nul-terminated.
There was a problem hiding this comment.
Right, the length check was pointless if the copy itself falls back to strlen. Changed to partsMaterial[i].assign((const char*)(base + consumed), nameLength) so it uses the validated length directly and never walks looking for a NUL.
| size_t indexCount = header.indexCount; | ||
| if (indexCount > 0 && indexSize > SIZE_MAX / indexCount) { | ||
| utils::slog.e << "Index buffer size overflow." << utils::io::endl; | ||
| return {}; |
There was a problem hiding this comment.
What happens for the created mesh.indexBuffer if we return here?
There was a problem hiding this comment.
It leaks. Same issue applies to the malloc/decode failure paths below it and to the vertex-buffer block further down. Fix was to move both compressed-size overflow checks above the IndexBuffer::Builder / VertexBuffer::Builder calls (so those rejections return before anything is built), and to call engine->destroy(mesh.indexBuffer) / engine->destroy(mesh.vertexBuffer) on the malloc and decode failure paths. Every early return from loadMeshFromBuffer should be leak-free now.
- Reformulate the material-name bounds check as "nameLength >= dataSize - consumed" so the + 1 cannot wrap when size_t is 32-bit. - Use std::string::assign(ptr, length) instead of implicit construction from a C-string pointer, so reading the material name does not fall back to strlen if the buffer is not NUL-terminated at the expected offset. - Hoist the compressed index/vertex size-overflow checks above the IndexBuffer/VertexBuffer builders so those rejections return before any engine resources are allocated. For malloc and decode failures, destroy the already-built buffers before returning so no engine resource leaks on the error paths.
4 participants
Summary
Hardens two attacker-reachable parsers against malformed input while fixing three latent correctness bugs that exist in
maintoday.libs/filameshio— MeshReaderloadMeshFromBufferoverload that acceptsdataSizeand bounds-checks every read against the buffer end (magic, header, vertexSize, indexSize, parts array, materialCount, each material nameLength). The existing overloads are kept as thin wrappers forwarding withdataSize = SIZE_MAX, so external callers compile unchanged.loadMeshFromFileforwards the known file size through the new bounds-validated path (plus the pre-existingfd < 0guard).Header,CompressionHeader, and eachPartare now read viamemcpyinto a local instead of cast through the caller-supplied buffer, which may not be naturally aligned foruint32_t.materialCount/nameLengthread fix: the original code wroteuint32_t x = (uint32_t) *p;— this dereferences a single byte and then advances bysizeof(uint32_t), silently truncating any value ≥ 256 and breaking on big-endian hosts. Replaced withmemcpyso all four bytes emitted by the writer (tools/filamesh/src/MeshWriter.cpp:253-255) are read.nameLength + 1overflow fix: promote tosize_tbefore+ 1, otherwisenameLength == UINT32_MAXwraps to zero and the bounds check is bypassed.versioncheck: reject unknown versions before interpreting fields whose layout could differ.indexSize * indexCount,vertexSize * vertexCount) and onheader.parts * sizeof(Part)before pointer arithmetic.mallocreturn checks on the two compressed-decode paths; the scratch buffer isfree'd if decoding fails instead of leaking.libs/image— LinearImageLinearImage(width, height, channels)previously computeduint32_t nfloats = width * height * channels;— silently truncating on values like 65536 × 65536 × 3 and producing an undersized allocation followed by out-of-boundsmemset/ pixel writes.width * heightfits inuint64_t; the multiply bychannelsis checked againstUINT64_MAX / channels), then validates againstSIZE_MAX / sizeof(float)before allocation. Throwsstd::runtime_erroron overflow, matching the convention already used inlibs/imageio(ImageDecoder.cpp,ImageEncoder.cpp).Test plan
test_filamesh(NonInterleaved,Interleaved) — updated to call the bounds-validated overload; should continue to pass.parts = 0xFFFFFFFFand verify the overflow check rejects it.materialCount = 300to confirm that, with thememcpyfix, all 300 material entries are parsed (old code would silently read 300 as 0x2C = 44 on little-endian).nameLength = 0xFFFFFFFFand verify thesize_t-promoted bounds check rejects it.Meshinstead of reading past the end.LinearImage(65536, 65536, 3)throwsstd::runtime_error..filameshsamples load unchanged via the backward-compat overload.