fix(security): add file content validation for map transfer

[bobtista]

• Closes Map transfers lack file content validation #1817

Implements file content validation during map transfer operations to prevent malformed or malicious files from being processed.

Implementation

Adds FileSystem::hasValidTransferFileContent() which validates transferred file content in memory before writing to disk. If validation fails, the file is never written and the transfer is silently aborted.

Validation methods:

• .map files - Validates magic bytes CkMp (Chunky Map header)
• .ini files - Checks for null bytes to ensure text format (rejects binary)
• .tga files - Validates minimum size (18 bytes for valid TGA header)
• .str, .txt, .wak - Size validation only

Maximum file sizes:

• .map: 5 MB
• .ini: 512 KB
• .str/.txt: 512 KB
• .tga: 2 MB
• .wak: 512 KB

[xezon]

Were all comments from SkyAero worked on?

[greptile-apps]

Greptile Summary

This PR refactors transfer file validation by moving it from FileSystem to ConnectionManager, consolidating all validation logic at the point where network file transfers are processed. The implementation validates file content in memory before writing to disk.

Key improvements from previous reviews:

• INI validation now scans the entire file for null bytes (up to 512 KB), addressing the previous security concern about only checking the first 512 bytes
• TGA validation properly uses the TGA2_SIGNATURE macro from TARGA.h instead of hardcoding the signature string
• Table columns are properly aligned per project style
• Minimum TGA size check correctly validates sizeof(TGAHeader) + sizeof(TGA2Footer)

Validation implemented:

• .map files: Magic bytes CkMp validation
• .ini files: Full null-byte scan to reject binary content
• .tga files: Size check and TGA 2.0 footer signature validation
• .str, .txt, .wak: Size limits only

The refactoring improves code organization by placing security validation closer to the network boundary where files are received.

Confidence Score: 4/5

• This PR is safe to merge with low risk
• The refactoring moves security validation logic to a more appropriate location without changing the validation behavior. All previous review concerns have been addressed: full INI scanning, proper TGA validation, macro usage, and table alignment. The code is well-structured with clear validation rules for each file type.
• No files require special attention

Important Files Changed

Filename	Overview
Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp	Moved transfer file validation to ConnectionManager with full INI null-byte scanning and proper TGA footer validation using TGA2_SIGNATURE macro

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Receive file transfer] --> B[Extract file extension]
    B --> C{Extension exists?}
    C -->|No| D[Reject: No extension]
    C -->|Yes| E[Get file type from lookup]
    E --> F{Valid file type?}
    F -->|No| G[Reject: Unknown extension]
    F -->|Yes| H{Size <= limit?}
    H -->|No| I[Reject: Size exceeded]
    H -->|Yes| J{File type?}
    J -->|.map| K[Validate CkMp magic bytes]
    J -->|.ini| L[Scan entire file for null bytes]
    J -->|.tga| M[Validate TGA2 footer signature]
    J -->|.str/.txt/.wak| N[Size check only]
    K --> O{Valid?}
    L --> O
    M --> O
    N --> O
    O -->|Yes| P[Write file to disk]
    O -->|No| Q[Reject: Content validation failed]
    P --> R[Transfer complete]
    Q --> S[Clean up buffer and abort]
    D --> S
    G --> S
    I --> S

Loading

_{Last reviewed commit: bf1adbd}

[greptile-apps]

_{3 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[Skyaero42]

This looks good to me.
Nice security addition to the game.

[greptile-apps]

_{3 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{3 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

PR has changed quite a bit. Haven't re-reviewed yet.

[xezon]

Looking good.

[xezon]

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

[xezon]

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.

[Skyaero42]

According to Kabuse, a 2 MB map is already insane, so 5 MB is on the safe side.
.str, .wak and .txt are always small, so 512 kb is more then enough.
Can't find the data on .tga and ini anymore. I think tga is fine, ini may need to go to 1 MB.

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.

[Caball009]

HLH - Black Dawn has a map.ini file slightly larger than 1024 KB fwiw.

[xezon]

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.

Can you elaborate on this security threat? How would that work?

[Skyaero42]

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.

Can you elaborate on this security threat? How would that work?

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.

[xezon]

Allowing users to configure that as an option defeats the purpose of such a cap.

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.

[Skyaero42]

I see your point. Agreed, making this configurable in the ini options gives mod creators the freedom they need.

[githubawn]

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.
We have GenHub for larger mods and we shouldn't increase the configuration surface here. Adding a file limit user option implies we also have to add a configurable timeout limit. I think bobtista original approach is the correct one.

[xezon]

No legitimate mod maker is going to instruct a player in the match chat to go edit their .ini files

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.

Please recheck sizes.