fix(security): add file content validation for map transfer by bobtista · Pull Request #1819 · TheSuperHackers/GeneralsGameCode

bobtista · 2025-11-07T05:35:44Z

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

Core/GameEngine/Source/Common/System/FileSystem.cpp

Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp

…rison

…returns

…idation failure

Core/GameEngine/Source/Common/System/FileSystem.cpp

xezon · 2026-01-24T19:44:33Z

Were all comments from SkyAero worked on?

Core/GameEngine/Source/Common/System/FileSystem.cpp

xezon · 2026-02-10T22:17:58Z

Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp

 		DEBUG_LOG(("Wrote %d bytes to file %s!", len, realFileName.str()));

+		// TheSuperHackers @security bobtista 06/11/2025 Validate file content after writing
+		if (!FileSystem::hasValidTransferFileContent(realFileName))


Why is this validated after write? Better validate when in memory, before file write.

…g to disk

greptile-apps · 2026-02-12T17:55:41Z

Greptile Summary

Adds in-memory file content validation for multiplayer map transfers, preventing malformed or malicious files from being written to disk. Introduces FileSystem::hasValidTransferFileContent() which validates files by extension (magic bytes for .map, null-byte detection for .ini, TGA 2.0 footer signature for .tga) with per-type size limits, and integrates the check into ConnectionManager::processFile() after decompression but before disk write.

Validation is well-structured using a lookup table with per-type rules in an anonymous namespace
TGA 2.0 footer signature check correctly validates the last 18 bytes (Signature[16] + RsvdChar + BZST) matching the TGA2Footer struct in TARGA.h
Memory cleanup for the COMPRESS_TARGAS decompression buffer is correctly handled on the early-return path
Silent abort (no progress notification) on validation failure is consistent with existing security checks in the same function

Confidence Score: 4/5

This PR is safe to merge with only a minor style nit to address.
The validation logic is correct and well-structured. TGA footer offset was verified against the TARGA.h struct definition. Memory management on early-return paths is properly handled. The only finding is a column alignment style issue in the lookup table per project conventions.
Minor style alignment needed in Core/GameEngine/Source/Common/System/FileSystem.cpp lookup table.

Important Files Changed

Filename	Overview
Core/GameEngine/Include/Common/FileSystem.h	Adds `hasValidTransferFileContent` static method declaration. Clean, minimal change to the public interface.
Core/GameEngine/Source/Common/System/FileSystem.cpp	Implements file content validation with per-type rules (magic bytes, null-byte detection, TGA 2.0 footer). Lookup table columns should be aligned per project style. Logic is sound and TGA footer offset is correct per TARGA.h definitions.
Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp	Integrates content validation call after TGA decompression but before disk write. Memory cleanup for COMPRESS_TARGAS path is correctly handled. Early return pattern is consistent with existing security checks in the function.

Flowchart

flowchart TD
    A[processFile receives NetFileCommandMsg] --> B{Path normalized?}
    B -->|No| C[Abort: invalid path]
    B -->|Yes| D{Valid extension?}
    D -->|No| E[Abort: bad extension]
    D -->|Yes| F{TGA compressed?}
    F -->|Yes| G[Decompress TGA]
    F -->|No| H[hasValidTransferFileContent]
    G --> H
    H --> I{Lookup extension rule}
    I -->|Unknown ext| J[Reject]
    I -->|Known ext| K{Size <= maxSize?}
    K -->|No| L[Reject: too large]
    K -->|Yes| M{Type-specific check}
    M -->|.map| N{Magic bytes CkMp?}
    M -->|.ini| O{No null bytes in first 512?}
    M -->|.tga| P{Size >= 44 AND footer signature?}
    M -->|.str/.txt/.wak| Q[Pass]
    N -->|Fail| R[Reject]
    N -->|Pass| Q
    O -->|Fail| S[Reject]
    O -->|Pass| Q
    P -->|Fail| T[Reject]
    P -->|Pass| Q
    Q --> U[Write file to disk]
    U --> V[Send 100% progress]

_{Last reviewed commit: bc37447}

greptile-apps

