Skip to content

Commit 7ec0ac6

Browse files
bobtistabobtista
committed
refactor(security): address code review feedback on extension validation
1 parent a22a246 commit 7ec0ac6

File tree

8 files changed

+92
-67
lines changed

8 files changed

+92
-67
lines changed

‎Core/GameEngine/Include/Common/FileSystem.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,6 @@ class FileSystem : public SubsystemInterface
161161

162162
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.
163163
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.
164-
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
165164

166165
protected:
167166
#if ENABLE_FILESYSTEM_EXISTENCE_CACHE

‎Core/GameEngine/Source/Common/System/FileSystem.cpp‎

Lines changed: 0 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -457,47 +457,3 @@ Bool FileSystem::isPathInDirectory(const AsciiString& testPath, const AsciiStrin
457457

458458
return true;
459459
}
460-
461-
//============================================================================
462-
// FileSystem::hasValidTransferFileExtension
463-
//============================================================================
464-
// TheSuperHackers @security bobtista 06/11/2025
465-
// Validates file extensions for map transfer operations to prevent arbitrary
466-
// file types from being transferred over the network or loaded from save games.
467-
// This is a security measure to mitigate arbitrary file write vulnerabilities.
468-
//============================================================================
469-
Bool FileSystem::hasValidTransferFileExtension(const AsciiString& filePath)
470-
{
471-
static const char* validExtensions[] = {
472-
".map",
473-
".ini",
474-
".str",
475-
".wak",
476-
".tga",
477-
".txt"
478-
};
479-
static const Int numValidExtensions = sizeof(validExtensions) / sizeof(validExtensions[0]);
480-
481-
const char* lastDot = strrchr(filePath.str(), '.');
482-
483-
if (lastDot == NULL || lastDot[1] == '\0')
484-
{
485-
DEBUG_LOG(("File path '%s' has no extension.", filePath.str()));
486-
return false;
487-
}
488-
489-
for (Int i = 0; i < numValidExtensions; ++i)
490-
{
491-
#ifdef _WIN32
492-
if (_stricmp(lastDot, validExtensions[i]) == 0)
493-
#else
494-
if (strcasecmp(lastDot, validExtensions[i]) == 0)
495-
#endif
496-
{
497-
return true;
498-
}
499-
}
500-
501-
DEBUG_LOG(("File path '%s' has invalid extension '%s' for transfer operations.", filePath.str(), lastDot));
502-
return false;
503-
}

‎Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -933,13 +933,6 @@ AsciiString GameState::portableMapPathToRealMapPath(const AsciiString& in) const
933933
return AsciiString::TheEmptyString;
934934
}
935935

936-
// TheSuperHackers @security bobtista 06/11/2025 Validate file extension to prevent arbitrary file types
937-
if (!FileSystem::hasValidTransferFileExtension(prefix))
938-
{
939-
DEBUG_LOG(("File path '%s' has invalid extension for map transfer operations.", prefix.str()));
940-
return AsciiString::TheEmptyString;
941-
}
942-
943936
prefix.toLower();
944937
return prefix;
945938
}

‎Generals/Code/GameEngine/Source/GameNetwork/ConnectionManager.cpp‎

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,45 @@
5353
#include "GameClient/DisconnectMenu.h"
5454
#include "GameClient/InGameUI.h"
5555

56+
//============================================================================
57+
// hasValidTransferFileExtension
58+
//============================================================================
59+
// TheSuperHackers @security bobtista 06/11/2025
60+
// Validates file extensions for map transfer operations to prevent arbitrary
61+
// file types from being transferred over the network or loaded from save games.
62+
//============================================================================
63+
static Bool hasValidTransferFileExtension(const AsciiString& filePath)
64+
{
65+
static const char* const validExtensions[] = {
66+
"map",
67+
"ini",
68+
"str",
69+
"wak",
70+
"tga",
71+
"txt"
72+
};
73+
74+
const char* fileExt = strrchr(filePath.str(), '.');
75+
76+
if (fileExt == NULL || fileExt[1] == '\0')
77+
{
78+
DEBUG_LOG(("File path '%s' has no extension.", filePath.str()));
79+
return false;
80+
}
81+
82+
fileExt++;
83+
84+
for (Int i = 0; i < ARRAY_SIZE(validExtensions); ++i)
85+
{
86+
if (stricmp(fileExt, validExtensions[i]) == 0)
87+
{
88+
return true;
89+
}
90+
}
91+
92+
DEBUG_LOG(("File path '%s' has invalid extension '%s' for transfer operations.", filePath.str(), fileExt));
93+
return false;
94+
}
5695

