[GEN][ZH] Fix security issue in map transfer by validating file paths when creating real paths from portable paths

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

@@ -136,6 +136,8 @@ class FileSystem : public SubsystemInterface
 	Bool areMusicFilesOnCD();
 	void loadMusicFilesFromCD();
 	void unloadMusicFilesFromCD();
+	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.
+	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.
 protected:
 
 

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

@@ -50,6 +50,7 @@ class LocalFileSystem : public SubsystemInterface
 	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.
 	virtual Bool getFileInfo(const AsciiString& filename, FileInfo *fileInfo) const = 0; ///< see FileSystem.h
 	virtual Bool createDirectory(AsciiString directory) = 0; ///< see FileSystem.h
+	virtual AsciiString normalizePath(const AsciiString& filePath) const = 0;	///< see FileSystem.h
 
 protected:
 };

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

@@ -322,3 +322,54 @@ void FileSystem::unloadMusicFilesFromCD()
 
 	TheArchiveFileSystem->closeArchiveFile( MUSIC_BIG );
 }
+
+//============================================================================
+// FileSystem::normalizePath
+//============================================================================
+AsciiString FileSystem::normalizePath(const AsciiString& path) const
+{
+	return TheLocalFileSystem->normalizePath(path);
+}
+
+//============================================================================
+// FileSystem::isPathInDirectory
+//============================================================================
+Bool FileSystem::isPathInDirectory(const AsciiString& testPath, const AsciiString& basePath)
+{
+	AsciiString testPathNormalized = TheFileSystem->normalizePath(testPath);
+	AsciiString basePathNormalized = TheFileSystem->normalizePath(basePath);
+
+	if (basePathNormalized.isEmpty())
+	{
+		DEBUG_CRASH(("Unable to normalize base directory path '%s'.\n", basePath.str()));
+		return false;
+	}
+	else if (testPathNormalized.isEmpty())
+	{
+		DEBUG_CRASH(("Unable to normalize file path '%s'.\n", testPath.str()));
+		return false;
+	}
+
+#ifdef _WIN32
+	const char* pathSep = "\\";
+#else
+	const char* pathSep = "/";
+#endif
+
+	if (!basePathNormalized.endsWith(pathSep))
+	{
+		basePathNormalized.concat(pathSep);
+	}
+
+#ifdef _WIN32
+	if (!testPathNormalized.startsWithNoCase(basePathNormalized))
+#else
+	if (!testPathNormalized.startsWith(basePathNormalized))
+#endif
+	{
+		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()));
+		return false;
+	}
+
+	return true;
+}

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

@@ -783,7 +783,7 @@ AsciiString GameState::getFilePathInSaveDirectory(const AsciiString& leaf) const
 //-------------------------------------------------------------------------------------------------
 Bool GameState::isInSaveDirectory(const AsciiString& path) const
 {
-	return path.startsWithNoCase(getSaveDirectory());
+	return FileSystem::isPathInDirectory(path, getSaveDirectory());
 }
 
 // ------------------------------------------------------------------------------------------------
@@ -902,33 +902,43 @@ AsciiString GameState::realMapPathToPortableMapPath(const AsciiString& in) const
 AsciiString GameState::portableMapPathToRealMapPath(const AsciiString& in) const
 {
 	AsciiString prefix;
+	// The directory where the real map path should be contained in.
+	AsciiString containingBasePath;
 	if (in.startsWithNoCase(PORTABLE_SAVE))
 	{
 		// the save dir ends with "\\"
 		prefix = getSaveDirectory();
+		containingBasePath = prefix;
 		prefix.concat(getMapLeafName(in));
 	}
 	else if (in.startsWithNoCase(PORTABLE_MAPS))
 	{
 		// the map dir DOES NOT end with "\\", must add it
 		prefix = TheMapCache->getMapDir();
 		prefix.concat("\\");
+		containingBasePath = prefix;
 		prefix.concat(getMapLeafAndDirName(in));
 	}
 	else if (in.startsWithNoCase(PORTABLE_USER_MAPS))
 	{
 		// the map dir DOES NOT end with "\\", must add it
 		prefix = TheMapCache->getUserMapDir();
 		prefix.concat("\\");
+		containingBasePath = prefix;
 		prefix.concat(getMapLeafAndDirName(in));
 	}
 	else
 	{
 		DEBUG_CRASH(("Map file was not found in any of the expected directories; this is impossible"));
-		//throw INI_INVALID_DATA;
-		// uncaught exceptions crash us. better to just use a bad path.
-		prefix = in;
+		// Empty string represents a failure, either caused by an invalid prefix or a relative path leading outside the base path
+		return AsciiString::TheEmptyString;
 	}
+
+	if (!FileSystem::isPathInDirectory(prefix, containingBasePath))
+	{
+		return AsciiString::TheEmptyString;
+	}
+
 	prefix.toLower();
 	return prefix;
 }

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