_{3 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

Core/GameEngine/Source/Common/System/FileSystem.cpp

Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

Core/GameEngine/Source/Common/System/FileSystem.cpp

Skyaero42

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

xezon · 2026-02-13T18:28:22Z

Core/GameEngine/Source/Common/System/FileSystem.cpp

+	else if (stricmp(lastDot, ".ini") == 0)
+	{
+		// Check for null bytes to ensure text format
+		Int bytesToCheck = dataSize < 512 ? dataSize : 512;


I think std::min here actually

Core/GameEngine/Source/Common/System/FileSystem.cpp

greptile-apps

_{3 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-16T17:22:45Z

Core/GameEngine/Source/Common/System/FileSystem.cpp

+			DEBUG_LOG(("TGA file '%s' is too small to be valid (minimum 18 bytes).", filePath.str()));
+			return false;
+		}
+		if (memcmp(data + dataSize - 18, "TRUEVISION-XFILE.", 18) != 0)


TGA footer validation is checking wrong offset and size. The TGA 2.0 footer is 26 bytes (see TARGA.h:134-142), not 18. The footer consists of Extension(4) + Developer(4) + Signature[16] + RsvdChar(1) + BZST(1) = 26 bytes total.

Current code checks last 18 bytes, which skips the 8-byte Extension and Developer fields and reads into image data instead of the footer.

Suggested change

if (memcmp(data + dataSize - 18, "TRUEVISION-XFILE.", 18) != 0)

if (memcmp(data + dataSize - 26, "TRUEVISION-XFILE.", 18) != 0)

Also update line 560 to check dataSize < 26 instead of dataSize < 18 since a valid TGA with footer needs at least header(18) + footer(26) = 44 bytes minimum.

Prompt To Fix With AI

This is a comment left during a code review. Path: Core/GameEngine/Source/Common/System/FileSystem.cpp Line: 565:565 Comment: TGA footer validation is checking wrong offset and size. The TGA 2.0 footer is 26 bytes (see `TARGA.h:134-142`), not 18. The footer consists of Extension(4) + Developer(4) + Signature[16] + RsvdChar(1) + BZST(1) = 26 bytes total. Current code checks last 18 bytes, which skips the 8-byte Extension and Developer fields and reads into image data instead of the footer. ```suggestion if (memcmp(data + dataSize - 26, "TRUEVISION-XFILE.", 18) != 0) ``` Also update line 560 to check `dataSize < 26` instead of `dataSize < 18` since a valid TGA with footer needs at least header(18) + footer(26) = 44 bytes minimum. How can I resolve this? If you propose a fix, please make it concise.

The offset is correct — you can verify in Core/Libraries/Source/WWVegas/WWLib/TARGA.h:134-142:

typedef struct _TGA2Footer { long Extension; // 4 bytes (offset 0 from footer start) long Developer; // 4 bytes (offset 4) char Signature[16]; // 16 bytes (offset 8) char RsvdChar; // 1 byte '.' (offset 24) char BZST; // 1 byte '\0' (offset 25) } TGA2Footer; // 26 bytes total

The Signature field starts at footer offset 8, which is EOF - 18 (26 - 8). Our memcmp(data +
dataSize - 18, "TRUEVISION-XFILE.", 18) checks exactly Signature + RsvdChar + BZST — the last 18
bytes. Starting at dataSize - 26 would read the Extension and Developer fields instead.

Core/GameEngine/Source/Common/System/FileSystem.cpp

…ed files

… with enum and switch

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-16T18:42:53Z

Core/GameEngine/Source/Common/System/FileSystem.cpp

+	{ ".map", 5 * 1024 * 1024, TRANSFER_FILE_MAP },
+	{ ".ini", 512 * 1024, TRANSFER_FILE_INI },
+	{ ".str", 512 * 1024, TRANSFER_FILE_STR },
+	{ ".txt", 512 * 1024, TRANSFER_FILE_TXT },
+	{ ".tga", 2 * 1024 * 1024, TRANSFER_FILE_TGA },
+	{ ".wak", 512 * 1024, TRANSFER_FILE_WAK },


Align columns in lookup table
Per project style, data-oriented lookup tables should have aligned columns for readability.

Suggested change

{ ".map", 5 * 1024 * 1024, TRANSFER_FILE_MAP },

{ ".ini", 512 * 1024, TRANSFER_FILE_INI },

{ ".str", 512 * 1024, TRANSFER_FILE_STR },

{ ".txt", 512 * 1024, TRANSFER_FILE_TXT },

{ ".tga", 2 * 1024 * 1024, TRANSFER_FILE_TGA },

{ ".wak", 512 * 1024, TRANSFER_FILE_WAK },

{ ".map", 5 * 1024 * 1024, TRANSFER_FILE_MAP },

{ ".ini", 512 * 1024, TRANSFER_FILE_INI },

{ ".str", 512 * 1024, TRANSFER_FILE_STR },

{ ".txt", 512 * 1024, TRANSFER_FILE_TXT },

{ ".tga", 2 * 1024 * 1024, TRANSFER_FILE_TGA },

{ ".wak", 512 * 1024, TRANSFER_FILE_WAK },

Context Used: Rule from dashboard - Align columns in data-oriented blocks (like lookup tables, initialization lists, or structured data)... (source)

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Prompt To Fix With AI

This is a comment left during a code review. Path: Core/GameEngine/Source/Common/System/FileSystem.cpp Line: 484:489 Comment: **Align columns in lookup table** Per project style, data-oriented lookup tables should have aligned columns for readability. ```suggestion { ".map", 5 * 1024 * 1024, TRANSFER_FILE_MAP }, { ".ini", 512 * 1024, TRANSFER_FILE_INI }, { ".str", 512 * 1024, TRANSFER_FILE_STR }, { ".txt", 512 * 1024, TRANSFER_FILE_TXT }, { ".tga", 2 * 1024 * 1024, TRANSFER_FILE_TGA }, { ".wak", 512 * 1024, TRANSFER_FILE_WAK }, ``` **Context Used:** Rule from `dashboard` - Align columns in data-oriented blocks (like lookup tables, initialization lists, or structured data)... ([source](https://app.greptile.com/review/custom-context?memory=e61e83cf-a14f-4759-bed8-3bab008b25b5)) <sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub> How can I resolve this? If you propose a fix, please make it concise.

… + 26 footer)

bobtista force-pushed the bobtista/security-content-validation branch from 630dbcb to 218f4b9 Compare November 7, 2025 05:41

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

97d61d7

bobtista force-pushed the bobtista/security-content-validation branch from 218f4b9 to 97d61d7 Compare November 11, 2025 21:48

bobtista marked this pull request as ready for review November 28, 2025 23:44

Skyaero42 reviewed Dec 17, 2025

View reviewed changes

bobtista added 5 commits December 17, 2025 17:18

refactor(security): add STRICMP macro for cross-platform string compa…

73e763d

…rison

refactor(security): split validation into helper function with early …

0599333

…returns

refactor(security): simplify .str/.txt extension handling logic

c2d0135

fix(security): reduce file size limits to reasonable values

58ae26c

docs(security): add comment explaining transfer abort behavior on val…

59f93ca

…idation failure

Skyaero42 reviewed Dec 18, 2025

View reviewed changes

Core/GameEngine/Source/Common/System/FileSystem.cpp Outdated Show resolved Hide resolved

Skyaero42 reviewed Dec 18, 2025

View reviewed changes

Core/GameEngine/Source/Common/System/FileSystem.cpp Outdated Show resolved Hide resolved

Skyaero42 reviewed Feb 9, 2026

View reviewed changes

Core/GameEngine/Source/Common/System/FileSystem.cpp Outdated Show resolved Hide resolved

xezon reviewed Feb 10, 2026

View reviewed changes

feat(network): Validate transfer file content in memory before writin…

41048c3

…g to disk

greptile-apps bot reviewed Feb 12, 2026

View reviewed changes

style(security): Replace NULL with nullptr in transfer file validation

c87d395

greptile-apps bot reviewed Feb 12, 2026

View reviewed changes

Core/GameEngine/Source/Common/System/FileSystem.cpp Outdated Show resolved Hide resolved

Skyaero42 approved these changes Feb 12, 2026

View reviewed changes

xezon added Major Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Security Is security related labels Feb 13, 2026

xezon added this to the Major bug fixes milestone Feb 13, 2026

xezon reviewed Feb 13, 2026

View reviewed changes

greptile-apps bot reviewed Feb 16, 2026

View reviewed changes

bobtista added 2 commits February 16, 2026 13:00

style(security): Use std::min for INI null-byte check byte count

1812801

style(security): Use memcmp for map file magic bytes check

fc9a68c

bobtista added 2 commits February 16, 2026 13:10

feat(security): Add TGA 2.0 footer signature validation for transferr…

2bdc12a

…ed files

refactor(security): Extract TransferFileRule into anonymous namespace…

4505e7b

… with enum and switch

greptile-apps bot reviewed Feb 16, 2026

View reviewed changes

fix(security): Require minimum 44 bytes for TGA validation (18 header…

bc37447

… + 26 footer)

	if (memcmp(data + dataSize - 18, "TRUEVISION-XFILE.", 18) != 0)
	if (memcmp(data + dataSize - 26, "TRUEVISION-XFILE.", 18) != 0)

Conversation

bobtista commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xezon commented Jan 24, 2026

Uh oh!

Uh oh!

xezon Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Flowchart

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Skyaero42 left a comment

Choose a reason for hiding this comment

Uh oh!

xezon Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

bobtista Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

bobtista Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bobtista commented Nov 7, 2025 •

edited

Loading

greptile-apps bot commented Feb 12, 2026 •

edited

Loading