Inventory: Protect Craft and Drop actions (#10353)

Change dangerous pointer to unique_ptr for automated deletion.
minetest · Sep 7, 2020 · 0d128ab · 0d128ab
1 parent 6dcc9e6
commit 0d128ab
Showing 1 changed file with 35 additions and 44 deletions.
diff --git a/src/network/serverpackethandler.cpp b/src/network/serverpackethandler.cpp
@@ -600,7 +600,7 @@ void Server::handleCommand_InventoryAction(NetworkPacket* pkt)
 		<< std::endl;
 	std::istringstream is(datastring, std::ios_base::binary);
 	// Create an action
-	InventoryAction *a = InventoryAction::deSerialize(is);
+	std::unique_ptr<InventoryAction> a(InventoryAction::deSerialize(is));
 	if (!a) {
 		infostream << "TOSERVER_INVENTORY_ACTION: "
 				<< "InventoryAction::deSerialize() returned NULL"
@@ -617,11 +617,30 @@ void Server::handleCommand_InventoryAction(NetworkPacket* pkt)
 		where the client made a bad prediction.
 	*/
 
+	const bool player_has_interact = checkPriv(player->getName(), "interact");
+
+	auto check_inv_access = [player, player_has_interact] (
+			const InventoryLocation &loc) -> bool {
+		if (loc.type == InventoryLocation::CURRENT_PLAYER)
+			return false; // Only used internally on the client, never sent
+		if (loc.type == InventoryLocation::PLAYER) {
+			// Allow access to own inventory in all cases
+			return loc.name == player->getName();
+		}
+
+		if (!player_has_interact) {
+			infostream << "Cannot modify foreign inventory: "
+					<< "No interact privilege" << std::endl;
+			return false;
+		}
+		return true;
+	};
+
 	/*
 		Handle restrictions and special cases of the move action
 	*/
 	if (a->getType() == IAction::Move) {
-		IMoveAction *ma = (IMoveAction*)a;
+		IMoveAction *ma = (IMoveAction*)a.get();
 
 		ma->from_inv.applyCurrentPlayer(player->getName());
 		ma->to_inv.applyCurrentPlayer(player->getName());
@@ -630,21 +649,11 @@ void Server::handleCommand_InventoryAction(NetworkPacket* pkt)
 		if (ma->from_inv != ma->to_inv)
 			m_inventory_mgr->setInventoryModified(ma->to_inv);
 
-		bool from_inv_is_current_player = false;
-		if (ma->from_inv.type == InventoryLocation::PLAYER) {
-			if (ma->from_inv.name != player->getName())
-				return;
-			from_inv_is_current_player = true;
-		}
-
-		bool to_inv_is_current_player = false;
-		if (ma->to_inv.type == InventoryLocation::PLAYER) {
-			if (ma->to_inv.name != player->getName())
-				return;
-			to_inv_is_current_player = true;
-		}
+		if (!check_inv_access(ma->from_inv) ||
+				!check_inv_access(ma->to_inv))
+			return;
 
-		InventoryLocation *remote = from_inv_is_current_player ?
+		InventoryLocation *remote = ma->from_inv.type == InventoryLocation::PLAYER ?
 			&ma->to_inv : &ma->from_inv;
 
 		// Check for out-of-range interaction
@@ -664,7 +673,6 @@ void Server::handleCommand_InventoryAction(NetworkPacket* pkt)
 					<< (ma->from_inv.dump()) << ":" << ma->from_list
 					<< " to " << (ma->to_inv.dump()) << ":" << ma->to_list
 					<< " because src is " << ma->from_list << std::endl;
-			delete a;
 			return;
 		}
 
@@ -676,26 +684,14 @@ void Server::handleCommand_InventoryAction(NetworkPacket* pkt)
 					<< (ma->from_inv.dump()) << ":" << ma->from_list
 					<< " to " << (ma->to_inv.dump()) << ":" << ma->to_list
 					<< " because dst is " << ma->to_list << std::endl;
-			delete a;
-			return;
-		}
-
-		// Disallow moving items in elsewhere than player's inventory
-		// if not allowed to interact
-		if (!checkPriv(player->getName(), "interact") &&
-				(!from_inv_is_current_player ||
-				!to_inv_is_current_player)) {
-			infostream << "Cannot move outside of player's inventory: "
-					<< "No interact privilege" << std::endl;
-			delete a;
 			return;
 		}
 	}
 	/*
 		Handle restrictions and special cases of the drop action
 	*/
 	else if (a->getType() == IAction::Drop) {
-		IDropAction *da = (IDropAction*)a;
+		IDropAction *da = (IDropAction*)a.get();
 
 		da->from_inv.applyCurrentPlayer(player->getName());
 
@@ -708,52 +704,47 @@ void Server::handleCommand_InventoryAction(NetworkPacket* pkt)
 			infostream << "Ignoring IDropAction from "
 					<< (da->from_inv.dump()) << ":" << da->from_list
 					<< " because src is " << da->from_list << std::endl;
-			delete a;
 			return;
 		}
 
 		// Disallow dropping items if not allowed to interact
-		if (!checkPriv(player->getName(), "interact")) {
-			delete a;
+		if (!player_has_interact || !check_inv_access(da->from_inv))
 			return;
-		}
 
 		// Disallow dropping items if dead
 		if (playersao->isDead()) {
 			infostream << "Ignoring IDropAction from "
 					<< (da->from_inv.dump()) << ":" << da->from_list
 					<< " because player is dead." << std::endl;
-			delete a;
 			return;
 		}
 	}
 	/*
 		Handle restrictions and special cases of the craft action
 	*/
 	else if (a->getType() == IAction::Craft) {
-		ICraftAction *ca = (ICraftAction*)a;
+		ICraftAction *ca = (ICraftAction*)a.get();
 
 		ca->craft_inv.applyCurrentPlayer(player->getName());
 
 		m_inventory_mgr->setInventoryModified(ca->craft_inv);
 
-		//bool craft_inv_is_current_player =
-		//	(ca->craft_inv.type == InventoryLocation::PLAYER) &&
-		//	(ca->craft_inv.name == player->getName());
-
 		// Disallow crafting if not allowed to interact
-		if (!checkPriv(player->getName(), "interact")) {
+		if (!player_has_interact) {
 			infostream << "Cannot craft: "
 					<< "No interact privilege" << std::endl;
-			delete a;
 			return;
 		}
+
+		if (!check_inv_access(ca->craft_inv))
+			return;
+	} else {
+		// Unknown action. Ignored.
+		return;
 	}
 
 	// Do the action
 	a->apply(m_inventory_mgr.get(), playersao, this);
-	// Eat the action
-	delete a;
 }
 
 void Server::handleCommand_ChatMessage(NetworkPacket* pkt)