From 28c81c1a7c5e428580976e3afcf8761943298aca Mon Sep 17 00:00:00 2001 From: Bobby Battista Date: Thu, 6 Nov 2025 18:17:20 -0500 Subject: [PATCH 1/2] fix(security): add file extension validation for map transfer --- Core/GameEngine/Include/Common/FileSystem.h | 1 + .../Source/Common/System/FileSystem.cpp | 44 +++++++++++++++++++ .../Source/GameNetwork/ConnectionManager.cpp | 37 ++++++++++++++++ .../Common/System/SaveGame/GameState.cpp | 7 +++ .../Common/System/SaveGame/GameState.cpp | 7 +++ 5 files changed, 96 insertions(+) diff --git a/Core/GameEngine/Include/Common/FileSystem.h b/Core/GameEngine/Include/Common/FileSystem.h index 51bf9153ca..15cebfe2c7 100644 --- a/Core/GameEngine/Include/Common/FileSystem.h +++ b/Core/GameEngine/Include/Common/FileSystem.h @@ -161,6 +161,7 @@ class FileSystem : public SubsystemInterface static AsciiString normalizePath(const AsciiString& path); ///< normalizes a file path. The path can refer to a directory. File path must be absolute, but does not need to exist. Returns an empty string on failure. static Bool isPathInDirectory(const AsciiString& testPath, const AsciiString& basePath); ///< determines if a file path is within a base path. Both paths must be absolute, but do not need to exist. + static Bool hasValidTransferFileExtension(const AsciiString& filePath); ///< validates that a file path has an approved extension for map transfer operations. Returns true if extension is whitelisted (.map, .ini, .str, .wak, .tga, .txt), false otherwise. TheSuperHackers @security bobtista 06/11/2025 protected: #if ENABLE_FILESYSTEM_EXISTENCE_CACHE diff --git a/Core/GameEngine/Source/Common/System/FileSystem.cpp b/Core/GameEngine/Source/Common/System/FileSystem.cpp index 46d468d4a0..1e21dd7a63 100644 --- a/Core/GameEngine/Source/Common/System/FileSystem.cpp +++ b/Core/GameEngine/Source/Common/System/FileSystem.cpp @@ -457,3 +457,47 @@ Bool FileSystem::isPathInDirectory(const AsciiString& testPath, const AsciiStrin return true; } + +//============================================================================ +// FileSystem::hasValidTransferFileExtension +//============================================================================ +// TheSuperHackers @security bobtista 06/11/2025 +// Validates file extensions for map transfer operations to prevent arbitrary +// file types from being transferred over the network or loaded from save games. +// This is a security measure to mitigate arbitrary file write vulnerabilities. +//============================================================================ +Bool FileSystem::hasValidTransferFileExtension(const AsciiString& filePath) +{ + static const char* validExtensions[] = { + ".map", + ".ini", + ".str", + ".wak", + ".tga", + ".txt" + }; + static const Int numValidExtensions = sizeof(validExtensions) / sizeof(validExtensions[0]); + + const char* lastDot = strrchr(filePath.str(), '.'); + + if (lastDot == NULL || lastDot[1] == '\0') + { + DEBUG_LOG(("File path '%s' has no extension.", filePath.str())); + return false; + } + + for (Int i = 0; i < numValidExtensions; ++i) + { +#ifdef _WIN32 + if (_stricmp(lastDot, validExtensions[i]) == 0) +#else + if (strcasecmp(lastDot, validExtensions[i]) == 0) +#endif + { + return true; + } + } + + DEBUG_LOG(("File path '%s' has invalid extension '%s' for transfer operations.", filePath.str(), lastDot)); + return false; +} diff --git a/Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp b/Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp index 739874c387..66b4e0f2b4 100644 --- a/Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp +++ b/Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp @@ -53,6 +53,36 @@ #include "GameClient/DisconnectMenu.h" #include "GameClient/InGameUI.h" +static Bool hasValidTransferFileExtension(const AsciiString& filePath) +{ + static const char* const validExtensions[] = { + "map", + "ini", + "str", + "wak", + "tga", + "txt" + }; + + const char* fileExt = strrchr(filePath.str(), '.'); + + if (fileExt == NULL || fileExt[1] == '\0') + { + return false; + } + + fileExt++; + + for (Int i = 0; i < ARRAY_SIZE(validExtensions); ++i) + { + if (stricmp(fileExt, validExtensions[i]) == 0) + { + return true; + } + } + + return false; +} /** * Le destructor. @@ -665,6 +695,13 @@ void ConnectionManager::processFile(NetFileCommandMsg *msg) return; } + // TheSuperHackers @security bobtista 06/11/2025 Validate file extension to prevent arbitrary file types + if (!hasValidTransferFileExtension(realFileName)) + { + DEBUG_LOG(("File '%s' has invalid extension for transfer operations.", realFileName.str())); + return; + } + if (TheFileSystem->doesFileExist(realFileName.str())) { DEBUG_LOG(("File exists already!")); diff --git a/Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp b/Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp index ee1d58a385..45f9d18d63 100644 --- a/Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp +++ b/Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp @@ -933,6 +933,13 @@ AsciiString GameState::portableMapPathToRealMapPath(const AsciiString& in) const return AsciiString::TheEmptyString; } + // TheSuperHackers @security bobtista 06/11/2025 Validate file extension to prevent arbitrary file types + if (!FileSystem::hasValidTransferFileExtension(prefix)) + { + DEBUG_LOG(("File path '%s' has invalid extension for map transfer operations.", prefix.str())); + return AsciiString::TheEmptyString; + } + prefix.toLower(); return prefix; } diff --git a/GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp b/GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp index 5b6bd01a26..fd382a2abc 100644 --- a/GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp +++ b/GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp @@ -933,6 +933,13 @@ AsciiString GameState::portableMapPathToRealMapPath(const AsciiString& in) const return AsciiString::TheEmptyString; } + // TheSuperHackers @security bobtista 06/11/2025 Validate file extension to prevent arbitrary file types + if (!FileSystem::hasValidTransferFileExtension(prefix)) + { + DEBUG_LOG(("File path '%s' has invalid extension for map transfer operations.", prefix.str())); + return AsciiString::TheEmptyString; + } + prefix.toLower(); return prefix; } From 5648a6d4ef4388b9f5d9c36158298267da45a386 Mon Sep 17 00:00:00 2001 From: Bobby Battista Date: Mon, 10 Nov 2025 15:29:37 -0500 Subject: [PATCH 2/2] refactor(security): address code review feedback on extension validation --- Core/GameEngine/Include/Common/FileSystem.h | 1 - .../Source/Common/System/FileSystem.cpp | 44 ------------------- .../Common/System/SaveGame/GameState.cpp | 7 --- .../Common/System/SaveGame/GameState.cpp | 7 --- 4 files changed, 59 deletions(-) diff --git a/Core/GameEngine/Include/Common/FileSystem.h b/Core/GameEngine/Include/Common/FileSystem.h index 15cebfe2c7..51bf9153ca 100644 --- a/Core/GameEngine/Include/Common/FileSystem.h +++ b/Core/GameEngine/Include/Common/FileSystem.h @@ -161,7 +161,6 @@ class FileSystem : public SubsystemInterface static AsciiString normalizePath(const AsciiString& path); ///< normalizes a file path. The path can refer to a directory. File path must be absolute, but does not need to exist. Returns an empty string on failure. static Bool isPathInDirectory(const AsciiString& testPath, const AsciiString& basePath); ///< determines if a file path is within a base path. Both paths must be absolute, but do not need to exist. - static Bool hasValidTransferFileExtension(const AsciiString& filePath); ///< validates that a file path has an approved extension for map transfer operations. Returns true if extension is whitelisted (.map, .ini, .str, .wak, .tga, .txt), false otherwise. TheSuperHackers @security bobtista 06/11/2025 protected: #if ENABLE_FILESYSTEM_EXISTENCE_CACHE diff --git a/Core/GameEngine/Source/Common/System/FileSystem.cpp b/Core/GameEngine/Source/Common/System/FileSystem.cpp index 1e21dd7a63..46d468d4a0 100644 --- a/Core/GameEngine/Source/Common/System/FileSystem.cpp +++ b/Core/GameEngine/Source/Common/System/FileSystem.cpp @@ -457,47 +457,3 @@ Bool FileSystem::isPathInDirectory(const AsciiString& testPath, const AsciiStrin return true; } - -//============================================================================ -// FileSystem::hasValidTransferFileExtension -//============================================================================ -// TheSuperHackers @security bobtista 06/11/2025 -// Validates file extensions for map transfer operations to prevent arbitrary -// file types from being transferred over the network or loaded from save games. -// This is a security measure to mitigate arbitrary file write vulnerabilities. -//============================================================================ -Bool FileSystem::hasValidTransferFileExtension(const AsciiString& filePath) -{ - static const char* validExtensions[] = { - ".map", - ".ini", - ".str", - ".wak", - ".tga", - ".txt" - }; - static const Int numValidExtensions = sizeof(validExtensions) / sizeof(validExtensions[0]); - - const char* lastDot = strrchr(filePath.str(), '.'); - - if (lastDot == NULL || lastDot[1] == '\0') - { - DEBUG_LOG(("File path '%s' has no extension.", filePath.str())); - return false; - } - - for (Int i = 0; i < numValidExtensions; ++i) - { -#ifdef _WIN32 - if (_stricmp(lastDot, validExtensions[i]) == 0) -#else - if (strcasecmp(lastDot, validExtensions[i]) == 0) -#endif - { - return true; - } - } - - DEBUG_LOG(("File path '%s' has invalid extension '%s' for transfer operations.", filePath.str(), lastDot)); - return false; -} diff --git a/Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp b/Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp index 45f9d18d63..ee1d58a385 100644 --- a/Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp +++ b/Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp @@ -933,13 +933,6 @@ AsciiString GameState::portableMapPathToRealMapPath(const AsciiString& in) const return AsciiString::TheEmptyString; } - // TheSuperHackers @security bobtista 06/11/2025 Validate file extension to prevent arbitrary file types - if (!FileSystem::hasValidTransferFileExtension(prefix)) - { - DEBUG_LOG(("File path '%s' has invalid extension for map transfer operations.", prefix.str())); - return AsciiString::TheEmptyString; - } - prefix.toLower(); return prefix; } diff --git a/GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp b/GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp index fd382a2abc..5b6bd01a26 100644 --- a/GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp +++ b/GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp @@ -933,13 +933,6 @@ AsciiString GameState::portableMapPathToRealMapPath(const AsciiString& in) const return AsciiString::TheEmptyString; } - // TheSuperHackers @security bobtista 06/11/2025 Validate file extension to prevent arbitrary file types - if (!FileSystem::hasValidTransferFileExtension(prefix)) - { - DEBUG_LOG(("File path '%s' has invalid extension for map transfer operations.", prefix.str())); - return AsciiString::TheEmptyString; - } - prefix.toLower(); return prefix; }