From 4bb41a19dc74fa31cb021dc3b5622814d67fbd6f Mon Sep 17 00:00:00 2001 From: red-001 Date: Sun, 18 Feb 2018 21:33:42 +0000 Subject: [PATCH] Mitigate formspec exploits by verifying that the formspec was shown to the user by the server. (#6878) 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. --- src/network/serverpackethandler.cpp | 31 ++++++++++++++++++++++++++--- src/server.cpp | 5 +++++ src/server.h | 2 ++ 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/network/serverpackethandler.cpp b/src/network/serverpackethandler.cpp index 98697d72f..420a79757 100644 --- 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 index 00fd8565a..24fbb38c8 100644 --- 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 index b5db04c8a..13c21067c 100644 --- a/src/server.h +++ b/src/server.h @@ -591,6 +591,8 @@ private: */ std::queue m_peer_change_queue; + std::unordered_map m_formspec_state_data; + /* Random stuff */