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 <noreply@anthropic.com>

dCommon/BinaryIO.h

@@ -5,6 +5,8 @@
 
 #include <iostream>
 #include <fstream>
+#include <cstdint>
+#include <stdexcept>
 #include <string>
 
 #include "Game.h"
@@ -24,6 +26,27 @@ namespace BinaryIO {
 		return stream.read(reinterpret_cast<char*>(&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<uint64_t>(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<uint64_t>(size) > UNCHECKED_LENGTH) RequireRemaining(stream, static_cast<uint64_t>(size) * sizeof(uint16_t));
 		value.resize(size);
 		stream.read(reinterpret_cast<char*>(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<uint64_t>(size) > UNCHECKED_LENGTH) {
+			RequireRemaining(stream, static_cast<uint64_t>(size) * (readType == ReadType::WideString ? sizeof(uint16_t) : 1));
+		}
 		value.resize(size);
 		if (readType == ReadType::WideString) {
 			uint16_t wideChar;

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"

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<uint64_t>(techniqueCount) * (64 + 4 + 1 + 16 * 4));
 				obj.renderTechniques.resize(techniqueCount);
 				for (auto& technique : obj.renderTechniques) {
 					technique.name = ReadName64(file);

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<std::streamsize>(chunk.size()));
 		if (!file) throw std::runtime_error("Failed to read from istream.");

dZoneManager/CMakeLists.txt

@@ -1,6 +1,5 @@
 set(DZONEMANAGER_SOURCES "dZoneManager.cpp"
 	"Level.cpp"
-	"Raw.cpp"
 	"Spawner.cpp"
 	"Zone.cpp")
 

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")
 

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<uint32_t>(41).Put<uint32_t>(3).Put<uint32_t>(1150);
+	w.Point(1, 2, 3).Put(1.0f).Put(0.0f).Put(0.0f).Put(0.0f);
+	w.Put<uint32_t>(0);
+	w.Put<uint8_t>(0).Text("zone.raw").Text("Name").Text("Description");
+	w.Put<uint32_t>(0);
+	w.Put<uint32_t>(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<uint16_t>(30).Put<uint16_t>(0);
+	w.data.append(48 + 12, '\0');
+	w.Put<uint32_t>(0).Put<uint32_t>(1);
+	w.Put<int64_t>(10).Put<int32_t>(1);
+	w.Point(0, 0, 0).Put(1.0f).Put(0.0f).Put(0.0f).Put(0.0f).Put(1.0f);
+	w.Put<uint32_t>(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<uint16_t>(30).Put<uint16_t>(0);
+	w.data.append(48 + 12, '\0');
+	w.Put<uint32_t>(0).Put<uint32_t>(1);
+	w.Put<int64_t>(10).Put<int32_t>(1);
+	w.Point(0, 0, 0).Put(1.0f).Put(0.0f).Put(0.0f).Put(0.0f).Put(1.0f);
+	w.Put<uint32_t>(0);          // no config
+	w.Put<uint32_t>(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);
+}

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)