[Mod] FSC [fsc] Secure Formspec Context

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

[Mod] FSC [fsc] Secure Formspec Context

by sofar » Tue Jan 02, 2018 19:20

fsc

This mod is designed to help write more secure formspec handling code. It achieves this by throwing out the concept of "formspec names" entirely and giving each formspec shown to the player a unique, random ID. The player can only then submit form data using this unique ID, and, the handling code can invalidate the ID during processing automatically.

This reduces the risk that an attacker can forge formspec data and send uninvited packets to the server. The server will discard any form data that appears to come from a client that is attempting to use old or incorrect fsc-created forms and will note this event in the minetest log.

Because of the simplicity of the approach, mods will no longer need to focus on basic formspec handling code and can instead spent their time verifying the proper permissions and input data correctness.

This mod also provides a much more simple way to maintain a formspec "context" and pass it along to subsequent formspecs. This makes writing formspec code simpler as the context does not need to be maintained outside the formspec handling or creation code, and no memory leakage needs to be worried about.

A player can also only ever obtain one context, and attempting to use an invalid or outdated context will result in all current valid formspec contexts being revoked. Combined together, all these features make formspecs a lot safer to work with.

Usage

The basic workflow of `fsc` contains of a single function call. Outside of this function call, there are no other API functions or data.

Code: Select all
function fsc.show(name, formspec, context, callback)
   -- `name`: a playername,
   -- `formspec`: a valid formspec string,
   -- `context`: any data, may be `nil`
   -- `callback`: function(player, fields, context).


The return value of `fsc.show()` is always `nil` - it returns nothing.

The callback function will only be called if basic sanity checks on the data pass requirements. You can implement it simply as follows:

Code: Select all
local function callback(player, fields, context)
   -- `player`: player object,
   -- `fields`: table containing formspec data returned by the player client,
   -- `context`: any data, will never be `nil`. If no context was passed
   --            to `fsc.show()`, it will contain `{}`
   return true
end


The return value of the callback may be `nil` or `true`. If you return `nil`, the context is not invalidated, and the player may submit the formspec using the same ID again. This is useful if the player merely selects a list item or otherwise performs an action in the form that does not cause the form to be closed on the client, and you wish to
keep the form open.

If you return `true`, or if you return `nil` and `fields.quit` is set, then the fsc code will invalidate the ID and close the formspec. You should return `true` unless you want to keep the form open to the player.

Making a simple callback handler that shows a new form is therefore relatively straightforward. The below example passes the current context data through to the new form. The old form will close, and the new form will appear to the player with the new content.

Code: Select all
local function callback(player, fields, context)
   local name = player:get_player_name()
   if fields.rename then
      fsc.show(name,
         "field[new_name;What is the new name?;" .. minetest.formspec_escape(context.old_name) .. "]",
         context,
         callback)
      return
   else
      -- do something else
      return true
   end
end


In some cases, you may wish to show a form without having the need for a callback, in case the content is just informational and non-interactive. In that case, you can omit a callback handler by just inserting an empty callback handler, as follows:

Code: Select all
fsc.show(name, formspec, {}, function() end)

Node Formspecs

Node formspecs are not handled. Due to the nature of node/inventory formspecs, it is inherently impossible to perform the same checks on node/inventory formspecs as `fsc` can do for (normal) formspecs.

License: FormSpec Context ('fsc') mod for minetest, licensed under the `ISC` license:
Dependencies: none

Github: https://github.com/sofar/fsc
Archive (master): https://github.com/sofar/fsc/archive/master.zip
 

User avatar
rnd
Member
 
Posts: 207
Joined: Sun Dec 28, 2014 12:24
IRC: ac_minetest
In-game: rnd

Re: [mod]FSC[fsc] Secure Formspec Context

by rnd » Wed Jan 03, 2018 09:12

Much better than default way. Instead of sending names with sometimes weird extra(position/other stuff inside name) you simply send randomized 'token' and remember you sent it, and you can store 'extra data' inside that 'context' _data table.

1.I think this comment is not accurate:
Code: Select all
if data.name ~= name then
minetest.log("error", "fsc: possible hash collision or exploit (name mismatch)")


