This is a short guide that attempts to explain how you should write code that deals with formspecs in a secure way.
The reason I'm writing it is because I have recently spend a lot of time discovering vulnerabilities and simple and effective exploits against many subgames, mods and games, and discovered an alarming amount of these issues that could lead to significant issues.
The risk of having incorrect formspec handling code is large. It may allow a player to do something trivial that they already can, but just under slightly different circumstances, but it may also allow the player to crash the server or corrupt the world map.
The core concept of securing your code against these type of exploits can be summarized with 2 key security defense approaches:
1. TOCTOU
"Time Of Check ~=(is not) Time Of Use". This refers to the often made mistake that the check that something is allowed isn't done at the same time that it's needed. For instance, we check that a player has admin privileges before we show him an admin formspec, but when the formspec data is returned from the client to the server, the permission check isn't done again. This allows players to exploit race conditions, or even send crafted data to the server and have it bypass the entire privilege check altogether.
2. Never Trust User Provided Data
Any data that the user provides should be considered hostile. Obviously if you provide text input to players in a form, you should make sure that when the data comes back, that it isn't going to cause any problems later on in the code. Common mistakes are allowing players to do things like embed escape sequences, or even whole commands that could break an SQL server down the code, or just crash a server.
But formspecs should also not carry any information that the player shouldn't be able to influence or fake either. The node position that needs to be kept in memory for an interaction should not be part of the formspec, since any part of a formspec can be influenced or faked by a client. Naming your form "myform:<playername>" is a recipe for disaster, and naming it "chestform:(20,3,-15)" is also very bad.
A simple strategy to improve security is to allow the player to provide only the needed pieces of data for the code to function. If the player is interacting with a node, the position of the node shouldn't be part of the formspec data that the player returns, but instead should be cached and kept separately in a way that the player can't modify or even inspect it. This is usually done by keeping a `context` around that can be retrieved when it is needed.
While there are other security principles at play, none of them are as urgent as these two. Still, one should always consider defense in depth approaches and assure that code is written with overall good security in mind at all times.
So, show me, what does this all mean?
First, we have to revisit how data is handled in formspecs in minetest, so you understand the implications and why some things are secure and other things are not.
First, in the minetest Lua API, there are 2 parts involved in formspecs: (1) the part where the formspec is generated and sent to the client, and (2) the part where the data returns from the client after the player interacts with the formspec.
We'll need to design both these parts to result in a secure handling of data, and prevent malicious players or modified clients from influencing the transaction and making the server do something that they hadn't intended.
The best way to understand the problem and to outline issues is to have a good working example, so you can follow the problems that we've created by bad coding practices and learn how to correct them, and why.
The teleport block
We'll make a hypothetical "teleport block" mod. It adds a simple block that shows a formspec that allows a player to teleport to a "special" location. If an admin clicks the block, they can program the block and change the target location. Normal players shouldn't be able to reprogram the block, of course. And we should obviously be aware of players attempting to abuse our code from teleporting themselves to places, or teleporting others!
Code: Select all
1 minetest.register_node("tele:port", {
2 description = "A teleport node",
3 on_rightclick = function(pos, node, clicker, itemstack, pointed_thing)
4 local name = clicker:get_player_name()
5 local formspec
6 if minetest.check_player_privs(clicker, "server") then
7 -- send admin formspec
8 formspec = "field[pos;target;(0,0,0)]"
9 else
10 -- send teleport formspec
11 formspec = "size[8,8]button[1,1;6,6;teleport;teleport]"
12 end
13 minetest.show_formspec(name, "teleport:" .. minetest.pos_to_string(pos), formspec)
14 end
15 })
16
17 minetest.register_on_player_receive_fields(function(player, formname, fields)
18 local pos = minetest.string_to_pos(formname:gsub("teleport:", ""))
19 if fields.target then
20 local meta = minetest.get_meta(pos)
21 meta:set_string("target", fields.target)
22 end
23
24 if fields.teleport then
25 local meta = minetest.get_meta(pos)
26 local target = meta:get_string("target")
27 player:set_pos(minetest.string_to_pos(target))
28 end
29 end)
So, let's go over what is going wrong here part by part, and elaborate what could happen if we didn't resolve it:
(1) First, we're putting more data into the formspec than is needed. It's fairly hidden, but in line 13 we're embedding a `pos` string into the formspec name. It's important to understand that bad user data may come from anywhere, and so it isn't going to be just inside the `fields` member that is passed at line 17, but it can actually also be in the `formname`! If exploited, an attacker could make the callback get executed with any arbitrary node location they choose, so it's a bad thing for sure. So remember: the client gets to tell the server what the formname of the returned data is, and it should never be trusted.
(2) TOCTOU: the permission of the user is never checked when the data is received. Nowhere after line 17 are the permissions of `player` actually verified. An attacker could just send a crafted formspec data record to the server and this would allow them to (a) modify the node's meta, and (b) teleport themselves possibly to a location that they can set using the same callback, but less obvious is that this would cause the callback be executed possibly with data that is malformed and it could possibly crash the server at line 27.
I have to note that since there is no checking on `pos` being valid that the attacker can just have a meta value set for any node for `target` and have it point to any location, and then subsequently use that node to exploit it and teleport it, even without actually right clicking the node. This works because the server doesn't actually verify that the user has previously right clicked the node when we receive the form data back on the server. You have to do that check.
Even worse is that the attacks can be all done in a single attack, since the `if` statements in 19 and 24 are not exclusive, but they should be, obviously.
(3) More importantly, when the callback is executed, there is no verification that the player actually has interacted with any teleport node on the map. The callback just looks at the data from the form but never validates that the player had previously interacted with the teleport node, and so this would allow an attacker to use any teleport node on the entire map whenever they want to, without even getting close to the teleport nodes or clicking them. Free teleports!
So, how do we fix this?
First thing we have to realize is the mechanics of sending and receiving formspecs. These two actions are not connected in any way on the server. A server sends a formspec packet to the client, the client just sends "some" data back. If we want to maintain more data in between sending and receiving, we should store it ourselves in a secure way for the time period that it is needed. We do this by maintaining a "context". This is a simple bit of memory where we can store some arbitrary data for the time being.
Code: Select all
1 local context = {}
2 minetest.register_on_leaveplayer(function(player)
3 local name = player:get_player_name()
4 context[name] = nil
5 end)
We want to make sure that we don't waste memory, so we should always as much as possible make sure that we keep this context table to the smallest size possible. First, obviously, if players log out, we should remove a context `pos` value if we have stored any, so in good measure we should make sure to prune the table if a player logs out.
Second, we should make sure in the code that we also remove a context for a player if they no longer need it. We can remove it for instance when the player teleports, or when the admin finishes programming the node. But if the player cancels the teleport, we should obviously also erase the context data. In general, you should always erase the context data on a formspec receive handler, unless you display the formspec again with some modified data.
Code: Select all
17 minetest.register_on_player_receive_fields(function(player, formname, fields)
18 local name = player:get_player_name()
19 local pos = context[name]
Code: Select all
20 if fields.target then
21 local meta = minetest.get_meta(pos)
22 meta:set_string("target", fields.target)
23 end
24
25 if fields.teleport then
26 local meta = minetest.get_meta(pos)
27 local target = meta:get_string("target")
28 player:set_pos(minetest.string_to_pos(target))
29 end
30
31 context[name] = nil
32 end)
Code: Select all
3 on_rightclick = function(pos, node, clicker, itemstack, pointed_thing)
4 local name = clicker:get_player_name()
5 local formspec
6 if minetest.check_player_privs(clicker, "server") then
7 -- send admin formspec
8 formspec = "field[pos;target;(0,0,0)]"
9 else
10 -- send teleport formspec
11 formspec = "size[8,8]button[1,1;6,6;teleport;teleport]"
12 end
13 context[name] = minetest.pos_to_string(pos)
14 minetest.show_formspec(name, "tele:port", formspec)
15 end
Good enough, right? Now a hostile attacker can't just exploit this anywhere anymore, but he could still exploit any teleport node! We still haven't solved the problem where the attacker that has a valid context is essentially granted admin permissions on the teleport target, and can change it.
So, what we need to do is double check that the player has the needed permissions, again.
Code: Select all
20 if fields.target and minetest.check_player_privs(player, "server") then
21 local meta = minetest.get_meta(pos)
22 meta:set_string("target", fields.target)
23 end
24
Code: Select all
1 local context = {}
2 minetest.register_on_leaveplayer(function(player)
3 local name = player:get_player_name()
4 context[name] = nil
5 end)
6
7 minetest.register_node("tele:port", {
8 description = "A teleport node",
9 on_rightclick = function(pos, node, clicker, itemstack, pointed_thing)
10 local name = clicker:get_player_name()
11 local formspec
12 if minetest.check_player_privs(clicker, "server") then
13 -- send admin formspec
14 formspec = "field[pos;target;(0,0,0)]"
15 else
16 -- send teleport formspec
17 formspec = "size[8,8]button[1,1;6,6;teleport;teleport]"
18 end
19 context[name] = minetest.pos_to_string(pos)
20 minetest.show_formspec(name, "tele:port", formspec)
21 end
22 })
23
24 minetest.register_on_player_receive_fields(function(player, formname, fields)
25 local name = player:get_player_name()
26 local pos = context[name]
27 pos = minetest.string_to_pos(pos)
28 if fields.target and minetest.check_player_privs("server") then
29 local meta = minetest.get_meta(pos)
30 meta:set_string("target", fields.target)
31 end
32
33 if fields.teleport then
34 local meta = minetest.get_meta(pos)
35 local target = meta:get_string("target")
36 player:set_pos(minetest.string_to_pos(target))
37 end
38
39 context[name] = nil
40 end)
On top of that, a second big problem exists when the teleport node is removed while a player or admin has the form open. In that case, the context would be valid, but the node metadata would be erased, leading to a possible server crash! While it wouldn't grant an attacker any special privileges, it would take your server offline and is classified therefore as a Denial of Service attack (DoS). Don't confuse this with a DDoS attack, which is something different. In the case of a DDoS, the attacker isn't sending malicious packets, just a lot of legitimate ones from various places to overflow the server. A DoS attack can sometimes be done with just a single network packet, and can be just as disruptive.
let's fix those issues up as well then:
Code: Select all
1 local context = {}
2 minetest.register_on_leaveplayer(function(player)
3 local name = player:get_player_name()
4 context[name] = nil
5 end)
6
7 minetest.register_node("tele:port", {
8 description = "A teleport node",
9 on_rightclick = function(pos, node, clicker, itemstack, pointed_thing)
10 local name = clicker:get_player_name()
11 local formspec
12 if minetest.check_player_privs(clicker, "server") then
13 -- send admin formspec
14 formspec = "field[pos;target;(0,0,0)]"
15 else
16 -- send teleport formspec
17 formspec = "size[8,8]button[1,1;6,6;teleport;teleport]"
18 end
19 context[name] = minetest.pos_to_string(pos)
20 minetest.show_formspec(name, "tele:port", formspec)
21 end
22 })
23
24 minetest.register_on_player_receive_fields(function(player, formname, fields)
25 local name = player:get_player_name()
26 local pos = context[name]
27 if not pos then
28 return true
29 end
30 pos = minetest.string_to_pos(pos)
30 local node = minetest.get_node(pos)
31 if node.name ~= "tele:port" then
32 context[name] = nil
33 return true
34 end
35 if fields.target and minetest.check_player_privs(player, "server") then
36 local meta = minetest.get_meta(pos)
37 meta:set_string("target", fields.target)
38 end
39
40 if fields.teleport then
41 local meta = minetest.get_meta(pos)
42 local target = meta:get_string("target")
43 player:set_pos(minetest.string_to_pos(target))
44 end
45
46 context[name] = nil
47 end)
This code isn't entirely 100% secure, but the remaining vulnerabilities are minimal and would likely not be worth much to any serious attacker.
Exercise
Question: What remaining vulnerabilities exist in this code? There is at least one exploit still present with this code, and you could argue that there are more.
Thanks for reading!
I hope you enjoyed this short, but hopefully deep enough guide into writing secure formspec code. Writing secure code is difficult, and always a trade off between available time and resources and security, so you may find that your personal standards are not at the same level as others in the discussion. The important part is to understand that when you make compromises, that you do so in a very educated fashion. In a way, you should never assume your own code is safe, even if you are very skilled at writing code - someone may just discover a complete new way of exploiting that code that you had never thought of, or you didn't think it would be a problem.
Please do partake in the discussion thread below, there were already some really good discussion topics posted as this article wasn't even finished yet. Take the opportunity to learn some critical and useful skills!