Merge pull request #127 from tremwil/main

CVE-2022-24125 and CVE-2022-24126 serverside fixes
TLeonardUK · Mar 19, 2022 · 8bd051c · 8bd051c
2 parents 32c7dc1 + 6182330
commit 8bd051c
Show file tree

Hide file tree

Showing 12 changed files with 368 additions and 9 deletions.
diff --git a/Source/Server/Config/BuildConfig.h b/Source/Server/Config/BuildConfig.h
@@ -50,6 +50,16 @@ class BuildConfig
     inline static const bool DISCONNECT_ON_UNHANDLED_MESSAGE = true;
 #endif
 
+    // CVE-2022-24125 (RequestSendMessageToPlayers abuse) and CVE-2022-24126 (NRSessionSearchResult RCE) serverside fixes.
+    // These should only be disabled on a debug build for testing purposes.
+#if defined(_DEBUG)
+    constexpr inline static const bool SEND_MESSAGE_TO_PLAYERS_SANITY_CHECKS = false;
+    constexpr inline static const bool NRSSR_SANITY_CHECKS = false;
+#else
+    constexpr inline static const bool SEND_MESSAGE_TO_PLAYERS_SANITY_CHECKS = true;
+    constexpr inline static const bool NRSSR_SANITY_CHECKS = true;
+#endif
+
     // Dumps a diasssembly of each message to the output.
     constexpr inline static const bool DISASSEMBLE_RECIEVED_MESSAGES = false;
 

diff --git a/Source/Server/Server.vcxproj b/Source/Server/Server.vcxproj
@@ -179,6 +179,7 @@ copy $(SolutionDir)..\Resources\steam_appid.txt $(OutputPath)steam_appid.txt</Co
     <ClInclude Include="Server\GameService\GameService.h" />
     <ClInclude Include="Server\GameService\PlayerState.h" />
     <ClInclude Include="Server\GameService\Utils\GameIds.h" />
+    <ClInclude Include="Server\GameService\Utils\NRSSRSanitizer.h" />
     <ClInclude Include="Server\GameService\Utils\OnlineAreaPool.h" />
     <ClInclude Include="Server\LoginService\LoginClient.h" />
     <ClInclude Include="Server\LoginService\LoginService.h" />

diff --git a/Source/Server/Server.vcxproj.filters b/Source/Server/Server.vcxproj.filters
@@ -336,6 +336,9 @@
     <ClInclude Include="Core\Utils\DiffTracker.h">
       <Filter>Core\Utils</Filter>
     </ClInclude>
+    <ClInclude Include="Server\GameService\Utils\NRSSRSanitizer.h">
+      <Filter>Server\GameService\Utils</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="Server\Server.cpp">

diff --git a/Source/Server/Server/GameService/GameManagers/BloodMessage/BloodMessageManager.cpp b/Source/Server/Server/GameService/GameManagers/BloodMessage/BloodMessageManager.cpp
@@ -17,6 +17,9 @@
 #include "Server/Server.h"
 #include "Server/Database/ServerDatabase.h"
 
+#include "Config/BuildConfig.h"
+#include "Server/GameService/Utils/NRSSRSanitizer.h"
+
 #include "Core/Utils/Logging.h"
 #include "Core/Utils/File.h"
 #include "Core/Utils/Strings.h"
@@ -248,6 +251,20 @@ MessageHandleResult BloodMessageManager::Handle_RequestCreateBloodMessage(GameCl
     std::vector<uint8_t> MessageData;
     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)
