Securing formspec code (code examples)

sofar
Developer
Posts: 2146
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Securing formspec code (code examples)

by sofar » Post

Securing Formspec Code

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)
If you noticed the error in this, hold up, it's not just one error that has been made here!

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)
Here we create a mod-local context table where, per player, we'll store some data that we shouldn't pass to the player - in this case the `pos` value of a teleport node.

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]
And later we should erase the context data:

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)
And of course, we actually have to create and set the context value when we the player opens the formspec, so that is done in the on_rightclick:

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
Note that we now have a much more simple invocation of the `show_formspec` as well.

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 
Let's put it all together now so we can quickly reassess what we have and double check everything:

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)
One more, possible big mistake remains. Since the client can call the formspec receive handler at any time with malicious data, it is possible that a malicious client causes the method to execute when there is no context at all for the player. So, we must make sure that we properly check that the context actually exists.

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)
And now the code should be working. I've posted a fully functional copy of this mod here: https://github.com/sofar/tele .

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!

sofar
Developer
Posts: 2146
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: [GUIDE]Securing formspec code[WIP]

by sofar » Post

Reserved for a changelog/history.

2017-12-23 rev #1 - initial start of document, first example code
2017-12-24 rev #2 - completed including code examples posted on github

User avatar
rnd
Member
Posts: 220
Joined: Sun Dec 28, 2014 12:24
GitHub: ac-minetest
IRC: ac_minetest
In-game: rnd

Re: [GUIDE]Securing formspec code[WIP]

by rnd » Post

1.Solution is to not to send data you don't need to to 'attacker' but just store it locally for later use together with various 'reminders' like "yes we did send that form then":

you could use separate 'authtable' variable in lua storing data what player can/can not do at certain times together with timestamps like authtable[name][formname] = {data,timestamp} where data contains pos... Then before sending form you can check privs and save confirmation in 'authtable'.

When you receive form you first check for timeout and later reset timestamp to prevent attacker from sending you several requests.

To compromise that attacker must be able to change variables in memory not just fake form events.

2. Or you could just embed 'authorization tokens' in formname specifying what 'attacker' can do - like they do it on the 'internets'. You could for example do: token = secure_hash(name..data..secret_key)
EDIT: main idea here is it is 'impossible' to fake secure_hash value if you don't know what was hashed. So client sending you token 'proves' he received it earlier under specified conditions.

3. another problem is: is connection client server encrypted (like TLS in https) or just plain text? if just plain text it can be easily spoofed. I know srp protocol is used to verify login (good) but what about later? EDIT: could simply calculate shared secret key from SRP and use that to encrypt 'delicate' client-server exchanges.
Last edited by rnd on Sun Dec 24, 2017 13:12, edited 2 times in total.
1EvCmxbzl5KDu6XAunE1K853Lq6VVOsT

User avatar
rubenwardy
Moderator
Posts: 6972
Joined: Tue Jun 12, 2012 18:11
GitHub: rubenwardy
IRC: rubenwardy
In-game: rubenwardy
Location: Bristol, United Kingdom
Contact:

Re: [GUIDE]Securing formspec code[WIP]

by rubenwardy » Post

Use contexts to store related data such as teleport data server side


There's little need to overcomplicate things with timestamps or tokens.
Renewed Tab (my browser add-on) | Donate | Mods | Minetest Modding Book

Hello profile reader

User avatar
rnd
Member
Posts: 220
Joined: Sun Dec 28, 2014 12:24
GitHub: ac-minetest
IRC: ac_minetest
In-game: rnd

Re: [GUIDE]Securing formspec code[WIP]

by rnd » Post

'context' is same as what i said in 1, just different name.
1EvCmxbzl5KDu6XAunE1K853Lq6VVOsT

User avatar
burli
Member
Posts: 1643
Joined: Fri Apr 10, 2015 13:18

Re: [GUIDE]Securing formspec code[WIP]

by burli » Post

Interesting, thx

sofar
Developer
Posts: 2146
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: [GUIDE]Securing formspec code[WIP]

by sofar » Post

This will be part of the next revision where I solve the issues in the code example. No need to get ahead of the document, this is a LOT of writing and it's going to take me some time to finish.

Sokomine
Member
Posts: 4276
Joined: Sun Sep 09, 2012 17:31
GitHub: Sokomine
IRC: Sokomine
In-game: Sokomine