5796
/**
5897
* Le destructor.
@@ -661,12 +700,17 @@ void ConnectionManager::processFile(NetFileCommandMsg *msg)
661700
// TheSuperHackers @security slurmlord 18/06/2025 As the file name/path from the NetFileCommandMsg failed to normalize,
662701
// in other words is bogus and points outside of the approved target directory, avoid an arbitrary file overwrite vulnerability
663702
// by simply returning and let the transfer time out.
664-
// TheSuperHackers @security bobtista 06/11/2025 This check also rejects files with invalid extensions (not in whitelist),
665-
// as portableMapPathToRealMapPath() performs extension validation and returns empty string on failure.
666703
DEBUG_LOG(("Got a file name transferred that failed to normalize: '%s'!", msg->getPortableFilename().str()));
667704
return;
668705
}
669706

707+
// TheSuperHackers @security bobtista 06/11/2025 Validate file extension to prevent arbitrary file types
708+
if (!hasValidTransferFileExtension(realFileName))
709+
{
710+
DEBUG_LOG(("File '%s' has invalid extension for transfer operations.", realFileName.str()));
711+
return;
712+
}
713+
670714
if (TheFileSystem->doesFileExist(realFileName.str()))
671715
{
672716
DEBUG_LOG(("File exists already!"));

‎Generals/Code/GameEngine/Source/GameNetwork/GameInfo.cpp‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1094,8 +1094,6 @@ Bool ParseAsciiStringToGameInfo(GameInfo *game, AsciiString options)
10941094
// TheSuperHackers @security slurmlord 18/06/2025 As the map file name/path from the AsciiString failed to normalize,
10951095
// in other words is bogus and points outside of the approved target directory for maps, avoid an arbitrary file overwrite vulnerability
10961096
// if the save or network game embeds a custom map to store at the location, by flagging the options as not OK and rejecting the game.
1097-
// TheSuperHackers @security bobtista 06/11/2025 This check also rejects map names with invalid extensions,
1098-
// as portableMapPathToRealMapPath() performs extension validation and returns empty string on failure.
10991097
optionsOk = FALSE;
11001098
DEBUG_LOG(("ParseAsciiStringToGameInfo - saw bogus map name ('%s'); quitting", mapName.str()));
11011099
break;

‎GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -933,13 +933,6 @@ AsciiString GameState::portableMapPathToRealMapPath(const AsciiString& in) const
933933
return AsciiString::TheEmptyString;
934934
}
935935

936-
// TheSuperHackers @security bobtista 06/11/2025 Validate file extension to prevent arbitrary file types
937-
if (!FileSystem::hasValidTransferFileExtension(prefix))
938-
{
939-
DEBUG_LOG(("File path '%s' has invalid extension for map transfer operations.", prefix.str()));
940-
return AsciiString::TheEmptyString;
941-
}
942-
943936
prefix.toLower();
944937
return prefix;
945938
}

‎GeneralsMD/Code/GameEngine/Source/GameNetwork/ConnectionManager.cpp‎

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,45 @@
5353
#include "GameClient/DisconnectMenu.h"
5454
#include "GameClient/InGameUI.h"
5555

56+
//============================================================================
57+
// hasValidTransferFileExtension
58+
//============================================================================
59+
// TheSuperHackers @security bobtista 06/11/2025
60+
// Validates file extensions for map transfer operations to prevent arbitrary
61+
// file types from being transferred over the network or loaded from save games.
62+
//============================================================================
63+
static Bool hasValidTransferFileExtension(const AsciiString& filePath)
64+
{
65+
static const char* const validExtensions[] = {
66+
"map",
67+
"ini",
68+
"str",
69+
"wak",
70+
"tga",
71+
"txt"
72+
};
73+
74+
const char* fileExt = strrchr(filePath.str(), '.');
75+
76+
if (fileExt == NULL || fileExt[1] == '\0')
77+
{
78+
DEBUG_LOG(("File path '%s' has no extension.", filePath.str()));
79+
return false;
80+
}
81+
82+
fileExt++;
83+
84+
for (Int i = 0; i < ARRAY_SIZE(validExtensions); ++i)
85+
{
86+
if (stricmp(fileExt, validExtensions[i]) == 0)
87+
{
88+
return true;
89+
}
90+
}
91+
92+
DEBUG_LOG(("File path '%s' has invalid extension '%s' for transfer operations.", filePath.str(), fileExt));
93+
return false;
94+
}
5695

5796
/**
5897
* Le destructor.
@@ -661,12 +700,17 @@ void ConnectionManager::processFile(NetFileCommandMsg *msg)
661700
// TheSuperHackers @security slurmlord 18/06/2025 As the file name/path from the NetFileCommandMsg failed to normalize,
662701
// in other words is bogus and points outside of the approved target directory, avoid an arbitrary file overwrite vulnerability
663702
// by simply returning and let the transfer time out.
664-
// TheSuperHackers @security bobtista 06/11/2025 This check also rejects files with invalid extensions (not in whitelist),
665-
// as portableMapPathToRealMapPath() performs extension validation and returns empty string on failure.
666703
DEBUG_LOG(("Got a file name transferred that failed to normalize: '%s'!", msg->getPortableFilename().str()));
667704
return;
668705
}
669706

707+
// TheSuperHackers @security bobtista 06/11/2025 Validate file extension to prevent arbitrary file types
708+
if (!hasValidTransferFileExtension(realFileName))
709+
{
710+
DEBUG_LOG(("File '%s' has invalid extension for transfer operations.", realFileName.str()));
711+
return;
712+
}
713+
670714
if (TheFileSystem->doesFileExist(realFileName.str()))
671715
{
672716
DEBUG_LOG(("File exists already!"));

‎GeneralsMD/Code/GameEngine/Source/GameNetwork/GameInfo.cpp‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1094,8 +1094,6 @@ Bool ParseAsciiStringToGameInfo(GameInfo *game, AsciiString options)
10941094
// TheSuperHackers @security slurmlord 18/06/2025 As the map file name/path from the AsciiString failed to normalize,
10951095
// in other words is bogus and points outside of the approved target directory for maps, avoid an arbitrary file overwrite vulnerability
10961096
// if the save or network game embeds a custom map to store at the location, by flagging the options as not OK and rejecting the game.
1097-
// TheSuperHackers @security bobtista 06/11/2025 This check also rejects map names with invalid extensions,
1098-
// as portableMapPathToRealMapPath() performs extension validation and returns empty string on failure.
10991097
optionsOk = FALSE;
11001098
DEBUG_LOG(("ParseAsciiStringToGameInfo - saw bogus map name ('%s'); quitting", mapName.str()));
11011099
break;

0 commit comments

Comments
 (0)