From 8aa61174508c84c297dff8cb2a5556363271d5bf Mon Sep 17 00:00:00 2001 From: William Tremblay Date: Sat, 19 Mar 2022 19:45:39 -0400 Subject: [PATCH 1/3] Fixed a mistake in the NRSSRSanitizer that leads to a potential OOB read for the server. Shouldn't be exploitable at all, but it should still be fixed. --- .../Server/Server/GameService/Utils/NRSSRSanitizer.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Source/Server/Server/GameService/Utils/NRSSRSanitizer.h b/Source/Server/Server/GameService/Utils/NRSSRSanitizer.h index e5767c05..76e034d6 100644 --- a/Source/Server/Server/GameService/Utils/NRSSRSanitizer.h +++ b/Source/Server/Server/GameService/Utils/NRSSRSanitizer.h @@ -152,8 +152,9 @@ class NRSSRSanitizer pos += 8; break; case 4: // Case 4 : Null-terminated wide string field - len = wcsnlen_s((wchar_t*)(nrssr_data + pos), size - pos); - if (len == size - pos || len >= MAX_PROP_WSTR_SIZE) return ValidationResult::NRSSR_PropertyString_Overflow; + size_t max_wstr_len = (size - pos) / 2; + len = wcsnlen_s((wchar_t*)(nrssr_data + pos), max_wstr_len); + if (len >= max_wstr_len || len >= MAX_PROP_WSTR_SIZE) return ValidationResult::NRSSR_PropertyString_Overflow; pos += 2 * (len + 1); break; default: @@ -162,8 +163,9 @@ class NRSSRSanitizer } // Check host name - len = wcsnlen_s((wchar_t*)(nrssr_data + pos), size - pos); - if (len == size - pos || len >= MAX_NAME_WSTR_SIZE) return ValidationResult::NRSSR_NameString_Overflow; + size_t max_wstr_len = (size - pos) / 2; + len = wcsnlen_s((wchar_t*)(nrssr_data + pos), max_wstr_len); + if (len >= max_wstr_len || len >= MAX_NAME_WSTR_SIZE) return ValidationResult::NRSSR_NameString_Overflow; pos += 2 * (len + 1); // Check host online id and session data size From 06981966d81d4727b30f1b94b0e89871aabff57b Mon Sep 17 00:00:00 2001 From: William Tremblay Date: Sun, 20 Mar 2022 00:17:39 -0400 Subject: [PATCH 2/3] Make NRSSRSanitizer code style consistent with the rest of the project --- .../BloodMessage/BloodMessageManager.cpp | 4 +- .../Bloodstain/BloodstainManager.cpp | 6 +- .../GameManagers/Ghosts/GhostManager.cpp | 4 +- .../GameManagers/Misc/MiscManager.cpp | 6 +- .../QuickMatch/QuickMatchManager.cpp | 4 +- .../GameManagers/Signs/SignManager.cpp | 6 +- .../GameManagers/Visitor/VisitorManager.cpp | 4 +- .../Server/GameService/Utils/NRSSRSanitizer.h | 86 +++++++++---------- 8 files changed, 60 insertions(+), 60 deletions(-) diff --git a/Source/Server/Server/GameService/GameManagers/BloodMessage/BloodMessageManager.cpp b/Source/Server/Server/GameService/GameManagers/BloodMessage/BloodMessageManager.cpp index 70774083..387e7384 100644 --- a/Source/Server/Server/GameService/GameManagers/BloodMessage/BloodMessageManager.cpp +++ b/Source/Server/Server/GameService/GameManagers/BloodMessage/BloodMessageManager.cpp @@ -252,9 +252,9 @@ MessageHandleResult BloodMessageManager::Handle_RequestCreateBloodMessage(GameCl MessageData.assign(Request->message_data().data(), Request->message_data().data() + Request->message_data().size()); // There is no NRSSR struct in blood messsage data, but we still make sure the size-delimited entry list is valid. - if (BuildConfig::NRSSR_SANITY_CHECKS) + if constexpr (BuildConfig::NRSSR_SANITY_CHECKS) { - auto ValidationResult = NRSSRSanitizer::IsValidEntryList(MessageData.data(), MessageData.size()); + auto ValidationResult = NRSSRSanitizer::ValidateEntryList(MessageData.data(), MessageData.size()); if (ValidationResult != NRSSRSanitizer::ValidationResult::Valid) { WarningS(Client->GetName().c_str(), "Blood message data recieved from client is invalid (error code %i).", diff --git a/Source/Server/Server/GameService/GameManagers/Bloodstain/BloodstainManager.cpp b/Source/Server/Server/GameService/GameManagers/Bloodstain/BloodstainManager.cpp index c4cfe3d2..326b5c92 100644 --- a/Source/Server/Server/GameService/GameManagers/Bloodstain/BloodstainManager.cpp +++ b/Source/Server/Server/GameService/GameManagers/Bloodstain/BloodstainManager.cpp @@ -95,9 +95,9 @@ MessageHandleResult BloodstainManager::Handle_RequestCreateBloodstain(GameClient GhostData.assign(Request->ghost_data().data(), Request->ghost_data().data() + Request->ghost_data().size()); // There is no NRSSR struct in bloodstain or ghost data, but we still make sure the size-delimited entry list is valid. - if (BuildConfig::NRSSR_SANITY_CHECKS) + if constexpr (BuildConfig::NRSSR_SANITY_CHECKS) { - auto ValidationResult = NRSSRSanitizer::IsValidEntryList(Data.data(), Data.size()); + auto ValidationResult = NRSSRSanitizer::ValidateEntryList(Data.data(), Data.size()); if (ValidationResult != NRSSRSanitizer::ValidationResult::Valid) { WarningS(Client->GetName().c_str(), "Bloodstain metadata recieved from client is invalid (error code %i).", @@ -105,7 +105,7 @@ MessageHandleResult BloodstainManager::Handle_RequestCreateBloodstain(GameClient return MessageHandleResult::Handled; } - ValidationResult = NRSSRSanitizer::IsValidEntryList(GhostData.data(), GhostData.size()); + ValidationResult = NRSSRSanitizer::ValidateEntryList(GhostData.data(), GhostData.size()); if (ValidationResult != NRSSRSanitizer::ValidationResult::Valid) { WarningS(Client->GetName().c_str(), "Ghost data recieved from client is invalid (error code %i).", diff --git a/Source/Server/Server/GameService/GameManagers/Ghosts/GhostManager.cpp b/Source/Server/Server/GameService/GameManagers/Ghosts/GhostManager.cpp index f5a42d90..2dea2a64 100644 --- a/Source/Server/Server/GameService/GameManagers/Ghosts/GhostManager.cpp +++ b/Source/Server/Server/GameService/GameManagers/Ghosts/GhostManager.cpp @@ -89,9 +89,9 @@ MessageHandleResult GhostManager::Handle_RequestCreateGhostData(GameClient* Clie Data.assign(Request->data().data(), Request->data().data() + Request->data().size()); // There is no NRSSR struct in ghost data, but we still make sure the size-delimited entry list is valid. - if (BuildConfig::NRSSR_SANITY_CHECKS) + if constexpr (BuildConfig::NRSSR_SANITY_CHECKS) { - auto ValidationResult = NRSSRSanitizer::IsValidEntryList(Data.data(), Data.size()); + auto ValidationResult = NRSSRSanitizer::ValidateEntryList(Data.data(), Data.size()); if (ValidationResult != NRSSRSanitizer::ValidationResult::Valid) { WarningS(Client->GetName().c_str(), "Ghost data recieved from client is invalid (error code %i).", diff --git a/Source/Server/Server/GameService/GameManagers/Misc/MiscManager.cpp b/Source/Server/Server/GameService/GameManagers/Misc/MiscManager.cpp index 5968ed42..cee7c0d8 100644 --- a/Source/Server/Server/GameService/GameManagers/Misc/MiscManager.cpp +++ b/Source/Server/Server/GameService/GameManagers/Misc/MiscManager.cpp @@ -128,7 +128,7 @@ MessageHandleResult MiscManager::Handle_RequestSendMessageToPlayers(GameClient* // The game seems to only use this function in the BreakIn (invasions) implementation. // Hence we should make sure that a malicious client does not use this send arbitrary data to other players. bool ShouldProcessRequest = true; - if (BuildConfig::SEND_MESSAGE_TO_PLAYERS_SANITY_CHECKS) + if constexpr (BuildConfig::SEND_MESSAGE_TO_PLAYERS_SANITY_CHECKS) { // The request should contain a PushRequestBreakInTarget message Frpg2RequestMessage::PushRequestAllowBreakInTarget EmbdeddedMsg; @@ -146,9 +146,9 @@ MessageHandleResult MiscManager::Handle_RequestSendMessageToPlayers(GameClient* ShouldProcessRequest = false; } // Make sure the NRSSR data contained within this message is valid (if the CVE-2022-24126 fix is enabled) - else if (BuildConfig::NRSSR_SANITY_CHECKS) + else if constexpr (BuildConfig::NRSSR_SANITY_CHECKS) { - auto ValidationResult = NRSSRSanitizer::IsValidEntryList(EmbdeddedMsg.player_struct().data(), EmbdeddedMsg.player_struct().size()); + auto ValidationResult = NRSSRSanitizer::ValidateEntryList(EmbdeddedMsg.player_struct().data(), EmbdeddedMsg.player_struct().size()); if (ValidationResult != NRSSRSanitizer::ValidationResult::Valid) { WarningS(Client->GetName().c_str(), "PushRequestAllowBreakInTarget message recieved from client contains ill formated binary data (error code %i).", diff --git a/Source/Server/Server/GameService/GameManagers/QuickMatch/QuickMatchManager.cpp b/Source/Server/Server/GameService/GameManagers/QuickMatch/QuickMatchManager.cpp index 841a09f6..d1a112f1 100644 --- a/Source/Server/Server/GameService/GameManagers/QuickMatch/QuickMatchManager.cpp +++ b/Source/Server/Server/GameService/GameManagers/QuickMatch/QuickMatchManager.cpp @@ -362,9 +362,9 @@ MessageHandleResult QuickMatchManager::Handle_RequestAcceptQuickMatch(GameClient bool ShouldProcessRequest = true; // Make sure the NRSSR data contained within this message is valid (if the CVE-2022-24126 fix is enabled) - if (BuildConfig::NRSSR_SANITY_CHECKS) + if constexpr (BuildConfig::NRSSR_SANITY_CHECKS) { - auto ValidationResult = NRSSRSanitizer::IsValidEntryList(Request->data().data(), Request->data().size()); + auto ValidationResult = NRSSRSanitizer::ValidateEntryList(Request->data().data(), Request->data().size()); if (ValidationResult != NRSSRSanitizer::ValidationResult::Valid) { WarningS(Client->GetName().c_str(), "RequestAcceptQuickMatch message recieved from client contains ill formated binary data (error code %i).", diff --git a/Source/Server/Server/GameService/GameManagers/Signs/SignManager.cpp b/Source/Server/Server/GameService/GameManagers/Signs/SignManager.cpp index 9a2e46d5..0dde7835 100644 --- a/Source/Server/Server/GameService/GameManagers/Signs/SignManager.cpp +++ b/Source/Server/Server/GameService/GameManagers/Signs/SignManager.cpp @@ -234,9 +234,9 @@ MessageHandleResult SignManager::Handle_RequestCreateSign(GameClient* Client, co Frpg2RequestMessage::RequestCreateSign* Request = (Frpg2RequestMessage::RequestCreateSign*)Message.Protobuf.get(); // There is no NRSSR struct in the sign metadata, but we still make sure the size-delimited entry list is valid. - if (BuildConfig::NRSSR_SANITY_CHECKS) + if constexpr (BuildConfig::NRSSR_SANITY_CHECKS) { - auto ValidationResult = NRSSRSanitizer::IsValidEntryList(Request->player_struct().data(), Request->player_struct().size()); + auto ValidationResult = NRSSRSanitizer::ValidateEntryList(Request->player_struct().data(), Request->player_struct().size()); if (ValidationResult != NRSSRSanitizer::ValidationResult::Valid) { WarningS(Client->GetName().c_str(), "RequestCreateSign message recieved from client contains ill formated binary data (error code %i).", @@ -342,7 +342,7 @@ MessageHandleResult SignManager::Handle_RequestSummonSign(GameClient* Client, co // Make sure the NRSSR data contained within this message is valid (if the CVE-2022-24126 fix is enabled) if (BuildConfig::NRSSR_SANITY_CHECKS) { - auto ValidationResult = NRSSRSanitizer::IsValidEntryList(Request->player_struct().data(), Request->player_struct().size()); + auto ValidationResult = NRSSRSanitizer::ValidateEntryList(Request->player_struct().data(), Request->player_struct().size()); if (ValidationResult != NRSSRSanitizer::ValidationResult::Valid) { WarningS(Client->GetName().c_str(), "RequestSummonSign message recieved from client contains ill formated binary data (error code %i).", diff --git a/Source/Server/Server/GameService/GameManagers/Visitor/VisitorManager.cpp b/Source/Server/Server/GameService/GameManagers/Visitor/VisitorManager.cpp index 2b9f794e..f7a95e84 100644 --- a/Source/Server/Server/GameService/GameManagers/Visitor/VisitorManager.cpp +++ b/Source/Server/Server/GameService/GameManagers/Visitor/VisitorManager.cpp @@ -128,9 +128,9 @@ MessageHandleResult VisitorManager::Handle_RequestVisit(GameClient* Client, cons bool bSuccess = true; // Make sure the NRSSR data contained within this message is valid (if the CVE-2022-24126 fix is enabled) - if (BuildConfig::NRSSR_SANITY_CHECKS) + if constexpr (BuildConfig::NRSSR_SANITY_CHECKS) { - auto ValidationResult = NRSSRSanitizer::IsValidEntryList(Request->data().data(), Request->data().size()); + auto ValidationResult = NRSSRSanitizer::ValidateEntryList(Request->data().data(), Request->data().size()); if (ValidationResult != NRSSRSanitizer::ValidationResult::Valid) { WarningS(Client->GetName().c_str(), "RequestVisit message recieved from client contains ill formated binary data (error code %i).", diff --git a/Source/Server/Server/GameService/Utils/NRSSRSanitizer.h b/Source/Server/Server/GameService/Utils/NRSSRSanitizer.h index 76e034d6..f7b507ae 100644 --- a/Source/Server/Server/GameService/Utils/NRSSRSanitizer.h +++ b/Source/Server/Server/GameService/Utils/NRSSRSanitizer.h @@ -85,95 +85,95 @@ class NRSSRSanitizer // Verify if length-delimited entry list data used to store session join information is valid. // Will also validate any NRSSR data stored in the entries. - static ValidationResult IsValidEntryList(const uint8_t* entry_list, size_t size) + static ValidationResult ValidateEntryList(const uint8_t* EntryList, size_t Size) { // Go over the entries in the entry list and make sure any NRSSR entry is valid size_t i = 0; - for (size_t entry_sz = 0; i + 8 <= size; i += entry_sz + 8) + for (size_t EntrySize = 0; i + 8 <= Size; i += EntrySize + 8) { - entry_sz = *(uint32_t*)(entry_list + i + 4); - if (i + 8 + entry_sz > size) return ValidationResult::EntryList_SizeMismatch; + EntrySize = *reinterpret_cast(EntryList + i + 4); + if (i + 8 + EntrySize > Size) return ValidationResult::EntryList_SizeMismatch; - const uint8_t* entry_data = entry_list + i + 8; + const uint8_t* EntryData = EntryList + i + 8; // Use the NRSSR signature and version to detect NRSSR data inside the entry list. // This may result in valid packets getting flagged, but from my testing it has never happened. - if (CheckNRSSRSignatureAndVersion(entry_data, entry_sz)) + if (CheckNRSSRSignatureAndVersion(EntryData, EntrySize)) { - ValidationResult res = IsValidNRSSRData(entry_data, entry_sz); - if (res != ValidationResult::Valid) return res; + ValidationResult Result = ValidateNRSSRData(EntryData, EntrySize); + if (Result != ValidationResult::Valid) return Result; } } // Make sure there is no data left is the buffer after the final entry, i.e. every byte has been // accounted for - return (i == size) ? ValidationResult::Valid : ValidationResult::EntryList_SizeMismatch; + return (i == Size) ? ValidationResult::Valid : ValidationResult::EntryList_SizeMismatch; } - static ValidationResult IsValidEntryList(const char* entry_list, size_t size) + static ValidationResult ValidateEntryList(const char* EntryList, size_t Size) { - return IsValidEntryList((const uint8_t*)entry_list, size); + return ValidateEntryList(reinterpret_cast(EntryList), Size); } // Verify if some data has correct NRSessionSearchResult signature and version number. - static bool CheckNRSSRSignatureAndVersion(const uint8_t* data, size_t size) + static bool CheckNRSSRSignatureAndVersion(const uint8_t* Data, size_t Size) { - if (size < 6) return false; - uint32_t sig = *(uint32_t*)(data + 0); - uint16_t ver = *(uint16_t*)(data + 4); - return sig == SIGNATURE && ver == VERSION_NUMBER; + if (Size < 6) return false; + uint32_t Signature = *reinterpret_cast(Data + 0); + uint16_t Version = *reinterpret_cast(Data + 4); + return Signature == SIGNATURE && Version == VERSION_NUMBER; } // Verify if serialized NRSessionSearchResult data is valid. - static ValidationResult IsValidNRSSRData(const uint8_t* nrssr_data, size_t size) + static ValidationResult ValidateNRSSRData(const uint8_t* NRSSRData, size_t Size) { // Check if signature and version number matches - if (!CheckNRSSRSignatureAndVersion(nrssr_data, size)) return ValidationResult::NRSSR_SignatureOrVersion_Mismatch; + if (!CheckNRSSRSignatureAndVersion(NRSSRData, Size)) return ValidationResult::NRSSR_SignatureOrVersion_Mismatch; // Make sure that we have enough data to read the property count field - if (size < 7) return ValidationResult::NRSSR_PropertyMetadata_InsufficientData;; - uint8_t prop_cnt = *(uint8_t*)(nrssr_data + 6); + if (Size < 7) return ValidationResult::NRSSR_PropertyMetadata_InsufficientData;; + uint8_t PropertyCount = NRSSRData[6]; // Parse the property list and verify each entry has a valid type and length - size_t pos = 7, len; - for (int i = 0; i < prop_cnt; i++) + size_t Position = 7, StrLength, NumWideCharLeft; + for (int i = 0; i < PropertyCount; i++) { // We don't care about the ID or unknown value, just check sizes - if (size - pos < 6) return ValidationResult::NRSSR_PropertyMetadata_InsufficientData; - uint8_t type = nrssr_data[pos + 4]; - pos += 6; + if (Size - Position < 6) return ValidationResult::NRSSR_PropertyMetadata_InsufficientData; + uint8_t Type = NRSSRData[Position + 4]; + Position += 6; - switch (type) + switch (Type) { case 1: // Case 1 : 4 byte field - if (size - pos < 4) return ValidationResult::NRSSR_Property4Byte_InsufficientData; - pos += 4; + if (Size - Position < 4) return ValidationResult::NRSSR_Property4Byte_InsufficientData; + Position += 4; break; case 2: case 3: // Cases 2-3 : 8 byte field (perhaps signed/unsigned?) - if (size - pos < 8) return ValidationResult::NRSSR_Property8Byte_InsufficientData; - pos += 8; + if (Size - Position < 8) return ValidationResult::NRSSR_Property8Byte_InsufficientData; + Position += 8; break; case 4: // Case 4 : Null-terminated wide string field - size_t max_wstr_len = (size - pos) / 2; - len = wcsnlen_s((wchar_t*)(nrssr_data + pos), max_wstr_len); - if (len >= max_wstr_len || len >= MAX_PROP_WSTR_SIZE) return ValidationResult::NRSSR_PropertyString_Overflow; - pos += 2 * (len + 1); + NumWideCharLeft = (Size - Position) / 2; + StrLength = wcsnlen_s(reinterpret_cast(NRSSRData + Position), NumWideCharLeft); + if (StrLength >= NumWideCharLeft || StrLength >= MAX_PROP_WSTR_SIZE) return ValidationResult::NRSSR_PropertyString_Overflow; + Position += 2 * (StrLength + 1); break; default: return ValidationResult::NRSSR_PropertyMetadata_InvalidType; } } - // Check host name - size_t max_wstr_len = (size - pos) / 2; - len = wcsnlen_s((wchar_t*)(nrssr_data + pos), max_wstr_len); - if (len >= max_wstr_len || len >= MAX_NAME_WSTR_SIZE) return ValidationResult::NRSSR_NameString_Overflow; - pos += 2 * (len + 1); + // Check if the host name is null terminated and has valid length + NumWideCharLeft = (Size - Position) / 2; + StrLength = wcsnlen_s(reinterpret_cast(NRSSRData + Position), NumWideCharLeft); + if (StrLength >= NumWideCharLeft || StrLength >= MAX_NAME_WSTR_SIZE) return ValidationResult::NRSSR_NameString_Overflow; + Position += 2 * (StrLength + 1); // Check host online id and session data size - if (size - pos != sizeof(SESSION_DATA_SIZE) + HOST_ONLINE_ID_SIZE + SESSION_DATA_SIZE) + if (Size - Position != sizeof(SESSION_DATA_SIZE) + HOST_ONLINE_ID_SIZE + SESSION_DATA_SIZE) return ValidationResult::NRSSR_RemainingDataSize_Mismatch; - // One single big endian number when everything else is little endian, this is weird - uint16_t data_size = (uint16_t)nrssr_data[pos + HOST_ONLINE_ID_SIZE] << 8 | (uint16_t)nrssr_data[pos + HOST_ONLINE_ID_SIZE + 1]; - return (data_size == SESSION_DATA_SIZE) ? ValidationResult::Valid : ValidationResult::NRSSR_SessionSize_Abnormal; + // Read the big-endian sessin data size field and make sure it matches the normal value for the game + uint16_t SessionDataSize = _byteswap_ushort(*reinterpret_cast(Position + HOST_ONLINE_ID_SIZE)); + return (SessionDataSize == SESSION_DATA_SIZE) ? ValidationResult::Valid : ValidationResult::NRSSR_SessionSize_Abnormal; } }; \ No newline at end of file From a31feb005a2bd33ef0365840c68013f5b49d800f Mon Sep 17 00:00:00 2001 From: William Tremblay Date: Sun, 20 Mar 2022 00:17:39 -0400 Subject: [PATCH 3/3] Make NRSSRSanitizer code style consistent with the rest of the project --- .../Server/GameService/GameManagers/Signs/SignManager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Server/Server/GameService/GameManagers/Signs/SignManager.cpp b/Source/Server/Server/GameService/GameManagers/Signs/SignManager.cpp index 0dde7835..5ccf806b 100644 --- a/Source/Server/Server/GameService/GameManagers/Signs/SignManager.cpp +++ b/Source/Server/Server/GameService/GameManagers/Signs/SignManager.cpp @@ -340,7 +340,7 @@ MessageHandleResult SignManager::Handle_RequestSummonSign(GameClient* Client, co bool bSuccess = true; // Make sure the NRSSR data contained within this message is valid (if the CVE-2022-24126 fix is enabled) - if (BuildConfig::NRSSR_SANITY_CHECKS) + if constexpr (BuildConfig::NRSSR_SANITY_CHECKS) { auto ValidationResult = NRSSRSanitizer::ValidateEntryList(Request->player_struct().data(), Request->player_struct().size()); if (ValidationResult != NRSSRSanitizer::ValidationResult::Valid)