Jump to content

[8754][CrashFix][8618] Fix crash caused by spoofed packets


Guest elecyb
 Share

Recommended Posts

* What bug does the patch fix? What features does the patch add?

- Added Tab number checks in Guild Bank operations. This prevent crash caused when player send a packet with Tab number higher than the max Tab.

- Typo in Guild::SendGuildBankTabText()

* For which repository revision was the patch created?

8618

* Is there a thread in the bug report section or at lighthouse? If yes, please add a link to the thread.

I don't think so, cause packet editing is needed, but here is a short description:

First you need a packet editor (I would perefer WPE :lol:), now you need to modify the offset related to Tab destination in GBank transaction, IE: if you have bought all Tabs the last Tab should be 05 (Tabs starts in 00) if you change the offset 016 with a number 06 or higher when you try to pull any item to GBank will cause 100% server crash.

* Who has been writing this patch? Please include either forum user names or email addresses.

me

@@ -1924,10 +1924,15 @@ uint8 Guild::CanStoreItem( uint8 tab, uint8 slot, GuildItemPosCountVec &dest, ui
        return EQUIP_ERR_COULDNT_SPLIT_ITEMS;

    if (pItem->IsSoulBound())
        return EQUIP_ERR_CANT_DROP_SOULBOUND;

+    // in specific tab
+    if (tab >=  m_TabListMap.size() || tab >= GUILD_BANK_MAX_TABS) {
+        return EQUIP_ERR_ITEM_DOESNT_GO_INTO_BAG;
+    }
+
    // in specific slot
    if (slot != NULL_SLOT)
    {
        uint8 res = _CanStoreItem_InSpecificSlot(tab,slot,dest,count,swap,pItem);
        if (res != EQUIP_ERR_OK)
@@ -1984,11 +1989,11 @@ void Guild::SetGuildBankTabText(uint8 TabId, std::string text)
    SendGuildBankTabText(NULL,TabId);
}

void Guild::SendGuildBankTabText(WorldSession *session, uint8 TabId)
{
-    if (TabId > GUILD_BANK_MAX_TABS)
+    if (TabId >= GUILD_BANK_MAX_TABS)  // tabs starts in 0
        return;

    GuildBankTab const *tab = GetBankTab(TabId);
    if (!tab)
        return;

Link to comment
Share on other sites

Some related checkek already exist in WorldSession::HandleGuildBankSwapItems

So i think it must be update instead adding checks in more low level function as Guild::CanStoreItem

Thank you Vlad for your comment and suggestion :), as you said we should add something like this:

@@ -1014,11 +1014,12 @@ void WorldSession::HandleGuildBankSwapItems( WorldPacket & recv_data )
        recv_data >> BankTabSlot;
        recv_data >> ItemEntry;
        recv_data >> unk2;                                  // always 0
        recv_data >> SplitedAmount;

-        if (BankTabSlotDst >= GUILD_BANK_MAX_SLOTS || (BankTabDst == BankTab && BankTabSlotDst == BankTabSlot))
+
+        if (BankTabSlotDst >= GUILD_BANK_MAX_SLOTS || (BankTabDst == BankTab && BankTabSlotDst == BankTabSlot) || BankTab >= GUILD_BANK_MAX_TABS)
        {
            recv_data.rpos(recv_data.wpos());               // prevent additional spam at rejected packet
            return;
        }
    }
@@ -1040,11 +1041,11 @@ void WorldSession::HandleGuildBankSwapItems( WorldPacket & recv_data )
            recv_data >> PlayerSlot;
            recv_data >> ToChar;
            recv_data >> SplitedAmount;
        }

-        if (BankTabSlot >= GUILD_BANK_MAX_SLOTS && BankTabSlot != 0xFF)
+        if ((BankTabSlot >= GUILD_BANK_MAX_SLOTS && BankTabSlot != 0xFF) || BankTab >= GUILD_BANK_MAX_TABS)
        {
            recv_data.rpos(recv_data.wpos());               // prevent additional spam at rejected packet
            return;
        }
    }

BTW we need to check the number of purchased tabs, because if we don't, in the case of a player have less than 6 purchased tabs he can send a packet with BankTab = 05 causing the crash, so I think that we need to call the function m_TabListMap.size() and this can't be done in WorldSession::HandleGuildBankSwapItems()

I will try to find better implementation for this, and remember I' am kinda new at this so any suggestion is welcomed ;)

Link to comment
Share on other sites

  • 3 weeks later...

should work as intended (not tested yet) :

       if(Guild *pGuild = objmgr.GetGuildById(GuildId))
       {
           uint8 purchasedTabs = pGuild->GetPurchasedTabs();
           // Bank <-> Bank
           if (BankToBank)
           {
               if(purchasedTabs < BankTabDst  || purchasedTabs < BankTab)
               {
                   _player->SendEquipError( EQUIP_ERR_NONE, NULL, NULL );
                   return;
               }
               pGuild->SwapItems(_player, BankTab, BankTabSlot, BankTabDst, BankTabSlotDst, SplitedAmount);
               return;
           }

           // Player <-> Bank

           // allow work with inventory only
           if((!Player::IsInventoryPos(PlayerBag, PlayerSlot) && !(PlayerBag == NULL_BAG && PlayerSlot == NULL_SLOT) ) || purchasedTabs < BankTab)
           {
               _player->SendEquipError( EQUIP_ERR_NONE, NULL, NULL );
               return;
           }

           // BankToChar swap or char to bank remaining
           if (ToChar)                                             // Bank -> Char cases
               pGuild->MoveFromBankToChar(_player, BankTab, BankTabSlot, PlayerBag, PlayerSlot, SplitedAmount);
           else                                                    // Char -> Bank cases
               pGuild->MoveFromCharToBank(_player, PlayerBag, PlayerSlot, BankTab, BankTabSlot, SplitedAmount);
       }

we have to proof case for player bank bags too

ok i was looking for any exploits possible in itemtransfer elsewhere but seems to be already checked there :)

[EDIT:] ok tested! exploit not working any more :P i was not sure about purchasedTabs should it be purchasedTabs - 1 in check or just purchasedTabs?

@ elecyb : pls PM me if you got any other stuff like this to fix it soon as possible ;)

Link to comment
Share on other sites

 Share

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue. Privacy Policy Terms of Use