If this were to happen, it would mean that player 'name' would have gotten written some other 'name1' when doing fsc.show but clearly
Code: Select all
_data[name] = {
      id = id,
      name = name,
      context = context,
      callback = callback,
}


it sets same 'name'. What you are implying is saying there is BUG IN LUA itself, in implementation of its hashtable, so that _data[name] and _data[name1] can point to same object sometimes. There is a thing called 'collision resolution' which ensures this does not happen - ever. Either by chaining, open addressing,...

2. misleading naming: variable 'context'. What is context is actually '_data' table, variable 'context' used here is just extra data you remember inside '_data' that goes together with form, you never do anything else with it - at least if you compare with what was said at 'secure formspec' forum post.
 

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

Re: [mod]FSC[fsc] Secure Formspec Context

by sofar » Wed Jan 03, 2018 22:19

Thanks. I've updated the comments and error message here, although it's somewhat circumstantial and doesn't change any functionality itself. Still, I appreciate the review!
 

User avatar
stu
Member
 
Posts: 907
Joined: Sat Feb 02, 2013 02:51
Location: United Kingdom
GitHub: stujones11

Re: [Mod] FSC [fsc] Secure Formspec Context

by stu » Wed Jan 03, 2018 23:00

I really like what this does but surely things as important as this belong in the builtin scripts. You are a core developer, right?

Nice job all the same, let's hope you can encourage enough modders to actually use it :)
 

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

Re: [Mod] FSC [fsc] Secure Formspec Context

by sofar » Wed Jan 03, 2018 23:12

stu wrote:I really like what this does but surely things as important as this belong in the builtin scripts. You are a core developer, right?

Nice job all the same, let's hope you can encourage enough modders to actually use it :)


Yes, this fits my standards for `it should be default`. However, as I wrote this, I quickly realized that it changes formspec handling code in a subtle way that may require significant rewrite and/or thought, plus it makes some methods of calling handlers entirely obsolete, stuff used in the inventory formspec for instance works very different.

So, as usual with code I write, I tend to post PoC or working and solid code separately first and see whether the interest and support for the mechanism is there. The API isn't particularly simple, although it's arguably a lot better than the old code. So I might have to adjust little things like documentation, and even though it's small, there might still be a bug in it.

Also, `fsc` is still a terrible name ;)
 

Chem871
Member
 
Posts: 870
Joined: Sat Aug 19, 2017 21:49
Location: SCP-2935
GitHub: Chemguy99
In-game: Nyx Serris

Re: [Mod] FSC [fsc] Secure Formspec Context

by Chem871 » Wed Jan 03, 2018 23:14

I think you got th "f" and the "s" in the mod name switched.
What is SCP-055 again? I forgot.
 

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

Re: [Mod] FSC [fsc] Secure Formspec Context

by sofar » Wed Jan 03, 2018 23:43

Chem871 wrote:I think you got th "f" and the "s" in the mod name switched.


lol, no. The original acronym stood for "FormSpec Context"... =)
 

bell07
Member
 
Posts: 534
Joined: Sun Sep 04, 2016 15:15
GitHub: bell07

Re: [Mod] FSC [fsc] Secure Formspec Context

by bell07 » Thu Jan 04, 2018 09:33

This mod introduces context in formspecs in other words a state or session. It is an design decision that should not be default. Each mod should decide byself if the implementation is stateful or stateless.

For full-stated mods the smartfs exists, that support states for all inventory types including nodemeta and inventory. Maybe overdone, the state is for each formspec element oO

As food for thought for default/engine, maybe the REST concept should be implemented in formspec like the URL-Parameters in HTTP. See https://en.wikipedia.org/wiki/Represent ... e_transfer
In the current implementation only "formspec name" and "fields" table ist transferred to indentiy the state. Sending of andditional custom data back from client could be usefull for state transfer. An proposal: Maybe a hidden 'context[a;b]' formspec element can be implemented as next that sends allways a fields.a = 'b' back to the form each input?

At this point I have again the question why the minetest implements an own proprietary formspec format instead of using something standardized, like SOAP-XML + XSL client-site or HTML/CSS
 

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

Re: [Mod] FSC [fsc] Secure Formspec Context

by Wuzzy » Thu Jan 04, 2018 13:31

