mirror of
https://github.com/mangosfour/server.git
synced 2025-12-15 19:37:02 +00:00
[8378] Use exceptions instead of explicit size checking for each packet
CHECK_PACKET_SIZE was pretty error prone; once it was forgotten mangosd could crash due to the asserts in ByteBuffer.h. That was exploitable by malicious players. Furthermore, there were duplicate checks: Additionally to CHECK_PACKET_SIZE, the ByteBuffer assertions keept an eye on not exceeding the packet boundaries - just to crash the server for sure in such a case. To prevent memory leaks or other undesirable states, please read in every handler all variables _before_ doing any concrete handling.
This commit is contained in:
parent
c26c7395a1
commit
a24f39a36f
32 changed files with 129 additions and 741 deletions
|
|
@ -173,43 +173,56 @@ bool WorldSession::Update(uint32 /*diff*/)
|
|||
else
|
||||
{
|
||||
OpcodeHandler& opHandle = opcodeTable[packet->GetOpcode()];
|
||||
switch (opHandle.status)
|
||||
try
|
||||
{
|
||||
case STATUS_LOGGEDIN:
|
||||
if(!_player)
|
||||
{
|
||||
// skip STATUS_LOGGEDIN opcode unexpected errors if player logout sometime ago - this can be network lag delayed packets
|
||||
if(!m_playerRecentlyLogout)
|
||||
logUnexpectedOpcode(packet, "the player has not logged in yet");
|
||||
}
|
||||
else if(_player->IsInWorld())
|
||||
(this->*opHandle.handler)(*packet);
|
||||
// lag can cause STATUS_LOGGEDIN opcodes to arrive after the player started a transfer
|
||||
break;
|
||||
case STATUS_TRANSFER:
|
||||
if(!_player)
|
||||
logUnexpectedOpcode(packet, "the player has not logged in yet");
|
||||
else if(_player->IsInWorld())
|
||||
logUnexpectedOpcode(packet, "the player is still in world");
|
||||
else
|
||||
(this->*opHandle.handler)(*packet);
|
||||
break;
|
||||
case STATUS_AUTHED:
|
||||
// prevent cheating with skip queue wait
|
||||
if(m_inQueue)
|
||||
{
|
||||
logUnexpectedOpcode(packet, "the player not pass queue yet");
|
||||
switch (opHandle.status)
|
||||
{
|
||||
case STATUS_LOGGEDIN:
|
||||
if(!_player)
|
||||
{
|
||||
// skip STATUS_LOGGEDIN opcode unexpected errors if player logout sometime ago - this can be network lag delayed packets
|
||||
if(!m_playerRecentlyLogout)
|
||||
logUnexpectedOpcode(packet, "the player has not logged in yet");
|
||||
}
|
||||
else if(_player->IsInWorld())
|
||||
(this->*opHandle.handler)(*packet);
|
||||
// lag can cause STATUS_LOGGEDIN opcodes to arrive after the player started a transfer
|
||||
break;
|
||||
}
|
||||
case STATUS_TRANSFER:
|
||||
if(!_player)
|
||||
logUnexpectedOpcode(packet, "the player has not logged in yet");
|
||||
else if(_player->IsInWorld())
|
||||
logUnexpectedOpcode(packet, "the player is still in world");
|
||||
else
|
||||
(this->*opHandle.handler)(*packet);
|
||||
break;
|
||||
case STATUS_AUTHED:
|
||||
// prevent cheating with skip queue wait
|
||||
if(m_inQueue)
|
||||
{
|
||||
logUnexpectedOpcode(packet, "the player not pass queue yet");
|
||||
break;
|
||||
}
|
||||
|
||||
m_playerRecentlyLogout = false;
|
||||
(this->*opHandle.handler)(*packet);
|
||||
break;
|
||||
case STATUS_NEVER:
|
||||
sLog.outError( "SESSION: received not allowed opcode %s (0x%.4X)",
|
||||
LookupOpcodeName(packet->GetOpcode()),
|
||||
packet->GetOpcode());
|
||||
break;
|
||||
m_playerRecentlyLogout = false;
|
||||
(this->*opHandle.handler)(*packet);
|
||||
break;
|
||||
case STATUS_NEVER:
|
||||
sLog.outError( "SESSION: received not allowed opcode %s (0x%.4X)",
|
||||
LookupOpcodeName(packet->GetOpcode()),
|
||||
packet->GetOpcode());
|
||||
break;
|
||||
}
|
||||
}
|
||||
catch(ByteBufferException &exception)
|
||||
{
|
||||
sLog.outError("WorldSession::Update ByteBufferException occured while parsing a packet (opcode: %u) from client %s, accountid=%i. Skipped packet.",
|
||||
packet->GetOpcode(), GetRemoteAddress().c_str(), GetAccountId());
|
||||
if(sLog.IsOutDebug())
|
||||
{
|
||||
sLog.outDebug("Dumping error causing packet:");
|
||||
packet->hexlike();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -629,7 +642,6 @@ void WorldSession::SaveTutorialsData()
|
|||
|
||||
void WorldSession::ReadMovementInfo(WorldPacket &data, MovementInfo *mi)
|
||||
{
|
||||
CHECK_PACKET_SIZE(data, data.rpos()+4+2+4+4+4+4+4);
|
||||
data >> mi->flags;
|
||||
data >> mi->unk1;
|
||||
data >> mi->time;
|
||||
|
|
@ -643,7 +655,6 @@ void WorldSession::ReadMovementInfo(WorldPacket &data, MovementInfo *mi)
|
|||
if(!data.readPackGUID(mi->t_guid))
|
||||
return;
|
||||
|
||||
CHECK_PACKET_SIZE(data, data.rpos()+4+4+4+4+4+1);
|
||||
data >> mi->t_x;
|
||||
data >> mi->t_y;
|
||||
data >> mi->t_z;
|
||||
|
|
@ -654,16 +665,13 @@ void WorldSession::ReadMovementInfo(WorldPacket &data, MovementInfo *mi)
|
|||
|
||||
if((mi->HasMovementFlag(MovementFlags(MOVEMENTFLAG_SWIMMING | MOVEMENTFLAG_FLYING2))) || (mi->unk1 & 0x20))
|
||||
{
|
||||
CHECK_PACKET_SIZE(data, data.rpos()+4);
|
||||
data >> mi->s_pitch;
|
||||
}
|
||||
|
||||
CHECK_PACKET_SIZE(data, data.rpos()+4);
|
||||
data >> mi->fallTime;
|
||||
|
||||
if(mi->HasMovementFlag(MOVEMENTFLAG_JUMPING))
|
||||
{
|
||||
CHECK_PACKET_SIZE(data, data.rpos()+4+4+4+4);
|
||||
data >> mi->j_unk;
|
||||
data >> mi->j_sinAngle;
|
||||
data >> mi->j_cosAngle;
|
||||
|
|
@ -672,7 +680,6 @@ void WorldSession::ReadMovementInfo(WorldPacket &data, MovementInfo *mi)
|
|||
|
||||
if(mi->HasMovementFlag(MOVEMENTFLAG_SPLINE))
|
||||
{
|
||||
CHECK_PACKET_SIZE(data, data.rpos()+4);
|
||||
data >> mi->u_unk1;
|
||||
}
|
||||
}
|
||||
|
|
@ -687,6 +694,12 @@ void WorldSession::ReadAddonsInfo(WorldPacket &data)
|
|||
if(!size)
|
||||
return;
|
||||
|
||||
if(size > 0xFFFFF)
|
||||
{
|
||||
sLog.outError("WorldSession::ReadAddonsInfo addon info too big, size %u", size);
|
||||
return;
|
||||
}
|
||||
|
||||
uLongf uSize = size;
|
||||
|
||||
uint32 pos = data.rpos();
|
||||
|
|
@ -711,10 +724,6 @@ void WorldSession::ReadAddonsInfo(WorldPacket &data)
|
|||
|
||||
addonInfo >> addonName;
|
||||
|
||||
// recheck next addon data format correctness
|
||||
if(addonInfo.rpos()+1+4+4 > addonInfo.size())
|
||||
return;
|
||||
|
||||
addonInfo >> enabled >> crc >> unk1;
|
||||
|
||||
sLog.outDebug("ADDON: Name: %s, Enabled: 0x%x, CRC: 0x%x, Unknown2: 0x%x", addonName.c_str(), enabled, crc, unk1);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue