Create Security module for sec related patches

src/Components/Loader.cpp

@@ -84,6 +84,7 @@ namespace Components
 		Loader::Register(new ModelSurfs());
 		Loader::Register(new PlayerName());
 		Loader::Register(new QuickPatch());
+		Loader::Register(new Security());
 		Loader::Register(new ServerInfo());
 		Loader::Register(new ServerList());
 		Loader::Register(new SlowMotion());

src/Components/Loader.hpp

@@ -85,6 +85,7 @@ namespace Components
 #include "Modules/Network.hpp"
 #include "Modules/Theatre.hpp"
 #include "Modules/QuickPatch.hpp"
+#include "Modules/Security.hpp"
 #include "Modules/Node.hpp"
 #include "Modules/RCon.hpp"
 #include "Modules/Party.hpp" // Destroys the order, but requires network classes :D

src/Components/Modules/PlayerName.cpp

@@ -8,7 +8,7 @@ namespace Components
     {
         if (!sv_allowColoredNames.get<bool>())
         {
-            char nameBuffer[64] = { 0 };
+            char nameBuffer[64] = {0};
             TextRenderer::StripColors(name, nameBuffer, sizeof(nameBuffer));
             TextRenderer::StripAllTextIcons(nameBuffer, buffer, size);
         }
@@ -26,12 +26,12 @@ namespace Components
         }
     }
 
-    __declspec(naked) void PlayerName::ClientUserinfoChanged()
+    __declspec(naked) void PlayerName::ClientCleanName()
     {
         __asm
         {
             mov eax, [esp + 4h] // length
-            //sub eax, 1
+
             push eax
 
             push ecx // name
@@ -53,12 +53,57 @@ namespace Components
 
         return buf;
     }
+
 	char* PlayerName::CleanStrStub(char* string)
 	{
 		TextRenderer::StripColors(string, string, strlen(string) + 1);
 		return string;
 	}
 
+	bool PlayerName::CopyClientNameCheck(char* dest, const char* source, int size)
+	{
+		Utils::Hook::Call<void(char*, const char*, int)>(0x4D6F80)(dest, source, size); // I_strncpyz
+
+		auto i = 0;
+		while (i < size - 1 && dest[i] != '\0')
+		{
+			if (dest[i] > 125 || dest[i] < 32 || dest[i] == '%')
+			{
+				return false; // Illegal string
+			}
+
+			++i;
+		}
+
+		return true;
+	}
+
+	__declspec(naked) void PlayerName::SV_UserinfoChangedStub()
+	{
+		__asm
+		{
+			call CopyClientNameCheck
+			test al, al
+
+			jnz returnSafe
+
+			pushad
+
+			push 1 // tellThem
+			push INVALID_NAME_MSG // reason
+			push edi // drop
+			mov eax, 0x4D1600 // SV_DropClient
+			call eax
+			add esp, 0xC
+
+			popad
+
+		returnSafe:
+			push 0x401988
+			retn
+		}
+	}
+
 	PlayerName::PlayerName()
 	{
 		sv_allowColoredNames = Dvar::Register<bool>("sv_allowColoredNames", true, Game::dvar_flag::DVAR_NONE, "Allow colored names on the server");
@@ -66,13 +111,17 @@ namespace Components
 		// Disable SV_UpdateUserinfo_f, to block changing the name ingame
 		Utils::Hook::Set<BYTE>(0x6258D0, 0xC3);
 
-		// Allow colored names ingame
+		// Allow colored names ingame. Hook placed in ClientUserinfoChanged
-		Utils::Hook(0x5D8B40, ClientUserinfoChanged, HOOK_JUMP).install()->quick();
+		Utils::Hook(0x5D8B40, ClientCleanName, HOOK_JUMP).install()->quick();
 
 		// Though, don't apply that to overhead names.
 		Utils::Hook(0x581932, GetClientName, HOOK_CALL).install()->quick();
 
 		// Patch I_CleanStr
 		Utils::Hook(0x4AD470, CleanStrStub, HOOK_JUMP).install()->quick();
+
+		// Detect invalid characters including '%' to prevent format string vulnerabilities.
+		// Kicks the player as soon as possible
+		Utils::Hook(0x401983, SV_UserinfoChangedStub, HOOK_JUMP).install()->quick();
 	}
 }

