Mitigate formspec exploits by verifying that the formspec was shown to the user by the server. (#6878)

red-001 · nerzhul · commit 4bb41a19dc74 · 2018-02-18T22:33:42.000+01:00
This doesn't check the fields in anyway whatsoever so it should only be seen as a way to mitigate exploits, a last line of defense to make it harder to exploit bugs in mods, not as a reason to not do all the usually checks.
diff --git a/src/network/serverpackethandler.cpp b/src/network/serverpackethandler.cpp
@@ -1470,10 +1470,10 @@ void Server::handleCommand_NodeMetaFields(NetworkPacket* pkt)
 
 void Server::handleCommand_InventoryFields(NetworkPacket* pkt)
 {
-	std::string formname;
+	std::string client_formspec_name;
 	u16 num;
 
-	*pkt >> formname >> num;
+	*pkt >> client_formspec_name >> num;
 
 	StringMap fields;
 	for (u16 k = 0; k < num; k++) {
@@ -1501,7 +1501,32 @@ void Server::handleCommand_InventoryFields(NetworkPacket* pkt)
 		return;
 	}
 
-	m_script->on_playerReceiveFields(playersao, formname, fields);
+	if (client_formspec_name.empty()) { // pass through inventory submits
+		m_script->on_playerReceiveFields(playersao, client_formspec_name, fields);
+		return;
+	}
+
+	// verify that we displayed the formspec to the user
+	const auto peer_state_iterator = m_formspec_state_data.find(pkt->getPeerId());
+	if (peer_state_iterator != m_formspec_state_data.end()) {
+		const std::string &server_formspec_name = peer_state_iterator->second;
+		if (client_formspec_name == server_formspec_name) {
+			m_script->on_playerReceiveFields(playersao, client_formspec_name, fields);
+			if (fields["quit"] == "true")
+				m_formspec_state_data.erase(peer_state_iterator);
+			return;
+		}
+		actionstream << "'" << player->getName()
+			<< "' submitted formspec ('" << client_formspec_name
+			<< "') but the name of the formspec doesn't match the"
+			" expected name ('" << server_formspec_name << "')";
+
+	} else {
+		actionstream << "'" << player->getName()
+			<< "' submitted formspec ('" << client_formspec_name
+			<< "') but server hasn't sent formspec to client";
+	}
+	actionstream << ", possible exploitation attempt" << std::endl;
 }
 
 void Server::handleCommand_FirstSrp(NetworkPacket* pkt)
diff --git a/src/server.cpp b/src/server.cpp
@@ -1571,8 +1571,10 @@ void Server::SendShowFormspecMessage(session_t peer_id, const std::string &forms
 	NetworkPacket pkt(TOCLIENT_SHOW_FORMSPEC, 0 , peer_id);
 	if (formspec.empty()){
 		//the client should close the formspec
+		m_formspec_state_data.erase(peer_id);
 		pkt.putLongString("");
 	} else {
+		m_formspec_state_data[peer_id] = formname;
 		pkt.putLongString(FORMSPEC_VERSION_STRING + formspec);
 	}
 	pkt << formname;
@@ -2660,6 +2662,9 @@ void Server::DeleteClient(session_t peer_id, ClientDeletionReason reason)
 				++i;
 		}
 
+		// clear formspec info so the next client can't abuse the current state
+		m_formspec_state_data.erase(peer_id);
+
 		RemotePlayer *player = m_env->getPlayer(peer_id);
 
 		/* Run scripts and remove from environment */
diff --git a/src/server.h b/src/server.h
@@ -591,6 +591,8 @@ class Server : public con::PeerHandler, public MapEventReceiver,
 	*/
 	std::queue<con::PeerChange> m_peer_change_queue;
 
+	std::unordered_map<session_t, std::string> m_formspec_state_data;
+
 	/*
 		Random stuff
 	*/
