Mitigate formspec exploits by verifying that the formspec was shown to the user by...

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
index 98697d72f15ca4110f9a409c8410dbc326a57eb9..420a79757dc6ac685e74d3b6b1aadd62510f6a62 100644 (file)
--- 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 00fd8565a2742e8cfa07dfcbe5a5d14003d699e5..24fbb38c86d402887bbd3cebfbc2921d7c2435c5 100644 (file)
--- 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 b5db04c8ac018bb68759a7a52e706363185b7795..13c21067c5dd42af0ea79933331d5cc5a16ffc5d 100644 (file)
--- a/src/server.h
+++ b/src/server.h
@@ -591,6 +591,8 @@ private:
        */
        std::queue<con::PeerChange> m_peer_change_queue;
 
+       std::unordered_map<session_t, std::string> m_formspec_state_data;
+
        /*
                Random stuff
        */