Skip to content

Commit

Permalink
Merge pull request #128 from tremwil/main
Browse files Browse the repository at this point in the history 
Fix OOB read bug and change code style in NRSSR sanitizer
  • Loading branch information
@TLeonardUK
TLeonardUK authored Mar 21, 2022
2 parents 8ea4f74 + a31feb0 commit b2c0ca8
Show file tree
Hide file tree
Showing 8 changed files with 61 additions and 59 deletions.
4 changes: 2 additions & 2 deletions Source/Server/Server/GameService/GameManagers/BloodMessage/BloodMessageManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).",
Expand Down
6 changes: 3 additions & 3 deletions Source/Server/Server/GameService/GameManagers/Bloodstain/BloodstainManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,17 +95,17 @@ 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).",
static_cast<uint32_t>(ValidationResult));

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).",
Expand Down
4 changes: 2 additions & 2 deletions Source/Server/Server/GameService/GameManagers/Ghosts/GhostManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).",
Expand Down
6 changes: 3 additions & 3 deletions Source/Server/Server/GameService/GameManagers/Misc/MiscManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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).",
Expand Down
4 changes: 2 additions & 2 deletions Source/Server/Server/GameService/GameManagers/QuickMatch/QuickMatchManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).",
Expand Down
8 changes: 4 additions & 4 deletions Source/Server/Server/GameService/GameManagers/Signs/SignManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).",
Expand Down Expand Up @@ -340,9 +340,9 @@ 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::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).",
Expand Down
4 changes: 2 additions & 2 deletions Source/Server/Server/GameService/GameManagers/Visitor/VisitorManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).",
Expand Down
84 changes: 43 additions & 41 deletions Source/Server/Server/GameService/Utils/NRSSRSanitizer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,93 +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<const uint32_t*>(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<const uint8_t*>(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<const uint32_t*>(Data + 0);
uint16_t Version = *reinterpret_cast<const uint16_t*>(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
len = wcsnlen_s((wchar_t*)(nrssr_data + pos), size - pos);
if (len == size - pos || len >= MAX_PROP_WSTR_SIZE) return ValidationResult::NRSSR_PropertyString_Overflow;
pos += 2 * (len + 1);
NumWideCharLeft = (Size - Position) / 2;
StrLength = wcsnlen_s(reinterpret_cast<const wchar_t*>(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
len = wcsnlen_s((wchar_t*)(nrssr_data + pos), size - pos);
if (len == size - pos || 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<const wchar_t*>(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<const uint16_t*>(Position + HOST_ONLINE_ID_SIZE));
return (SessionDataSize == SESSION_DATA_SIZE) ? ValidationResult::Valid : ValidationResult::NRSSR_SessionSize_Abnormal;
}
};

0 comments on commit b2c0ca8

Please sign in to comment.