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>
This commit is contained in:
SmallJoker 2023-04-22 17:42:36 +02:00 committed by GitHub
parent 4158b72971
commit 0fb6dbab36
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 54 additions and 0 deletions

@ -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);

@ -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

@ -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;