Re: [GUIDE]Securing formspec code[WIP]

by Sokomine » Post

Thanks for this article. It's important to always keep in mind never to trust user input.
sofar wrote: "Time Of Check ~=(is not) Time Of Use".
While that may be a point in itself in some rare scenarios, it seems that most of the time it will be covered by point 2.
sofar wrote: 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.
That's even more of a danger than unchecked privs. There does not seem to be a way to *really* validate user input. With so many languages and charsets out there it's not exactly easy with regular expressions either. I'd be glad if you could write more about this point regarding lua, MT and its api in particular.

Pretty much of formspec handling is more or less always the same. It would be great to have libraries in MT in order to reduce redundancy and to be able to fix errors for more than one mod at the same time. Actual libraries that can be expected to be already installed and don't have to be downloaded and installed manually.
A list of my mods can be found here.

sofar
Developer
Posts: 2146
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: [GUIDE]Securing formspec code[WIP]

by sofar » Post

rnd wrote:3. another problem is: is connection client server encrypted (like TLS in https) or just plain text? if just plain text it can be easily spoofed. I know srp protocol is used to verify login (good) but what about later? EDIT: could simply calculate shared secret key from SRP and use that to encrypt 'delicate' client-server exchanges.
I'll skip over your first 2 points for now as they are a significant design change, and we should focus on the (now complete) article contents first to make sure we're not going crazy deep into the discussion until everyone understands the basics.

As for encryption, no, Minetest doesn't use any encryption and the data is entirely plaintext. Since this isn't something we can easily fix inside lua, I consider it *outside* the scope of this discussion, since this is about securing the most egregorious formspec handling mistakes that I've encountered, and not about much more complex attacks where formspecs are entirely uninteresting for an attacker.

sofar
Developer
Posts: 2146
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: [GUIDE]Securing formspec code[WIP]

by sofar » Post

Sokomine wrote:Thanks for this article. It's important to always keep in mind never to trust user input.
sofar wrote: "Time Of Check ~=(is not) Time Of Use".
While that may be a point in itself in some rare scenarios, it seems that most of the time it will be covered by point 2.
Not rare at all, in my professional experience. Also, the example code here was modelled after I found several mods making this very mistake.

It's one of the most common security mistakes of all, if not the most common security mistake. It can, and often is, one of the most difficult class of security bugs to secure against, especially with race condition exploits becoming increasingly more complex, and thus hard to test against.

Arguably with static code analysis tools, it has somewhat improved over time, since TOCTOU is often easily spotted by that methodology in an automated fashion.

sofar
Developer
Posts: 2146
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: [GUIDE]Securing formspec code[WIP]

by sofar » Post

Sokomine wrote:
sofar wrote: 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.
That's even more of a danger than unchecked privs. There does not seem to be a way to *really* validate user input. With so many languages and charsets out there it's not exactly easy with regular expressions either. I'd be glad if you could write more about this point regarding lua, MT and its api in particular.

Code: Select all

minetest.formspec_escape(string)
This topic is somewhat more difficult to write into a simple guide, since the amount of ways that things can go wrong are ... well, there's just a ton. Perhaps this is a second article/Guide worth.

A lot of how you defend against `odd` data stems not from how you get it, but what you do with it. If you display it in a formspec, you must `formspec_escape` it, for sure. If you convert it, you should always check the conversion result. If you pass it to another API, it should conform to the requirements of the API, etc..

In the code example, we haven't even discussed the input data format of `target`, for obvious reasons I've left this out.

sofar
Developer
Posts: 2146
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: [GUIDE]Securing formspec code[WIP]

by sofar » Post

The article content I wanted to cover is now finished. Note I have left an "exercise" to the reader, as there are definitely still some issues left (on purpose) in this code, in order to get some discussion going :).

red-001
Member
Posts: 205
Joined: Tue Jan 26, 2016 20:15
GitHub: red-001
IRC: red-001

Re: [GUIDE]Securing formspec code[CODE EXAMPLES]

by red-001 » Post

Found two issue so far, you mentioned leaving in a few for people to find themselves, so I put it in a spoiler in case someone wants to find them or fix them themselves.
Spoiler
1. You can crash the server if the teleport node has an invalid destination set, this could be either because someone placed it but never configured it or deliberately/accidentally by someone editing the target, privs needed to exploit: give
2. You can set a pos thats too large for set_pos, e.g. (-23445455, 445563445, 345456656), privs needed to exploit: server
Spoiler

