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)