fix: session keys are 16 bytes sent as 32 hex characters

The key was a 32 bit number, sent to the client as the MD5 hash of its
decimal text. The client only stores the key and sends it back, so it
is now 16 bytes from the system entropy source written as 32 lowercase
hex characters, which fills the same 33 character field of the login
response.

The auth, master and world servers pass the key itself between them as
a 33 character string in place of the number: SET_SESSION_KEY,
SESSION_KEY_RESPONSE and NEW_SESSION_ALERT keep their IDs and carry the
string where the number was. The master server keeps one key for each
username, and a world server compares the key a client sends with the
one from the master server in constant time. A key stays valid until
the next login of its account replaces it, as before.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>

dCommon/GeneralUtils.cpp

@@ -369,3 +369,30 @@ namespace GeneralUtils::details {
 }
 
 #endif
+
+std::string GeneralUtils::GenerateUnpredictableHexString(const uint32_t byteCount) {
+	constexpr std::string_view hexDigits = "0123456789abcdef";
+
+	std::string hex;
+	hex.reserve(static_cast<uint64_t>(byteCount) * 2);
+	uint32_t word = 0;
+	for (uint32_t i = 0; i < byteCount; i++) {
+		// One 32 bit draw gives four bytes
+		if (i % 4 == 0) word = GenerateUnpredictableNumber<uint32_t>();
+		const uint8_t byte = static_cast<uint8_t>(word >> ((i % 4) * 8));
+		hex.push_back(hexDigits[byte >> 4]);
+		hex.push_back(hexDigits[byte & 0x0F]);
+	}
+	return hex;
+}
+
+bool GeneralUtils::EqualsInConstantTime(const std::string_view a, const std::string_view b) {
+	if (a.size() != b.size()) return false;
+
+	// volatile keeps the compiler from leaving the loop at the first difference
+	volatile uint8_t difference = 0;
+	for (uint64_t i = 0; i < a.size(); i++) {
+		difference = difference | (static_cast<uint8_t>(a[i]) ^ static_cast<uint8_t>(b[i]));
+	}
+	return difference == 0;
+}

dCommon/GeneralUtils.h

@@ -401,6 +401,18 @@ namespace GeneralUtils {
 		return value;
 	}
 
+	/**
+	 * Generates byteCount bytes from the operating system's entropy source
+	 * and returns them as lowercase hex, two characters for each byte.
+	 */
+	std::string GenerateUnpredictableHexString(const uint32_t byteCount);
+
+	/**
+	 * Compares two strings in a time that depends on their lengths only, not on where they differ.
+	 * Strings of different lengths are never equal.
+	 */
+	bool EqualsInConstantTime(const std::string_view a, const std::string_view b);
+
 	// https://www.quora.com/How-do-you-round-to-specific-increments-like-0-5-in-C
 	// Rounds to the nearest floating point value specified.
 	template <typename T, std::enable_if_t<std::is_floating_point_v<T>, int> = 0>

dMasterServer/MasterServer.cpp

@@ -92,7 +92,8 @@ bool shutdownSequenceStarted = false;
 int ShutdownSequence(int32_t signal = -1);
 int32_t FinalizeShutdown(int32_t signal = -1);
 void HandlePacket(Packet* packet);
-std::map<uint32_t, std::string> activeSessions;
+// The session key of each logged in account, by username
+std::map<std::string, std::string> activeSessions;
 SystemAddress authServerMasterPeerSysAddr;
 SystemAddress chatServerMasterPeerSysAddr;
 SystemAddress dashboardServerMasterPeerSysAddr;
