fix(security): add file content validation for map transfer #1819
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ | |
| #include "Common/CRCDebug.h" | ||
| #include "Common/Debug.h" | ||
| #include "Common/file.h" | ||
| #include "Common/FileSystem.h" | ||
| #include "Common/GameAudio.h" | ||
| #include "Common/LocalFileSystem.h" | ||
| #include "Common/Player.h" | ||
|
|
@@ -52,6 +53,7 @@ | |
| #include "GameLogic/VictoryConditions.h" | ||
| #include "GameClient/DisconnectMenu.h" | ||
| #include "GameClient/InGameUI.h" | ||
| #include "TARGA.h" | ||
|
|
||
| static Bool hasValidTransferFileExtension(const AsciiString& filePath) | ||
| { | ||
|
|
@@ -84,6 +86,125 @@ static Bool hasValidTransferFileExtension(const AsciiString& filePath) | |
| return false; | ||
| } | ||
|
|
||
| enum TransferFileType | ||
| { | ||
| TransferFileType_Invalid = -1, | ||
| TransferFileType_Map, | ||
| TransferFileType_Ini, | ||
| TransferFileType_Str, | ||
| TransferFileType_Txt, | ||
| TransferFileType_Tga, | ||
| TransferFileType_Wak, | ||
| TransferFileType_Count | ||
| }; | ||
|
|
||
| struct TransferFileRule | ||
| { | ||
| const char* ext; | ||
| UnsignedInt maxSize; | ||
| }; | ||
|
|
||
| static const TransferFileRule transferFileRules[TransferFileType_Count] = | ||
| { | ||
| { ".map", 5 * 1024 * 1024 }, | ||
| { ".ini", 512 * 1024 }, | ||
| { ".str", 512 * 1024 }, | ||
| { ".txt", 512 * 1024 }, | ||
| { ".tga", 2 * 1024 * 1024 }, | ||
| { ".wak", 512 * 1024 }, | ||
|
There was a problem hiding this comment. I was about to merge this change, but looking over these sizes I do wonder how they came to be. TGA size looks pretty big, INI maybe too small for a massive Mod Map? And why is TXT there? Does the game actually transfer it? Also, I think we need to make these values configurable in an INI file, so that Mods can customize file transfer limits. There was a problem hiding this comment. According to Kabuse, a 2 MB map is already insane, so 5 MB is on the safe side. I would be against a configurable ini. This would defeat the purpose of the security measure as a malicious host could still transfer large files. There was a problem hiding this comment. HLH - Black Dawn has a map.ini file slightly larger than 1024 KB fwiw. There was a problem hiding this comment.
Can you elaborate on this security threat? How would that work? There was a problem hiding this comment.
Uncapped file size in general poses a security thread (denial of service), which could lead to overfilled memory and/or disk - causing game or computer crash. The 2 minute timeout in the transfer window partially that infinite dos isn't possible - which alleviates some of the thread. File size of transfers should be capped in the application for proper security. Allowing users to configure that as an option defeats the purpose of such a cap. Make it 5 or 10 MB if you want to play it safe, but I think it is better to understand what a realistic filesize is for each type. I've got 500 maps installed on my PC, the largest files are .tga and .map, each still well under 1 MB, so we are looking at the extreme maps. Based on @Caball009 feedback, ini may have to go to 2 MB, but the rest should be good. There was a problem hiding this comment.
It still is unclear how that would become an issue. INI file settings are local. If the local user does not change them, then there is no threat level for his local machine. The INI configurability allows Mod Creators to customize the limits for when they ship larger content than the base game does. There was a problem hiding this comment. I see your point. Agreed, making this configurable in the ini options gives mod creators the freedom they need. There was a problem hiding this comment. No legitimate mod maker is going to instruct a player in the match chat to go edit their .ini files; if we're priming users to make local security changes based on lobby chat, we have much bigger problems than file size limits, the threat is social not technical. There was a problem hiding this comment.
That is not the intent. The intent is that the Mod Creator can customize these limits that previously did not exist, if he knows that his Mod has different requirements than the retail game content. |
||
| }; | ||
|
|
||
| static TransferFileType getTransferFileType(const char* extension) | ||
| { | ||
| for (Int i = 0; i < TransferFileType_Count; ++i) | ||
| { | ||
| if (stricmp(extension, transferFileRules[i].ext) == 0) | ||
| { | ||
| return static_cast<TransferFileType>(i); | ||
| } | ||
| } | ||
| return TransferFileType_Invalid; | ||
| } | ||
|
|
||
| static Bool hasValidTransferFileContent(const AsciiString& filePath, const UnsignedByte* data, UnsignedInt dataSize) | ||
| { | ||
| const char* fileExt = strrchr(filePath.str(), '.'); | ||
| if (fileExt == nullptr) | ||
| { | ||
| DEBUG_LOG(("File '%s' has no extension for content validation.", filePath.str())); | ||
| return false; | ||
| } | ||
|
|
||
| const TransferFileType fileType = getTransferFileType(fileExt); | ||
| if (fileType == TransferFileType_Invalid) | ||
| { | ||
| DEBUG_LOG(("File '%s' has unrecognized extension '%s' for content validation.", filePath.str(), fileExt)); | ||
| return false; | ||
| } | ||
|
|
||
| // Check size limit | ||
| const TransferFileRule& rule = transferFileRules[fileType]; | ||
| if (dataSize > rule.maxSize) | ||
| { | ||
| DEBUG_LOG(("File '%s' exceeds maximum size (%u bytes, limit %u bytes).", filePath.str(), dataSize, rule.maxSize)); | ||
| return false; | ||
| } | ||
|
|
||
| // Extension-specific content validation | ||
| switch (fileType) | ||
| { | ||
| case TransferFileType_Map: | ||
| { | ||
| if (dataSize < 4 || memcmp(data, "CkMp", 4) != 0) | ||
| { | ||
| DEBUG_LOG(("Map file '%s' has invalid magic bytes.", filePath.str())); | ||
| return false; | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| case TransferFileType_Ini: | ||
| { | ||
| for (UnsignedInt i = 0; i < dataSize; ++i) | ||
| { | ||
| if (data[i] == 0) | ||
| { | ||
| DEBUG_LOG(("INI file '%s' contains null bytes (likely binary).", filePath.str())); | ||
| return false; | ||
| } | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| case TransferFileType_Tga: | ||
| { | ||
| if (dataSize < sizeof(TGAHeader) + sizeof(TGA2Footer)) | ||
| { | ||
| DEBUG_LOG(("TGA file '%s' is too small to be valid.", filePath.str())); | ||
| return false; | ||
| } | ||
| TGA2Footer footer; | ||
| memcpy(&footer, data + dataSize - sizeof(footer), sizeof(footer)); | ||
| const Bool isTGA2 = memcmp(footer.Signature, TGA2_SIGNATURE, sizeof(footer.Signature)) == 0 | ||
| && footer.RsvdChar == '.' | ||
| && footer.BZST == '\0'; | ||
| if (!isTGA2) | ||
| { | ||
| DEBUG_LOG(("TGA file '%s' is missing TRUEVISION-XFILE footer signature.", filePath.str())); | ||
| return false; | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| default: | ||
| { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Le destructor. | ||
| */ | ||
|
|
@@ -734,6 +855,20 @@ void ConnectionManager::processFile(NetFileCommandMsg *msg) | |
| } | ||
| #endif // COMPRESS_TARGAS | ||
|
|
||
| // TheSuperHackers @security bobtista 12/02/2026 Validate file content in memory before writing to disk | ||
| if (!hasValidTransferFileContent(realFileName, buf, len)) | ||
| { | ||
| DEBUG_LOG(("File '%s' failed content validation. Transfer aborted.", realFileName.str())); | ||
| #ifdef COMPRESS_TARGAS | ||
| if (deleteBuf) | ||
| { | ||
| delete[] buf; | ||
| buf = nullptr; | ||
| } | ||
| #endif // COMPRESS_TARGAS | ||
| return; | ||
| } | ||
|
|
||
| File *fp = TheFileSystem->openFile(realFileName.str(), File::CREATE | File::BINARY | File::WRITE); | ||
| if (fp) | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this is merged, can you make a follow refactor and merge this logic with the new stuff added below? It looks like this function can be greatly simplified.
See
getTransferFileType