From 26b188852fe426fe56d237d23fe940470c6b1ac3 Mon Sep 17 00:00:00 2001 From: Aaron Kimbrell Date: Tue, 10 Nov 2026 15:37:08 -0600 Subject: [PATCH] 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 --- dCommon/GeneralUtils.cpp | 27 +++++++++ dCommon/GeneralUtils.h | 12 ++++ dMasterServer/MasterServer.cpp | 38 +++++------- dNet/AuthPackets.cpp | 9 ++- dWorldServer/WorldServer.cpp | 15 ++--- tests/dCommonTests/TestGeneralUtils.cpp | 80 +++++++++++++++++++++++++ 6 files changed, 145 insertions(+), 36 deletions(-) diff --git a/dCommon/GeneralUtils.cpp b/dCommon/GeneralUtils.cpp index 7004b2fdc..d21d30b4d 100644 --- a/dCommon/GeneralUtils.cpp +++ b/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(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(); + const uint8_t byte = static_cast(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(a[i]) ^ static_cast(b[i])); + } + return difference == 0; +} diff --git a/dCommon/GeneralUtils.h b/dCommon/GeneralUtils.h index 150c6165d..c734b0ff9 100644 --- a/dCommon/GeneralUtils.h +++ b/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 , int> = 0> diff --git a/dMasterServer/MasterServer.cpp b/dMasterServer/MasterServer.cpp index b797f6f5c..32f5b1307 100644 --- a/dMasterServer/MasterServer.cpp +++ b/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 activeSessions; +// The session key of each logged in account, by username +std::map 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); } } diff --git a/dNet/AuthPackets.cpp b/dNet/AuthPackets.cpp index 1549f1884..536f9a78c 100644 --- a/dNet/AuthPackets.cpp +++ b/dNet/AuthPackets.cpp @@ -308,10 +308,9 @@ void AuthPackets::SendLoginResponse(dServer* server, const SystemAddress& sysAdd GeneralUtils::TryParse(Game::config->GetValue("version_minor")).value_or(ClientVersion::minor); // The user key - uint32_t sessionKey = GeneralUtils::GenerateUnpredictableNumber(); - 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); diff --git a/dWorldServer/WorldServer.cpp b/dWorldServer/WorldServer.cpp index 6d931eea0..96f79385e 100644 --- a/dWorldServer/WorldServer.cpp +++ b/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; diff --git a/tests/dCommonTests/TestGeneralUtils.cpp b/tests/dCommonTests/TestGeneralUtils.cpp index 6af7f95fe..c21bf6557 100644 --- a/tests/dCommonTests/TestGeneralUtils.cpp +++ b/tests/dCommonTests/TestGeneralUtils.cpp @@ -2,7 +2,10 @@ #include #include +#include +#include +#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 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> 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)); +}