RCON server improvements

* Prevent attacker from being able to abuse and overflow the banned list vector.
* Improved IPv6 comparison performance.
* Change size fields of payload frame from unsigned to signed.
* Close all accepted sockets on RCON server shutdown.

r5dev/common/igameserverdata.h

@@ -31,8 +31,8 @@ class CConnectedNetConsoleData
 {
 public:
 	SocketHandle_t m_hSocket;
-	u_long m_nPayloadLen;   // Num bytes for this message.
+	int  m_nPayloadLen;     // Num bytes for this message.
-	u_long m_nPayloadRead;  // Num read bytes from input buffer.
+	int  m_nPayloadRead;    // Num read bytes from input buffer.
 	int  m_nFailedAttempts; // Num failed authentication attempts.
 	int  m_nIgnoredMessage; // Count how many times client ignored the no-auth message.
 	bool m_bValidated;      // Revalidates netconsole if false.

r5dev/engine/server/sv_rcon.cpp

@@ -58,10 +58,12 @@ void CRConServer::Init(void)
 //-----------------------------------------------------------------------------
 void CRConServer::Shutdown(void)
 {
+	m_Socket.CloseAllAcceptedSockets();
 	if (m_Socket.IsListening())
 	{
 		m_Socket.CloseListenSocket();
 	}
+
 	m_bInitialized = false;
 }
 
@@ -78,7 +80,7 @@ void CRConServer::Think(void)
 		for (m_nConnIndex = nCount - 1; m_nConnIndex >= 0; m_nConnIndex--)
 		{
 			const netadr_t& netAdr = m_Socket.GetAcceptedSocketAddress(m_nConnIndex);
-			if (strcmp(netAdr.ToString(true), sv_rcon_whitelist_address->GetString()) != 0)
+			if (!m_WhiteListAddress.CompareAdr(netAdr))
 			{
 				const CConnectedNetConsoleData* pData = m_Socket.GetAcceptedSocketData(m_nConnIndex);
 				if (!pData->m_bAuthorized)
@@ -110,11 +112,12 @@ bool CRConServer::SetPassword(const char* pszPassword)
 	m_Socket.CloseAllAcceptedSockets();
 
 	const size_t nLen = std::strlen(pszPassword);
-	if (nLen < 8)
+	if (nLen < RCON_MIN_PASSWORD_LEN)
 	{
-		if (nLen > 0)
+		if (nLen > NULL)
 		{
-			Warning(eDLL_T::SERVER, "Remote server access requires a password of at least 8 characters\n");
+			Warning(eDLL_T::SERVER, "Remote server access requires a password of at least %i characters\n",
+				RCON_MIN_PASSWORD_LEN);
 		}
 
 		this->Shutdown();
@@ -128,6 +131,16 @@ bool CRConServer::SetPassword(const char* pszPassword)
 	return true;
 }
 
+//-----------------------------------------------------------------------------
+// Purpose: sets the white list address
+// Input  : *pszAddress - 
+// Output : true on success, false otherwise
+//-----------------------------------------------------------------------------
+bool CRConServer::SetWhiteListAddress(const char* pszAddress)
+{
+	return m_WhiteListAddress.SetFromString(pszAddress);
+}
+
 //-----------------------------------------------------------------------------
 // Purpose: server RCON main loop (run this every frame)
 //-----------------------------------------------------------------------------
@@ -338,7 +351,8 @@ void CRConServer::Authenticate(const cl_rcon::request& cl_request, CConnectedNet
 			m_Socket.CloseListenSocket();
 
 			this->CloseNonAuthConnection();
-			this->Send(pData->m_hSocket, this->Serialize(s_pszAuthMessage, sv_rcon_sendlogs->GetString(), sv_rcon::response_t::SERVERDATA_RESPONSE_AUTH, static_cast<int>(eDLL_T::NETCON)));
+			this->Send(pData->m_hSocket, this->Serialize(s_pszAuthMessage, sv_rcon_sendlogs->GetString(),
+				sv_rcon::response_t::SERVERDATA_RESPONSE_AUTH, static_cast<int>(eDLL_T::NETCON)));
 		}
 		else // Bad password.
 		{
@@ -348,7 +362,8 @@ void CRConServer::Authenticate(const cl_rcon::request& cl_request, CConnectedNet
 				DevMsg(eDLL_T::SERVER, "Bad RCON password attempt from '%s'\n", netAdr.ToString());
 			}
 
-			this->Send(pData->m_hSocket, this->Serialize(s_pszWrongPwMessage, "", sv_rcon::response_t::SERVERDATA_RESPONSE_AUTH, static_cast<int>(eDLL_T::NETCON)));
+			this->Send(pData->m_hSocket, this->Serialize(s_pszWrongPwMessage, "",
+				sv_rcon::response_t::SERVERDATA_RESPONSE_AUTH, static_cast<int>(eDLL_T::NETCON)));
 
 			pData->m_bAuthorized = false;
 			pData->m_bValidated = false;
@@ -407,7 +422,7 @@ void CRConServer::ProcessBuffer(const char* pRecvBuf, int nRecvLen, CConnectedNe
 				pData->m_nPayloadRead = 0;
 			}
 		}
-		else if (pData->m_nPayloadRead < sizeof(u_long)) // Read size field.
+		else if (pData->m_nPayloadRead+1 <= sizeof(int)) // Read size field.
 		{
 			pData->m_RecvBuffer[pData->m_nPayloadRead++] = *pRecvBuf;
 
@@ -416,7 +431,7 @@ void CRConServer::ProcessBuffer(const char* pRecvBuf, int nRecvLen, CConnectedNe
 		}
 		else // Build prefix.
 		{
-			pData->m_nPayloadLen = ntohl(*reinterpret_cast<u_long*>(&pData->m_RecvBuffer[0]));
+			pData->m_nPayloadLen = int(ntohl(*reinterpret_cast<u_long*>(&pData->m_RecvBuffer[0])));
 			pData->m_nPayloadRead = 0;
 
 			if (!pData->m_bAuthorized)
@@ -532,12 +547,36 @@ bool CRConServer::CheckForBan(CConnectedNetConsoleData* pData)
 		return false;
 	}
 
+	const netadr_t netAdr = m_Socket.GetAcceptedSocketAddress(m_nConnIndex);
+	const char* szNetAdr = netAdr.ToString(true);
+
+	if (m_BannedList.size() >= RCON_MAX_BANNEDLIST_SIZE)
+	{
+		const char* pszWhiteListAddress = sv_rcon_whitelist_address->GetString();
+		if (!pszWhiteListAddress[0])
+		{
+			DevMsg(eDLL_T::SERVER, "Banned list overflowed; please use a whitelist address. RCON shutting down...\n");
+			this->Shutdown();
+
+			return true;
+		}
+
+		// Only allow whitelisted at this point.
+		if (!m_WhiteListAddress.CompareAdr(netAdr))
+		{
+			if (sv_rcon_debug->GetBool())
+			{
+				DevMsg(eDLL_T::SERVER, "Banned list is full; dropping '%s'\n", szNetAdr);
+			}
+
+			return true;
+		}
+	}
+
 	pData->m_bValidated = true;
-	netadr_t netAdr = m_Socket.GetAcceptedSocketAddress(m_nConnIndex);
 
 	// Check if IP is in the banned list.
-	if (std::find(m_BannedList.begin(), m_BannedList.end(),
+	if (m_BannedList.find(szNetAdr) != m_BannedList.end())
-		netAdr.ToString(true)) != m_BannedList.end())
 	{
 		return true;
 	}
@@ -547,15 +586,15 @@ bool CRConServer::CheckForBan(CConnectedNetConsoleData* pData)
 		|| pData->m_nIgnoredMessage >= sv_rcon_maxignores->GetInt())
 	{
 		// Don't add white listed address to banned list.
-		if (strcmp(netAdr.ToString(true), sv_rcon_whitelist_address->GetString()) == 0)
+		if (szNetAdr == sv_rcon_whitelist_address->GetString())
 		{
 			pData->m_nFailedAttempts = 0;
 			pData->m_nIgnoredMessage = 0;
 			return false;
 		}
 
-		DevMsg(eDLL_T::SERVER, "Banned '%s' for RCON hacking attempts\n", netAdr.ToString());
+		DevMsg(eDLL_T::SERVER, "Banned '%s' for RCON hacking attempts\n", szNetAdr);
-		m_BannedList.push_back(netAdr.ToString(true));
+		m_BannedList.insert(szNetAdr);
 		return true;
 	}
 	return false;

r5dev/engine/server/sv_rcon.h

@@ -9,6 +9,9 @@ constexpr char s_pszWrongPwMessage[] = "Admin password incorrect.\n";
 constexpr char s_pszBannedMessage[]  = "Go away.\n";
 constexpr char s_pszAuthMessage[]    = "Authentication successful.\n";
 
+#define RCON_MIN_PASSWORD_LEN 8
+#define RCON_MAX_BANNEDLIST_SIZE 512
+
 class CRConServer
 {
 public:
@@ -17,7 +20,9 @@ public:
 
 	void Init(void);
 	void Shutdown(void);
+
 	bool SetPassword(const char* pszPassword);
+	bool SetWhiteListAddress(const char* pszAddress);
 
 	void Think(void);
 	void RunFrame(void);
@@ -53,10 +58,11 @@ private:
 
 	bool                     m_bInitialized;
 	int                      m_nConnIndex;
-	std::vector<std::string> m_BannedList;
+	std::unordered_set<std::string> m_BannedList;
 	std::string              m_svPasswordHash;
 	netadr_t                 m_Address;
+	netadr_t                 m_WhiteListAddress;
 	CSocketCreator           m_Socket;
 };
 
-CRConServer* RCONServer();
+CRConServer* RCONServer();

r5dev/tier1/cvar.cpp

@@ -348,7 +348,7 @@ void ConVar::StaticInit(void)
 	sv_rcon_maxfailures = ConVar::StaticCreate("sv_rcon_maxfailures", "10", FCVAR_RELEASE, "Max number of times a user can fail rcon authentication before being banned.", true, 1.f, false, 0.f, nullptr, nullptr);
 	sv_rcon_maxignores  = ConVar::StaticCreate("sv_rcon_maxignores" , "15", FCVAR_RELEASE, "Max number of times a user can ignore the instruction message before being banned.", true, 1.f, false, 0.f, nullptr, nullptr);
 	sv_rcon_maxsockets  = ConVar::StaticCreate("sv_rcon_maxsockets" , "32", FCVAR_RELEASE, "Max number of accepted sockets before the server starts closing redundant sockets.", true, 1.f, false, 0.f, nullptr, nullptr);
-	sv_rcon_whitelist_address = ConVar::StaticCreate("sv_rcon_whitelist_address", "", FCVAR_RELEASE, "This address is not considered a 'redundant' socket and will never be banned for failed authentication attempts.", false, 0.f, false, 0.f, nullptr, "Format: '::ffff:127.0.0.1'");
+	sv_rcon_whitelist_address = ConVar::StaticCreate("sv_rcon_whitelist_address", "", FCVAR_RELEASE, "This address is not considered a 'redundant' socket and will never be banned for failed authentication attempts.", false, 0.f, false, 0.f, &RCON_WhiteListAddresChanged_f, "Format: '::ffff:127.0.0.1'");
 
 	sv_quota_stringCmdsPerSecond = ConVar::StaticCreate("sv_quota_stringCmdsPerSecond", "16", FCVAR_RELEASE, "How many string commands per second clients are allowed to submit, 0 to disallow all string commands.", true, 0.f, false, 0.f, nullptr, nullptr);
 	sv_validatePersonaName  = ConVar::StaticCreate("sv_validatePersonaName" , "1" , FCVAR_RELEASE, "Validate the client's textual persona name on connect.", true, 0.f, false, 0.f, nullptr, nullptr);

r5dev/vstdlib/callback.cpp

@@ -922,8 +922,8 @@ void RCON_Disconnect_f(const CCommand& args)
 =====================
 RCON_PasswordChanged_f
 
-  Change password on RCON server
+  Change RCON password
-  and RCON client
+  on server and client
 =====================
 */
 void RCON_PasswordChanged_f(IConVar* pConVar, const char* pOldString, float flOldValue)
@@ -946,6 +946,30 @@ void RCON_PasswordChanged_f(IConVar* pConVar, const char* pOldString, float flOl
 	}
 }
 
+#ifndef CLIENT_DLL
+/*
+=====================
+RCON_WhiteListAddresChanged_f
+
+  Change whitelist address
+  on RCON server
+=====================
+*/
+void RCON_WhiteListAddresChanged_f(IConVar* pConVar, const char* pOldString, float flOldValue)
+{
+	if (ConVar* pConVarRef = g_pCVar->FindVar(pConVar->GetCommandName()))
+	{
+		if (strcmp(pOldString, pConVarRef->GetString()) == NULL)
+			return; // Same address.
+
+		if (!RCONServer()->SetWhiteListAddress(pConVarRef->GetString()))
+		{
+			Warning(eDLL_T::COMMON, "Failed to set RCON whitelist address: %s\n", pConVarRef->GetString());
+		}
+	}
+}
+#endif // !CLIENT_DLL
+
 /*
 =====================
 SQVM_ServerScript_f

r5dev/vstdlib/callback.h

@@ -52,6 +52,7 @@ void RCON_Disconnect_f(const CCommand& args);
 #endif // !DEDICATED
 void RCON_PasswordChanged_f(IConVar* pConVar, const char* pOldString, float flOldValue);
 #ifndef CLIENT_DLL
+void RCON_WhiteListAddresChanged_f(IConVar* pConVar, const char* pOldString, float flOldValue);
 void SQVM_ServerScript_f(const CCommand& args);
 #endif // !CLIENT_DLL
 #ifndef DEDICATED