src/Components/Modules/PlayerName.hpp

@@ -11,9 +11,14 @@ namespace Components
 
 	private:
 		static Dvar::Var sv_allowColoredNames;
+		// Message used when kicking players
+		static constexpr auto INVALID_NAME_MSG = "Invalid name detected";
 
 		static char* CleanStrStub(char* string);
-		static void ClientUserinfoChanged();
+		static void ClientCleanName();
 		static char* GetClientName(int localClientNum, int index, char* buf, size_t size);
+
+		static bool CopyClientNameCheck(char* dest, const char* source, int size);
+		static void SV_UserinfoChangedStub();
 	};
 }

src/Components/Modules/QuickPatch.cpp

@@ -47,62 +47,6 @@ namespace Components
 		}
 	}
 
-	int QuickPatch::MsgReadBitsCompressCheckSV(const char *from, char *to, int size)
-	{
-		static char buffer[0x8000];
-
-		if (size > 0x800) return 0;
-		size = Game::MSG_ReadBitsCompress(from, buffer, size);
-
-		if (size > 0x800) return 0;
-		std::memcpy(to, buffer, size);
-
-		return size;
-	}
-
-	int QuickPatch::MsgReadBitsCompressCheckCL(const char *from, char *to, int size)
-	{
-		static char buffer[0x100000];
-
-		if (size > 0x20000) return 0;
-		size = Game::MSG_ReadBitsCompress(from, buffer, size);
-
-		if (size > 0x20000) return 0;
-		std::memcpy(to, buffer, size);
-
-		return size;
-	}
-
-	int QuickPatch::SVCanReplaceServerCommand(Game::client_t* /*client*/, const char* /*cmd*/)
-	{
-		// This is a fix copied from V2. As I don't have time to investigate, let's simply trust them
-		return -1;
-	}
-
-	long QuickPatch::AtolAdjustPlayerLimit(const char* string)
-	{
-		return std::min(atol(string), 18l);
-	}
-
-	void QuickPatch::SelectStringTableEntryInDvarStub()
-	{
-		Command::ClientParams params;
-
-		if (params.size() >= 4)
-		{
-			const auto* dvarName = params[3];
-			const auto* dvar = Game::Dvar_FindVar(dvarName);
-
-			if (Command::Find(dvarName) ||
-				(dvar != nullptr && dvar->flags & (Game::DVAR_WRITEPROTECTED | Game::DVAR_CHEAT | Game::DVAR_READONLY)))
-			{
-				return;
-			}
-		}
-
-		Game::CL_SelectStringTableEntryInDvar_f();
-	}
-
 	__declspec(naked) void QuickPatch::JavelinResetHookStub()
 	{
 		__asm
@@ -117,69 +61,6 @@ namespace Components
 		}
 	}
 