+    {
+        auto ValidationResult = NRSSRSanitizer::IsValidEntryList(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).",
+                static_cast<uint32_t>(ValidationResult));
+
+            // Simply ignore the request. Perhaps sending a response with an invalid sign id or disconnecting the client would be better?
+            return MessageHandleResult::Handled;
+        }
+    }
+
     if (std::shared_ptr<BloodMessage> ActiveMessage = Database.CreateBloodMessage((OnlineAreaId)Request->online_area_id(), Player.GetPlayerId(), Player.GetSteamId(), Request->character_id(), MessageData))
     {
         Response.set_message_id(ActiveMessage->MessageId);

diff --git a/Source/Server/Server/GameService/GameManagers/Bloodstain/BloodstainManager.cpp b/Source/Server/Server/GameService/GameManagers/Bloodstain/BloodstainManager.cpp
@@ -15,6 +15,9 @@
 #include "Config/RuntimeConfig.h"
 #include "Server/Server.h"
 
+#include "Config/BuildConfig.h"
+#include "Server/GameService/Utils/NRSSRSanitizer.h"
+
 #include "Core/Utils/Logging.h"
 #include "Core/Utils/Strings.h"
 
@@ -91,6 +94,27 @@ MessageHandleResult BloodstainManager::Handle_RequestCreateBloodstain(GameClient
     Data.assign(Request->data().data(), Request->data().data() + Request->data().size());
     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)
+    {
+        auto ValidationResult = NRSSRSanitizer::IsValidEntryList(Data.data(), Data.size());
+        if (ValidationResult != NRSSRSanitizer::ValidationResult::Valid)
+        {
+            WarningS(Client->GetName().c_str(), "Bloodstain metadata recieved from client is invalid (error code %i).",
+                static_cast<uint32_t>(ValidationResult));
+
+            return MessageHandleResult::Handled;
+        }
+        ValidationResult = NRSSRSanitizer::IsValidEntryList(GhostData.data(), GhostData.size());
+        if (ValidationResult != NRSSRSanitizer::ValidationResult::Valid)
+        {
+            WarningS(Client->GetName().c_str(), "Ghost data recieved from client is invalid (error code %i).",
+                static_cast<uint32_t>(ValidationResult));
+
+            return MessageHandleResult::Handled;
+        }
+    }
+
     if (std::shared_ptr<Bloodstain> ActiveStain = Database.CreateBloodstain((OnlineAreaId)Request->online_area_id(), Player.GetPlayerId(), Player.GetSteamId(), Data, GhostData))
     {
         LiveCache.Add(ActiveStain->OnlineAreaId, ActiveStain->BloodstainId, ActiveStain);

diff --git a/Source/Server/Server/GameService/GameManagers/Ghosts/GhostManager.cpp b/Source/Server/Server/GameService/GameManagers/Ghosts/GhostManager.cpp
@@ -15,6 +15,9 @@
 #include "Config/RuntimeConfig.h"
 #include "Server/Server.h"
 
+#include "Config/BuildConfig.h"
+#include "Server/GameService/Utils/NRSSRSanitizer.h"
+
 #include "Core/Utils/Logging.h"
 #include "Core/Utils/Strings.h"
 
@@ -85,6 +88,19 @@ MessageHandleResult GhostManager::Handle_RequestCreateGhostData(GameClient* Clie
     std::vector<uint8_t> Data;
     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)
+    {
+        auto ValidationResult = NRSSRSanitizer::IsValidEntryList(Data.data(), Data.size());
+        if (ValidationResult != NRSSRSanitizer::ValidationResult::Valid)
+        {
+            WarningS(Client->GetName().c_str(), "Ghost data recieved from client is invalid (error code %i).",
+                static_cast<uint32_t>(ValidationResult));
+
+            return MessageHandleResult::Handled;
+        }
+    }
+
     if (std::shared_ptr<Ghost> ActiveGhost = Database.CreateGhost((OnlineAreaId)Request->online_area_id(), Player.GetPlayerId(), Player.GetSteamId(), Data))
     {
         LiveCache.Add(ActiveGhost->OnlineAreaId, ActiveGhost->GhostId, ActiveGhost);

diff --git a/Source/Server/Server/GameService/GameManagers/Misc/MiscManager.cpp b/Source/Server/Server/GameService/GameManagers/Misc/MiscManager.cpp
@@ -16,6 +16,9 @@
 #include "Config/RuntimeConfig.h"
 #include "Server/Server.h"
 
+#include "Config/BuildConfig.h"
+#include "Server/GameService/Utils/NRSSRSanitizer.h"
+
 #include "Core/Utils/Logging.h"
 #include "Core/Utils/Strings.h"
 
@@ -121,20 +124,58 @@ MessageHandleResult MiscManager::Handle_RequestSendMessageToPlayers(GameClient*
     std::vector<uint8_t> MessageData;
     MessageData.assign(Request->message().data(), Request->message().data() + Request->message().size());
 
-    for (int i = 0; i < Request->player_ids_size(); i++)
+    // RequestSendMessageToPlayers sanity checks (patch CVE-2022-24125)
+    // 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)
     {
-        uint32_t PlayerId = Request->player_ids(i);
+        // The request should contain a PushRequestBreakInTarget message
+        Frpg2RequestMessage::PushRequestAllowBreakInTarget EmbdeddedMsg;
 
-        std::shared_ptr<GameClient> TargetClient = GameServiceInstance->FindClientByPlayerId(PlayerId);
-        if (!TargetClient)
+        // The request should specify exactly one player
+        if (Request->player_ids_size() > 1)
+        {
+            WarningS(Client->GetName().c_str(), "Client attempted to send message to more than one (%i) client. This is not normal behaviour.", Request->player_ids_size());
+            ShouldProcessRequest = false;
+        }
+        // The request should only ever contain a PushRequestAllowBreakInTarget message
+        if (!EmbdeddedMsg.ParseFromString(Request->message()) || EmbdeddedMsg.push_message_id() != Frpg2RequestMessage::PushMessageId::PushID_PushRequestAllowBreakInTarget)
         {
-            WarningS(Client->GetName().c_str(), "Client attempted to send message to other client %i, but client doesn't exist.", PlayerId);
+            WarningS(Client->GetName().c_str(), "RequestSendMessageToPlayers embedded message provided by client is not a valid PushRequestAllowBreakInTarget message.");
+            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)
+        {
+            auto ValidationResult = NRSSRSanitizer::IsValidEntryList(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).",
+                    static_cast<uint32_t>(ValidationResult));
+
+                ShouldProcessRequest = false;
+            }
         }
-        else
+    }
+
+    if (ShouldProcessRequest)
+    {
+        for (int i = 0; i < Request->player_ids_size(); i++)
         {
-            if (!TargetClient->MessageStream->SendRawProtobuf(MessageData))
+            uint32_t PlayerId = Request->player_ids(i);
+
+            std::shared_ptr<GameClient> TargetClient = GameServiceInstance->FindClientByPlayerId(PlayerId);
+            if (!TargetClient)
+            {
+                WarningS(Client->GetName().c_str(), "Client attempted to send message to other client %i, but client doesn't exist.", PlayerId);
+            }
+            else
             {
-                WarningS(Client->GetName().c_str(), "Failed to send raw protobuf from RequestSendMessageToPlayers to %s.", TargetClient->GetName().c_str());
+                if (!TargetClient->MessageStream->SendRawProtobuf(MessageData))
+                {
+                    WarningS(Client->GetName().c_str(), "Failed to send raw protobuf from RequestSendMessageToPlayers to %s.", TargetClient->GetName().c_str());
+                }
             }
         }
     }

diff --git a/Source/Server/Server/GameService/GameManagers/QuickMatch/QuickMatchManager.cpp b/Source/Server/Server/GameService/GameManagers/QuickMatch/QuickMatchManager.cpp
@@ -16,6 +16,9 @@
 #include "Config/RuntimeConfig.h"
 #include "Server/Server.h"
 
+#include "Config/BuildConfig.h"
+#include "Server/GameService/Utils/NRSSRSanitizer.h"
+
 #include "Core/Utils/Logging.h"
 #include "Core/Utils/File.h"
 #include "Core/Utils/Strings.h"
@@ -357,7 +360,22 @@ MessageHandleResult QuickMatchManager::Handle_RequestAcceptQuickMatch(GameClient
 
     Frpg2RequestMessage::RequestAcceptQuickMatch* Request = (Frpg2RequestMessage::RequestAcceptQuickMatch*)Message.Protobuf.get();
 
-    if (std::shared_ptr<GameClient> TargetClient = GameServiceInstance->FindClientByPlayerId(Request->join_player_id()))
+    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)
+    {
+        auto ValidationResult = NRSSRSanitizer::IsValidEntryList(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).",
+                static_cast<uint32_t>(ValidationResult));
+
+            ShouldProcessRequest = false;
+        }
+    }
+
+    std::shared_ptr<GameClient> TargetClient = GameServiceInstance->FindClientByPlayerId(Request->join_player_id());
+    if (ShouldProcessRequest && TargetClient != nullptr)
     {    
         LogS(Client->GetName().c_str(), "RequestAcceptQuickMatch: Accepting join request from %s", TargetClient->GetName().c_str());
 

diff --git a/Source/Server/Server/GameService/GameManagers/Signs/SignManager.cpp b/Source/Server/Server/GameService/GameManagers/Signs/SignManager.cpp
@@ -16,6 +16,9 @@
 #include "Config/RuntimeConfig.h"
 #include "Server/Server.h"
 
+#include "Config/BuildConfig.h"
+#include "Server/GameService/Utils/NRSSRSanitizer.h"
+
 #include "Core/Utils/Logging.h"
 #include "Core/Utils/File.h"
 #include "Core/Utils/Strings.h"
@@ -230,6 +233,20 @@ 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)
+    {
+        auto ValidationResult = NRSSRSanitizer::IsValidEntryList(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).",
+                static_cast<uint32_t>(ValidationResult));
+
+            // Simply ignore the request. Perhaps sending a response with an invalid sign id or disconnecting the client would be better?
+            return MessageHandleResult::Handled;
+        }
+    }
+
     std::shared_ptr<SummonSign> Sign = std::make_shared<SummonSign>();
     Sign->SignId = NextSignId++;
     Sign->OnlineAreaId = (OnlineAreaId)Request->online_area_id();