Code: Select all

git diff
diff --git a/init.lua b/init.lua
index 111a502..666390c 100644
--- a/init.lua
+++ b/init.lua
@@ -18,12 +18,27 @@ SOFTWARE.

 ]]--

+local MAP_LIMIT = math.pow(2, 16) / 2
+
 local context = {}
 minetest.register_on_leaveplayer(function(player)
        local name = player:get_player_name()
        context[name] = nil
 end)

+local function check_coord_range(co_ord)
+       return ((co_ord < MAP_LIMIT) and (co_ord > -MAP_LIMIT))
+end
+
+local function check_pos(pos)
+       if pos and (pos.x ~= nil and pos.y ~= nil and pos.z ~= nil) then
+               if (check_coord_range(pos.x) and check_coord_range(pos.y) and check_coord_range(pos.z)) then
+                       return true
+               end
+       end
+       return false
+end
+
 minetest.register_node("tele:port", {
        description = "A teleport node",
        on_rightclick = function(pos, node, clicker, itemstack, pointed_thing)
@@ -59,8 +74,16 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
        end
        if fields.teleport then
                local meta = minetest.get_meta(pos)
-               local target = meta:get_string("target")
-               player:set_pos(minetest.string_to_pos(target))
+               local target = minetest.string_to_pos(meta:get_string("target"))
+               if target then
+                       if check_pos(target) then
+                               player:set_pos(target)
+                       else
+                               minetest.chat_send_player(name, "Address out of range set for teleport node")
+                       end
+               else
+                       minetest.chat_send_player(name, "Invalid address set for teleport node")
+               end
        end

        context[name] = nil

Oh and this isn't an exploit as far as I can see but the formspec handler doesn't check the formname which seems like a bad idea considering another mod could use the same fields names.

User avatar
Wuzzy
Member
Posts: 4786
Joined: Mon Sep 24, 2012 15:01
GitHub: Wuzzy2
IRC: Wuzzy
In-game: Wuzzy
Contact:

Re: [GUIDE]Securing formspec code[CODE EXAMPLES]

by Wuzzy » Post

Good information, and thanks a lot for this. But why isn't this part of the official documentation? It's no surprise there are vulnerabilities.
It's not good to scatter crucial info like this in forums posts. This one is not even sticky. :O
It's weird that the low-quality information is in lua_api.txt while the high-quality information can only be found in forum posts. xD

I did not know/consider the fact that clients can fake the formspec name. I hope none of my mods are affected, but there might be.
Thanks for this. This must definitely mentioned in lua_api.txt, it's very important but not obvious.

sofar
Developer
Posts: 2146
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: [GUIDE]Securing formspec code[CODE EXAMPLES]

by sofar » Post

Wuzzy wrote:Good information, and thanks a lot for this. But why isn't this part of the official documentation? It's no surprise there are vulnerabilities.
It's not good to scatter crucial info like this in forums posts. This one is not even sticky. :O
It's weird that the low-quality information is in lua_api.txt while the high-quality information can only be found in forum posts. xD

I did not know/consider the fact that clients can fake the formspec name. I hope none of my mods are affected, but there might be. Thanks for this. This must definitely mentioned in lua_api.txt, it's very important but not obvious.
I consider API documentation to be just that - it shouldn't delve too deeply into non-API aspects of the implementation. In general, if (any) API says that an event can be called with data that comes from a user interaction, all the items that are provided to the callee are suspect and should consider it spoofable data.

If you want me to look at particular formspec handling code, I'm totally OK with doing that, it's actualy I consider fun, and it allows me to make useful improvements, as well as a fun video sometimes where I show what an exploit could do ;)

User avatar
sorcerykid
Member
Posts: 1841
Joined: Fri Aug 26, 2016 15:36
GitHub: sorcerykid
In-game: Nemo
Location: Illinois, USA

Re: Securing formspec code (code examples)

by sorcerykid » Post

Interesting topic. I successfully tackled these issues by developing a framework that abstracts the builtin formspec API of Minetest. It even addresses three of the primary vulnerabilities mentioned above:
  • Secure Session Tracking
    Formspec names have been deprecated as they can be easily forged. Now each formspec session is assigned a unique session ID. Due to the persistent nature of the Minetest client-server protocol (unlike HTTP, for example), all session tracking is performed server-side. Negotiation and validation with the client is entirely unnecessary. Thus, integrity of the session ID is always guaranteed.

    Session-Based State Table
    Since the session ID token is retained throughout the lifetime of the formspec, it is therefore possible to update a formspec dynamically (e.g. in response to an event) with contextual data spanning multiple instances. This data is stored server-side via a session-based state table and it can even be initialized from within the formspec string itself using a new "hidden" element.

    Localized Event Handling
    The minetest.register_on_player_receive_fields( ) method has also been deprecated. Instead, each formspec is assigned its own callback function at runtime, which allows for completely localized event handling. This callback function is invoked after any event associated with the formspec (hence the moniker "ActiveFormspecs"). Both the meta table and form fields are passed as arguments.
A detached formspec can be shown to the player using the minetest.create_form( ) method:
  • minetest.create_form( meta, player, formspec, on_close)
    • meta - an optional session-based state-table (can be nil)
    • player - a player object
    • formspec - a standard formspec string
    • on_close - a callback function to be invoked after an event
The associated on_close( ) callback function will be automatically invoked with three arguments during a formspec event:
  • on_close( meta, player, fields )
    • meta - the session-based state table
    • player - the player object
    • fields - the table of submitted fields
Formspecs are also supported within a node definition. If an on_open( ) method is defined, then it will be invoked whenever the player right-clicks the node. The return value must be a standard formspec string, or nil to abort.
  • nodedef.on_open( pos, player, fields )
    • pos - the position of the node
    • player - the player object
The on_close( ) method is invoked during any formspec event. If defined, it will receive three arguments:
  • nodedef.on_close( pos, player, fields )
    • pos - the position of the node
    • player - the player object
    • fields - the table of submitted fields
The "hidden" formspec element allows for a slightly different workflow, by presetting the state table from the formspec string itself. The content of hidden fields is is only used for initializing the state table, but is ignored thereafter.

Code: Select all

local formspec = "size[5,3]"
     .. default.gui_bg_img
     .. "hidden[owner;" .. player:get_player_name( ) .. "]"
     .. "hidden[is_shared;true]"
     .. "field[0,0;4,0.25;message;Enter a Message;" .. minetest.formspec_escape( message ) .. "]"
     .. "button[0,1;2,1;save;Save]"

     -- Using these hidden elements, meta.is_shared will be initalized to "true" and
     -- meta.owner will be initalized to the player name by minetest.create_form( )

     minetest.create_form( nil, player, formspec, on_close )
Here is the typical methodology of the ActiveFormspecs framework, In this example, an "uptime" chat command is registered that shows the current server uptime in either minutes or seconds:

Code: Select all

minetest.register_chatcommand( "uptime", {
        description = "View the uptime of the server interactively",
        func = function( name, param )
                local player = minetest.get_player_by_name( name )
                local meta = { is_minutes = false }

                local get_formspec = function( meta )
                        local uptime = minetest.get_server_uptime( )
                        local formspec = "size[4,3]"
                                .. default.gui_bg_img
                                .. string.format( "label[0.5,0.5;%s: %0.1f %s]",
                                        minetest.colorize( "#FFFF00", "Server Uptime:" ),
                                        meta.is_minutes and uptime / 60 or uptime,
                                        meta.is_minutes and "mins" or "secs"
                                )
                                .. "checkbox[0.5,1;is_minutes;Show Minutes;" .. tostring( meta.is_minutes ) .. "]"
                                .. "button[0.5,2;2.5,1;update;Refresh]"

                        return formspec
                end
                local on_close = function( meta, player, fields )
                        if fields.update then
                                minetest.update_form( player, get_formspec( meta ) )
                        elseif fields.is_minutes then
                                meta.is_minutes = fields.is_minutes == "true"
                                minetest.update_form( player, get_formspec( meta ) )
                        end
                end

                minetest.create_form( meta, player, get_formspec( meta ), on_close )
        end
} )
ActiveFormspecs has been in use on my server since December 2016 and has undergone only a few minor revisions, so it's definitely suitable for a production environment. Currently every installed mod (including default) uses this new API. The following screenshots are just a small sampling of the various scenarios in which ActiveFormspecs has been rigorously "put to the test" on the JT2 server.
  • Player Information Panel
    Image

    Realtime System Monitor
    Image

    Chat History Viewer
    Image

    Macro Crafting Manager
    Image

    Teleporter Staff
    Image

    Weather Summary Panel
    Image
Although ActiveFormspecs has not been formally "released" per se, I did publish it a few weeks ago as a necessary dependency for the GiftBox Mod. Of course, anyone is welcome to download ActiveFormspecs from my repository below. It's very easy to use, and arguably a more robust and secure alternative to the builtin formspecs API :)

http://jt2.intraversal.net/repo/release/mods/formspecs/

On a sidenote, I've had a lot of requests over the past few months for source code from the just_test_tribute subgame, So I will probably go ahead and create a separate topic for ActiveFormspecs with instructions and code examples, so that other mod authors can start making use of this framework as well!

sofar
Developer
Posts: 2146
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: Securing formspec code (code examples)

by sofar » Post

Note that I'm solving some, but not all the issues in fsc:

viewtopic.php?f=9&t=19203

- node formspecs are inherently insecure, don't use them if you can avoid it
- inventory formspecs similarly are insecure

`fsc` is expressly minimalistic. No fancy screenshots.

I hadn't heard about sorcerykids' solution obviously, not that the 70 lines or code were hard to write. I'll have to review the code and see if anything pops out that makes sense to port (either way).

User avatar
sorcerykid
Member
Posts: 1841
Joined: Fri Aug 26, 2016 15:36
GitHub: sorcerykid
In-game: Nemo
Location: Illinois, USA

Re: Securing formspec code (code examples)

by sorcerykid » Post

I contributed to the discussion since you asked for people's input and ideas. I had a solution that has been working well on my server, and I felt it worthwhile to share with others. The screenshots were simply to show what is possible.

Your reply comes across as snarky, rather than supportive and appreciative.

Incidentally, I do not use "node formspecs". That was the primary reason that I created Active Formspecs in the first place.

sofar
Developer
Posts: 2146
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: Securing formspec code (code examples)

by sofar » Post

sorcerykid wrote:I contributed to the discussion since you asked for people's input and ideas. I had a solution that has been working well on my server, and I felt it worthwhile to share with others. The screenshots were simply to show what is possible.

Your reply comes across as snarky, rather than supportive and appreciative.

Incidentally, I do not use "node formspecs". That was the primary reason that I created Active Formspecs in the first place.
I just earlier today posted my `fsc` mod in `WIP Mods`, so it seemed appropriate to link it in here since you essentially did the same. Nothing snarky intended, I was being brief. I am debugging some code and wanted to focus on that instead of lengthy forum interchanges. But I didn't want to confuse people or get into a "he stole she stole" type of argument either. We both just ... wrote the same code. It's not that complex either.

I'm a software professional. Unless I call your code junk, statements like "I'll have to review your code" are supportive in my vocabulary, I don't go and praise people for creations until I can figure out how it works, and making nice looking formspecs isn't part of this discussion.

If anything, the fact that you wrote a proper solution for an important part of the problem is already a compliment by itself. As a dutch person, I don't hand out much more elaborate compliments that quickly.

User avatar
Wuzzy
Member
Posts: 4786
Joined: Mon Sep 24, 2012 15:01
GitHub: Wuzzy2
IRC: Wuzzy
In-game: Wuzzy
Contact:

Re: Securing formspec code (code examples)

by Wuzzy » Post

I still think the docs should at least state that a few particular formspec names (like player name) are taboo.
This security hole is NOT obvious to modders.

User avatar
sorcerykid
Member
Posts: 1841
Joined: Fri Aug 26, 2016 15:36
GitHub: sorcerykid
In-game: Nemo
Location: Illinois, USA

Re: Securing formspec code (code examples)

by sorcerykid » Post

and making nice looking formspecs isn't part of this discussion.
The discussion (or so I thought) was about securing formspec code. In this way, representative examples of secure formspecs depicting the various possible use-cases are absolutely relevant. Some mod authors, after all, may be under the mistaken impression that these security implementations are either a) complex or b) limiting. I was intending to show that is absolutely not the case. Sometimes it's about "selling" an idea, to get an important point across.