-	__declspec(naked) int QuickPatch::G_GetClientScore()
-	{
-		__asm
-		{
-			mov eax, [esp + 4]		// index
-			mov ecx, ds : 1A831A8h	// level: &g_clients
-
-			test ecx, ecx;
-			jz invalid_ptr;
-			
-			imul eax, 366Ch
-			mov eax, [eax + ecx + 3134h]
-			ret
-			
-		invalid_ptr:
-			xor eax, eax
-			ret
-		}
-	}
-
-	bool QuickPatch::InvalidNameCheck(char* dest, const char* source, int size)
-	{
-		Utils::Hook::Call<void(char*, const char*, int)>(0x4D6F80)(dest, source, size); // I_strncpyz
-
-		for (int i = 0; i < size - 1; i++)
-		{
-			if (!dest[i]) break;
-
-			if (dest[i] > 125 || dest[i] < 32 || dest[i] == '%')
-			{
-				return false;
-			}
-		}
-
-		return true;
-	}
-
-	__declspec(naked) void QuickPatch::InvalidNameStub()
-	{
-		static const char* kick_reason = "Invalid name detected.";
-
-		__asm
-		{
-			call InvalidNameCheck;
-			test al, al
-
-			jnz returnSafe;
-
-			pushad;
-			push 1;
-			push kick_reason;
-			push edi;
-			mov eax, 0x004D1600; // SV_DropClientInternal
-			call eax;
-			add esp, 12;
-			popad;
-
-		returnSafe:
-			push 0x00401988;
-			retn;
-		}
-	}
-
 	Game::dvar_t* QuickPatch::g_antilag;
 	__declspec(naked) void QuickPatch::ClientEventsFireWeaponStub()
 	{
@@ -378,9 +259,6 @@ namespace Components
 		Utils::Hook(0x5D6D56, QuickPatch::ClientEventsFireWeaponStub, HOOK_JUMP).install()->quick();
 		Utils::Hook(0x5D6D6A, QuickPatch::ClientEventsFireWeaponMeleeStub, HOOK_JUMP).install()->quick();
 
-		// Disallow invalid player names
-		Utils::Hook(0x401983, QuickPatch::InvalidNameStub, HOOK_JUMP).install()->quick();
-
 		// Javelin fix
 		Utils::Hook(0x578F52, QuickPatch::JavelinResetHookStub, HOOK_JUMP).install()->quick();
 
@@ -644,21 +522,6 @@ namespace Components
 			}
 		});
 
-		// Exploit fixes
-		Utils::Hook::Set<BYTE>(0x412370, 0xC3);                                                      // SV_SteamAuthClient
-		Utils::Hook::Set<BYTE>(0x5A8C70, 0xC3);                                                      // CL_HandleRelayPacket
-		Utils::Hook(0x414D92, QuickPatch::MsgReadBitsCompressCheckSV, HOOK_CALL).install()->quick(); // SV_ExecuteClientCommands
-		Utils::Hook(0x4A9F56, QuickPatch::MsgReadBitsCompressCheckCL, HOOK_CALL).install()->quick(); // CL_ParseServerMessage
-		Utils::Hook(0x407376, QuickPatch::SVCanReplaceServerCommand , HOOK_CALL).install()->quick(); // SV_CanReplaceServerCommand
-		Utils::Hook(0x5B67ED, QuickPatch::AtolAdjustPlayerLimit     , HOOK_CALL).install()->quick(); // PartyHost_HandleJoinPartyRequest
-		Utils::Hook::Nop(0x41698E, 5); // Disable Svcmd_EntityList_f
-
-		// Patch selectStringTableEntryInDvar
-		Utils::Hook::Set(0x405959, QuickPatch::SelectStringTableEntryInDvarStub);
-
-		// Patch G_GetClientScore for uninitialised game
-		Utils::Hook(0x469AC0, QuickPatch::G_GetClientScore, HOOK_JUMP).install()->quick();
-
 		// Ignore call to print 'Offhand class mismatch when giving weapon...'
 		Utils::Hook(0x5D9047, 0x4BB9B0, HOOK_CALL).install()->quick();
 

src/Components/Modules/QuickPatch.hpp

@@ -12,21 +12,8 @@ namespace Components
 		static void UnlockStats();
 
 	private:
-		static void SelectStringTableEntryInDvarStub();
-
-		static int SVCanReplaceServerCommand(Game::client_t *client, const char *cmd);
-		static int G_GetClientScore();
-
-		static int MsgReadBitsCompressCheckSV(const char *from, char *to, int size);
-		static int MsgReadBitsCompressCheckCL(const char *from, char *to, int size);
-
-		static long AtolAdjustPlayerLimit(const char* string);
-
 		static void JavelinResetHookStub();
 
-		static bool InvalidNameCheck(char* dest, const char* source, int size);
-		static void InvalidNameStub();
-
 		static Dvar::Var r_customAspectRatio;
 		static Game::dvar_t* Dvar_RegisterAspectRatioDvar(const char* dvarName, const char** valueList, int defaultIndex, unsigned __int16 flags, const char* description);
 		static void SetAspectRatioStub();

