Skip to content

Commit 887892d

Browse files
slurmlordslurmlord
authored
[GEN][ZH] Fix security issue in map transfer by validating file paths when creating real paths from portable paths (#1058)
1 parent cdffbac commit 887892d

File tree

18 files changed

+238
-16
lines changed

18 files changed

+238
-16
lines changed

‎Generals/Code/GameEngine/Include/Common/FileSystem.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,8 @@ class FileSystem : public SubsystemInterface
136136
Bool areMusicFilesOnCD();
137137
void loadMusicFilesFromCD();
138138
void unloadMusicFilesFromCD();
139+
AsciiString normalizePath(const AsciiString& path) const; ///< 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.
140+
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.
139141
protected:
140142

141143

‎Generals/Code/GameEngine/Include/Common/LocalFileSystem.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ class LocalFileSystem : public SubsystemInterface
5050
virtual void getFileListInDirectory(const AsciiString& currentDirectory, const AsciiString& originalDirectory, const AsciiString& searchName, FilenameList &filenameList, Bool searchSubdirectories) const = 0; ///< search the given directory for files matching the searchName (egs. *.ini, *.rep). Possibly search subdirectories.
5151
virtual Bool getFileInfo(const AsciiString& filename, FileInfo *fileInfo) const = 0; ///< see FileSystem.h
5252
virtual Bool createDirectory(AsciiString directory) = 0; ///< see FileSystem.h
53+
virtual AsciiString normalizePath(const AsciiString& filePath) const = 0; ///< see FileSystem.h
5354

5455
protected:
5556
};

‎Generals/Code/GameEngine/Source/Common/System/FileSystem.cpp

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,3 +322,54 @@ void FileSystem::unloadMusicFilesFromCD()
322322

323323
TheArchiveFileSystem->closeArchiveFile( MUSIC_BIG );
324324
}
325+
326+
//============================================================================
327+
// FileSystem::normalizePath
328+
//============================================================================
329+
AsciiString FileSystem::normalizePath(const AsciiString& path) const
330+
{
331+
return TheLocalFileSystem->normalizePath(path);
332+
}
333+
334+
//============================================================================
335+
// FileSystem::isPathInDirectory
336+
//============================================================================
337+
Bool FileSystem::isPathInDirectory(const AsciiString& testPath, const AsciiString& basePath)
338+
{
339+
AsciiString testPathNormalized = TheFileSystem->normalizePath(testPath);
340+
AsciiString basePathNormalized = TheFileSystem->normalizePath(basePath);
341+
342+
if (basePathNormalized.isEmpty())
343+
{
344+
DEBUG_CRASH(("Unable to normalize base directory path '%s'.\n", basePath.str()));
345+
return false;
346+
}
347+
else if (testPathNormalized.isEmpty())
348+
{
349+
DEBUG_CRASH(("Unable to normalize file path '%s'.\n", testPath.str()));
350+
return false;
351+
}
352+
353+
#ifdef _WIN32
354+
const char* pathSep = "\\";
355+
#else
356+
const char* pathSep = "/";
357+
#endif
358+
359+
if (!basePathNormalized.endsWith(pathSep))
360+
{
361+
basePathNormalized.concat(pathSep);
362+
}
363+
364+
#ifdef _WIN32
365+
if (!testPathNormalized.startsWithNoCase(basePathNormalized))
366+
#else
367+
if (!testPathNormalized.startsWith(basePathNormalized))
368+
#endif
369+
{
370+
DEBUG_CRASH(("Normalized file path for '%s': '%s' was outside the expected base path of '%s' (normalized: '%s').\n", testPath.str(), testPathNormalized.str(), basePath.str(), basePathNormalized.str()));
371+
return false;
372+
}
373+
374+
return true;
375+
}

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

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -783,7 +783,7 @@ AsciiString GameState::getFilePathInSaveDirectory(const AsciiString& leaf) const
783783
//-------------------------------------------------------------------------------------------------
784784
Bool GameState::isInSaveDirectory(const AsciiString& path) const
785785
{
786-
return path.startsWithNoCase(getSaveDirectory());
786+
return FileSystem::isPathInDirectory(path, getSaveDirectory());
787787
}
788788

