HL1: Fix out of bound access #6269
Conversation
|
WalkthroughThis pull request adds buffer size validation to the HL1MDL importer chain to prevent heap out-of-bounds reads. A new buffer size parameter is threaded through the loader hierarchy with bounds checks inserted at entry points and throughout data reading operations. Minor refactoring and test additions accompany the core security fixes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
code/AssetLib/MDL/MDLLoader.cpp (1)
168-183: File size handling regression:iFileSizeis never set, breaking range checks and HL1 header guard
InternReadFilenow assignsfile->FileSize()only tomBuffersize(Line 180), leavingiFileSizeat its default‑initialized value (0). However:
IsPosValidstill usesthis->mBuffer + this->iFileSizeas the upper bound, so allVALIDATE_FILE_SIZEcalls are effectively checking against a zero‑length buffer.InternReadFile_HL1still testsif (iFileSize < sizeof(HalfLife::Header_HL1)), which will always be true and wrongly reject every HL1 MDL as “too small”.This is a functional regression and also risks masking or mis‑handling precisely the kind of malformed input this PR is trying to harden against.
A minimal fix is to restore
iFileSizefromFileSize()and keepmBuffersizein sync, and to base the HL1 header check onmBuffersizefor clarity:@@ void MDLImporter::InternReadFile(const std::string &pFile, - // the HL1 sequence group header is one of the smallest, afaik - mBuffersize = (unsigned int)file->FileSize(); - if (mBuffersize < sizeof(HalfLife::SequenceHeader_HL1)) { + // the HL1 sequence group header is one of the smallest, afaik + iFileSize = static_cast<unsigned int>(file->FileSize()); + mBuffersize = iFileSize; + if (mBuffersize < sizeof(HalfLife::SequenceHeader_HL1)) { throw DeadlyImportError("MDL File is too small."); } @@ void MDLImporter::InternReadFile_HL1(const std::string &pFile, const uint32_t iMagicWord) { - // Check if the buffer is large enough to hold the header - if (iFileSize < sizeof(HalfLife::Header_HL1)) { + // Check if the buffer is large enough to hold the header + if (mBuffersize < sizeof(HalfLife::Header_HL1)) { throw DeadlyImportError("HL1 MDL file is too small to contain header."); }With this change,
IsPosValidonce again sees the real file size (throughiFileSize), and the HL1 header size guard is evaluated against the same buffer size that is passed intoHL1MDLLoader.Also applies to: 284-288, 1979-1997
code/AssetLib/MDL/HalfLife/HL1MDLLoader.cpp (1)
973-1073: Add bounds validation for pointer arithmetic in animation and accessory functions.Functions including
read_animations,read_sequence_groups_info,read_sequence_infos,read_transitions,read_attachments,read_hitboxes, andread_bone_controllersperform pointer arithmetic using header indices (header_->seqindex,header_->seqgroupindex,header_->transitionindex,header_->attachmentindex,header_->hitboxindex,header_->bonecontrollerindex) but lack the bounds validation present inread_textures,read_bones, andread_meshes. Add corresponding bounds checks before dereferencing these pointers to prevent out-of-bounds reads.
🧹 Nitpick comments (4)
test/unit/ImportExport/MDL/utH1MDLImporter.cpp (1)
42-60: Invalid‑file regression test looks good; minor @file name mismatchThe test correctly asserts that an obviously invalid binary input yields a
nullptrscene underaiProcess_ValidateDataStructure, which is appropriate for guarding against the reported HL1 crash/OOB vector. Only nit: the@filetag still saysutMDLImporter_HL1_ImportSettings.cpp, which doesn’t match this file’s name; consider updating it for clarity.code/AssetLib/MDL/MDLLoader.h (1)
427-444: NewmBuffersizemember is reasonable but duplicatesiFileSizeAdding
size_t mBuffersize = 0;alongsidemBufferis fine and matches the need to track the buffer size explicitly. Just be aware thatiFileSizeremains as a separate “size of the input file” field; correctness now depends on keeping these two in sync in MDLLoader.cpp (see comment there).test/unit/ImportExport/utMDLImporter.cpp (1)
55-75: UnqualifiedImporterparameter is fine; consider aligning local variable styleSwitching the parameter type to
Importer *const importeris harmless givenusing namespace Assimp;. For consistency, you could also drop theAssimp::qualifier on the localimportervariable at Line 57 so both declarations use the same spelling.test/unit/ImportExport/MDL/utMDLImporter_HL1_Nodes.cpp (1)
77-120: UnqualifiedImporterusage is fine; consider making it consistent across testsSwitching to
Importer importer;in a few methods is correct givenusing namespace Assimp;at the top. For readability, you might want to align the rest of the file (e.g.,emptyBonesNames,emptyBodypartsNames, etc.) to also use the unqualifiedImporterso you don’t mix bothImporterandAssimp::Importerin the same test class.Also applies to: 209-230, 254-285
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/models/invalid/af51e2973bb947199c1192c98a4b3fe6.binis excluded by!**/*.bin
📒 Files selected for processing (12)
code/AssetLib/MDL/HalfLife/HL1FileData.h(1 hunks)code/AssetLib/MDL/HalfLife/HL1MDLLoader.cpp(14 hunks)code/AssetLib/MDL/HalfLife/HL1MDLLoader.h(2 hunks)code/AssetLib/MDL/MDLLoader.cpp(3 hunks)code/AssetLib/MDL/MDLLoader.h(1 hunks)code/Common/Importer.h(0 hunks)include/assimp/Importer.hpp(0 hunks)test/CMakeLists.txt(1 hunks)test/unit/ImportExport/MDL/utH1MDLImporter.cpp(1 hunks)test/unit/ImportExport/MDL/utMDLImporter_HL1_ImportSettings.cpp(1 hunks)test/unit/ImportExport/MDL/utMDLImporter_HL1_Nodes.cpp(3 hunks)test/unit/ImportExport/utMDLImporter.cpp(1 hunks)
💤 Files with no reviewable changes (2)
- include/assimp/Importer.hpp
- code/Common/Importer.h
🧰 Additional context used
🧬 Code graph analysis (4)
test/unit/ImportExport/utMDLImporter.cpp (1)
test/unit/ImportExport/MDL/utMDLImporter_HL1_Nodes.cpp (9)
importer(78-120)importer(136-156)importer(172-193)importer(209-230)importer(254-285)importer(301-321)importer(337-357)importer(374-395)importer(412-433)
code/AssetLib/MDL/HalfLife/HL1MDLLoader.cpp (1)
code/AssetLib/MDL/HalfLife/HL1MDLLoader.h (1)
HL1MDLLoader(67-228)
code/AssetLib/MDL/MDLLoader.cpp (2)
code/AssetLib/C4D/C4DImporter.cpp (1)
mBuffer(131-131)code/AssetLib/LWO/LWOLoader.cpp (1)
mBuffer(146-146)
test/unit/ImportExport/MDL/utMDLImporter_HL1_ImportSettings.cpp (1)
test/unit/ImportExport/utMDLImporter.cpp (3)
importer(55-63)importer(66-75)importer(66-66)
⏰ 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). (10)
- GitHub Check: adress-sanitizer
- GitHub Check: undefined-behavior-sanitizer
- GitHub Check: windows-latest-cl.exe-build-and-test
- GitHub Check: Fuzzing
- GitHub Check: macos-latest-clang++-build-and-test
- GitHub Check: windows-hunter-latest-cl.exe-build-and-test
- GitHub Check: windows-latest-clang.exe-build-and-test
- GitHub Check: ubuntu-latest-g++-build-and-test
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (8)
code/AssetLib/MDL/HalfLife/HL1FileData.h (1)
139-144: Docstring typo fix is correctComment now accurately reads “Offset of the first attachment chunk.” No code behavior change.
code/AssetLib/MDL/HalfLife/HL1MDLLoader.h (1)
72-80: Constructor andmBuffersizeaddition make sense; ensure it’s fully wired into bounds checksPassing
buffersizeinto the constructor and storing it inmBuffersizeis the right direction to support robust range checking in the HL1 loader. The remaining crucial part is inHL1MDLLoader.cpp: all pointer arithmetic and index‑based access (especially inread_meshesand any code using header‑derived indices) must validate againstmBuffersize(or equivalent per‑buffer sizes for texture/sequence files), not just header fields.Please double‑check the .cpp to ensure every use of
header_->*index/modelindex/bodypartindexthat computes a pointer intobuffer_is guarded by abuffersize‑based bounds check.Also applies to: 155-166
test/CMakeLists.txt (1)
165-176: New HL1 importer test is correctly added to the test suiteIncluding
unit/ImportExport/MDL/utH1MDLImporter.cppin theIMPORTERSlist correctly ensures the new HL1 MDL regression test is built and run with the rest of the importer tests.test/unit/ImportExport/MDL/utMDLImporter_HL1_ImportSettings.cpp (1)
56-56: LGTM! Minor refactoring changes.The addition of
using namespace Assimp;, unqualifiedImporterusage, and removal of theinlinespecifier from a static member function in the .cpp file are all standard practices. These changes have no functional impact.Also applies to: 190-190, 197-197
code/AssetLib/MDL/HalfLife/HL1MDLLoader.cpp (4)
75-90: LGTM! Essential security improvement.Adding the
buffersizeparameter and storing it inmBuffersizeenables bounds checking throughout the loader to prevent out-of-bounds reads.
397-397: Good refactoring - compile-time constants.Marking these constants as
constexpris a good practice that enables compile-time evaluation and may improve performance.Also applies to: 407-407, 413-413, 417-417
506-506: Good practice - consistent types.Using
size_tfor loop variables that iterate over.size()is good practice and avoids signed/unsigned comparison warnings.Also applies to: 522-522
513-513: Good practice - const correctness.Adding
constto the parameter clarifies that the function doesn't modify the bone and improves code safety.
| if (static_cast<size_t>(texture_header_->textureindex) > mBuffersize) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Incomplete bounds validation - potential for remaining OOB reads.
The bounds checks only verify that the offset/index is less than mBuffersize, but they don't validate that there's sufficient space to read the actual structures at those offsets. This leaves the door open for out-of-bounds reads when accessing data at these validated offsets.
Issues:
-
Structure size not validated: For example, at line 374, if
textureindex == mBuffersize - 1, the check passes, but dereferencing(const Texture_HL1 *)((uint8_t *)texture_header_ + texture_header_->textureindex)at line 378 will read beyond the buffer. -
No negative index validation: The indices from the header are cast directly to
size_twithout checking if they're negative. Negative values become very large positive numbers after casting, which could bypass the bounds check in some edge cases. -
Silent failures: Early returns provide no diagnostic information about why parsing failed, making debugging difficult and potentially masking malformed files.
-
Redundant checks:
header_->bodypartindexis checked three times inread_meshes(lines 602, 633, 660).
Recommended fixes:
- Validate structure sizes:
-if (static_cast<size_t>(texture_header_->textureindex) > mBuffersize) {
+if (texture_header_->textureindex < 0 ||
+ static_cast<size_t>(texture_header_->textureindex) + sizeof(Texture_HL1) > mBuffersize) {
+ ASSIMP_LOG_ERROR(MDL_HALFLIFE_LOG_HEADER "Texture index out of bounds");
return;
}- For arrays, validate the entire array:
-if (static_cast<size_t>(header_->boneindex) > mBuffersize) {
+if (header_->boneindex < 0 ||
+ static_cast<size_t>(header_->boneindex) + (header_->numbones * sizeof(Bone_HL1)) > mBuffersize) {
+ ASSIMP_LOG_ERROR(MDL_HALFLIFE_LOG_HEADER "Bone data out of bounds");
return;
}- Consider consolidating redundant checks and adding error messages to aid in debugging malformed files.
Also applies to: 454-456, 602-604, 633-635, 645-647, 660-662, 747-749, 763-765
🤖 Prompt for AI Agents
code/AssetLib/MDL/HalfLife/HL1MDLLoader.cpp lines 374-376 (also apply similar
fixes at 454-456, 602-604, 633-635, 645-647, 660-662, 747-749, 763-765): the
current bounds checks only compare an offset/index to mBuffersize after casting
to size_t which allows negative indices and does not ensure the full referenced
structure/array fits inside the buffer; update each check to (1) verify the raw
index is non-negative before casting, (2) compute the required end = offset +
sizeof(TargetStruct) or for arrays offset + count * elementSize and ensure end
<= mBuffersize, (3) replace plain early returns with a logged error/diagnostic
(or return a failure status) so malformed files are visible, and (4) consolidate
duplicate header_->bodypartindex checks into a single validated check used by
read_meshes to avoid repetition. Ensure these validation patterns are applied
consistently at the listed line ranges.
|
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.