fix ItemMove dupe exploit fix

[masodikbela 1333]

Hi there devs,

Its been a while since my last topic, and the reason for waking up from my eternal hyper dream is no less significant than a small possible item dupe exploit that is present in the original leaked source, and therefore very likely still present on most of the active servers including the official. I've seen topics from late 2021 where this bug was abused on prestigious international servers, so its for sure known by some people for a long time, moreover even a related partial fix for the exploit is already present on the board:

How does it work?

Spoiler

Well obviously I will not attach any code or video that could help anyone reproduce the bug to prevent abusing it on other servers, but I will explain how it works. This bug cannot be reproduced under normal circumstances, so its no surprise that it was able to survive for this long. To be able to abuse this, you either have to modify uiinvenotry.py, or have an external tool that can either attach to the binary and call the SendItemMovePacket function, or can send packets either through the client's connection or through a "fake" client mimicing its behaviour.

So as you can see depending on the server's structure/protection it can be rather hard to execute. The main issue with the server's code is that its either not checking if the source and the destination cell is the same in CHARACTER::MoveItem (in the default source) or in some codes it might be checked, but a potential loophole might left open. The problem lies within the TItemPos's == operator combined with the way EQUIPMENT and INVENTORY window types work. The == operator checks both window and cell, and only returns true if both are the same. On the other hand we all know that EQUIPMENT window type only exists in theory, the actual cells for it are actually arranged after the cells of the INVENTORY. As you can see in IsEquipPosition, the window type can be either INVENTORY or EQUIPMENT, it doesn't matter, as long as its not something else and the cell is in the right range. From the normally existing window types these two that has this behaviour, and therefore the src and dst positions in the MoveItem function can be constructed in a way that both has the same value for the cell, but one has INVENTORY for window type and the other has EQUIPMENT for that.

Obviously this is a major flaw and after fixing it this type of exploit is no longer possible, but there is an interesting question how this escalates into an actual item dupe when abused. For this question I only have a strong theory, which I didn't actually fact-checked yet:

When you unstack an item, a completely new "item" is generated with new ID. Its nothing new for most of us, its important to keep in mind. When you unstack an item and you put the "new" stack on the old's position, unless you used the entire stack, its not gonna get destroyed/removed and it will be updated, but won't be visible anymore inside the client, because the new stack will "overwrite" it visually. This stack will be "flushed" to the db at some point (whenever its time for the core to flush the items) so this won't disappear just like that, but on the db's side there is no check for items position, so nothing blocks it to save 2 items on the same position, same with the database there is no unique keys or whatnot to throw an error for a query trying to put 2 items on the same position.

However whenever you relog to this same core, the original stack of the item will not appear anymore (or at least until the item "cache" (or rather say the DelayedFlush stuff) of the core got flushed).
On the other hand, if you go to another core, one of the stacks will be put into the first empty position inside the inventory. Now this raises a suspicion. The reason behind rests inside the destructor of the player's character.

On the destructor all the items of the player in every inventory will be flushed instantly to the db (so other cores will have them up-to-date) and will be removed from the item manager. However on the core where we executed the exploit, the old stack is in a state where its not technically inside the player's inventory (its owner is the player of course, but the item's pointer is no longer stored in the player's item vector) and therefore wont get destroyed when the player logs out (see CHARACTER::ClearItem()). The item will be marked for delayed save, and the clock is ticking for it even if we are already on a different channel. This means that if we time our actions right, the state of the item from the original core will overwrite the cache inside the db, so lets say we delete this stack on the second core (by putting together the item with another stack) and log out (so the state of the item on the second core will be flushed to the db) and then wait for the first core to flush this old state of that now "deleted" stack, basically restoring the item to that old state.

The reason this "ghost" item will never appears again in the first core if we log in multiple times rests inside the item manager's CreateItem function. As you can see, at the beginning of the function it checks the global map if the item ID is already in there (spoiler: it is obviously in there) and then spitting out a sys_err message about ITEM_ID_DUP and then returning basically nothing, and not doing anything with this ghost item, so basically after the delayed save called once that item will be invisible forever on that core (only gonna overwrite itself on the db once tho), as long as the core is running. So we get a small memory leak next to our dupe exploit. (If you ask me this is more of a serious matter than the exploit )

How to fix it?

I think I filled my quota of talking in the how does it work section above, so without further ado:

Spoiler

In char_item.cpp at the very beginning of the CHARACTER::MoveItem function add this:

    {
        TItemPos tempSrc = Cell;
        TItemPos tempDst = DestCell;
        if (tempSrc.window_type == EQUIPMENT)
            tempSrc.window_type = INVENTORY;

        if (tempDst.window_type == EQUIPMENT)
            tempDst.window_type = INVENTORY;

        if (tempSrc == tempDst)
            return false;
    }

Thats it, but if you are planning on commenting about the fix, here is the most important part of the whole post that you must read before doing so:

Spoiler