Thank you for trying to solve the problem, but: It's only a mod. :-(
If I get a dollar for every mod which supposedly fixes an engine bug / shortcoming … xD

There's a simple test for mods like this: Is this a mod which practically every subgame wants/needs? If the answer is “yes”, then it probably needs to go into builtin or even engine. And for this mod, the answer is definitely “yes”. :P

The real problem is that the current official formspec API makes it is incredibly easy to screw up, as you demonstrated here: viewtopic.php?f=47&t=19129

It's an obvious design flaw that modders have to jump through a lot of (undocumented!) hoops in order to secure their formspecs. And none of this is properly documented. No, a forum post is NOT proper documentation. :P

IMO formspec API needs a redesign anyway. This is not even controversial. Official docs state that formspecs are experimental. The modders have been warned. Minetest is still in Alpha. So please don't be afraid to break things if the new revision is a major improvement. Especially if it's fixing some serious security/design flaws. And formspecs are in serious need of a redesign anyway. Security is just another of many things wrong with formspecs.
Maybe it would be a good idea to analyze the current (bad) formspec API, collect all the design flaws and problems (like weird coordinates, security fails, …), then use the new information to carefully design a new (breaking) API and fix everything at once. The idea is to break the API only once, but do it right. :-)

IMO it's incredibly harmful for Minetest to be overly conservative, especially at this stage of development, because you will stick with legacy and halfplemented or half-broken features forever.
My creations. I gladly take any bitcoins you have lying around: 17fsUywHxeMHKG41UFfu34F1rAxZcrVoqH
 

User avatar
ExeterDad
Member
 
Posts: 1717
Joined: Sun Jun 01, 2014 20:00
Location: New Hampshire U.S.A
In-game: ExeterDad

Re: [Mod] FSC [fsc] Secure Formspec Context

by ExeterDad » Thu Jan 04, 2018 14:29

Wuzzy wrote:it's incredibly harmful for Minetest to be overly conservative, especially at this stage of development, because you will stick with legacy and halfplemented or half-broken features forever.

Beautifully written.
/me get's emotional
 

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

Re: [Mod] FSC [fsc] Secure Formspec Context

by sofar » Thu Jan 04, 2018 16:28

Wuzzy wrote:There's a simple test for mods like this: Is this a mod which practically every subgame wants/needs? If the answer is “yes”, then it probably needs to go into builtin or even engine. And for this mod, the answer is definitely “yes”.


I already stated before:

sofar wrote:Yes, this fits my standards for `it should be default`.


sofar wrote:So, as usual with code I write, I tend to post PoC or working and solid code separately first and see whether the interest and support for the mechanism is there.


How fast and in what shape this makes it into core, default or something else is likely inevitable, but it's not clear to me how it would look. With code, there's something to discuss and play with, which only will speed it up.
 

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

Re: [Mod] FSC [fsc] Secure Formspec Context

by sofar » Thu Jan 04, 2018 16:30

bell07 wrote:An proposal: Maybe a hidden 'context[a;b]' formspec element can be implemented as next that sends allways a fields.a = 'b' back to the form each input?


We're trying to improve security, not make it worse. That is just a terrible idea, sorry.
 

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

Re: [Mod] FSC [fsc] Secure Formspec Context

by sofar » Thu Jan 04, 2018 16:35

bell07 wrote:At this point I have again the question why the minetest implements an own proprietary formspec format instead of using something standardized


I don't think you understand the meaning of the word "proprietary". Being OSS, formspecs as minetest implements them are per definition not proprietary.
 

bell07
Member
 
Posts: 534
Joined: Sun Sep 04, 2016 15:15
GitHub: bell07

Re: [Mod] FSC [fsc] Secure Formspec Context

by bell07 » Thu Jan 04, 2018 17:03

We're trying to improve security, not make it worse. That is just a terrible idea, sorry.
I did not realized the focus on security. Agree my proposal take it more worse for modders without security knowledge. But used right the proposed formspec enhancement can be usefull. For example to store the selected page in inventory formspec for example (button="1" on page="2" instead of button "page2_button1")

For security the
1. TOCTOU
2. Never Trust User Provided Data

is most important as you described. The "scrambling" formspec names does not help for this both points, to enforce the server-site context state does help only partially for 2.

