Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Core/GameEngine/Include/Common/FileSystem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ class FileSystem : public SubsystemInterface

static AsciiString normalizePath(const AsciiString& path); ///< 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.
static Bool hasValidTransferFileContent(const AsciiString& filePath); ///< validates that a file's content matches expected format for its extension during map transfer operations. Checks file headers, magic bytes, size limits, and basic structure. TheSuperHackers @security bobtista 06/11/2025

protected:
#if ENABLE_FILESYSTEM_EXISTENCE_CACHE
Expand Down
171 changes: 171 additions & 0 deletions Core/GameEngine/Source/Common/System/FileSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,3 +457,174 @@ Bool FileSystem::isPathInDirectory(const AsciiString& testPath, const AsciiStrin

return true;
}

//============================================================================
// FileSystem::hasValidTransferFileContent
//============================================================================
// TheSuperHackers @security bobtista 06/11/2025
// Validates file content format during map transfer operations.
// Checks file headers, magic bytes, size limits, and basic structure to prevent
// malformed or malicious files from being processed.
//============================================================================
Bool FileSystem::hasValidTransferFileContent(const AsciiString& filePath)
{
File* file = TheLocalFileSystem->openFile(filePath.str(), File::READ | File::BINARY);
if (file == NULL)
{
DEBUG_LOG(("Cannot open file '%s' for content validation.", filePath.str()));
return false;
}

const Int fileSize = file->size();
Bool isValid = false;

const char* lastDot = strrchr(filePath.str(), '.');
if (lastDot == NULL)
{
file->close();
return false;
}

const Int MAX_MAP_SIZE = 50 * 1024 * 1024;
const Int MAX_INI_SIZE = 10 * 1024 * 1024;
const Int MAX_STR_SIZE = 5 * 1024 * 1024;
const Int MAX_TGA_SIZE = 20 * 1024 * 1024;
const Int MAX_TXT_SIZE = 5 * 1024 * 1024;
const Int MAX_WAK_SIZE = 10 * 1024 * 1024;

#ifdef _WIN32
if (_stricmp(lastDot, ".map") == 0)
#else
if (strcasecmp(lastDot, ".map") == 0)
#endif
{
if (fileSize > MAX_MAP_SIZE)
{
DEBUG_LOG(("Map file '%s' exceeds maximum size (%d bytes).", filePath.str(), fileSize));
isValid = false;
}
else
{
UnsignedByte header[4];
file->read(header, 4);
if (header[0] == 'C' && header[1] == 'k' && header[2] == 'M' && header[3] == 'p')
{
isValid = true;
}
else
{
DEBUG_LOG(("Map file '%s' has invalid magic bytes.", filePath.str()));
isValid = false;
}
}
}
#ifdef _WIN32
else if (_stricmp(lastDot, ".ini") == 0)
#else
else if (strcasecmp(lastDot, ".ini") == 0)
#endif
{
if (fileSize > MAX_INI_SIZE)
{
DEBUG_LOG(("INI file '%s' exceeds maximum size (%d bytes).", filePath.str(), fileSize));
isValid = false;
}
else
{
UnsignedByte sample[512];
Int bytesToRead = fileSize < 512 ? fileSize : 512;
file->read(sample, bytesToRead);

Bool hasNullBytes = false;
for (Int i = 0; i < bytesToRead; ++i)
{
if (sample[i] == 0)
{
hasNullBytes = true;
break;
}
}

if (hasNullBytes)
{
DEBUG_LOG(("INI file '%s' contains null bytes (likely binary).", filePath.str()));
isValid = false;
}
else
{
isValid = true;
}
}
}
#ifdef _WIN32
else if (_stricmp(lastDot, ".str") == 0 || _stricmp(lastDot, ".txt") == 0)
#else
else if (strcasecmp(lastDot, ".str") == 0 || strcasecmp(lastDot, ".txt") == 0)
#endif
{
Int maxSize = MAX_STR_SIZE;
#ifdef _WIN32
if (_stricmp(lastDot, ".txt") == 0)
#else
if (strcasecmp(lastDot, ".txt") == 0)
#endif
{
maxSize = MAX_TXT_SIZE;
}

if (fileSize > maxSize)
{
DEBUG_LOG(("Text file '%s' exceeds maximum size (%d bytes).", filePath.str(), fileSize));
isValid = false;
}
else
{
isValid = true;
}
}
#ifdef _WIN32
else if (_stricmp(lastDot, ".tga") == 0)
#else
else if (strcasecmp(lastDot, ".tga") == 0)
#endif
{
if (fileSize > MAX_TGA_SIZE)
{
DEBUG_LOG(("TGA file '%s' exceeds maximum size (%d bytes).", filePath.str(), fileSize));
isValid = false;
}
else if (fileSize < 18)
{
DEBUG_LOG(("TGA file '%s' is too small to be valid (minimum 18 bytes).", filePath.str()));
isValid = false;
}
else
{
isValid = true;
}
}
#ifdef _WIN32
else if (_stricmp(lastDot, ".wak") == 0)
#else
else if (strcasecmp(lastDot, ".wak") == 0)
#endif
{
if (fileSize > MAX_WAK_SIZE)
{
DEBUG_LOG(("WAK file '%s' exceeds maximum size (%d bytes).", filePath.str(), fileSize));
isValid = false;
}
else
{
isValid = true;
}
}
else
{
DEBUG_LOG(("File '%s' has unrecognized extension for content validation.", filePath.str()));
isValid = false;
}

file->close();
return isValid;
}
14 changes: 14 additions & 0 deletions Generals/Code/GameEngine/Source/GameNetwork/ConnectionManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,20 @@ void ConnectionManager::processFile(NetFileCommandMsg *msg)
fp = NULL;
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))
{
DEBUG_LOG(("File '%s' failed content validation, deleting.", realFileName.str()));
remove(realFileName.str());
#ifdef COMPRESS_TARGAS
if (deleteBuf)
{
delete[] buf;
buf = NULL;
}
#endif
return;
}
}
else
{
Expand Down
14 changes: 14 additions & 0 deletions GeneralsMD/Code/GameEngine/Source/GameNetwork/ConnectionManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,20 @@ void ConnectionManager::processFile(NetFileCommandMsg *msg)
fp = NULL;
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))
{
DEBUG_LOG(("File '%s' failed content validation, deleting.", realFileName.str()));
remove(realFileName.str());
#ifdef COMPRESS_TARGAS
if (deleteBuf)
{
delete[] buf;
buf = NULL;
}
#endif
return;
}
}
else
{
Expand Down
Loading