src/Components/Modules/Security.cpp

@@ -0,0 +1,112 @@
+#include <STDInclude.hpp>
+
+namespace Components
+{
+	int Security::MsgReadBitsCompressCheckSV(const char* from, char* to, int size)
+	{
+		static char buffer[0x8000];
+
+		if (size > 0x800) return 0;
+		size = Game::MSG_ReadBitsCompress(from, buffer, size);
+
+		if (size > 0x800) return 0;
+		std::memcpy(to, buffer, size);
+
+		return size;
+	}
+
+	int Security::MsgReadBitsCompressCheckCL(const char* from, char* to, int size)
+	{
+		static char buffer[0x100000];
+
+		if (size > 0x20000) return 0;
+		size = Game::MSG_ReadBitsCompress(from, buffer, size);
+
+		if (size > 0x20000) return 0;
+		std::memcpy(to, buffer, size);
+
+		return size;
+	}
+
+	int Security::SVCanReplaceServerCommand(Game::client_t* /*client*/, const char* /*cmd*/)
+	{
+		// This is a fix copied from V2. As I don't have time to investigate, let's simply trust them
+		return -1;
+	}
+
+	long Security::AtolAdjustPlayerLimit(const char* string)
+	{
+		return std::min<long>(std::atol(string), 18);
+	}
+
+	void Security::SelectStringTableEntryInDvarStub()
+	{
+		Command::ClientParams params;
+
+		if (params.size() >= 4)
+		{
+			const auto* dvarName = params[3];
+			const auto* dvar = Game::Dvar_FindVar(dvarName);
+
+			if (Command::Find(dvarName) ||
+				(dvar != nullptr && dvar->flags & (Game::DVAR_WRITEPROTECTED | Game::DVAR_CHEAT | Game::DVAR_READONLY)))
+			{
+				Logger::Print(0, "CL_SelectStringTableEntryInDvar_f: illegal parameter\n");
+				return;
+			}
+		}
+
+		Game::CL_SelectStringTableEntryInDvar_f();
+	}
+
+	__declspec(naked) int Security::G_GetClientScore()
+	{
+		__asm
+		{
+			mov eax, [esp + 4] // index
+			mov ecx, ds:1A831A8h // level: &g_clients
+
+			test ecx, ecx
+			jz invalid_ptr
+
+			imul eax, 366Ch
+			mov eax, [eax + ecx + 3134h]
+			ret
+
+		invalid_ptr:
+			xor eax, eax
+			ret
+		}
+	}
+
+	void Security::G_LogPrintfStub(const char* fmt)
+	{
+		Game::G_LogPrintf("%s", fmt);
+	}
+
+	Security::Security()
+	{
+		// Exploit fixes
+		Utils::Hook(0x414D92, MsgReadBitsCompressCheckSV, HOOK_CALL).install()->quick(); // SV_ExecuteClientCommands
+		Utils::Hook(0x4A9F56, MsgReadBitsCompressCheckCL, HOOK_CALL).install()->quick(); // CL_ParseServerMessage
+		Utils::Hook(0x407376, SVCanReplaceServerCommand, HOOK_CALL).install()->quick(); // SV_CanReplaceServerCommand
+
+		Utils::Hook::Set<BYTE>(0x412370, 0xC3); // SV_SteamAuthClient
+		Utils::Hook::Set<BYTE>(0x5A8C70, 0xC3); // CL_HandleRelayPacket
+
+		Utils::Hook::Nop(0x41698E, 5); // Disable Svcmd_EntityList_f
+
+		// Patch selectStringTableEntryInDvar
+		Utils::Hook::Set<void(*)()>(0x405959, Security::SelectStringTableEntryInDvarStub);
+
+		// Patch G_GetClientScore for uninitialized game
+		Utils::Hook(0x469AC0, G_GetClientScore, HOOK_JUMP).install()->quick();
+
+		// Requests can be malicious
+		Utils::Hook(0x5B67ED, AtolAdjustPlayerLimit, HOOK_CALL).install()->quick(); // PartyHost_HandleJoinPartyRequest
+
+		// Patch unsecure call to G_LogPrint inside GScr_LogPrint
+		// This function is unsafe because IW devs forgot to G_LogPrintf("%s", fmt)
+		Utils::Hook(0x5F70B5, G_LogPrintfStub, HOOK_CALL).install()->quick();
+	}
+}