@@ -322,6 +339,19 @@ 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)
+    {
+        auto ValidationResult = NRSSRSanitizer::IsValidEntryList(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).",
+                static_cast<uint32_t>(ValidationResult));
+
+            bSuccess = false;
+        }
+    }
+
     // First check the sign still exists, if it doesn't, send a reject message as its probably already used.
     std::shared_ptr<SummonSign> Sign = LiveCache.Find((OnlineAreaId)Request->online_area_id(), Request->sign_info().sign_id());
     if (!Sign)

diff --git a/Source/Server/Server/GameService/GameManagers/Visitor/VisitorManager.cpp b/Source/Server/Server/GameService/GameManagers/Visitor/VisitorManager.cpp
@@ -16,6 +16,9 @@
 #include "Config/RuntimeConfig.h"
 #include "Server/Server.h"
 
+#include "Config/BuildConfig.h"
+#include "Server/GameService/Utils/NRSSRSanitizer.h"
+
 #include "Core/Utils/Logging.h"
 #include "Core/Utils/Strings.h"
 
@@ -124,6 +127,19 @@ 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)
+    {
+        auto ValidationResult = NRSSRSanitizer::IsValidEntryList(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).",
+                static_cast<uint32_t>(ValidationResult));
+
+            bSuccess = false;
+        }
+    }
+
     // Check client still exists.
     std::shared_ptr<GameClient> TargetClient = GameServiceInstance->FindClientByPlayerId(Request->player_id());
     if (!TargetClient)