@@ -688,7 +688,17 @@ void ConnectionManager::processFile(NetFileCommandMsg *msg)
 	DEBUG_LOG(("%ls\n", log.str()));
 #endif
 
-	if (TheFileSystem->doesFileExist(msg->getRealFilename().str()))
+	AsciiString realFileName = msg->getRealFilename();
+	if (realFileName.isEmpty())
+	{
+		// TheSuperHackers @security slurmlord 18/06/2025 As the file name/path from the NetFileCommandMsg failed to normalize,
+		// in other words is bogus and points outside of the approved target directory, avoid an arbitrary file overwrite vulnerability
+		// by simply returning and let the transfer time out.
+		DEBUG_LOG(("Got a file name transferred that failed to normalize: '%s'!\n", msg->getPortableFilename().str()));
+		return;
+	}
+
+	if (TheFileSystem->doesFileExist(realFileName.str()))
 	{
 		DEBUG_LOG(("File exists already!\n"));
 		//return;
@@ -720,13 +730,13 @@ void ConnectionManager::processFile(NetFileCommandMsg *msg)
 	}
 #endif // COMPRESS_TARGAS
 
-	File *fp = TheFileSystem->openFile(msg->getRealFilename().str(), File::CREATE | File::BINARY | File::WRITE);
+	File *fp = TheFileSystem->openFile(realFileName.str(), File::CREATE | File::BINARY | File::WRITE);
 	if (fp)
 	{
 		fp->write(buf, len);
 		fp->close();
 		fp = NULL;
-		DEBUG_LOG(("Wrote %d bytes to file %s!\n",len,msg->getRealFilename().str()));
+		DEBUG_LOG(("Wrote %d bytes to file %s!\n", len, realFileName.str()));
 
 	}
 	else

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

@@ -1044,7 +1044,17 @@ Bool ParseAsciiStringToGameInfo(GameInfo *game, AsciiString options)
 			mapName.concat(token);
 			mapName.concat('.');
 			mapName.concat(TheMapCache->getMapExtension());
-			mapName = TheGameState->portableMapPathToRealMapPath(mapName);
+			AsciiString realMapName = TheGameState->portableMapPathToRealMapPath(mapName);
+			if (realMapName.isEmpty())
+			{
+				// TheSuperHackers @security slurmlord 18/06/2025 As the map file name/path from the AsciiString failed to normalize,
+				// in other words is bogus and points outside of the approved target directory for maps, avoid an arbitrary file overwrite vulnerability
+				// 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.
+				optionsOk = FALSE;
+				DEBUG_LOG(("ParseAsciiStringToGameInfo - saw bogus map name ('%s'); quitting\n", mapName.str()));
+				break;
+			}
+			mapName = realMapName;
 			sawMap = true;
 			DEBUG_LOG(("ParseAsciiStringToGameInfo - map name is %s\n", mapName.str()));
 		}

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

@@ -49,6 +49,7 @@ class Win32LocalFileSystem : public LocalFileSystem
 	virtual Bool getFileInfo(const AsciiString& filename, FileInfo *fileInfo) const;
 
 	virtual Bool createDirectory(AsciiString directory);
+	virtual AsciiString normalizePath(const AsciiString& filePath) const;
 
 protected:
 };

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

@@ -214,3 +214,23 @@ Bool Win32LocalFileSystem::createDirectory(AsciiString directory)
 	}
 	return FALSE;
 }
