From 00e79268d7c7eeca92f5d9978380adf45c187f2b Mon Sep 17 00:00:00 2001 From: Kawe Mazidjatari <48657826+Mauler125@users.noreply.github.com> Date: Sat, 16 Sep 2023 16:18:18 +0200 Subject: [PATCH] Fix infinite memalloc exploit in NET_SetConVar Reported by Wanderer. Don't allow the creation of new ConVar entries in CClient::m_ConVars after the initial creation. --- r5dev/common/netmessages.h | 14 ++++ r5dev/engine/client/client.cpp | 88 ++++++++++++++++++++---- r5dev/engine/client/client.h | 28 +++++--- r5dev/engine/server/vengineserver_impl.h | 12 ++-- 4 files changed, 113 insertions(+), 29 deletions(-) diff --git a/r5dev/common/netmessages.h b/r5dev/common/netmessages.h index 4e8c5cce..7b6648d5 100644 --- a/r5dev/common/netmessages.h +++ b/r5dev/common/netmessages.h @@ -7,6 +7,7 @@ #pragma once #include "tier1/bitbuf.h" +#include "common/qlimits.h" #include "public/inetchannel.h" #include "public/inetmessage.h" #include "public/inetmsghandler.h" @@ -440,6 +441,19 @@ struct NET_StringCmd : CNetMessage char buffer[1024]; }; +class NET_SetConVar : public CNetMessage +{ +public: + + typedef struct cvar_s + { + char name[MAX_OSPATH]; + char value[MAX_OSPATH]; + } cvar_t; + + CUtlVector m_ConVars; +}; + /////////////////////////////////////////////////////////////////////////////////////// // This message is subclassed by 'SVC_CmdKeyValues' and 'CLC_CmdKeyValues' class Base_CmdKeyValues : public CNetMessage diff --git a/r5dev/engine/client/client.cpp b/r5dev/engine/client/client.cpp index e35a3e61..a6a1131e 100644 --- a/r5dev/engine/client/client.cpp +++ b/r5dev/engine/client/client.cpp @@ -49,6 +49,9 @@ void CClient::VClear(CClient* pClient) //--------------------------------------------------------------------------------- bool CClient::Connect(const char* szName, void* pNetChannel, bool bFakePlayer, void* a5, char* szMessage, int nMessageSize) { +#ifndef CLIENT_DLL + g_ServerPlayer[GetUserID()].Reset(); // Reset ServerPlayer slot. +#endif // !CLIENT_DLL return v_CClient_Connect(this, szName, pNetChannel, bFakePlayer, a5, szMessage, nMessageSize); } @@ -65,11 +68,7 @@ bool CClient::Connect(const char* szName, void* pNetChannel, bool bFakePlayer, v //--------------------------------------------------------------------------------- bool CClient::VConnect(CClient* pClient, const char* szName, void* pNetChannel, bool bFakePlayer, void* a5, char* szMessage, int nMessageSize) { - bool bResult = v_CClient_Connect(pClient, szName, pNetChannel, bFakePlayer, a5, szMessage, nMessageSize); -#ifndef CLIENT_DLL - g_ServerPlayer[pClient->GetUserID()].Reset(); // Reset ServerPlayer slot. -#endif // !CLIENT_DLL - return bResult; + return pClient->Connect(szName, pNetChannel, bFakePlayer, a5, szMessage, nMessageSize);; } //--------------------------------------------------------------------------------- @@ -148,6 +147,19 @@ void* CClient::VSendSnapshot(CClient* pClient, CClientFrame* pFrame, int nTick, return v_CClient_SendSnapshot(pClient, pFrame, nTick, nTickAck); } +//--------------------------------------------------------------------------------- +// Purpose: internal hook to 'CClient::SendNetMsgEx' +// Input : *pClient - +// *pMsg - +// bLocal - +// bForceReliable - +// bVoice - +//--------------------------------------------------------------------------------- +bool CClient::VSendNetMsgEx(CClient* pClient, CNetMessage* pMsg, char bLocal, bool bForceReliable, bool bVoice) +{ + return pClient->SendNetMsgEx(pMsg, bLocal, bForceReliable, bVoice); +} + //--------------------------------------------------------------------------------- // Purpose: some versions of the binary have an optimization that shifts the 'this' // pointer of the CClient structure by 8 bytes to avoid having to cache the vftable @@ -241,16 +253,60 @@ bool CClient::VProcessStringCmd(CClient* pClient, NET_StringCmd* pMsg) } //--------------------------------------------------------------------------------- -// Purpose: internal hook to 'CClient::SendNetMsgEx' -// Input : *pClient - +// Purpose: process set convar +// Input : *pClient - (ADJ) // *pMsg - -// bLocal - -// bForceReliable - -// bVoice - +// Output : //--------------------------------------------------------------------------------- -bool CClient::VSendNetMsgEx(CClient* pClient, CNetMessage* pMsg, char bLocal, bool bForceReliable, bool bVoice) +bool CClient::VProcessSetConVar(CClient* pClient, NET_SetConVar* pMsg) { - return pClient->SendNetMsgEx(pMsg, bLocal, bForceReliable, bVoice); + CClient* pAdj = AdjustShiftedThisPointer(pClient); + ServerPlayer_t* pSlot = &g_ServerPlayer[pAdj->GetUserID()]; + + // This loop never exceeds 255 iterations, NET_SetConVar::ReadFromBuffer(...) + // reads and inserts up to 255 entries in the vector (reads a byte for size). + FOR_EACH_VEC(pMsg->m_ConVars, i) + { + const NET_SetConVar::cvar_t& entry = pMsg->m_ConVars[i]; + const char* const name = entry.name; + const char* const value = entry.value; + + // Discard any ConVar change request if it contains funky characters. + bool bFunky = false; + for (const char* s = name; *s != '\0'; ++s) + { + if (!isalnum(*s) && *s != '_') + { + bFunky = true; + break; + } + } + if (bFunky) + { + DevWarning(eDLL_T::SERVER, "Ignoring ConVar change request for variable '%s' from client '%s'; invalid characters in the variable name\n", + name, pAdj->GetClientName()); + continue; + } + + // The initial set of ConVars must contain all client ConVars that are + // flagged UserInfo. This is a simple fix to exploits that send bogus + // data later, and catches bugs, such as new UserInfo ConVars appearing + // later, which shouldn't happen. + if (pSlot->m_bInitialConVarsSet && !pAdj->m_ConVars->FindKey(name)) + { + DevWarning(eDLL_T::SERVER, "UserInfo update from \"%s\" ignored: %s = %s\n", + pAdj->GetClientName(), name, value); + continue; + } + + pAdj->m_ConVars->SetString(name, value); + DevMsg(eDLL_T::SERVER, "UserInfo update from \"%s\": %s = %s\n", pAdj->GetClientName(), name, value); + } + + pSlot->m_bInitialConVarsSet = true; + pAdj->m_bConVarsChanged = true; + + return true; } void VClient::Attach(void) const @@ -259,9 +315,11 @@ void VClient::Attach(void) const DetourAttach((LPVOID*)&v_CClient_Clear, &CClient::VClear); DetourAttach((LPVOID*)&v_CClient_Connect, &CClient::VConnect); DetourAttach((LPVOID*)&v_CClient_ActivatePlayer, &CClient::VActivatePlayer); - DetourAttach((LPVOID*)&v_CClient_ProcessStringCmd, &CClient::VProcessStringCmd); DetourAttach((LPVOID*)&v_CClient_SendNetMsgEx, &CClient::VSendNetMsgEx); //DetourAttach((LPVOID*)&p_CClient_SendSnapshot, &CClient::VSendSnapshot); + + DetourAttach((LPVOID*)&v_CClient_ProcessStringCmd, &CClient::VProcessStringCmd); + DetourAttach((LPVOID*)&v_CClient_ProcessSetConVar, &CClient::VProcessSetConVar); #endif // !CLIENT_DLL } void VClient::Detach(void) const @@ -270,8 +328,10 @@ void VClient::Detach(void) const DetourDetach((LPVOID*)&v_CClient_Clear, &CClient::VClear); DetourDetach((LPVOID*)&v_CClient_Connect, &CClient::VConnect); DetourDetach((LPVOID*)&v_CClient_ActivatePlayer, &CClient::VActivatePlayer); - DetourDetach((LPVOID*)&v_CClient_ProcessStringCmd, &CClient::VProcessStringCmd); DetourDetach((LPVOID*)&v_CClient_SendNetMsgEx, &CClient::VSendNetMsgEx); //DetourDetach((LPVOID*)&p_CClient_SendSnapshot, &CClient::VSendSnapshot); + + DetourDetach((LPVOID*)&v_CClient_ProcessStringCmd, &CClient::VProcessStringCmd); + DetourDetach((LPVOID*)&v_CClient_ProcessSetConVar, &CClient::VProcessSetConVar); #endif // !CLIENT_DLL } diff --git a/r5dev/engine/client/client.h b/r5dev/engine/client/client.h index 0bb7c853..4f70a017 100644 --- a/r5dev/engine/client/client.h +++ b/r5dev/engine/client/client.h @@ -101,10 +101,12 @@ public: public: // Hook statics: static void VClear(CClient* pClient); static void VActivatePlayer(CClient* pClient); - static bool VProcessStringCmd(CClient* pClient, NET_StringCmd* pMsg); static void* VSendSnapshot(CClient* pClient, CClientFrame* pFrame, int nTick, int nTickAck); static bool VSendNetMsgEx(CClient* pClient, CNetMessage* pMsg, char bLocal, bool bForceReliable, bool bVoice); + static bool VProcessStringCmd(CClient* pClient, NET_StringCmd* pMsg); + static bool VProcessSetConVar(CClient* pClient, NET_SetConVar* pMsg); + private: // Stub reimplementation to avoid the 'no overrider' compiler errors in the // CServer class (contains a static array of MAX_PLAYERS of this class). @@ -150,7 +152,7 @@ private: char pad_0016[59]; int64_t m_iTeamNum; KeyValues* m_ConVars; - bool m_bInitialConVarsSet; + bool m_bConVarsChanged; bool m_bSendServerInfo; bool m_bSendSignonData; bool m_bFullStateAchieved; @@ -213,9 +215,6 @@ inline void(*v_CClient_Clear)(CClient* pClient); inline CMemory p_CClient_ActivatePlayer; inline void(*v_CClient_ActivatePlayer)(CClient* pClient); -inline CMemory p_CClient_ProcessStringCmd; -inline bool(*v_CClient_ProcessStringCmd)(CClient* pClient, NET_StringCmd* pMsg); - inline CMemory p_CClient_SetSignonState; inline bool(*v_CClient_SetSignonState)(CClient* pClient, SIGNONSTATE signon); @@ -225,6 +224,12 @@ inline bool(*v_CClient_SendNetMsgEx)(CClient* pClient, CNetMessage* pMsg, bool b inline CMemory p_CClient_SendSnapshot; inline void*(*v_CClient_SendSnapshot)(CClient* pClient, CClientFrame* pFrame, int nTick, int nTickAck); +inline CMemory p_CClient_ProcessStringCmd; +inline bool(*v_CClient_ProcessStringCmd)(CClient* pClient, NET_StringCmd* pMsg); + +inline CMemory p_CClient_ProcessSetConVar; +inline bool(*v_CClient_ProcessSetConVar)(CClient* pClient, NET_SetConVar* pMsg); + /////////////////////////////////////////////////////////////////////////////// class VClient : public IDetour { @@ -234,10 +239,11 @@ class VClient : public IDetour LogFunAdr("CClient::Disconnect", p_CClient_Disconnect.GetPtr()); LogFunAdr("CClient::Clear", p_CClient_Clear.GetPtr()); LogFunAdr("CClient::ActivatePlayer", p_CClient_ActivatePlayer.GetPtr()); - LogFunAdr("CClient::ProcessStringCmd", p_CClient_ProcessStringCmd.GetPtr()); LogFunAdr("CClient::SetSignonState", p_CClient_SetSignonState.GetPtr()); LogFunAdr("CClient::SendNetMsgEx", p_CClient_SendNetMsgEx.GetPtr()); LogFunAdr("CClient::SendSnapshot", p_CClient_SendSnapshot.GetPtr()); + LogFunAdr("CClient::ProcessStringCmd", p_CClient_ProcessStringCmd.GetPtr()); + LogFunAdr("CClient::ProcessSetConVar", p_CClient_ProcessSetConVar.GetPtr()); } virtual void GetFun(void) const { @@ -250,25 +256,29 @@ class VClient : public IDetour p_CClient_Clear = g_GameDll.FindPatternSIMD("40 53 41 56 41 57 48 83 EC 20 48 8B D9 48 89 74"); #if defined (GAMEDLL_S0) || defined (GAMEDLL_S1) p_CClient_ActivatePlayer = g_GameDll.FindPatternSIMD("40 53 57 41 57 48 83 EC 30 8B 81 ?? ?? ?? ??"); - p_CClient_ProcessStringCmd = g_GameDll.FindPatternSIMD("48 89 5C 24 ?? 55 48 81 EC ?? ?? ?? ?? 49 8B D8"); p_CClient_SendNetMsg = g_GameDll.FindPatternSIMD("48 89 5C 24 ?? 48 89 6C 24 ?? 48 89 74 24 ?? 57 41 56 41 57 48 83 EC 30 48 8B 05 ?? ?? ?? ?? 45 0F B6 F1"); p_CClient_SendSnapshot = g_GameDll.FindPatternSIMD("44 89 44 24 ?? 48 89 4C 24 ?? 55 53 56 57 41 55"); + p_CClient_ProcessStringCmd = g_GameDll.FindPatternSIMD("48 89 5C 24 ?? 55 48 81 EC ?? ?? ?? ?? 49 8B D8"); #elif defined (GAMEDLL_S2) || defined (GAMEDLL_S3) p_CClient_ActivatePlayer = g_GameDll.FindPatternSIMD("40 53 48 83 EC 20 8B 81 B0 03 ?? ?? 48 8B D9 C6"); - p_CClient_ProcessStringCmd = g_GameDll.FindPatternSIMD("48 89 6C 24 ?? 57 48 81 EC ?? ?? ?? ?? 48 8B 7A 20"); p_CClient_SendNetMsgEx = g_GameDll.FindPatternSIMD("40 53 55 56 57 41 56 48 83 EC 40 48 8B 05 ?? ?? ?? ??"); p_CClient_SendSnapshot = g_GameDll.FindPatternSIMD("48 89 5C 24 ?? 55 56 41 55 41 56 41 57 48 8D 6C 24 ??"); + p_CClient_ProcessStringCmd = g_GameDll.FindPatternSIMD("48 89 6C 24 ?? 57 48 81 EC ?? ?? ?? ?? 48 8B 7A 20"); #endif // !GAMEDLL_S0 || !GAMEDLL_S1 + + p_CClient_ProcessSetConVar = g_GameDll.FindPatternSIMD("48 83 EC 28 48 83 C2 20"); p_CClient_SetSignonState = g_GameDll.FindPatternSIMD("48 8B C4 48 89 58 10 48 89 70 18 57 48 81 EC ?? ?? ?? ?? 0F 29 70 E8 8B F2"); v_CClient_Connect = p_CClient_Connect.RCast(); v_CClient_Disconnect = p_CClient_Disconnect.RCast(); v_CClient_Clear = p_CClient_Clear.RCast(); v_CClient_ActivatePlayer = p_CClient_ActivatePlayer.RCast(); - v_CClient_ProcessStringCmd = p_CClient_ProcessStringCmd.RCast(); v_CClient_SetSignonState = p_CClient_SetSignonState.RCast(); v_CClient_SendNetMsgEx = p_CClient_SendNetMsgEx.RCast(); v_CClient_SendSnapshot = p_CClient_SendSnapshot.RCast(); + + v_CClient_ProcessStringCmd = p_CClient_ProcessStringCmd.RCast(); + v_CClient_ProcessSetConVar = p_CClient_ProcessSetConVar.RCast(); } virtual void GetVar(void) const { } virtual void GetCon(void) const { } diff --git a/r5dev/engine/server/vengineserver_impl.h b/r5dev/engine/server/vengineserver_impl.h index 605ecc67..0d0164ba 100644 --- a/r5dev/engine/server/vengineserver_impl.h +++ b/r5dev/engine/server/vengineserver_impl.h @@ -13,23 +13,23 @@ inline bool* m_bIsDedicated = nullptr; struct ServerPlayer_t { ServerPlayer_t(void) - : m_flCurrentNetProcessTime(0.0) - , m_flLastNetProcessTime(0.0) - , m_flStringCommandQuotaTimeStart(0.0) - , m_nStringCommandQuotaCount(0) - {} + { + Reset(); + } inline void Reset(void) { m_flCurrentNetProcessTime = 0.0; m_flLastNetProcessTime = 0.0; m_flStringCommandQuotaTimeStart = 0.0; - m_nStringCommandQuotaCount = 0; + m_nStringCommandQuotaCount = NULL; + m_bInitialConVarsSet = false; } double m_flCurrentNetProcessTime; double m_flLastNetProcessTime; double m_flStringCommandQuotaTimeStart; int m_nStringCommandQuotaCount; + bool m_bInitialConVarsSet; }; extern ServerPlayer_t g_ServerPlayer[MAX_PLAYERS];