src/Components/Modules/Security.hpp

@@ -0,0 +1,24 @@
+#pragma once
+
+namespace Components
+{
+	class Security : public Component
+	{
+	public:
+		Security();
+
+	private:
+		static int MsgReadBitsCompressCheckSV(const char* from, char* to, int size);
+		static int MsgReadBitsCompressCheckCL(const char* from, char* to, int size);
+
+		static int SVCanReplaceServerCommand(Game::client_t* client, const char* cmd);
+
+		static long AtolAdjustPlayerLimit(const char* string);
+
+		static void SelectStringTableEntryInDvarStub();
+
+		static int G_GetClientScore();
+
+		static void G_LogPrintfStub(const char* fmt);
+	};
+}

src/Components/Modules/TextRenderer.cpp

@@ -1356,11 +1356,11 @@ namespace Components
                 if (*in) // height
                     in++;
 
-                if(*in) // material name length + material name characters
+                if (*in) // material name length + material name characters
                 {
                     const auto materialNameLength = *in;
                     in++;
-                    for(auto i = 0; i < materialNameLength; i++)
+                    for (auto i = 0; i < materialNameLength; i++)
                     {
                         if (*in)
                             in++;
@@ -1370,7 +1370,7 @@ namespace Components
                 continue;
             }
 
-            if(*in == FONT_ICON_SEPARATOR_CHARACTER)
+            if (*in == FONT_ICON_SEPARATOR_CHARACTER)
             {
                 const auto* fontIconEndPos = &in[1];
                 FontIconInfo fontIcon{};
@@ -1386,6 +1386,7 @@ namespace Components
             ++current;
             ++in;
         }
+
         *out = '\0';
     }
 

src/Game/Functions.cpp

@@ -150,6 +150,7 @@ namespace Game
 	FS_IsShippedIWD_t FS_IsShippedIWD = FS_IsShippedIWD_t(0x642440);
 	FS_Delete_t FS_Delete = FS_Delete_t(0x48A5B0);
 
+	G_LogPrintf_t G_LogPrintf = G_LogPrintf_t(0x4B0150);
 	G_GetWeaponIndexForName_t G_GetWeaponIndexForName = G_GetWeaponIndexForName_t(0x49E540);
 	G_SpawnEntitiesFromString_t G_SpawnEntitiesFromString = G_SpawnEntitiesFromString_t(0x4D8840);
 

src/Game/Functions.hpp

@@ -371,6 +371,9 @@ namespace Game
 	typedef int(__cdecl* FS_Delete_t)(const char* fileName);
 	extern FS_Delete_t FS_Delete;
 
+	typedef void(__cdecl * G_LogPrintf_t)(const char* fmt, ...);
+	extern G_LogPrintf_t G_LogPrintf;
+
 	typedef unsigned int(__cdecl * G_GetWeaponIndexForName_t)(const char*);
 	extern G_GetWeaponIndexForName_t G_GetWeaponIndexForName;
 

src/Utils/String.hpp

@@ -11,7 +11,7 @@ namespace Utils
 			static_assert(Buffers != 0 && MinBufferSize != 0, "Buffers and MinBufferSize mustn't be 0");
 
 			VAProvider() : currentBuffer(0) {}
-			~VAProvider() {}
+			~VAProvider() = default;
 
 			const char* get(const char* format, va_list ap)
 			{
@@ -25,7 +25,7 @@ namespace Utils
 
 				while (true)
 				{
-					int res = vsnprintf_s(entry->buffer, entry->size, _TRUNCATE, format, ap);
+					const auto res = _vsnprintf_s(entry->buffer, entry->size, _TRUNCATE, format, ap);
 					if (res > 0) break; // Success
 					if (res == 0) return ""; // Error