From 0fb6dbab360813536b5b62a7ee4aa03e7757eeb4 Mon Sep 17 00:00:00 2001
From: SmallJoker <SmallJoker@users.noreply.github.com>
Date: Sat, 22 Apr 2023 17:42:36 +0200
Subject: [PATCH] InventoryManager: Disallow resizing or deleting inventory
 lists that are in use (#13360)

Naive solution to prevent InventoryList UAF and OOB ItemStack access caused by shrink/clear operations on InventoryLists within callbacks of an inventory action.

Co-authored-by: Desour <ds.desour@proton.me>
---
 src/inventory.cpp        | 24 ++++++++++++++++++++++++
 src/inventory.h          | 19 +++++++++++++++++++
 src/inventorymanager.cpp | 11 +++++++++++
 3 files changed, 54 insertions(+)

diff --git a/src/inventory.cpp b/src/inventory.cpp
index 0f12ad579..43f45a2d4 100644
--- a/src/inventory.cpp
+++ b/src/inventory.cpp
@@ -452,6 +452,9 @@ void InventoryList::setSize(u32 newsize)
 	if (newsize == m_items.size())
 		return;
 
+	if (newsize < m_items.size())
+		checkResizeLock();
+
 	m_items.resize(newsize);
 	m_size = newsize;
 	setModified();
@@ -549,6 +552,8 @@ void InventoryList::deSerialize(std::istream &is)
 
 InventoryList & InventoryList::operator = (const InventoryList &other)
 {
+	checkResizeLock();
+
 	m_items = other.m_items;
 	m_size = other.m_size;
 	m_width = other.m_width;
@@ -796,6 +801,15 @@ u32 InventoryList::moveItem(u32 i, InventoryList *dest, u32 dest_i,
 	return (oldcount - item1.count);
 }
 
+void InventoryList::checkResizeLock()
+{
+	if (m_resize_locks == 0)
+		return; // OK
+
+	throw BaseException("InventoryList '" + m_name
+			+ "' is currently in use and cannot be deleted or resized.");
+}
+
 /*
 	Inventory
 */
@@ -807,6 +821,12 @@ Inventory::~Inventory()
 
 void Inventory::clear()
 {
+	for (auto &m_list : m_lists) {
+		// Placing this check within the destructor would be a logical solution
+		// but that's generally a bad idea, thus manual calls beforehand:
+		m_list->checkResizeLock();
+	}
+
 	for (auto &m_list : m_lists) {
 		delete m_list;
 	}
@@ -948,7 +968,9 @@ InventoryList * Inventory::addList(const std::string &name, u32 size)
 	// Remove existing lists
 	s32 i = getListIndex(name);
 	if (i != -1) {
+		m_lists[i]->checkResizeLock();
 		delete m_lists[i];
+
 		m_lists[i] = new InventoryList(name, size, m_itemdef);
 		m_lists[i]->setModified();
 		return m_lists[i];
@@ -978,6 +1000,8 @@ bool Inventory::deleteList(const std::string &name)
 	if(i == -1)
 		return false;
 
+	m_lists[i]->checkResizeLock();
+
 	setModified();
 	delete m_lists[i];
 	m_lists.erase(m_lists.begin() + i);
diff --git a/src/inventory.h b/src/inventory.h
index 6d2a54e43..9ec15d3aa 100644
--- a/src/inventory.h
+++ b/src/inventory.h
@@ -284,6 +284,24 @@ public:
 	inline bool checkModified() const { return m_dirty; }
 	inline void setModified(bool dirty = true) { m_dirty = dirty; }
 
+	// Problem: C++ keeps references to InventoryList and ItemStack indices
+	// until a better solution is found, this serves as a guard to prevent side-effects
+	struct ResizeUnlocker {
+		void operator()(InventoryList *invlist)
+		{
+			invlist->m_resize_locks -= 1;
+		}
+	};
+	using ResizeLocked = std::unique_ptr<InventoryList, ResizeUnlocker>;
+
+	void checkResizeLock();
+
+	inline ResizeLocked resizeLock()
+	{
+		m_resize_locks += 1;
+		return ResizeLocked(this);
+	}
+
 private:
 	std::vector<ItemStack> m_items;
 	std::string m_name;
@@ -291,6 +309,7 @@ private:
 	u32 m_width = 0;
 	IItemDefManager *m_itemdef;
 	bool m_dirty = true;
+	int m_resize_locks = 0; // Lua callback sanity
 };
 
 class Inventory
diff --git a/src/inventorymanager.cpp b/src/inventorymanager.cpp
index 913c959ce..782a17112 100644
--- a/src/inventorymanager.cpp
+++ b/src/inventorymanager.cpp
@@ -278,6 +278,9 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 		return;
 	}
 
+	auto list_from_lock = list_from->resizeLock();
+	auto list_to_lock = list_to->resizeLock();
+
 	if (move_somewhere) {
 		s16 old_to_i = to_i;
 		u16 old_count = count;
@@ -570,6 +573,7 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 	/*
 		Report move to endpoints
 	*/
+	list_to_lock.reset();
 
 	// Source = destination => move
 	if (from_inv == to_inv) {
@@ -683,6 +687,8 @@ void IDropAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 		return;
 	}
 
+	auto list_from_lock = list_from->resizeLock();
+
 	/*
 		Do not handle rollback if inventory is player's
 	*/
@@ -763,6 +769,7 @@ void IDropAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 	/*
 		Report drop to endpoints
 	*/
+	list_from_lock.reset();
 
 	switch (from_inv.type) {
 	case InventoryLocation::DETACHED:
@@ -879,6 +886,10 @@ void ICraftAction::apply(InventoryManager *mgr,
 		return;
 	}
 
+	auto list_craft_lock       = list_craft->resizeLock();
+	auto list_craftresult_lock = list_craftresult->resizeLock();
+	auto list_main_lock        = list_main->resizeLock();
+
 	ItemStack crafted;
 	ItemStack craftresultitem;
 	int count_remaining = count;