+
+AsciiString Win32LocalFileSystem::normalizePath(const AsciiString& filePath) const
+{
+	DWORD retval = GetFullPathNameA(filePath.str(), 0, NULL, NULL);
+	if (retval == 0)
+	{
+		DEBUG_LOG(("Unable to determine buffer size for normalized file path. Error=(%u).\n", GetLastError()));
+		return AsciiString::TheEmptyString;
+	}
+
+	AsciiString normalizedFilePath;
+	retval = GetFullPathNameA(filePath.str(), retval, normalizedFilePath.getBufferForRead(retval - 1), NULL);
+	if (retval == 0)
+	{
+		DEBUG_LOG(("Unable to normalize file path '%s'. Error=(%u).\n", filePath.str(), GetLastError()));
+		return AsciiString::TheEmptyString;
+	}
+
+	return normalizedFilePath;
+}

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

@@ -140,6 +140,8 @@ class FileSystem : public SubsystemInterface
 	Bool areMusicFilesOnCD();
 	void loadMusicFilesFromCD();
 	void unloadMusicFilesFromCD();
+	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.
+	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.
 protected:
   mutable std::map<unsigned,bool> m_fileExist;
 };

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

@@ -50,6 +50,7 @@ class LocalFileSystem : public SubsystemInterface
 	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.
 	virtual Bool getFileInfo(const AsciiString& filename, FileInfo *fileInfo) const = 0; ///< see FileSystem.h
 	virtual Bool createDirectory(AsciiString directory) = 0; ///< see FileSystem.h
+	virtual AsciiString normalizePath(const AsciiString& filePath) const = 0;	///< see FileSystem.h
 
 protected:
 };

GeneralsMD/Code/GameEngine/Source/Common/System/FileSystem.cpp

@@ -339,3 +339,54 @@ void FileSystem::unloadMusicFilesFromCD()
 
 	TheArchiveFileSystem->closeArchiveFile( MUSIC_BIG );
 }
+
+//============================================================================
+// FileSystem::normalizePath
+//============================================================================
+AsciiString FileSystem::normalizePath(const AsciiString& path) const
+{
+	return TheLocalFileSystem->normalizePath(path);
+}
+
+//============================================================================
+// FileSystem::isPathInDirectory
+//============================================================================
+Bool FileSystem::isPathInDirectory(const AsciiString& testPath, const AsciiString& basePath)
+{
+	AsciiString testPathNormalized = TheFileSystem->normalizePath(testPath);
+	AsciiString basePathNormalized = TheFileSystem->normalizePath(basePath);
+
+	if (basePathNormalized.isEmpty())
+	{
+		DEBUG_CRASH(("Unable to normalize base directory path '%s'.\n", basePath.str()));
+		return false;
+	}
+	else if (testPathNormalized.isEmpty())
+	{
+		DEBUG_CRASH(("Unable to normalize file path '%s'.\n", testPath.str()));
+		return false;
+	}
+
+#ifdef _WIN32
+	const char* pathSep = "\\";
+#else
+	const char* pathSep = "/";
+#endif
+
+	if (!basePathNormalized.endsWith(pathSep))
+	{
+		basePathNormalized.concat(pathSep);
+	}
+
+#ifdef _WIN32
+	if (!testPathNormalized.startsWithNoCase(basePathNormalized))
+#else
+	if (!testPathNormalized.startsWith(basePathNormalized))
+#endif
+	{
+		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()));
+		return false;
+	}
+
+	return true;
+}

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

@@ -783,7 +783,7 @@ AsciiString GameState::getFilePathInSaveDirectory(const AsciiString& leaf) const
 //-------------------------------------------------------------------------------------------------
 Bool GameState::isInSaveDirectory(const AsciiString& path) const
 {
-	return path.startsWithNoCase(getSaveDirectory());
+	return FileSystem::isPathInDirectory(path, getSaveDirectory());
 }
 
 // ------------------------------------------------------------------------------------------------