789789
// ------------------------------------------------------------------------------------------------
@@ -902,33 +902,43 @@ AsciiString GameState::realMapPathToPortableMapPath(const AsciiString& in) const
902902
AsciiString GameState::portableMapPathToRealMapPath(const AsciiString& in) const
903903
{
904904
AsciiString prefix;
905+
// The directory where the real map path should be contained in.
906+
AsciiString containingBasePath;
905907
if (in.startsWithNoCase(PORTABLE_SAVE))
906908
{
907909
// the save dir ends with "\\"
908910
prefix = getSaveDirectory();
911+
containingBasePath = prefix;
909912
prefix.concat(getMapLeafName(in));
910913
}
911914
else if (in.startsWithNoCase(PORTABLE_MAPS))
912915
{
913916
// the map dir DOES NOT end with "\\", must add it
914917
prefix = TheMapCache->getMapDir();
915918
prefix.concat("\\");
919+
containingBasePath = prefix;
916920
prefix.concat(getMapLeafAndDirName(in));
917921
}
918922
else if (in.startsWithNoCase(PORTABLE_USER_MAPS))
919923
{
920924
// the map dir DOES NOT end with "\\", must add it
921925
prefix = TheMapCache->getUserMapDir();
922926
prefix.concat("\\");
927+
containingBasePath = prefix;
923928
prefix.concat(getMapLeafAndDirName(in));
924929
}
925930
else
926931
{
927932
DEBUG_CRASH(("Map file was not found in any of the expected directories; this is impossible"));
928-
//throw INI_INVALID_DATA;
929-
// uncaught exceptions crash us. better to just use a bad path.
930-
prefix = in;
933+
// Empty string represents a failure, either caused by an invalid prefix or a relative path leading outside the base path
934+
return AsciiString::TheEmptyString;
931935
}
936+
937+
if (!FileSystem::isPathInDirectory(prefix, containingBasePath))
938+
{
939+
return AsciiString::TheEmptyString;
940+
}
941+
932942
prefix.toLower();
933943
return prefix;
934944
}

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

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -688,7 +688,17 @@ void ConnectionManager::processFile(NetFileCommandMsg *msg)
688688
DEBUG_LOG(("%ls\n", log.str()));
689689
#endif
690690

691-
if (TheFileSystem->doesFileExist(msg->getRealFilename().str()))
691+
AsciiString realFileName = msg->getRealFilename();
692+
if (realFileName.isEmpty())
693+
{
694+
// TheSuperHackers @security slurmlord 18/06/2025 As the file name/path from the NetFileCommandMsg failed to normalize,
695+
// in other words is bogus and points outside of the approved target directory, avoid an arbitrary file overwrite vulnerability
696+
// by simply returning and let the transfer time out.
697+
DEBUG_LOG(("Got a file name transferred that failed to normalize: '%s'!\n", msg->getPortableFilename().str()));
698+
return;
699+
}
700+
701+
if (TheFileSystem->doesFileExist(realFileName.str()))
692702
{
693703
DEBUG_LOG(("File exists already!\n"));
694704
//return;
@@ -720,13 +730,13 @@ void ConnectionManager::processFile(NetFileCommandMsg *msg)
720730
}
721731
#endif // COMPRESS_TARGAS
722732