Before you type something like " Erm actually you could have just checked the input arguments (Cell, DestCell) and change them, there is actually no need to introduce this 2 new temporary variables just to delete them instantly and increase the code size and the memory consumption by approximately 16 bytes and slowing the whole function down by circa 500 nanoseconds " let me spare the comment, the reason for it is that I do not know your code, and your code might be relying at some point on the correct window_type, and changing it on the input variables might introduce another bug in that part of the code. The extra code block is there because why not, I just didn't want to leave any footprint of those temp variables and have someone use them later by accident after the block.

 

 

Edited August 1, 2023 by masodikbela

[martysama0134 6999]

How to make it more pretty:

common/length.h inside SItemPos struct (above IsEquipPosition()), paste:

	bool IsSamePosition(const SItemPos & other) const
	{
		return *this==other
			|| ((INVENTORY == window_type || EQUIPMENT == window_type)
				&& (INVENTORY == other.window_type || EQUIPMENT == other.window_type)
				&& cell == other.cell);
	}

 

game/src/char_item.cpp, at the beginning of CHARACTER::MoveItem function:

bool CHARACTER::MoveItem(TItemPos Cell, TItemPos DestCell, uint8_t count)
{
	if (Cell.IsSamePosition(DestCell)) // @fixme196
		return false;

 

I'd probably rename IsSamePosition to HasSamePosition, but ignore that.

[MT2Dev 318]

Thanks for the fix, i think after that we need gonna add extra check for possible dupe problems we will face in the future.

 

input_main.cpp

 

Find;

case HEADER_CG_ITEM_MOVE:

Add Under;

			if (ch->m_pkTimedEvent)   // DevFix 74
			{
				ch->ChatPacket (CHAT_TYPE_INFO, LC_TEXT ("취소 되었습니다."));
				event_cancel (&ch->m_pkTimedEvent);
			}

 

I don't remember the exact time but WebZen add this check for almost all actions few years ago, this is not a "fix" exactly but extra check always needed, best regards. (PS; You can add this check for other actions, if you want tho.)

[filipw1 1838]

On 8/1/2023 at 10:13 PM, martysama0134 said:

How to make it more pretty:

common/length.h inside SItemPos struct (above IsEquipPosition()), paste:

	bool IsSamePosition(const SItemPos & other) const
	{
		return *this==other
			|| ((INVENTORY == window_type || EQUIPMENT == window_type)
				&& (INVENTORY == other.window_type || EQUIPMENT == other.window_type)
				&& cell == other.cell);
	}

 

game/src/char_item.cpp, at the beginning of CHARACTER::MoveItem function:

bool CHARACTER::MoveItem(TItemPos Cell, TItemPos DestCell, uint8_t count)
{
	if (Cell.IsSamePosition(DestCell)) // @fixme196
		return false;

 

I'd probably rename IsSamePosition to HasSamePosition, but ignore that.

https://en.cppreference.com/w/cpp/language/operators

Operator overloading comes in handy with these 

[Blacky96 0]

On 8/13/2023 at 5:27 PM, MT2Dev said:

Thanks for the fix, i think after that we need gonna add extra check for possible dupe problems we will face in the future.

 

input_main.cpp

 

Find;

case HEADER_CG_ITEM_MOVE:

Add Under;

			if (ch->m_pkTimedEvent)   // DevFix 74
			{
				ch->ChatPacket (CHAT_TYPE_INFO, LC_TEXT ("취소 되었습니다."));
				event_cancel (&ch->m_pkTimedEvent);
			}

 

I don't remember the exact time but WebZen add this check for almost all actions few years ago, this is not a "fix" exactly but extra check always needed, best regards. (PS; You can add this check for other actions, if you want tho.)

this not work

[Grzyb 10]

@MT2Dev@ martysama0134@ masodikbela

Very strange solutions, the main problem is

void ITEM_MANAGER::Update()

function in item_manager.cpp

when you fix this function, you fix this problem exploit without MoveItem

[masodikbela 1333]

10 hours ago, Grzyb said:

@MT2Dev@ martysama0134@ masodikbela

Very strange solutions, the main problem is

void ITEM_MANAGER::Update()

function in item_manager.cpp

when you fix this function, you fix this problem exploit without MoveItem

As I explained in details in the "How does it work?" part, there is nothing really to fix in the update function. The main problem is that the newly created item overwrites the original stack, and doesn't remove it properly from the core, creating an actual memory leak and later on loading the id is inside the global id map thus preventing it to be created again. But besides all of this putting an item to an occupied cell is an invalid operation and should be always prohibited.

[AndreiS 2]

1 hour ago, masodikbela said:

As I explained in details in the "How does it work?" part, there is nothing really to fix in the update function. The main problem is that the newly created item overwrites the original stack, and doesn't remove it properly from the core, creating an actual memory leak and later on loading the id is inside the global id map thus preventing it to be created again. But besides all of this putting an item to an occupied cell is an invalid operation and should be always prohibited.

is martysama's rewrite also OK? can I use that alone? Will be enough?

Edited September 18, 2023 by AndreiS

[masodikbela 1333]

2 hours ago, AndreiS said:

is martysama's rewrite also OK? can I use that alone? Will be enough?

Sure it will have the same affect.

[AndreiS 2]

49 minutes ago, masodikbela said:

Sure it will have the same affect.

thank you both

[TrC 1]

 

[metrix2020 4]

Doesen't work with special storage, can somebody help?