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)); +}