From bf34bbfe80b9aac24abfa2f4340b9846e171a36b Mon Sep 17 00:00:00 2001 From: staphen Date: Sun, 22 Mar 2026 23:09:49 -0400 Subject: [PATCH 1/2] Do not process packet headers or buffered messages from invalid players --- Source/CMakeLists.txt | 2 + Source/msg.cpp | 6 ++ Source/multi.cpp | 151 ++++++++++++++++++---------------- Source/players/validation.cpp | 34 ++++++++ Source/players/validation.hpp | 16 ++++ 5 files changed, 138 insertions(+), 71 deletions(-) create mode 100644 Source/players/validation.cpp create mode 100644 Source/players/validation.hpp diff --git a/Source/CMakeLists.txt b/Source/CMakeLists.txt index f09f6508482..b1770d39c78 100644 --- a/Source/CMakeLists.txt +++ b/Source/CMakeLists.txt @@ -142,6 +142,8 @@ set(libdevilutionx_SRCS platform/locale.cpp + players/validation.cpp + portals/validation.cpp qol/autopickup.cpp diff --git a/Source/msg.cpp b/Source/msg.cpp index 38b5f981ae0..21d0e7207d4 100644 --- a/Source/msg.cpp +++ b/Source/msg.cpp @@ -48,6 +48,7 @@ #include "pack.h" #include "pfile.h" #include "player.h" +#include "players/validation.hpp" #include "plrmsg.h" #include "portals/validation.hpp" #include "quests/validation.hpp" @@ -452,6 +453,11 @@ void PrePacket() return; } + if (!IsNetPlayerValid(playerId)) { + Log("Source of network message is no longer valid"); + return; + } + const size_t size = ParseCmd(playerId, reinterpret_cast(data), remainingBytes); if (size == 0) { Log("Discarding bad network message"); diff --git a/Source/multi.cpp b/Source/multi.cpp index 02cb117986e..4e0131b6fec 100644 --- a/Source/multi.cpp +++ b/Source/multi.cpp @@ -35,6 +35,7 @@ #include "options.h" #include "pfile.h" #include "player.h" +#include "players/validation.hpp" #include "plrmsg.h" #include "qol/chatlog.h" #include "storm/storm_net.hpp" @@ -183,16 +184,6 @@ void NetReceivePlayerData(TPkt *pkt) pkt->hdr.pdir = static_cast(myPlayer._pdir); } -bool IsNetPlayerValid(const Player &player) -{ - // we no longer check character level here, players with out-of-range clevels are not allowed to join the game and we don't observe change clevel messages that would set it out of range - // (there's no code path that would result in _pLevel containing an out of range value in the DevilutionX code) - return static_cast(player._pClass) < GetNumPlayerClasses() - && player.plrlevel < NUMLEVELS - && InDungeonBounds(player.position.tile) - && !std::string_view(player._pName).empty(); -} - void CheckPlayerInfoTimeouts() { for (uint8_t i = 0; i < Players.size(); i++) { @@ -366,6 +357,60 @@ void BeginTimeout() CheckDropPlayer(); } +void SyncPacketHeaderData(Player &player, const TPktHdr &pkt) +{ + const Point syncPosition = { pkt.px, pkt.py }; + player.position.last = syncPosition; + if (&player != MyPlayer) { + assert(gbBufferMsgs != 2); + player._pHitPoints = Swap32LE(pkt.php); + player._pMaxHP = Swap32LE(pkt.pmhp); + player._pMana = Swap32LE(pkt.mana); + player._pMaxMana = Swap32LE(pkt.maxmana); + const bool cond = gbBufferMsgs == 1; + player._pBaseStr = pkt.bstr; + player._pBaseMag = pkt.bmag; + player._pBaseDex = pkt.bdex; + + if (!cond && player.plractive && !player.hasNoLife()) { + if (player.isOnActiveLevel() && !player._pLvlChanging) { + const uint8_t rawDir = pkt.pdir; + if (rawDir <= static_cast(Direction::SouthEast)) { + const auto newDir = static_cast(rawDir); + if (player._pdir != newDir && player._pmode == PM_STAND) { + player._pdir = newDir; + StartStand(player, newDir); + } + } + if (player.position.tile.WalkingDistance(syncPosition) > 3 && PosOkPlayer(player, syncPosition)) { + // got out of sync, clear the tiles around where we last thought the player was located + FixPlrWalkTags(player); + + player.position.old = player.position.tile; + // then just in case clear the tiles around the current position (probably unnecessary) + FixPlrWalkTags(player); + player.position.tile = syncPosition; + player.position.future = syncPosition; + if (player.isWalking()) + player.position.temp = syncPosition; + SetPlayerOld(player); + player.occupyTile(player.position.tile, false); + } + if (player.position.future.WalkingDistance(player.position.tile) > 1) { + player.position.future = player.position.tile; + } + const Point target = { pkt.targx, pkt.targy }; + if (target != Point {}) // does the client send a desired (future) position of remote player? + MakePlrPath(player, target, true); + } else { + player.position.tile = syncPosition; + player.position.future = syncPosition; + SetPlayerOld(player); + } + } + } +} + void HandleAllPackets(uint8_t pnum, const std::byte *data, size_t size) { for (size_t offset = 0; offset < size;) { @@ -692,78 +737,42 @@ void ProcessGameMessagePackets() uint8_t playerId = std::numeric_limits::max(); TPktHdr *pkt; - size_t dwMsgSize = 0; - while (SNetReceiveMessage(&playerId, (void **)&pkt, &dwMsgSize)) { + size_t totalPacketSize = 0; + while (SNetReceiveMessage(&playerId, (void **)&pkt, &totalPacketSize)) { dwRecCount++; ClearPlayerLeftState(); - if (dwMsgSize < sizeof(TPktHdr)) + if (totalPacketSize < sizeof(TPktHdr)) continue; if (playerId >= Players.size()) continue; if (pkt->wCheck != HeaderCheckVal) continue; - if (Swap16LE(pkt->wLen) != dwMsgSize) + if (Swap16LE(pkt->wLen) != totalPacketSize) continue; + + // Distrust all messages until player info is received Player &player = Players[playerId]; - if (!IsNetPlayerValid(player)) { - const _cmd_id cmd = *(const _cmd_id *)(pkt + 1); - if (gbBufferMsgs == 0 && IsNoneOf(cmd, CMD_SEND_PLRINFO, CMD_ACK_PLRINFO)) { - // Distrust all messages until - // player info is received - continue; - } + const bool isTrustedPacket = IsNetPlayerValid(player); + if (isTrustedPacket) { + SyncPacketHeaderData(player, *pkt); } - const Point syncPosition = { pkt->px, pkt->py }; - player.position.last = syncPosition; - if (&player != MyPlayer) { - assert(gbBufferMsgs != 2); - player._pHitPoints = Swap32LE(pkt->php); - player._pMaxHP = Swap32LE(pkt->pmhp); - player._pMana = Swap32LE(pkt->mana); - player._pMaxMana = Swap32LE(pkt->maxmana); - const bool cond = gbBufferMsgs == 1; - player._pBaseStr = pkt->bstr; - player._pBaseMag = pkt->bmag; - player._pBaseDex = pkt->bdex; - - if (!cond && player.plractive && !player.hasNoLife()) { - if (player.isOnActiveLevel() && !player._pLvlChanging) { - const uint8_t rawDir = pkt->pdir; - if (rawDir <= static_cast(Direction::SouthEast)) { - const auto newDir = static_cast(rawDir); - if (player._pdir != newDir && player._pmode == PM_STAND) { - player._pdir = newDir; - StartStand(player, newDir); - } - } - if (player.position.tile.WalkingDistance(syncPosition) > 3 && PosOkPlayer(player, syncPosition)) { - // got out of sync, clear the tiles around where we last thought the player was located - FixPlrWalkTags(player); - - player.position.old = player.position.tile; - // then just in case clear the tiles around the current position (probably unnecessary) - FixPlrWalkTags(player); - player.position.tile = syncPosition; - player.position.future = syncPosition; - if (player.isWalking()) - player.position.temp = syncPosition; - SetPlayerOld(player); - player.occupyTile(player.position.tile, false); - } - if (player.position.future.WalkingDistance(player.position.tile) > 1) { - player.position.future = player.position.tile; - } - const Point target = { pkt->targx, pkt->targy }; - if (target != Point {}) // does the client send a desired (future) position of remote player? - MakePlrPath(player, target, true); - } else { - player.position.tile = syncPosition; - player.position.future = syncPosition; - SetPlayerOld(player); - } - } + + const bool isBufferingMessages = gbBufferMsgs != 0; + const std::byte *message = (const std::byte *)(pkt + 1); + const size_t messageSize = totalPacketSize - sizeof(TPktHdr); + + // It's okay to buffer untrusted messages + // because the player will be validated again later + if (!isTrustedPacket && !isBufferingMessages) { + if (messageSize < 1) + continue; + + const _cmd_id cmd = static_cast<_cmd_id>(message[0]); + if (IsNoneOf(cmd, CMD_SEND_PLRINFO, CMD_ACK_PLRINFO)) + continue; } - HandleAllPackets(playerId, (const std::byte *)(pkt + 1), dwMsgSize - sizeof(TPktHdr)); + + HandleAllPackets(playerId, message, messageSize); } CheckPlayerInfoTimeouts(); } diff --git a/Source/players/validation.cpp b/Source/players/validation.cpp new file mode 100644 index 00000000000..f79f5db2877 --- /dev/null +++ b/Source/players/validation.cpp @@ -0,0 +1,34 @@ +/** + * @file player/validation.cpp + * + * Implementation of functions for validation of player data. + */ + +#include "players/validation.hpp" + +#include + +#include "diablo.h" +#include "levels/gendung.h" +#include "player.h" +#include "tables/playerdat.hpp" + +namespace devilution { + +bool IsNetPlayerValid(const Player &player) +{ + // we no longer check character level here, players with out-of-range clevels are not allowed to join the game and we don't observe change clevel messages that would set it out of range + // (the only code path that would result in _pLevel containing an out of range value in the DevilutionX code is a zero-initialized player, in which case _pName is empty) + return static_cast(player._pClass) < GetNumPlayerClasses() + && player.plrlevel < NUMLEVELS + && InDungeonBounds(player.position.tile) + && !std::string_view(player._pName).empty(); +} + +bool IsNetPlayerValid(size_t playerId) +{ + const Player &player = Players[playerId]; + return IsNetPlayerValid(player); +} + +} // namespace devilution diff --git a/Source/players/validation.hpp b/Source/players/validation.hpp new file mode 100644 index 00000000000..40cd54928f5 --- /dev/null +++ b/Source/players/validation.hpp @@ -0,0 +1,16 @@ +/** + * @file players/validation.hpp + * + * Interface of functions for validation of player data. + */ +#pragma once + +#include + +namespace devilution { + +struct Player; +bool IsNetPlayerValid(const Player &player); +bool IsNetPlayerValid(size_t playerId); + +} // namespace devilution From eb732afcf5c5681add772fc59c211dc12d9f8680 Mon Sep 17 00:00:00 2001 From: Anders Jenbo Date: Fri, 10 Apr 2026 22:12:13 +0200 Subject: [PATCH 2/2] Apply suggestion from @AJenbo --- Source/players/validation.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/players/validation.cpp b/Source/players/validation.cpp index f79f5db2877..3e744c6ae97 100644 --- a/Source/players/validation.cpp +++ b/Source/players/validation.cpp @@ -1,5 +1,5 @@ /** - * @file player/validation.cpp + * @file players/validation.cpp * * Implementation of functions for validation of player data. */