I think for beter security the permission checks should be more supported and connected between formspec creation and provided data. That means if you define in formspec definition the field "target" is allowed only for server privilege, this privilege is checked again if data is received back automatically for example.
Agree the node position in formspec name is bad by design. We have nodemeta-formspecs for this use case already. But the nodemeta-formspecs are created without player-relation so no permissions are possible at formspec creation time.

I don't think you understand the meaning of the word "proprietary"
Agree, I use the word wider then defined. All non-standard formats, protocols or dialects used exclusively in just one solution is proprietär for me. German "proprietär" definition is between the english and my one ;-)
But it is not the point, my question was why the wheel was reinvented again.
 

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

Re: [Mod] FSC [fsc] Secure Formspec Context

by sofar » Thu Jan 04, 2018 17:45

bell07 wrote:Agree my proposal take it more worse for modders without security knowledge. But used right the proposed formspec enhancement can be usefull. For example to store the selected page in inventory formspec for example (button="1" on page="2" instead of button "page2_button1")


Lua doesn't offer you any way of determining that "fields.a" was provided by yourself and trustworthy, or that "fields.a" was provided by the player and thus unsafe. This is why you have to separate "fields" from "context" and maintain the separation. Mixing data of the 2 sources in "fields" would make it impossible (not difficult!) to maintain security properly.
 

bell07
Member
 
Posts: 534
Joined: Sun Sep 04, 2016 15:15
GitHub: bell07

Re: [Mod] FSC [fsc] Secure Formspec Context

by bell07 » Thu Jan 04, 2018 18:23

Lua doesn't offer you any way of determining that "fields.a" was provided by yourself and trustworthy, or that "fields.a" was provided by the player and thus unsafe.
I know ;-). Never Trust User Provided Data.
But it allow client-site to request or trigger anything directly (stateless) from page 3 without previous paging to the page 3. In interaction with client-site mods very usefull.
Of course TOCTOU needs to be happen to be sure the player is allowed to do anything on page 3.
As I read the first time about REST concepts with all session related data in URL I had the same concerns. But it enforces and educates the developer to consider the TOCTOU. But agree this enhancement proposal is off-topic, as I said I overlooked the security relation in your topic.

Back to the security: The nodemeta formspecs are maybe the beter example for the mentioned security issue. The formspecs are generated without the player relation, or maybe with the placer-relation in "on_place", or maybe with last-used-player relation. Between the formspec creation and usage maybe the server was restarted and mods updated or replaced. It is most difficult example but if solution found, the solution will be more general. Maybe you enhance your PoC to the secure nodemeta formspecs?
My proposal is to store the "private context" in playermeta for "usual" formspecs and in nodemeta for nodes.
I seen the https://github.com/minetest/minetest/pull/5702/ but not tested yet. Usable to secure the node+player related context? Can the "formspec" be secured on node?
 

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

Re: [Mod] FSC [fsc] Secure Formspec Context

by sofar » Thu Jan 04, 2018 18:35

bell07 wrote:The nodemeta formspecs are maybe the beter example for the mentioned security issue. The formspecs are generated without the player relation, or maybe with the placer-relation in "on_place", or maybe with last-used-player relation. Between the formspec creation and usage maybe the server was restarted and mods updated or replaced. It is most difficult example but if solution found, the solution will be more general. Maybe you enhance your PoC to the secure nodemeta formspecs?


I don't believe it's possible. There is no guarantee in the engine that it can't be bypassed. The only sane thing to do is to remove all node formspecs and call `fsc.show` in the on_rightclick() methods.

Exploiting node formspecs is trivial, too. The engine has only very limited tools to prevent abuse.
 

Byakuren
Member
 
Posts: 816
Joined: Tue Apr 14, 2015 01:59
GitHub: raymoo
IRC: Hijiri
In-game: Raymoo + Clownpiece

Re: [Mod] FSC [fsc] Secure Formspec Context

by Byakuren » Thu Jan 04, 2018 18:54

This seems like something I would use. Better than keeping a table for every stateful formspec.
Every time a mod API is left undocumented, a koala dies.
 


Return to Mod Releases



Who is online

Users browsing this forum: No registered users and 3 guests