From 33afe1fb56273251400fed1eb13b2b407a3d0420 Mon Sep 17 00:00:00 2001 From: Jozef Behran Date: Mon, 21 Jan 2019 03:53:09 -0500 Subject: Fix randomly rejected form field submits (#8091) If a formspec is submitted from a form fields handling callback of another form (or "formspec shown from another formspec"), the fields submitted for it can get rejected by the form exploit mitigation subsystem with a message like "'zorman2000' submitted formspec ('formspec_error:form2') but server hasn't sent formspec to client, possible exploitation attempt" being sent to logs. This was already reported as #7374 and a change was made that fixed the simple testcase included with that bug report but the bug still kept lurking around and popping out in more complicated scenarios like the advtrains TSS route programming UI. Deep investigation of the problem revealed that this sequence of events is entirely possible and leads to the bug: 1. Server: show form1 2. Client *shows form1* 3. Client: submits form1 4. Server: show form2 5. Client: says form1 closed 6. Client *shows form2* 7. Client: submits form2 What happens inside the code is that when the server in step 4 sends form2, the registry of opened forms is updated to reflect the fact that form2 is now the valid form for the client to submit. Then when in step 5 client says "form1 was closed", the exploit mitigation subsystem code deletes the registry entry for the client without bothering to check whether the form client says was closed just now is indeed the form that is recorded in that entry as the valid form. Then later, in step 7 the client tries to submit its valid form fields, these will be rejected because the entry is missing. It turns out the procedure where the broken code resides already gets the form name so a simple "if" around the offending piece of code fixes the whole thing. And advtrains TSS agrees with that. --- src/server.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/server.cpp b/src/server.cpp index f1fbe8668..41ac87527 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -1577,7 +1577,11 @@ 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); + //but make sure there wasn't another one open in meantime + const auto it = m_formspec_state_data.find(peer_id); + if (it != m_formspec_state_data.end() && it->second == formname) { + m_formspec_state_data.erase(peer_id); + } pkt.putLongString(""); } else { m_formspec_state_data[peer_id] = formname; -- cgit v1.2.3