Security issues in the code

[ell1e]

Since this seems to be turning into an active fork which is amazing. You might be interested in this: https://chaos.social/@gsuberland/114077566516453231 It seems like there are some problem with potentially writing to people's disk via remote attacks.

[xezon]

Can you perhaps write a summary of these information in here? I did not check if the long conversation explores more details.

[ell1e]

I wouldn't understand the details, but one bug seems to relate to map loading during multiplayer. Apparently missing maps or missing assets are put into a location indicated by the remote (e.g. an attacker), and if that location path is formatted a certain way it seems like you can make the game write that file anywhere on disk with server-controlled contents. GameState::portableMapPathToRealMapPath was mentioned.

The other bug seems to somehow relate to replay files when opened for viewing being able to cause disk writes in arbitrary locations, but again I wouldn't understand the details since I didn't find these bugs.

That's what that poster claimed, from what I can tell.

[tintinhamans]

Maybe @gsuberland can comment.

[gsuberland]

The posts I made there were based on a very brief dig through the code on a whim, so I haven't actually gone through and done a build to trace through and verify any findings, but I can give you a rough overview of the flow I spotted.

When a game save is made, either embedPristineMap or embedInUseMap may be called, embedding the map file in the save file.

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameStateMap.cpp

Lines 359 to 384 in 0a05454

	if( xfer->getXferMode() == XFER_SAVE )
	{
	//
	// if this is a first save from a pristine map load, we need to copy the pristine
	// map into the save game file
	//
	if( firstSave == TRUE )
	{
	embedPristineMap( saveGameInfo->pristineMapName, xfer );
	} // end if, first save
	else
	{
	//
	// this is *NOT* a first save from a pristine map, just read the map file
	// that was extracted from the save game file during the last load and embedd
	// that into the save game file
	//
	embedInUseMap( saveGameInfo->saveGameMapName, xfer );
	} // end else
	} // end if, save

When the game save is loaded in GameStateMap::xfer, the name of the map is read from the file. This string is passed through portableMapPathToRealMapPath to convert the "portable path" (a generic cross-platform path representation) to a real filesystem path.

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameStateMap.cpp

Lines 328 to 332 in 0a05454

	// read the save game map name
	AsciiString tmp;
	xfer->xferAsciiString( &tmp );
	saveGameInfo->saveGameMapName = TheGameState->portableMapPathToRealMapPath(tmp);

In GameState::portableMapPathToRealMapPath there is a fall-through case where, if the map name string does not match any of the expected prefixes, the input string is passed straight through. There is an assertion here, but it's only enabled for debug builds, leaving release builds vulnerable.

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

Lines 925 to 933 in 0a05454

	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;
	}
	prefix.toLower();
	return prefix;

Once this returns back to GameStateMap::xfer, there is an attempt to verify that saveGameMapName, which now contains the local file path, is within the save directory. However, this check is flawed and pristineMapName is also not checked.

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameStateMap.cpp

Lines 332 to 346 in 0a05454

	saveGameInfo->saveGameMapName = TheGameState->portableMapPathToRealMapPath(tmp);
	if (!TheGameState->isInSaveDirectory(saveGameInfo->saveGameMapName))
	{
	DEBUG_CRASH(("GameState::xfer - The map filename read from the file '%s' is not in the SAVE directory, but should be\n",
	saveGameInfo->saveGameMapName.str()) );
	throw SC_INVALID_DATA;
	}
	// set this map as the map to load in the global data
	TheWritableGlobalData->m_mapName = saveGameInfo->saveGameMapName;
	// read the pristine map filename
	xfer->xferAsciiString( &saveGameInfo->pristineMapName );
	saveGameInfo->pristineMapName = TheGameState->portableMapPathToRealMapPath(saveGameInfo->pristineMapName);

The check uses GameState::isInSaveDirectory, which is implemented as follows:

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

Lines 784 to 787 in 0a05454

	Bool GameState::isInSaveDirectory(const AsciiString& path) const
	{
	return path.startsWithNoCase(getSaveDirectory());
	}