@@ -830,37 +831,30 @@ namespace {
 	}
 
 	void OnSetSessionKey(const SetSessionKey& request, const SystemAddress& sysAddr) {
-		const uint32_t sessionKey = request.sessionKey;
+		const LUString& sessionKey = request.sessionKey;
 		const LUString& username = request.username;
 
-		for (auto it : activeSessions) {
-			if (it.second == username.string) {
-				activeSessions.erase(it.first);
-
-				NewSessionAlert alert;
-				alert.sessionKey = sessionKey;
-				alert.username = username;
-				alert.Broadcast();
-
-				break;
-			}
+		// A new login replaces the key, so tell the worlds to drop the old session
+		if (activeSessions.contains(username.string)) {
+			NewSessionAlert alert;
+			alert.sessionKey = sessionKey;
+			alert.username = username;
+			alert.Broadcast();
 		}
 
-		activeSessions.insert(std::make_pair(sessionKey, username.string));
+		activeSessions.insert_or_assign(username.string, sessionKey.string);
 		LOG("Got a session key for user %s", username.string.c_str());
 	}
 
 	void OnRequestSessionKey(const RequestSessionKey& request, const SystemAddress& sysAddr) {
 		const LUWString& username = request.username;
 		LOG("Requesting session key for %s", username.GetAsString().c_str());
-		for (auto key : activeSessions) {
-			if (key.second == username.GetAsString()) {
-				SessionKeyResponse response;
-				response.sessionKey = key.first;
-				response.username = username;
-				MasterPackets::SendTo(sysAddr, response);
-				break;
-			}
+		const auto session = activeSessions.find(username.GetAsString());
+		if (session != activeSessions.end()) {
+			SessionKeyResponse response;
+			response.sessionKey = LUString(session->second);
+			response.username = username;
+			MasterPackets::SendTo(sysAddr, response);
 		}
 	}
 

dNet/AuthPackets.cpp

@@ -308,10 +308,9 @@ void AuthPackets::SendLoginResponse(dServer* server, const SystemAddress& sysAdd
 		GeneralUtils::TryParse<uint16_t>(Game::config->GetValue("version_minor")).value_or(ClientVersion::minor);
 
 	// The user key
-	uint32_t sessionKey = GeneralUtils::GenerateUnpredictableNumber<uint32_t>();
-	std::string userHash = std::to_string(sessionKey);
-	userHash = md5(userHash);
-	loginResponse.userKey = LUWString(userHash);
+	// 16 bytes as 32 hex characters, which fills the client's key field
+	const std::string sessionKey = GeneralUtils::GenerateUnpredictableHexString(16);
+	loginResponse.userKey = LUWString(sessionKey);
 
 	// World Server IP
 	loginResponse.worldServerIP = LUString(wServerIP);
@@ -325,7 +324,7 @@ void AuthPackets::SendLoginResponse(dServer* server, const SystemAddress& sysAdd
 	if (responseCode == eLoginResponse::SUCCESS) {
 		stamps.Add(eStamps::PASSPORT_AUTH_IM_COMMUNICATION_START);
 		MasterPackets::SetSessionKey setSessionKey;
-		setSessionKey.sessionKey = sessionKey;
+		setSessionKey.sessionKey = LUString(sessionKey);
 		setSessionKey.username = LUString(username);
 		stamps.Add(eStamps::PASSPORT_AUTH_IM_LOGIN_START);
 		MasterPackets::SendToMaster(setSessionKey, server);

dWorldServer/WorldServer.cpp

@@ -873,19 +873,16 @@ void HandlePacketChat(Packet* packet) {
 namespace {
 	void OnSessionKeyResponse(const MasterPackets::SessionKeyResponse& response, const SystemAddress& masterAddr) {
 		//Read our session key and to which user it belongs:
-		const uint32_t sessionKey = response.sessionKey;
+		const LUString& sessionKey = response.sessionKey;
 		const LUWString& username = response.username;
 
 		//Find them:
 		auto it = g_PendingUsers.find(username.GetAsString());
 		if (it == g_PendingUsers.end()) return;
 
-		//Convert our key:
-		std::string userHash = std::to_string(sessionKey);
-		userHash = md5(userHash);
-
-		//Verify it:
-		if (userHash != it->second.hash) {
+		//Verify it, the key the client sent has to be the one master holds:
+		const std::string& userHash = sessionKey.string;
+		if (userHash.empty() || !GeneralUtils::EqualsInConstantTime(userHash, it->second.hash)) {
 			LOG("Session key mismatch for user %s", username.GetAsString().c_str());
 			Game::server->Disconnect(it->second.sysAddr, eServerDisconnectIdentifiers::INVALID_SESSION_KEY);
 			return;
@@ -958,7 +955,7 @@ namespace {
 	}
 
 	void OnNewSessionAlert(const MasterPackets::NewSessionAlert& alert, const SystemAddress& sysAddr) {
-		const uint32_t sessionKey = alert.sessionKey;
+		const LUString& sessionKey = alert.sessionKey;
 		const LUString& username = alert.username;
 		LOG("Got new session alert for user %s", username.string.c_str());
 		//Find them:
@@ -969,7 +966,7 @@ namespace {
 		}
 
 		//Check the key:
-		if (sessionKey != std::atoi(user->GetSessionKey().c_str())) {
+		if (!GeneralUtils::EqualsInConstantTime(sessionKey.string, user->GetSessionKey())) {
 			LOG("But the session key is invalid!", username.string.c_str());
 			Game::server->Disconnect(user->GetSystemAddress(), eServerDisconnectIdentifiers::INVALID_SESSION_KEY);
 			return;

tests/dCommonTests/TestGeneralUtils.cpp

@@ -2,7 +2,10 @@
 
 #include <random>
 #include <set>
+#include <string>
+#include <vector>
 
+#include "BitStreamUtils.h"
 #include "Game.h"
 #include "GeneralUtils.h"
 
@@ -66,3 +69,80 @@ TEST(GeneralUtilsTest, ReadWStringReadsWhatTheStreamHolds) {
 	RakNet::BitStream empty;
 	EXPECT_EQ(GeneralUtils::ReadWString(empty), u"");
 }
+
+TEST(GeneralUtilsTest, UnpredictableHexStringIsLowercaseHexOfTheAskedLength) {
+	EXPECT_TRUE(GeneralUtils::GenerateUnpredictableHexString(0).empty());
+	EXPECT_EQ(GeneralUtils::GenerateUnpredictableHexString(1).size(), 2);
+	EXPECT_EQ(GeneralUtils::GenerateUnpredictableHexString(5).size(), 10);
+
+	for (uint32_t i = 0; i < 16; i++) {
+		const auto key = GeneralUtils::GenerateUnpredictableHexString(16);
+		ASSERT_EQ(key.size(), 32);
+		EXPECT_EQ(key.find_first_not_of("0123456789abcdef"), std::string::npos);
+	}
+}
+
+TEST(GeneralUtilsTest, UnpredictableHexStringDiffersEveryTime) {
+	std::set<std::string> seen;
+	for (uint32_t i = 0; i < 64; i++) {
+		// The same seed every time would give the same string every time from the game engine
+		Game::randomEngine = std::mt19937(1234);
+		seen.insert(GeneralUtils::GenerateUnpredictableHexString(16));
+	}
+
+	EXPECT_EQ(seen.size(), 64);
+}
+
+TEST(GeneralUtilsTest, UnpredictableHexStringFillsEveryCharacter) {
+	// Every one of the 32 characters has to vary, none may be left at a fixed value
+	std::vector<std::set<char>> seen(32);
+	for (uint32_t i = 0; i < 64; i++) {
+		const auto key = GeneralUtils::GenerateUnpredictableHexString(16);
+		for (uint32_t j = 0; j < key.size(); j++) seen[j].insert(key[j]);
+	}
+
+	for (const auto& characters : seen) EXPECT_GT(characters.size(), 4);
+}
+
+TEST(GeneralUtilsTest, EqualsInConstantTime) {
+	const std::string key = "0123456789abcdef0123456789abcdef";
+
+	EXPECT_TRUE(GeneralUtils::EqualsInConstantTime(key, key));
+	EXPECT_TRUE(GeneralUtils::EqualsInConstantTime(key, std::string(key)));
+	EXPECT_TRUE(GeneralUtils::EqualsInConstantTime("", ""));
+
+	// A difference anywhere, first, middle or last character
+	EXPECT_FALSE(GeneralUtils::EqualsInConstantTime(key, "1123456789abcdef0123456789abcdef"));
+	EXPECT_FALSE(GeneralUtils::EqualsInConstantTime(key, "0123456789abcdef1123456789abcdef"));
+	EXPECT_FALSE(GeneralUtils::EqualsInConstantTime(key, "0123456789abcdef0123456789abcdee"));
+	EXPECT_FALSE(GeneralUtils::EqualsInConstantTime(key, "0123456789ABCDEF0123456789ABCDEF"));
+
+	// A prefix, a longer string and an empty one never match
+	EXPECT_FALSE(GeneralUtils::EqualsInConstantTime(key, key.substr(0, 31)));
+	EXPECT_FALSE(GeneralUtils::EqualsInConstantTime(key, key + "0"));
+	EXPECT_FALSE(GeneralUtils::EqualsInConstantTime(key, ""));
+	EXPECT_FALSE(GeneralUtils::EqualsInConstantTime("", key));
+
+	// A zero byte inside the string is compared like any other
+	EXPECT_FALSE(GeneralUtils::EqualsInConstantTime(std::string("ab\0d", 4), std::string("ab\0e", 4)));
+}
+
+// The key goes to the client as 33 wide characters, comes back the same way,
+// and travels between the servers as 33 narrow ones
+TEST(GeneralUtilsTest, SessionKeySurvivesItsPacketFields) {
+	const auto key = GeneralUtils::GenerateUnpredictableHexString(16);
+
+	RakNet::BitStream bitStream;
+	bitStream.Write(LUWString(key));
+	bitStream.Write(LUString(key));
+	ASSERT_EQ(bitStream.GetNumberOfBytesUsed(), 33 * 2 + 33);
+
+	LUWString fromClient;
+	LUString fromServer;
+	ASSERT_TRUE(bitStream.Read(fromClient));
+	ASSERT_TRUE(bitStream.Read(fromServer));
+
+	EXPECT_EQ(fromClient.GetAsString(), key);
+	EXPECT_EQ(fromServer.string, key);
+	EXPECT_TRUE(GeneralUtils::EqualsInConstantTime(fromClient.GetAsString(), fromServer.string));
+}