@@ -902,33 +902,43 @@ AsciiString GameState::realMapPathToPortableMapPath(const AsciiString& in) const
 AsciiString GameState::portableMapPathToRealMapPath(const AsciiString& in) const
 {
 	AsciiString prefix;
+	// The directory where the real map path should be contained in.
+	AsciiString containingBasePath;
 	if (in.startsWithNoCase(PORTABLE_SAVE))
 	{
 		// the save dir ends with "\\"
 		prefix = getSaveDirectory();
+		containingBasePath = prefix;
 		prefix.concat(getMapLeafName(in));
 	}
 	else if (in.startsWithNoCase(PORTABLE_MAPS))
 	{
 		// the map dir DOES NOT end with "\\", must add it
 		prefix = TheMapCache->getMapDir();
 		prefix.concat("\\");
+		containingBasePath = prefix;
 		prefix.concat(getMapLeafAndDirName(in));
 	}
 	else if (in.startsWithNoCase(PORTABLE_USER_MAPS))
 	{
 		// the map dir DOES NOT end with "\\", must add it
 		prefix = TheMapCache->getUserMapDir();
 		prefix.concat("\\");
+		containingBasePath = prefix;
 		prefix.concat(getMapLeafAndDirName(in));
 	}
 	else
 	{
 		DEBUG_CRASH(("Map file was not found in any of the expected directories; this is impossible"));
-		//throw INI_INVALID_DATA;
-		// uncaught exceptions crash us. better to just use a bad path.
-		prefix = in;
+		// Empty string represents a failure, either caused by an invalid prefix or a relative path leading outside the base path.
+		return AsciiString::TheEmptyString;
 	}
+
+	if (!FileSystem::isPathInDirectory(prefix, containingBasePath))
+	{
+		return AsciiString::TheEmptyString;
+	}
+
 	prefix.toLower();
 	return prefix;
 }

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

@@ -688,7 +688,17 @@ void ConnectionManager::processFile(NetFileCommandMsg *msg)
 	DEBUG_LOG(("%ls\n", log.str()));
 #endif
 
-	if (TheFileSystem->doesFileExist(msg->getRealFilename().str()))
+	AsciiString realFileName = msg->getRealFilename();
+	if (realFileName.isEmpty())
+	{
+		// TheSuperHackers @security slurmlord 18/06/2025 As the file name/path from the NetFileCommandMsg failed to normalize,
+		// in other words is bogus and points outside of the approved target directory, avoid an arbitrary file overwrite vulnerability
+		// by simply returning and let the transfer time out.
+		DEBUG_LOG(("Got a file name transferred that failed to normalize: '%s'!\n", msg->getPortableFilename().str()));
+		return;
+	}
+
+	if (TheFileSystem->doesFileExist(realFileName.str()))
 	{
 		DEBUG_LOG(("File exists already!\n"));
 		//return;
@@ -720,13 +730,13 @@ void ConnectionManager::processFile(NetFileCommandMsg *msg)
 	}
 #endif // COMPRESS_TARGAS
 
-	File *fp = TheFileSystem->openFile(msg->getRealFilename().str(), File::CREATE | File::BINARY | File::WRITE);
+	File *fp = TheFileSystem->openFile(realFileName.str(), File::CREATE | File::BINARY | File::WRITE);
 	if (fp)
 	{
 		fp->write(buf, len);
 		fp->close();
 		fp = NULL;
-		DEBUG_LOG(("Wrote %d bytes to file %s!\n",len,msg->getRealFilename().str()));
+		DEBUG_LOG(("Wrote %d bytes to file %s!\n", len, realFileName.str()));
 
 	}
 	else

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

@@ -1081,7 +1081,17 @@ Bool ParseAsciiStringToGameInfo(GameInfo *game, AsciiString options)
 			mapName.concat(token);
 			mapName.concat('.');
 			mapName.concat(TheMapCache->getMapExtension());
-			mapName = TheGameState->portableMapPathToRealMapPath(mapName);
+			AsciiString realMapName = TheGameState->portableMapPathToRealMapPath(mapName);
+			if (realMapName.isEmpty())
+			{
+				// TheSuperHackers @security slurmlord 18/06/2025 As the map file name/path from the AsciiString failed to normalize,
+				// in other words is bogus and points outside of the approved target directory for maps, avoid an arbitrary file overwrite vulnerability
+				// 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.
+				optionsOk = FALSE;
+				DEBUG_LOG(("ParseAsciiStringToGameInfo - saw bogus map name ('%s'); quitting\n", mapName.str()));
+				break;
+			}
+			mapName = realMapName;
 			sawMap = true;
 			DEBUG_LOG(("ParseAsciiStringToGameInfo - map name is %s\n", mapName.str()));
 		}