723-
File *fp = TheFileSystem->openFile(msg->getRealFilename().str(), File::CREATE | File::BINARY | File::WRITE);
733+
File *fp = TheFileSystem->openFile(realFileName.str(), File::CREATE | File::BINARY | File::WRITE);
724734
if (fp)
725735
{
726736
fp->write(buf, len);
727737
fp->close();
728738
fp = NULL;
729-
DEBUG_LOG(("Wrote %d bytes to file %s!\n",len,msg->getRealFilename().str()));
739+
DEBUG_LOG(("Wrote %d bytes to file %s!\n", len, realFileName.str()));
730740

731741
}
732742
else

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

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1044,7 +1044,17 @@ Bool ParseAsciiStringToGameInfo(GameInfo *game, AsciiString options)
10441044
mapName.concat(token);
10451045
mapName.concat('.');
10461046
mapName.concat(TheMapCache->getMapExtension());
1047-
mapName = TheGameState->portableMapPathToRealMapPath(mapName);
1047+
AsciiString realMapName = TheGameState->portableMapPathToRealMapPath(mapName);
1048+
if (realMapName.isEmpty())
1049+
{
1050+
// TheSuperHackers @security slurmlord 18/06/2025 As the map file name/path from the AsciiString failed to normalize,
1051+
// in other words is bogus and points outside of the approved target directory for maps, avoid an arbitrary file overwrite vulnerability
1052+
// 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.
1053+
optionsOk = FALSE;
1054+
DEBUG_LOG(("ParseAsciiStringToGameInfo - saw bogus map name ('%s'); quitting\n", mapName.str()));
1055+
break;
1056+
}
1057+
mapName = realMapName;
10481058
sawMap = true;
10491059
DEBUG_LOG(("ParseAsciiStringToGameInfo - map name is %s\n", mapName.str()));
10501060
}

‎Generals/Code/GameEngineDevice/Include/Win32Device/Common/Win32LocalFileSystem.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ class Win32LocalFileSystem : public LocalFileSystem
4949
virtual Bool getFileInfo(const AsciiString& filename, FileInfo *fileInfo) const;
5050

5151
virtual Bool createDirectory(AsciiString directory);
52+
virtual AsciiString normalizePath(const AsciiString& filePath) const;
5253

5354
protected:
5455
};

‎Generals/Code/GameEngineDevice/Source/Win32Device/Common/Win32LocalFileSystem.cpp

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -214,3 +214,23 @@ Bool Win32LocalFileSystem::createDirectory(AsciiString directory)
214214
}
215215
return FALSE;
216216
}
217+
218+
AsciiString Win32LocalFileSystem::normalizePath(const AsciiString& filePath) const
219+
{
220+
DWORD retval = GetFullPathNameA(filePath.str(), 0, NULL, NULL);
221+
if (retval == 0)
222+
{
223+
DEBUG_LOG(("Unable to determine buffer size for normalized file path. Error=(%u).\n", GetLastError()));
224+
return AsciiString::TheEmptyString;
225+
}
226+
227+
AsciiString normalizedFilePath;
228+
retval = GetFullPathNameA(filePath.str(), retval, normalizedFilePath.getBufferForRead(retval - 1), NULL);
229+
if (retval == 0)
230+
{
231+
DEBUG_LOG(("Unable to normalize file path '%s'. Error=(%u).\n", filePath.str(), GetLastError()));
232+
return AsciiString::TheEmptyString;
233+
}
234+
235+
return normalizedFilePath;
236+
}

‎GeneralsMD/Code/GameEngine/Include/Common/FileSystem.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,8 @@ class FileSystem : public SubsystemInterface
140140
Bool areMusicFilesOnCD();
141141
void loadMusicFilesFromCD();
142142
void unloadMusicFilesFromCD();
143+
AsciiString normalizePath(const AsciiString& path) const; ///< 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.
144+
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.
143145
protected:
144146
mutable std::map<unsigned,bool> m_fileExist;
145147
};

‎GeneralsMD/Code/GameEngine/Include/Common/LocalFileSystem.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ class LocalFileSystem : public SubsystemInterface
5050
virtual void getFileListInDirectory(const AsciiString& currentDirectory, const AsciiString& originalDirectory, const AsciiString& searchName, FilenameList &filenameList, Bool searchSubdirectories) const = 0; ///< search the given directory for files matching the searchName (egs. *.ini, *.rep). Possibly search subdirectories.
5151
virtual Bool getFileInfo(const AsciiString& filename, FileInfo *fileInfo) const = 0; ///< see FileSystem.h
5252
virtual Bool createDirectory(AsciiString directory) = 0; ///< see FileSystem.h
53+
virtual AsciiString normalizePath(const AsciiString& filePath) const = 0; ///< see FileSystem.h
5354

5455
protected:
5556
};

0 commit comments

Comments
 (0)