From f70433c430ab7298bc676a86409396c7283fd371 Mon Sep 17 00:00:00 2001 From: Aaron Kimbrell Date: Tue, 10 Nov 2026 17:41:01 -0600 Subject: [PATCH] fix(zone): lengths and counts in zone files are checked against what is left of the file A string length, the path chunk's length and an object's render technique count are compared with the bytes left before memory is set aside for them. The terrain reader is built into dCommon, so every server that reads zone files has it. Co-Authored-By: Claude Fable 5.2 --- dCommon/BinaryIO.h | 27 +++++++++++++++++ dCommon/CMakeLists.txt | 4 ++- dCommon/LevelFile.cpp | 2 ++ dCommon/ZoneFile.cpp | 1 + dZoneManager/CMakeLists.txt | 1 - tests/dCommonTests/CMakeLists.txt | 2 -- tests/dCommonTests/ZoneFileTests.cpp | 43 ++++++++++++++++++++++++++++ tests/dWebTests/CMakeLists.txt | 2 -- 8 files changed, 76 insertions(+), 6 deletions(-) diff --git a/dCommon/BinaryIO.h b/dCommon/BinaryIO.h index 634d7ce79..75fb2896b 100644 --- a/dCommon/BinaryIO.h +++ b/dCommon/BinaryIO.h @@ -5,6 +5,8 @@ #include #include +#include +#include #include #include "Game.h" @@ -24,6 +26,27 @@ namespace BinaryIO { return stream.read(reinterpret_cast(&value), sizeof(T)); } + // Bytes left to read in a seekable stream (0 when it can't tell) + inline uint64_t Remaining(std::istream& stream) { + const auto position = stream.tellg(); + if (position < 0) return 0; + stream.seekg(0, std::ios::end); + const auto end = stream.tellg(); + stream.seekg(position); + return end > position ? static_cast(end - position) : 0; + } + + /** + * Throws when the stream has fewer than `bytes` left. For counts and lengths read from a file, checked before + * memory is set aside for them, so a file can't ask for more than it holds. + */ + inline void RequireRemaining(std::istream& stream, uint64_t bytes) { + if (bytes > Remaining(stream)) throw std::runtime_error("Failed to read from istream."); + } + + // Lengths up to this are read without looking at what is left (every u8 length) + constexpr uint64_t UNCHECKED_LENGTH = 255; + enum class ReadType : int8_t { WideString = 0, String = 1, @@ -37,6 +60,7 @@ namespace BinaryIO { BinaryRead(stream, size); if (!stream.good()) throw std::runtime_error("Failed to read from istream."); + if (static_cast(size) > UNCHECKED_LENGTH) RequireRemaining(stream, static_cast(size) * sizeof(uint16_t)); value.resize(size); stream.read(reinterpret_cast(value.data()), size * sizeof(uint16_t)); } @@ -49,6 +73,9 @@ namespace BinaryIO { BinaryRead(stream, size); if (!stream.good()) throw std::runtime_error("Failed to read from istream."); + if (static_cast(size) > UNCHECKED_LENGTH) { + RequireRemaining(stream, static_cast(size) * (readType == ReadType::WideString ? sizeof(uint16_t) : 1)); + } value.resize(size); if (readType == ReadType::WideString) { uint16_t wideChar; diff --git a/dCommon/CMakeLists.txt b/dCommon/CMakeLists.txt index 66636b435..261aac37f 100644 --- a/dCommon/CMakeLists.txt +++ b/dCommon/CMakeLists.txt @@ -41,6 +41,8 @@ set(DCOMMON_SOURCES "Permissions.cpp" "PermissionGrants.cpp" "NifFile.cpp" + # The terrain reader: every server that reads zone files uses it, not only the worlds + "${PROJECT_SOURCE_DIR}/dZoneManager/Raw.cpp" ) # Workaround for compiler bug where the optimized code could result in a memcpy of 0 bytes, even though that isnt possible. @@ -58,7 +60,7 @@ endforeach() add_library(dCommon STATIC ${DCOMMON_SOURCES}) target_include_directories(dCommon PUBLIC "." "dClient" "dEnums" - "${PROJECT_SOURCE_DIR}/dZoneManager" # Raw.h (ZoneScenes.h); its reader is dZoneManager's + "${PROJECT_SOURCE_DIR}/dZoneManager" # Raw.h and its reader PRIVATE "${PROJECT_SOURCE_DIR}/dDatabase/GameDatabase" "${PROJECT_SOURCE_DIR}/dDatabase/GameDatabase/ITables" diff --git a/dCommon/LevelFile.cpp b/dCommon/LevelFile.cpp index 0a5409c8e..8b2388630 100644 --- a/dCommon/LevelFile.cpp +++ b/dCommon/LevelFile.cpp @@ -247,6 +247,8 @@ void LevelFile::ReadSceneObjectDataChunk(std::istream& file) { BinaryIO::BinaryRead(file, techniqueCount); if (techniqueCount != 0) { obj.renderTechniquesName = ReadName64(file); + // A technique is its 64 byte name, a count, a flag and 16 values + BinaryIO::RequireRemaining(file, static_cast(techniqueCount) * (64 + 4 + 1 + 16 * 4)); obj.renderTechniques.resize(techniqueCount); for (auto& technique : obj.renderTechniques) { technique.name = ReadName64(file); diff --git a/dCommon/ZoneFile.cpp b/dCommon/ZoneFile.cpp index c4eb85e27..dcebaed77 100644 --- a/dCommon/ZoneFile.cpp +++ b/dCommon/ZoneFile.cpp @@ -64,6 +64,7 @@ void ZoneFile::Read(std::istream& file) { if (fileFormatVersion >= FileFormatVersion::EarlyAlpha) { // The paths are a chunk of their own (LuzFile::ReadLUZFile copies it, ReadLUZPaths reads it) BinaryIO::BinaryRead(file, pathDataLength); + BinaryIO::RequireRemaining(file, pathDataLength); std::string chunk(pathDataLength, '\0'); file.read(chunk.data(), static_cast(chunk.size())); if (!file) throw std::runtime_error("Failed to read from istream."); diff --git a/dZoneManager/CMakeLists.txt b/dZoneManager/CMakeLists.txt index ea4e84719..32d9a785d 100644 --- a/dZoneManager/CMakeLists.txt +++ b/dZoneManager/CMakeLists.txt @@ -1,6 +1,5 @@ set(DZONEMANAGER_SOURCES "dZoneManager.cpp" "Level.cpp" - "Raw.cpp" "Spawner.cpp" "Zone.cpp") diff --git a/tests/dCommonTests/CMakeLists.txt b/tests/dCommonTests/CMakeLists.txt index 061904fbd..51d4f85d5 100644 --- a/tests/dCommonTests/CMakeLists.txt +++ b/tests/dCommonTests/CMakeLists.txt @@ -62,8 +62,6 @@ endif() target_link_libraries(dCommonTests ${COMMON_LIBRARIES} MD5 GTest::gtest_main) # SpareBackoff.h (header only) target_include_directories(dCommonTests PRIVATE "${PROJECT_SOURCE_DIR}/dMasterServer") -# The terrain reader (ClientZoneFilesTests reads every client .raw), without the rest of dZoneManager -target_sources(dCommonTests PRIVATE "${PROJECT_SOURCE_DIR}/dZoneManager/Raw.cpp") # ChatFilterCore.h (header only) target_include_directories(dCommonTests PRIVATE "${PROJECT_SOURCE_DIR}/dChatFilter") diff --git a/tests/dCommonTests/ZoneFileTests.cpp b/tests/dCommonTests/ZoneFileTests.cpp index 2ea75b951..622def895 100644 --- a/tests/dCommonTests/ZoneFileTests.cpp +++ b/tests/dCommonTests/ZoneFileTests.cpp @@ -571,3 +571,46 @@ TEST(LevelFileTests, DamagedFilesKeepWhatWasRead) { EXPECT_THROW(level.Read(stream), std::runtime_error); EXPECT_EQ(level.objects.size(), 1u); } + +// A length or count larger than what is left of the file is refused before any memory is set aside for it +TEST(ZoneFileTests, OversizedPathChunkLengthThrows) { + ZoneBytes w; + w.Put(41).Put(3).Put(1150); + w.Point(1, 2, 3).Put(1.0f).Put(0.0f).Put(0.0f).Put(0.0f); + w.Put(0); + w.Put(0).Text("zone.raw").Text("Name").Text("Description"); + w.Put(0); + w.Put(0xFFFFFFF0); // the path chunk's length, with nothing after it + std::istringstream stream(w.data); + ZoneFile file; + EXPECT_THROW(file.Read(stream), std::runtime_error); +} + +TEST(LevelFileTests, OversizedConfigLengthThrows) { + ZoneBytes w; + w.Put(30).Put(0); + w.data.append(48 + 12, '\0'); + w.Put(0).Put(1); + w.Put(10).Put(1); + w.Point(0, 0, 0).Put(1.0f).Put(0.0f).Put(0.0f).Put(0.0f).Put(1.0f); + w.Put(0x7FFFFFFF); // the config's length in characters + std::istringstream stream(w.data); + LevelFile level; + EXPECT_THROW(level.Read(stream), std::runtime_error); + EXPECT_TRUE(level.objects.empty()); +} + +TEST(LevelFileTests, OversizedRenderTechniqueCountThrows) { + ZoneBytes w; + w.Put(30).Put(0); + w.data.append(48 + 12, '\0'); + w.Put(0).Put(1); + w.Put(10).Put(1); + w.Point(0, 0, 0).Put(1.0f).Put(0.0f).Put(0.0f).Put(0.0f).Put(1.0f); + w.Put(0); // no config + w.Put(0x0FFFFFFF); // render techniques + w.data.append(64, '\0'); // their name + std::istringstream stream(w.data); + LevelFile level; + EXPECT_THROW(level.Read(stream), std::runtime_error); +} diff --git a/tests/dWebTests/CMakeLists.txt b/tests/dWebTests/CMakeLists.txt index 89618c562..695448118 100644 --- a/tests/dWebTests/CMakeLists.txt +++ b/tests/dWebTests/CMakeLists.txt @@ -93,8 +93,6 @@ target_include_directories(dWebTests PRIVATE "${PROJECT_SOURCE_DIR}/thirdparty/nlohmann" ) -# The terrain reader (RawTerrainTests, TerrainMap), without the rest of dZoneManager -target_sources(dWebTests PRIVATE "${PROJECT_SOURCE_DIR}/dZoneManager/Raw.cpp") target_link_libraries(dWebTests ${COMMON_LIBRARIES} dWeb dUgc sqlite3 OpenSSL::Crypto CURL::libcurl GTest::gtest_main) gtest_discover_tests(dWebTests)