Engine: fix RCON bugs and improve security

- Upgraded hashing algorithm to SHA-512, and store the raw hash instead of a string copy, which is way cheaper to compute and compare.
- Only ever close sockets once in CRConServer::SetPassword().
- Made the game server & game client RCON singletons static.
- Added calls to gracefully shutdown RCON server and RCON client on Engine/SDK shutdown.
- Added more prints so RCON user knows when its shutdown, or when their password change is in effect, etc.
- Fixed bug where we could tokenize an empty string when we dispatch a console command.

src/core/init.cpp

@@ -72,6 +72,7 @@
 #include "rtech/rui/rui.h"
 #include "engine/client/cl_ents_parse.h"
 #include "engine/client/cl_main.h"
+#include "engine/client/cl_rcon.h"
 #include "engine/client/cl_splitscreen.h"
 #endif // !DEDICATED
 #include "engine/client/client.h"
@@ -94,6 +95,7 @@
 #include "engine/networkstringtable.h"
 #ifndef CLIENT_DLL
 #include "engine/server/sv_main.h"
+#include "engine/server/sv_rcon.h"
 #endif // !CLIENT_DLL
 #include "engine/sdk_dll.h"
 #include "engine/sys_dll.h"
@@ -267,6 +269,14 @@ void Systems_Init()
 
 void Systems_Shutdown()
 {
+	// Shutdown RCON (closes all open sockets)
+#ifndef CLIENT_DLL
+	RCONServer()->Shutdown();
+#endif// !CLIENT_DLL
+#ifndef SERVER_DLL
+	RCONClient()->Shutdown();
+#endif // !SERVER_DLL
+
 	CFastTimer shutdownTimer;
 	shutdownTimer.Start();
 

src/engine/client/cl_rcon.cpp

@@ -29,6 +29,9 @@ CRConClient::CRConClient()
 //-----------------------------------------------------------------------------
 CRConClient::~CRConClient(void)
 {
+	// NOTE: do not call Shutdown() from the destructor as the OS's socket
+	// system would be shutdown by now, call Shutdown() in application
+	// shutdown code instead
 }
 
 //-----------------------------------------------------------------------------
@@ -76,7 +79,7 @@ void CRConClient::Disconnect(const char* szReason)
 			szReason = "unknown reason";
 		}
 
-		Msg(eDLL_T::CLIENT, "Disconnect: (%s)\n", szReason);
+		Msg(eDLL_T::CLIENT, "RCON disconnect: (%s)\n", szReason);
 		m_Socket.CloseAcceptedSocket(0);
 	}
 }
@@ -221,8 +224,8 @@ bool CRConClient::IsConnected(void)
 }
 
 ///////////////////////////////////////////////////////////////////////////////
-CRConClient* g_RCONClient(new CRConClient());
+static CRConClient s_RCONClient;
 CRConClient* RCONClient() // Singleton RCON Client.
 {
-	return g_RCONClient;
+	return &s_RCONClient;
 }

src/engine/server/sv_rcon.cpp

@@ -13,8 +13,8 @@
 #include "engine/server/sv_rcon.h"
 #include "protoc/sv_rcon.pb.h"
 #include "protoc/cl_rcon.pb.h"
-#include "mathlib/sha256.h"
 #include "common/igameserverdata.h"
+#include "mbedtls/include/mbedtls/sha512.h"
 
 //-----------------------------------------------------------------------------
 // Purpose: 
@@ -39,6 +39,9 @@ CRConServer::CRConServer(void)
 //-----------------------------------------------------------------------------
 CRConServer::~CRConServer(void)
 {
+	// NOTE: do not call Shutdown() from the destructor as the OS's socket
+	// system would be shutdown by now, call Shutdown() in application
+	// shutdown code instead
 }
 
 //-----------------------------------------------------------------------------
@@ -73,13 +76,17 @@ void CRConServer::Init(void)
 //-----------------------------------------------------------------------------
 void CRConServer::Shutdown(void)
 {
+	m_bInitialized = false;
+
+	const int nConnCount = m_Socket.GetAcceptedSocketCount();
 	m_Socket.CloseAllAcceptedSockets();
+
 	if (m_Socket.IsListening())
 	{
 		m_Socket.CloseListenSocket();
 	}
 
-	m_bInitialized = false;
+	Msg(eDLL_T::SERVER, "Remote server access deinitialized ('%i' accepted sockets closed)\n", nConnCount);
 }
 
 //-----------------------------------------------------------------------------
