MD3: Fix MD3Importer surface header bounds checks to prevent heap overflow#6441
MD3: Fix MD3Importer surface header bounds checks to prevent heap overflow#6441kimkulling merged 2 commits intoassimp:masterfrom
Conversation
📝 WalkthroughWalkthroughAdded a local boundary-checking lambda and replaced composite offset checks in MD3Importer's ValidateSurfaceHeaderOffsets to ensure surface data ranges (triangles, shaders, texcoords, vertices) lie within the file bounds and throw on invalid ranges. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/AssetLib/MD3/MD3Loader.cpp (1)
397-402: Potential integer overflow intotalcalculation.While the multiplication overflow is correctly checked on line 399, the subsequent addition on line 400 (
abs_offset + size_t(count) * elem_size) could still overflow ifabs_offsetis large. For a security-critical fix, consider adding an explicit check.Suggested safer implementation
auto inRange = [this, ofs](uint32_t rel_offset, uint32_t count, size_t elem_size) -> bool { size_t abs_offset = ofs + rel_offset; + if (abs_offset > fileSize) return false; if (count > 0 && elem_size > 0 && count > SIZE_MAX / elem_size) return false; - size_t total = abs_offset + size_t(count) * elem_size; - return abs_offset <= fileSize && total <= fileSize; + size_t chunk_size = size_t(count) * elem_size; + if (chunk_size > fileSize - abs_offset) return false; // overflow-safe subtraction + return true; };This approach checks
abs_offsetfirst, then uses subtraction (fileSize - abs_offset) which is safe since we've confirmedabs_offset <= fileSize.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/AssetLib/MD3/MD3Loader.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: ubuntu-latest-clang++-build-and-test
- GitHub Check: ubuntu-latest-g++-build-and-test
- GitHub Check: windows-latest-clang.exe-build-and-test
- GitHub Check: windows-hunter-latest-cl.exe-build-and-test
- GitHub Check: macos-latest-clang++-build-and-test
- GitHub Check: windows-latest-cl.exe-build-and-test
- GitHub Check: Fuzzing
- GitHub Check: undefined-behavior-sanitizer
- GitHub Check: adress-sanitizer
🔇 Additional comments (1)
code/AssetLib/MD3/MD3Loader.cpp (1)
393-408: Solid defensive bounds checking for CVE-2025-3549.The fix correctly:
- Validates the surface header itself is within file bounds before reading its fields
- Validates all four data chunk offsets (triangles, shaders, texcoords, vertices) with their respective sizes and counts
- Uses a reusable lambda to ensure consistent validation logic
This addresses the heap-buffer-overflow vulnerability by preventing
pcSurffrom being used to read data outside the MD3 buffer.
…rflow Improve bounds checks in MD3Importer::ValidateSurfaceHeaderOffsets to prevent pcSurf from accessing data outside the MD3 buffer (fixes assimp#6070, CVE-2025-3549). Signed-off-by: mapengyuan <mapengyuan@xfusion.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/AssetLib/MD3/MD3Loader.cpp`:
- Around line 391-402: The subtraction pcSurf - this->mBuffer can be UB if
pcSurf lies outside the buffer and the inRange closure can overflow on 32-bit;
first validate that pcSurf is within [this->mBuffer, this->mBuffer + fileSize)
before computing ofs, using uintptr_t or size_t pointer-to-integer casts (e.g.
compute base = (uintptr_t)this->mBuffer, ptr = (uintptr_t)pcSurf and ensure ptr
>= base && ptr < base + fileSize), then compute ofs = ptr - base; in inRange
(and callers) switch arithmetic to use size_t/uintptr_t and perform
overflow-safe checks (verify count == 0 || elem_size == 0 early, ensure count <=
SIZE_MAX/elem_size before multiplying, and check abs_offset <= fileSize &&
abs_offset + count*elem_size <= fileSize with addition overflow guard) so no
wraparound or UB can bypass the boundary checks (references: pcSurf, mBuffer,
ofs, inRange, fileSize, MD3::Surface).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/AssetLib/MD3/MD3Loader.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: windows-latest-clang.exe-build-and-test
- GitHub Check: macos-latest-clang++-build-and-test
- GitHub Check: ubuntu-latest-g++-build-and-test
- GitHub Check: Fuzzing
- GitHub Check: windows-latest-cl.exe-build-and-test
- GitHub Check: windows-hunter-latest-cl.exe-build-and-test
- GitHub Check: ubuntu-latest-clang++-build-and-test
- GitHub Check: adress-sanitizer
- GitHub Check: undefined-behavior-sanitizer
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Calculate the relative offset of the surface | ||
| const int32_t ofs = int32_t((const unsigned char *)pcSurf - this->mBuffer); | ||
| if (ofs + sizeof(MD3::Surface) > fileSize) { | ||
| throw DeadlyImportError("Surface header is outside file bounds"); | ||
| } | ||
|
|
||
| auto inRange = [this, ofs](uint32_t rel_offset, uint32_t count, size_t elem_size) -> bool { | ||
| size_t abs_offset = ofs + rel_offset; | ||
| if (count > 0 && elem_size > 0 && count > SIZE_MAX / elem_size) return false; | ||
| size_t total = abs_offset + size_t(count) * elem_size; | ||
| return abs_offset <= fileSize && total <= fileSize; | ||
| }; |
There was a problem hiding this comment.
Harden offset arithmetic to avoid UB and overflow bypass.
pcSurf - mBuffer is undefined if pcSurf is outside the buffer (possible after a malicious OFS_END), and abs_offset + count*elem_size can overflow on 32‑bit, allowing the range check to wrap. Consider pointer-as-integer validation and overflow‑safe arithmetic.
🛠️ Proposed fix
- const int32_t ofs = int32_t((const unsigned char *)pcSurf - this->mBuffer);
- if (ofs + sizeof(MD3::Surface) > fileSize) {
+ const auto base = reinterpret_cast<uintptr_t>(this->mBuffer);
+ const auto surf = reinterpret_cast<uintptr_t>(pcSurf);
+ if (surf < base || surf + sizeof(MD3::Surface) > base + static_cast<uintptr_t>(fileSize)) {
throw DeadlyImportError("Surface header is outside file bounds");
}
+ const size_t ofs = static_cast<size_t>(surf - base);
- auto inRange = [this, ofs](uint32_t rel_offset, uint32_t count, size_t elem_size) -> bool {
- size_t abs_offset = ofs + rel_offset;
- if (count > 0 && elem_size > 0 && count > SIZE_MAX / elem_size) return false;
- size_t total = abs_offset + size_t(count) * elem_size;
- return abs_offset <= fileSize && total <= fileSize;
- };
+ auto inRange = [this, ofs](uint32_t rel_offset, uint32_t count, size_t elem_size) -> bool {
+ if (rel_offset > fileSize) return false;
+ const size_t abs_offset = ofs + static_cast<size_t>(rel_offset);
+ if (abs_offset > fileSize) return false;
+ if (count == 0) return true;
+ if (elem_size == 0) return false;
+ const size_t remaining = static_cast<size_t>(fileSize) - abs_offset;
+ return static_cast<size_t>(count) <= remaining / elem_size;
+ };🤖 Prompt for AI Agents
In `@code/AssetLib/MD3/MD3Loader.cpp` around lines 391 - 402, The subtraction
pcSurf - this->mBuffer can be UB if pcSurf lies outside the buffer and the
inRange closure can overflow on 32-bit; first validate that pcSurf is within
[this->mBuffer, this->mBuffer + fileSize) before computing ofs, using uintptr_t
or size_t pointer-to-integer casts (e.g. compute base =
(uintptr_t)this->mBuffer, ptr = (uintptr_t)pcSurf and ensure ptr >= base && ptr
< base + fileSize), then compute ofs = ptr - base; in inRange (and callers)
switch arithmetic to use size_t/uintptr_t and perform overflow-safe checks
(verify count == 0 || elem_size == 0 early, ensure count <= SIZE_MAX/elem_size
before multiplying, and check abs_offset <= fileSize && abs_offset +
count*elem_size <= fileSize with addition overflow guard) so no wraparound or UB
can bypass the boundary checks (references: pcSurf, mBuffer, ofs, inRange,
fileSize, MD3::Surface).
|
|
Merged, thank you for the contribution. |
Improve bounds checks in MD3Importer::ValidateSurfaceHeaderOffsets to prevent pcSurf from accessing data outside the MD3 buffer (fixes #6070, CVE-2025-3549).
Verification
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.