sofar
Developer
Posts: 2146
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: Securing formspec code (code examples)

by sofar » Post

sorcerykid wrote:
and making nice looking formspecs isn't part of this discussion.
The discussion (or so I thought) was about securing formspec code. In this way, representative examples of secure formspecs depicting the various possible use-cases are absolutely relevant. Some mod authors, after all, may be under the mistaken impression that these security implementations are either a) complex or b) limiting. I was intending to show that is absolutely not the case. Sometimes it's about "selling" an idea, to get an important point across.
yes, actually, I shouldn't have said that. Your examples are illustrative of the fact that this solution doesn't inhibit any of that, I am actually envious of the nice looking forms... :/

User avatar
KGM
Member
Posts: 191
Joined: Mon Nov 14, 2016 19:57
Location: Bonn, Germany

Re: Securing formspec code (code examples)

by KGM » Post

ever heard of cryptography?
if you send encrypted and signed data to the client,
he cannot fake anything. nor he can extract anything.
When I first came here, this was all swamp. Everyone said I was daft to build a castle on a swamp, but I built in all the same, just to show them.

sofar
Developer
Posts: 2146
Joined: Fri Jan 16, 2015 07:31
GitHub: sofar
IRC: sofar
In-game: sofar

Re: Securing formspec code (code examples)

