fix: security fixes (#1974)

* fix: security fixes

dont print passwords for worlds
bound strings from clients
actually enable encryption between rakpeers
dont allow underflow when reading a string

Tested that packets are encrypted
tested that models can still be built
tested that combat still works

* add check

* use c++ nullptr instead of NULL

* initialize to 0

* globalize constant (should be in a namespace at least in the future)

* Update GameMessages.cpp

* check bounds
This commit is contained in:
David Markowitz
2026-05-17 12:21:22 -07:00
committed by GitHub
parent 67bbe4c1f0
commit f5d33a773a
13 changed files with 43 additions and 9 deletions

View File

@@ -58,6 +58,7 @@ constexpr LWOCLONEID LWOCLONEID_INVALID = -1; //!< Invalid LWOCLONEID
constexpr LWOINSTANCEID LWOINSTANCEID_INVALID = -1; //!< Invalid LWOINSTANCEID
constexpr LWOMAPID LWOMAPID_INVALID = -1; //!< Invalid LWOMAPID
constexpr uint64_t LWOZONEID_INVALID = 0; //!< Invalid LWOZONEID
constexpr uint32_t MAX_MESSAGE_LENGTH = 0x500000; //!< Prevent exceptionally large msgs from being processed. Should always be used to check user provided inputs.
constexpr float PI = 3.14159f;

View File

@@ -61,6 +61,7 @@ public:
uint32_t sBitStreamLength{};
stream.Read(sBitStreamLength);
if (sBitStreamLength > MAX_MESSAGE_LENGTH) return false;
for (uint32_t k = 0; k < sBitStreamLength; k++) {
unsigned char character;
stream.Read(character);

View File

@@ -100,6 +100,7 @@ public:
uint32_t sBitStreamLength{};
stream.Read(sBitStreamLength);
if (sBitStreamLength > MAX_MESSAGE_LENGTH) return false;
for (uint32_t k = 0; k < sBitStreamLength; k++) {
unsigned char character;
stream.Read(character);

View File

@@ -47,6 +47,7 @@ public:
uint32_t sBitStreamLength{};
stream.Read(sBitStreamLength);
if (sBitStreamLength > MAX_MESSAGE_LENGTH) return false;
for (unsigned int k = 0; k < sBitStreamLength; k++) {
unsigned char character;
stream.Read(character);

View File

@@ -2405,12 +2405,26 @@ void GameMessages::SendUnSmash(Entity* entity, LWOOBJID builderID, float duratio
void GameMessages::HandleControlBehaviors(RakNet::BitStream& inStream, Entity* entity, const SystemAddress& sysAddr) {
AMFDeserialize reader;
std::unique_ptr<AMFArrayValue> amfArguments{ static_cast<AMFArrayValue*>(reader.Read(inStream).release()) };
std::unique_ptr<AMFArrayValue> amfArguments;
try {
auto deserializedData = reader.Read(inStream);
if (!deserializedData || deserializedData->GetValueType() != eAmf::Array) {
LOG("Failed to deserialize AMF data for control behaviors command: not an array");
return;
}
amfArguments.reset(static_cast<AMFArrayValue*>(deserializedData.release()));
} catch (...) {
LOG("Failed to deserialize AMF data for control behaviors command");
return;
}
if (amfArguments->GetValueType() != eAmf::Array) return;
uint32_t commandLength{};
inStream.Read(commandLength);
if (commandLength > MAX_MESSAGE_LENGTH) return; // Prevent DoS via unbounded command buffer
std::string command;
command.reserve(commandLength);
for (uint32_t i = 0; i < commandLength; ++i) {
@@ -3616,6 +3630,8 @@ void GameMessages::HandlePetTamingTryBuild(RakNet::BitStream& inStream, Entity*
inStream.Read(brickCount);
if (brickCount > MAX_MESSAGE_LENGTH) return; // Prevent DoS via unbounded brick count
bricks.reserve(brickCount);
for (uint32_t i = 0; i < brickCount; i++) {
@@ -5806,6 +5822,8 @@ void GameMessages::HandleReportBug(RakNet::BitStream& inStream, Entity* entity)
uint32_t messageLength;
inStream.Read(messageLength);
if (messageLength > MAX_MESSAGE_LENGTH) return;
for (uint32_t i = 0; i < (messageLength); ++i) {
uint16_t character;
inStream.Read(character);
@@ -5817,6 +5835,7 @@ void GameMessages::HandleReportBug(RakNet::BitStream& inStream, Entity* entity)
uint32_t clientVersionLength;
inStream.Read(clientVersionLength);
if (clientVersionLength > MAX_MESSAGE_LENGTH) return;
for (unsigned int k = 0; k < clientVersionLength; k++) {
unsigned char character;
inStream.Read(character);
@@ -5825,6 +5844,7 @@ void GameMessages::HandleReportBug(RakNet::BitStream& inStream, Entity* entity)
uint32_t nOtherPlayerIDLength;
inStream.Read(nOtherPlayerIDLength);
if (nOtherPlayerIDLength > MAX_MESSAGE_LENGTH) return;
for (unsigned int k = 0; k < nOtherPlayerIDLength; k++) {
unsigned char character;
inStream.Read(character);
@@ -5833,6 +5853,7 @@ void GameMessages::HandleReportBug(RakNet::BitStream& inStream, Entity* entity)
uint32_t selectionLength;
inStream.Read(selectionLength);
if (selectionLength > MAX_MESSAGE_LENGTH) return;
for (unsigned int k = 0; k < selectionLength; k++) {
unsigned char character;
inStream.Read(character);
@@ -6135,14 +6156,17 @@ void GameMessages::HandleUpdateInventoryGroup(RakNet::BitStream& inStream, Entit
uint32_t size{};
if (!inStream.Read(size)) return;
if (size > MAX_MESSAGE_LENGTH) return; // Bounds check before resize
action.resize(size);
if (!inStream.Read(action.data(), size)) return;
if (!inStream.Read(size)) return;
if (size > MAX_MESSAGE_LENGTH) return; // Bounds check before resize
groupUpdate.groupId.resize(size);
if (!inStream.Read(groupUpdate.groupId.data(), size)) return;
if (!inStream.Read(size)) return;
if (size > MAX_MESSAGE_LENGTH / 2) return; // Bounds check: size * 2 would overflow or exceed limit
groupName.resize(size);
if (!inStream.Read(reinterpret_cast<char*>(groupName.data()), size * 2)) return;

View File

@@ -54,6 +54,7 @@ public:
uint32_t sBitStreamLength{};
stream.Read(sBitStreamLength);
if (sBitStreamLength > MAX_MESSAGE_LENGTH) return false;
for (uint32_t k = 0; k < sBitStreamLength; k++) {
unsigned char character;
stream.Read(character);

View File

@@ -111,6 +111,7 @@ public:
uint32_t sBitStreamLength{};
stream.Read(sBitStreamLength);
if (sBitStreamLength > MAX_MESSAGE_LENGTH) return false;
for (uint32_t k = 0; k < sBitStreamLength; k++) {
unsigned char character;
stream.Read(character);

View File

@@ -46,6 +46,7 @@ public:
stream.Read(bDone);
uint32_t sBitStreamLength{};
stream.Read(sBitStreamLength);
if (sBitStreamLength > MAX_MESSAGE_LENGTH) return false;
for (uint32_t k = 0; k < sBitStreamLength; k++) {
unsigned char character;
stream.Read(character);

View File

@@ -308,7 +308,7 @@ const InstancePtr& InstanceManager::FindPrivateInstance(const std::string& passw
continue;
}
LOG("Password: %s == %s => %d", password.c_str(), instance->GetPassword().c_str(), password == instance->GetPassword());
LOG("Checking private zone password match (result: %d)", password == instance->GetPassword());
if (instance->GetPassword() == password) {
return instance;

View File

@@ -720,7 +720,7 @@ void HandlePacket(Packet* packet) {
password += character;
}
const auto& newInst = Game::im->CreatePrivateInstance(mapId, cloneId, password.c_str());
LOG("Creating private zone %i/%i/%i with password %s", newInst->GetMapID(), newInst->GetCloneID(), newInst->GetInstanceID(), password.c_str());
LOG("Creating private zone %i/%i/%i", newInst->GetMapID(), newInst->GetCloneID(), newInst->GetInstanceID());
break;
}
@@ -747,7 +747,7 @@ void HandlePacket(Packet* packet) {
const auto& instance = Game::im->FindPrivateInstance(password.c_str());
LOG("Join private zone: %llu %d %s %p", requestID, mythranShift, password.c_str(), instance.get());
LOG("Join private zone: %llu %d %p", requestID, mythranShift, instance.get());
if (instance == nullptr) {
return;

View File

@@ -307,6 +307,6 @@ void AuthPackets::SendLoginResponse(dServer* server, const SystemAddress& sysAdd
bitStream.Write(LUString(username));
server->SendToMaster(bitStream);
LOG("Set sessionKey: %i for user %s", sessionKey, username.c_str());
LOG("Set session key for user %s", username.c_str());
}
}

View File

@@ -11,14 +11,16 @@ ChatMessage ClientPackets::HandleChatMessage(Packet* packet) {
CINSTREAM_SKIP_HEADER;
ChatMessage message;
uint32_t messageLength;
int32_t messageLength{};
inStream.Read(message.chatChannel);
inStream.Read(message.unknown);
inStream.Read(messageLength);
for (uint32_t i = 0; i < (messageLength - 1); ++i) {
uint16_t character;
if (messageLength > MAX_MESSAGE_LENGTH || messageLength < 0) return message;
for (int32_t i = 0; i < (messageLength - 1); ++i) {
char16_t character;
inStream.Read(character);
message.message.push_back(character);
}

View File

@@ -215,6 +215,8 @@ bool dServer::Startup() {
mPeer = RakNetworkFactory::GetRakPeerInterface();
if (!mPeer) return false;
if (mUseEncryption) mPeer->InitializeSecurity(nullptr, nullptr, nullptr, nullptr);
if (!mPeer->Startup(mMaxConnections, 10, &mSocketDescriptor, 1)) return false;
if (mIsInternal) {
@@ -226,7 +228,6 @@ bool dServer::Startup() {
}
mPeer->SetMaximumIncomingConnections(mMaxConnections);
if (mUseEncryption) mPeer->InitializeSecurity(NULL, NULL, NULL, NULL);
return true;
}