Since this only checks if the incoming path starts with the expected path, it doesn't protect against relative path strings. As an example, let's say the expected save path is maps\. All an attacker needs to do is say the map file name is maps\..\..\evil.exe. Since maps\..\..\evil.exe starts with maps\, it is deemed as being within the save directory and passes the check. The correct thing to do here is canonicalise the paths first and then perform a prefix comparison (I'll go into details below).

Back in GameStateMap::xfer, after the check passes the extractAndSaveMap function is called, passing in the map path.

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameStateMap.cpp

Lines 388 to 392 in 0a05454

	//
	// take the embedded map file out of the save file, and save as its own .map file
	// in the save directory temporarily
	//
	extractAndSaveMap( saveGameInfo->saveGameMapName, xfer );

This opens the path, with no checks to ensure that the path is within the expected save file path, and no checks to ensure that the file is a valid map.

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameStateMap.cpp

Lines 190 to 195 in 0a05454

	static void extractAndSaveMap( AsciiString mapToSave, Xfer *xfer )
	{
	UnsignedInt dataSize;
	// open handle to output file
	FILE *fp = fopen( mapToSave.str(), "w+b" );

The issue here is that a maliciously crafted save file can embed arbitrary data in the map chunk and an arbitrary path to extract it to. When the user loads the save, the game extracts the data to the chosen path. This leads to an arbitrary file overwrite vulnerability.

As an immediate quick-and-dirty hotfix, I would recommend the following:

• Change GameState::portableMapPathToRealMapPath to throw an assertion failure (or otherwise return an error state that prevents the load process from continuing) if the incoming path string does not match one of the expected prefixes.
• Add a check to the beginning of GameState::portableMapPathToRealMapPath to see if the map string name contains .. and throw an assertion failure (or otherwise error out) if it does. This is probably easiest to do by adding an AsciiString::contains(...) function and using that.

I should be clear that this is by no means a completely robust solution, but it is a quick-and-easy fix you can do straight away with a minimum amount of code that will make it much harder to exploit.

The "proper" way to fix this is by implementing robust path traversal prevention. This is remarkably tricky to do correctly, especially if you wish to support both Windows and Linux/BSD, but here's a quick overview of the steps:

1. Translate the portable path provided in the save file to the local filesystem path, using portableMapPathToRealMapPath. I would recommend that you name the variable holding this path in such a way that it is clear that this is an untrusted path.
2. Canonicalise the untrusted local filesystem path. On Windows you can do this with GetFullPathName. This converts the path string provided (which may be relative or contain things like ..\) to its full canonical path on the filesystem, using the exact same logic as if you were going to open the file.
3. Get the path to the directory where you expect the map to be extracted to. Based on a brief read of the code it looks like it should get extracted to a temporary directory, but that may not be the only case.
4. Canonicalise that directory path, too, using the same approach (e.g. GetFullPathName on Windows).
5. If the canonicalised directory path does not end with a directory separator character (i.e. \ on Windows, / on Linux/BSD), append one to it. This is critical as it prevents a class of neighbouring directory traversal attacks.
6. Verify that the canonicalised filesystem path from step 2 begins with the canonicalised directory path from step 5, using the correct comparison type for the platform. On Windows you can assume a case-insensitive ordinal comparison is correct, and from a quick look it seems that AsciiString::startsWithNoCase implements this comparison. On Linux/BSD I'm not 100% sure but I would guess case-sensitive ordinal comparison would be the correct approach.

For further details and some example code, take a look at this proposal I wrote for .NET: dotnet/runtime#69591

In your case I would recommend implementing this path traversal check as its own helper function, e.g. bool isPathInDirectory(const AsciiString& untrustedPath, const AsciiString& baseDirectory) so that you can reuse it. You can then modify GameState::isInSaveDirectory to use that function internally. Don't forget to also modify GameStateMap::xfer to perform a second check on the pristine map file name so that it is also protected.

[xezon]

Other improvements that can be done:

• Validate file extensions (.map, .ini, .str, .wak)
• Validate file contents (advanced)

[bobtista]

Should this be closed given 1058 was merged?