by sofar » Post

KGM wrote:ever heard of cryptography?
if you send encrypted and signed data to the client,
he cannot fake anything. nor he can extract anything.
This shows that you don't understand any part of the problem. The client is permitted to send formspec data. To the server. There's nothing that you could do by encrypting data sent to the client. That literally would just be a waste of network packets.

User avatar
KGM
Member
Posts: 191
Joined: Mon Nov 14, 2016 19:57
Location: Bonn, Germany

Re: Securing formspec code (code examples)

by KGM » Post

NO!
when you send the form, you compose the formname out of mymod:+playername+other_data_that_the_client_must_not_modify

you store the md5 of this formname.

when you receive a form back you first check if you have its md5 stored.
if yes, you process the data in an protected function.
(to avoid malicious data crashing server)

afterwards, you remove the stored md5 to make shure replays don't work.

this fixes all threads (client modifying data (in for example formname) he must not modify, spoofing, malicious data, replay attack)

and it is much more general than the proposed "solution".
Spoiler
the attacker wants to attack our server. : (

The server uses an encrypting algorithm wich does not allow the attacker to get any piece of the encrypted information without the key.

Nor the attacker can predict what changes apply to the content when he changes the encrypted stuff in a specific manner.

randomly generated

on server startup, a random key K is generated.
on server startup, a random signature S is generated.
on server startup, a number N is initialized with 0.
on server startup, a table TAKENN is initialized as empty table.
on click, N is increased.

POS, PLAYERNAME and N is embedded in formname, signed with S and encrypted with K.

the whole receiving program is put in a pcall, to insure the server doesn't crash.

when the data is received, the stuff gets decrypted, and S, N, PLAYERNAME and POS get extracted.

if S is wrong,
the player gets banned and the process stops.

if N is in TAKENN,
the player gets banned and the process stops.

N is inserted into TAKENN.
the player with PLAYERNAME gets teleported to POS.

now, our attacker can't spoof the form, with changed contents, since he can't change encrypted data in a senseful way.

he also can't reuse the form data he used once because the old N is in TAKENN, and when he reuses the data with the known N, he gets banned and the process stops.

also, when the server restarts, any old data is invalid because it's encrypted with a different key.

i have to admit that my solution can be simplified.

you could just store a md5 for every package sent, and when you receive it back, you check if you have it's md5 stored.

any problems?

i think this is at least as secure as your solution, but it can be applied to any DATA-AUTHENTICITY-VERIFICATION-PROBLEM.

>"This shows that you don't understand any part of the problem."

Watch and learn instead of being rude.

Code: Select all

Watch and learn instead of being rude .

______________________________
\####################\        \
 \#########/\#########\        \
  \#######//\\#########\        \
          /  \
        ^^^^^^^^
         _________



Last edited by KGM on Tue Sep 04, 2018 17:53, edited 7 times in total.
When I first came here, this was all swamp. Everyone said I was daft to build a castle on a swamp, but I built in all the same, just to show them.

Post Reply

Who is online

Users browsing this forum: No registered users and 7 guests