@@ -123,9 +130,6 @@ void CRConServer::Think(void)
 //-----------------------------------------------------------------------------
 bool CRConServer::SetPassword(const char* pszPassword)
 {
-	m_bInitialized = false;
-	m_Socket.CloseAllAcceptedSockets();
-
 	const size_t nLen = strlen(pszPassword);
 	if (nLen < RCON_MIN_PASSWORD_LEN)
 	{
@@ -139,8 +143,26 @@ bool CRConServer::SetPassword(const char* pszPassword)
 		return false;
 	}
 
-	m_svPasswordHash = sha256(pszPassword);
-	Msg(eDLL_T::SERVER, "Password hash ('%s')\n", m_svPasswordHash.c_str());
+	// This is here so we only print the confirmation message if the user
+	// actually requested to change the password rather than initializing
+	// the RCON server
+	const bool wasInitialized = m_bInitialized;
+
+	m_bInitialized = false;
+	m_Socket.CloseAllAcceptedSockets();
+
+	const int nHashRet = mbedtls_sha512(reinterpret_cast<const uint8_t*>(pszPassword), nLen, m_PasswordHash, NULL);
+
+	if (nHashRet != 0)
+	{
+		Error(eDLL_T::SERVER, 0, "SHA-512 algorithm failed on RCON password [%i]\n", nHashRet);
+		return false;
+	}
+
+	if (wasInitialized)
+	{
+		Msg(eDLL_T::SERVER, "Successfully changed RCON server password\n");
+	}
 
 	m_bInitialized = true;
 	return true;
@@ -358,24 +380,21 @@ void CRConServer::Authenticate(const cl_rcon::request& request, CConnectedNetCon
 }
 
 //-----------------------------------------------------------------------------
-// Purpose: sha256 hashed password comparison
+// Purpose: sha512 hashed password comparison
 // Input  : &svPassword - 
 // Output : true if matches, false otherwise
 //-----------------------------------------------------------------------------
 bool CRConServer::Comparator(const string& svPassword) const
 {
-	string passwordHash = sha256(svPassword);
-	if (sv_rcon_debug->GetBool())
-	{
-		Msg(eDLL_T::SERVER, "+---------------------------------------------------------------------------+\n");
-		Msg(eDLL_T::SERVER, "[ Server: '%s']\n", m_svPasswordHash.c_str());
-		Msg(eDLL_T::SERVER, "[ Client: '%s']\n", passwordHash.c_str());
-		Msg(eDLL_T::SERVER, "+---------------------------------------------------------------------------+\n");
-	}
-	if (memcmp(passwordHash.data(), m_svPasswordHash.data(), SHA256::DIGEST_SIZE) == 0)
+	uint8_t clientPasswordHash[RCON_SHA512_HASH_SIZE];
+	mbedtls_sha512(reinterpret_cast<const uint8_t*>(svPassword.c_str()), svPassword.length(),
+		clientPasswordHash, NULL);
+
+	if (memcmp(clientPasswordHash, m_PasswordHash, RCON_SHA512_HASH_SIZE) == 0)
 	{
 		return true;
 	}
+
 	return false;
 }
 
@@ -492,7 +511,13 @@ void CRConServer::Execute(const cl_rcon::request& request) const
 	else // Invoke command callback directly.
 	{
 		CCommand cmd;
-		cmd.Tokenize(pValueString, cmd_source_t::kCommandSrcCode);
+
+		// Only tokenize if we actually have strings in the value buffer, some
+		// commands (like 'status') don't need any additional parameters.
+		if (VALID_CHARSTAR(pValueString))
+		{
+			cmd.Tokenize(pValueString, cmd_source_t::kCommandSrcCode);
+		}
 
 		v_Cmd_Dispatch(ECommandTarget_t::CBUF_SERVER, pCommandBase, &cmd, false);
 	}
@@ -653,8 +678,8 @@ int CRConServer::GetAuthenticatedCount(void) const
 }
 
 ///////////////////////////////////////////////////////////////////////////////
-CRConServer* g_RCONServer(new CRConServer());
+static CRConServer s_RCONServer;
 CRConServer* RCONServer() // Singleton RCON Server.
 {
-	return g_RCONServer;
+	return &s_RCONServer;
 }

src/engine/server/sv_rcon.h

@@ -7,6 +7,7 @@
 
 #define RCON_MIN_PASSWORD_LEN 8
 #define RCON_MAX_BANNEDLIST_SIZE 512
+#define RCON_SHA512_HASH_SIZE 64
 
 class CRConServer : public CNetConBase
 {
@@ -59,7 +60,7 @@ private:
 	int                      m_nAuthConnections;
 	bool                     m_bInitialized;
 	std::unordered_set<std::string> m_BannedList;
-	std::string              m_svPasswordHash;
+	uint8_t                  m_PasswordHash[RCON_SHA512_HASH_SIZE];
 	netadr_t                 m_WhiteListAddress;
 };