HL1: Fix out of bound access

code/AssetLib/MDL/HalfLife/HL1FileData.h

@@ -139,7 +139,7 @@ struct Header_HL1 : HalfLifeMDLBaseHeader {
     //! The number of attachments.
     int32_t numattachments;
 
-    //! Offset the the first attachment chunk.
+    //! Offset of the first attachment chunk.
     int32_t attachmentindex;
 
     //! Was "soundtable".

code/AssetLib/MDL/HalfLife/HL1MDLLoader.cpp

@@ -50,17 +50,15 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #include <assimp/BaseImporter.h>
 #include <assimp/StringUtils.h>
-#include <assimp/ai_assert.h>
 #include <assimp/qnan.h>
 #include <assimp/DefaultLogger.hpp>
-#include <assimp/Importer.hpp>
 
 #include <iomanip>
 #include <sstream>
 #include <map>
 
 #ifdef MDL_HALFLIFE_LOG_WARN_HEADER
-#undef MDL_HALFLIFE_LOG_WARN_HEADER
+#  undef MDL_HALFLIFE_LOG_WARN_HEADER
 #endif
 #define MDL_HALFLIFE_LOG_HEADER "[Half-Life 1 MDL] "
 #include "LogFunctions.h"
@@ -74,30 +72,22 @@ namespace HalfLife {
 #endif // _MSC_VER
 
 // ------------------------------------------------------------------------------------------------
-HL1MDLLoader::HL1MDLLoader(
-    aiScene *scene,
-    IOSystem *io,
-    const unsigned char *buffer,
-    const std::string &file_path,
-    const HL1ImportSettings &import_settings) :
-    scene_(scene),
-    io_(io),
-    buffer_(buffer),
-    file_path_(file_path),
-    import_settings_(import_settings),
-    header_(nullptr),
-    texture_header_(nullptr),
-    anim_headers_(nullptr),
-    texture_buffer_(nullptr),
-    anim_buffers_(nullptr),
-    num_sequence_groups_(0),
-    rootnode_children_(),
-    unique_name_generator_(),
-    unique_sequence_names_(),
-    unique_sequence_groups_names_(),
-    temp_bones_(),
-    num_blend_controllers_(0),
-    total_models_(0) {
+HL1MDLLoader::HL1MDLLoader(aiScene *scene, IOSystem *io, const unsigned char *buffer, size_t buffersize,
+            const std::string &file_path, const HL1ImportSettings &import_settings) :
+        scene_(scene),
+        io_(io),
+        buffer_(buffer),
+        mBuffersize(buffersize),
+        file_path_(file_path),
+        import_settings_(import_settings),
+        header_(nullptr),
+        texture_header_(nullptr),
+        anim_headers_(nullptr),
+        texture_buffer_(nullptr),
+        anim_buffers_(nullptr),
+        num_sequence_groups_(0),
+        num_blend_controllers_(0),
+        total_models_(0) {
     load_file();
 }
 
@@ -457,8 +447,11 @@ void HL1MDLLoader::read_bones() {
         return;
     }
 
-    const Bone_HL1 *pbone = (const Bone_HL1 *)((uint8_t *)header_ + header_->boneindex);
+    if (header_->boneindex > mBuffersize) {
+        return;
+    }
 
+    const Bone_HL1 *pbone = (const Bone_HL1 *)((uint8_t *)header_ + header_->boneindex);
     std::vector<std::string> unique_bones_names(header_->numbones);
     for (int i = 0; i < header_->numbones; ++i) {
         unique_bones_names[i] = pbone[i].name;
@@ -605,6 +598,9 @@ void HL1MDLLoader::read_meshes() {
 
     for (int i = 0; i < header_->numbodyparts; ++i, ++pbodypart) {
         unique_bodyparts_names[i] = pbodypart->name;
+        if (header_->bodypartindex > mBuffersize) {
+            return;
+        }
 
         pmodel = (Model_HL1 *)((uint8_t *)header_ + pbodypart->modelindex);
         for (int j = 0; j < pbodypart->nummodels; ++j, ++pmodel) {
@@ -633,6 +629,9 @@ void HL1MDLLoader::read_meshes() {
     unique_name_generator_.make_unique(unique_bodyparts_names);
 
     // Now do the same for each model.
+    if (header_->bodypartindex > mBuffersize) {
+        return;
+    }
     pbodypart = (const Bodypart_HL1 *)((uint8_t *)header_ + header_->bodypartindex);
 
     // Prepare template name for bodypart models.
@@ -642,6 +641,10 @@ void HL1MDLLoader::read_meshes() {
     unsigned int model_index = 0;
 
     for (int i = 0; i < header_->numbodyparts; ++i, ++pbodypart) {
+        if (pbodypart->modelindex > mBuffersize) {
+            continue;
+        }
+
         pmodel = (Model_HL1 *)((uint8_t *)header_ + pbodypart->modelindex);
         for (int j = 0; j < pbodypart->nummodels; ++j, ++pmodel, ++model_index)
             unique_models_names[model_index] = pmodel->name;
@@ -653,7 +656,9 @@ void HL1MDLLoader::read_meshes() {
     unsigned int mesh_index = 0;
 
     scene_->mMeshes = new aiMesh *[scene_->mNumMeshes];
-
+    if (header_->bodypartindex > mBuffersize) {
+        return;
+    }
     pbodypart = (const Bodypart_HL1 *)((uint8_t *)header_ + header_->bodypartindex);
 
     /* Create a node that will represent the mesh hierarchy.

code/AssetLib/MDL/HalfLife/HL1MDLLoader.h

@@ -74,6 +74,7 @@ public:
         aiScene *scene,
         IOSystem *io,
         const unsigned char *buffer,
+        size_t buffersize,
         const std::string &file_path,
         const HL1ImportSettings &import_settings);
 
@@ -160,6 +161,8 @@ private:
     /** Buffer from MDLLoader class. */
     const unsigned char *buffer_;
 
+    size_t mBuffersize = 0l;
+
     /** The full file path to the MDL file we are trying to load.
      * Used to locate other MDL files since MDL may store resources
      * in external MDL files. */

code/AssetLib/MDL/MDLLoader.cpp

@@ -176,8 +176,8 @@ void MDLImporter::InternReadFile(const std::string &pFile,
 
     // This should work for all other types of MDL files, too ...
     // the HL1 sequence group header is one of the smallest, afaik
-    iFileSize = (unsigned int)file->FileSize();
-    if (iFileSize < sizeof(MDL::HalfLife::SequenceHeader_HL1)) {
+    mBuffersize = (unsigned int)file->FileSize();
+    if (mBuffersize < sizeof(HalfLife::SequenceHeader_HL1)) {
         throw DeadlyImportError("MDL File is too small.");
     }
 
@@ -193,13 +193,13 @@ void MDLImporter::InternReadFile(const std::string &pFile,
 
     try {
         // Allocate storage and copy the contents of the file to a memory buffer
-        mBuffer = new unsigned char[iFileSize + 1];
-        file->Read((void *)mBuffer, 1, iFileSize);
+        mBuffer = new unsigned char[mBuffersize + 1];
+        file->Read((void *)mBuffer, 1, mBuffersize);
 
         // Append a binary zero to the end of the buffer.
         // this is just for safety that string parsing routines
         // find the end of the buffer ...
-        mBuffer[iFileSize] = '\0';
+        mBuffer[mBuffersize] = '\0';
         const uint32_t iMagicWord = *((uint32_t *)mBuffer);
 
         // Determine the file subtype and call the appropriate member function
@@ -1985,6 +1985,7 @@ void MDLImporter::InternReadFile_HL1(const std::string &pFile, const uint32_t iM
             pScene,
             mIOHandler,
             mBuffer,
+            mBuffersize,
             pFile,
             mHL1ImportSettings);
 }

code/AssetLib/MDL/MDLLoader.h

@@ -426,7 +426,7 @@ protected:
 
     /** Buffer to hold the loaded file */
     unsigned char* mBuffer;
-
+    size_t mBuffersize = 0;
     /** For GameStudio MDL files: The number in the magic word, either 3,4 or 5
      * (MDL7 doesn't need this, the format has a separate loader) */
     unsigned int iGSFileVersion;

test/CMakeLists.txt

@@ -169,6 +169,7 @@ SET( IMPORTERS
   unit/ImportExport/MDL/utMDLImporter_HL1_ImportSettings.cpp
   unit/ImportExport/MDL/utMDLImporter_HL1_Materials.cpp
   unit/ImportExport/MDL/utMDLImporter_HL1_Nodes.cpp
+  unit/ImportExport/MDL/utH1MDLImporter.cpp
   unit/ImportExport/RAW/utRAWImportExport.cpp
   unit/ImportExport/Terragen/utTerragenImportExport.cpp
   unit/ImportExport/Pbrt/utPbrtImportExport.cpp

test/unit/ImportExport/MDL/utH1MDLImporter.cpp

@@ -0,0 +1,60 @@
+/*
+---------------------------------------------------------------------------
+Open Asset Import Library (assimp)
+---------------------------------------------------------------------------
+
+Copyright (c) 2006-2025, assimp team
+
+All rights reserved.
+
+Redistribution and use of this software in source and binary forms,
+with or without modification, are permitted provided that the following
+conditions are met:
+
+* Redistributions of source code must retain the above
+copyright notice, this list of conditions and the
+following disclaimer.
+
+* Redistributions in binary form must reproduce the above
+copyright notice, this list of conditions and the
+following disclaimer in the documentation and/or other
+materials provided with the distribution.
+
+* Neither the name of the assimp team, nor the names of its
+contributors may be used to endorse or promote products
+derived from this software without specific prior
+written permission of the assimp team.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+---------------------------------------------------------------------------
+*/
+
+/** @file utMDLImporter_HL1_ImportSettings.cpp
+ *  @brief Half-Life 1 MDL loader import settings tests.
+ */
+
+#include "AbstractImportExportBase.h"
+#include "UnitTestPCH.h"
+#include <assimp/postprocess.h>
+#include <assimp/scene.h>
+#include <assimp/Importer.hpp>
+
+using namespace Assimp;
+
+class utH1MDLLoader : public ::testing::Test {};
+
+TEST_F(utH1MDLLoader, parseInvalidFile_1) {
+    Importer importer;
+    const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR"/invalid/af51e2973bb947199c1192c98a4b3fe6.bin", aiProcess_ValidateDataStructure);
+    EXPECT_EQ(nullptr, scene);
+}