Time  Nick       Message
18:53 sfan5      merging game#2579 in 5m
18:53 ShadowBot  https://github.com/minetest/minetest_game/issues/2579 -- Add traditional Chinese translation(default) by IFRFSX
19:10 Krock      (7 mins)
19:15 sfan5      oh well
19:20 Krock      what does the mgv7 "ridges" mgflag do?
19:21 Krock      ooh. the GUI translates that to "Rivers"
19:30 paramat    i might merge game#2586 in 3 hours
19:30 ShadowBot  https://github.com/minetest/minetest_game/issues/2586 -- Avoid error when 'get_player_information' returns 'nil' by paramat
19:31 paramat    game#2580 simple high priority bugfix
19:31 ShadowBot  https://github.com/minetest/minetest_game/issues/2580 -- Fix missing papyrus in savanna, add a dry dirt version by paramat
20:46 Wuzzy      Krock: ridges are those wide river channels that agressively cut through terrain, often generating huge cliffs as well
20:51 Krock      Wuzzy: in the meantime I also figured that out. Thanks for confirming.
22:50 Wuzzy      I am worried about the security of minetest.deserialize, based on what I learned from #9369
22:50 ShadowBot  https://github.com/minetest/minetest/issues/9369 -- Fix misleading documentation on minetest.deserialize(). by luk3yx
22:51 Wuzzy      (my security concern is independent from that PR. it's because the PR claims that this function may execute Lua code)
22:53 Wuzzy      If I understand correctly, if any user input is thrown into minetest.deserialize, you have an exploit?
22:54 sfan5      before this PR: correct, after this PR: not with mod security enabled
22:55 Wuzzy      ouch!
22:55 sfan5      in in either case not at all if you passed the undocumented (wtf?) safe=true parameter
22:55 sfan5      s/^in/and/
22:56 Wuzzy      but "This function should not be used on untrusted data, regardless of the
22:56 Wuzzy      value of safe."
22:57 sfan5      that mostly precautionary
22:57 sfan5      well
22:57 sfan5      denial of service / resource exhaustion is trivially possible even with safe=true
22:58 Wuzzy      i'm more like thinking about putting stuff like lua functions into a server
22:58 Wuzzy      to introduce cheats etc
22:59 Wuzzy      Why does this function have to be unsafe in the first place? could this (de)serlialization not be done differently? without executing arbitrary code?
23:01 sfan5      depending on the attack vector, the exploiter will have to get creative to introduce cheats that last after a server restart (especially with mod security enabled)
23:02 sfan5      parsing serialized lua tables manually would be lots of extra work and complexity.
23:02 Wuzzy      sure, but it still seems super silly to have an unsafe deserialize
23:03 Wuzzy      hmmm, if all mod security is down, whats the worst possible damage?
23:03 Wuzzy      is lua allowed to arbitrary write+read any random file it can find?
23:04 sfan5      yes?
23:04 sfan5      what do you think mod security is about
23:04 Wuzzy      ?
23:04 Wuzzy      its been a long time since i last touched this thing. i kinda remember now...
23:05 Wuzzy      Any idea for a safe implementation of deserialize?
23:06 sfan5      write a lexer, parser and interpreter for the extremely limited subset of lua minetest.serialize() outputs
23:06 sfan5      like I said, it's lots of extra work
23:07 Wuzzy      rriiiiiiight. that's going to be !!!FUN!!!
23:07 sfan5      the other alternative is to try to validate the string before running it, but it's hard to be sure that that's safe
23:07 Wuzzy      so i suppose loadstring is basically only used because its easy and it works
23:08 rubenwardy Using load string is fine when you sandbox it
23:08 rubenwardy Ie: setfenv
23:09 rubenwardy However, using Lua to serialise is troublesome to begin with, JSON would be preferable
23:09 sfan5      if you consider loadstring("while true do end")() to be "safe" yes
23:09 Wuzzy      hahahahaha
23:10 Wuzzy      looks like loadstring just dumps whatever it loads into the global env? boo!
23:10 sfan5      no
23:10 Wuzzy      Lua 5.3 lets you at least pick environment with `env` param
23:10 sfan5      loadstring returns a function whose body is the code you gave
23:10 Wuzzy      https://www.lua.org/manual/5.3/manual.html#pdf-load
23:10 sfan5      load != loadstring
23:11 rubenwardy You can use coroutines to limit execution time
23:11 rubenwardy But sandboxing is broken on LuaJIT
23:11 Wuzzy      note that loadstring does not exist in 5.3. it has been renamed to load
23:11 p_gimeno   rubenwardy: not if you disable the JIT
23:12 rubenwardy Sounds expensive
23:12 sfan5      oh cool
23:12 sfan5      Wuzzy: in any case you can pick an env in 5.[12] too by using setfenv, exactly what the PR does
23:13 rubenwardy Are there any cases where clients can provide data to deserialise?
23:13 Wuzzy      ah thats what this setfenv does. nice
23:13 rubenwardy Presumably the server doesn't allow clients to change item meta
23:14 Wuzzy      of course. deserialize is so frequently used, it would surprise me if *some* user input is serialized soewhere
23:14 Wuzzy      its a game thing
23:14 Wuzzy      but still
23:14 rubenwardy Having user input being serialised is fine
23:14 Wuzzy      this is unexpected, and a big trap for game+mod makers
23:14 Wuzzy      really?!
23:14 rubenwardy The problem is when user input is deserialised
23:14 Wuzzy      lol
23:14 rubenwardy Like, if the user directly provides the string to deserialise
23:15 sfan5      "do not unserialize arbitrary adversarial input" is not exactly unexpected
23:15 rubenwardy Yeah
23:15 sfan5      the docs should have made it clear though
23:15 Wuzzy      thats what i'm saying
23:15 rubenwardy I can't think of any case where user input would be directly deserialised
23:16 rubenwardy But yes, it should be made clear
23:16 Wuzzy      well it was unexpected to me. I did not know that deserialize is so deep into lua and actually leads to code execution
23:16 rubenwardy I suggest accepting that it can be used to ddos but otherwise sandboxing like in that PR
23:16 Wuzzy      I don't think games will directly let users input a string into deserialize
23:17 Wuzzy      however, it is much more likely that they will dump partial data into it. like  a string or number
23:17 sfan5      what?
23:18 sfan5      again, core.deserialize(core.serialize(input)) will never lead to insecurities
23:18 Wuzzy      how?
23:18 sfan5      what "how"?
23:19 rubenwardy Because any user input is a string, not a function
23:19 Wuzzy      why will it never lead to insecurity?
23:19 Wuzzy      i see
23:19 sfan5      even if it were a function that would not be a problem (also that doesn't work with mod security on)
23:19 rubenwardy The security issue is if you allow  core.deserialize(input)   not if you allow  core.deserialize(core.serialize(input))
23:20 Wuzzy      I still like the suggestion of rubenwardy to switch to a real data format instead of weird lua magic
23:20 rubenwardy Core.parse/write JSON exists
23:20 sfan5      that has some limitations IIRC
23:20 rubenwardy Yeah, because it's JSON
23:20 rubenwardy Wait
23:20 rubenwardy Does it support mixed integer and string keys?
23:21 Wuzzy      what do u mean?
23:21 p_gimeno   I've been researching Lua serialization because of these security issues, and found a couple libraries that can deal with Lua data
23:21 sfan5      rubenwardy: json as a format does not support that
23:21 p_gimeno   but for this case, only one I think
23:21 Wuzzy      actually, the only  stuff that needs serialization is: tables, numbers, strings, userdata. anything else can be ignored imo
23:21 Wuzzy      like functions. pointless to serialize those
23:21 sfan5      >userdata
23:21 sfan5      no
23:22 rubenwardy Well, I discourage using mixed integer and string keys
23:22 Wuzzy      userdata like ItemStack
23:22 rubenwardy ItemStacks should be serialisable
23:22 p_gimeno   https://github.com/gvx/Smallfolk
23:22 Wuzzy      right
23:22 rubenwardy Lol
23:22 Wuzzy      each userdata type should have its own serialization
23:22 sfan5      disagree on that
23:22 sfan5      modders should ensure whatever objects they want to serialize are first converted to strings
23:23 Wuzzy      well ItemStack for startes, but they already have to_string()
23:24 Wuzzy      if we only care about tables, numbers and strings, then JSON has everything for us
23:24 sfan5      not really
23:24 Wuzzy      what''s missing?
23:25 Wuzzy      ok. nil. thats null in json...
23:25 rubenwardy Keys can only be strings
23:25 Wuzzy      oops
23:25 rubenwardy Can't have recursive structures
23:25 Wuzzy      hmmmmm
23:25 sfan5      well, to_json helpfully converts tables that "look" like arrays to actual json arrays
23:26 rubenwardy Can't have structures with duplicate nodes
23:26 sfan5      but e.g. { [-1] = "foo" } or { [1] = "foo" } are impossible in json
23:26 Wuzzy      i cant remember when i ever wanted to save a recursive structure
23:26 rubenwardy Ie: the same table appearing twice
23:26 sfan5      p_gimeno: looks interesting but I think we could invent our own in C++ for more performance
23:26 rubenwardy JSON only supports a tree
23:26 Wuzzy      ah you mean stuff like references?
23:27 rubenwardy Tables are always references
23:27 Wuzzy      yes
23:27 sfan5      Wuzzy: it's extremely rare to want to do that honestly, but it's still something json can't do over core.serialize
23:27 rubenwardy So I mean that the same table could appear twice in the parent table, JSON would duplicate that
23:27 Wuzzy      i think you could do a workaround
23:28 Wuzzy      add some kind of "reference table" in which each table appears. and a "data table" in which the real data is inserted. tables are then refered by their reference
23:29 Wuzzy      not sure if that works, however. its a hack
23:29 sfan5      then it's not JSON anymore and we'd be better off inventing our own format
23:29 Wuzzy      welll it would technically still be json, but with trickery lol
23:29 p_gimeno   IMO deserialize() should be a custom parser that parses a subset of Lua, for compatibility
23:30 p_gimeno   and serialize() should stay as is
23:30 rubenwardy I don't think it's worth it given that having direct user input is rare, and we can limit the issue to ddos rather than full execution
23:30 p_gimeno   I can do that, I've written language parsers
23:31 Wuzzy      hmmmm let me think
23:31 Wuzzy      if the only thing an user can input are strings, could I even do DDoS?
23:31 Wuzzy      ok maybe restrict length to 1000 characters first
23:31 rubenwardy If you can do direct input, then you can do a while loop
23:32 rubenwardy core.deserialize("while true do end")
23:32 Wuzzy      ok a direct input into deserialize is very wrong, i never did that
23:32 Wuzzy      thats not what i meant
23:32 rubenwardy It's safe when you never do a direct input
23:32 sfan5      perhaps validating the input string is not that hard
23:32 Wuzzy      i meant a string that is part of a bigger data structure that is then (de)serializeD)
23:33 rubenwardy That's fine
23:33 sfan5      after ignoring string contents, there should be no parentheses (so no function calls possible) and no language keywords left
23:33 Wuzzy      e.g.
23:33 Wuzzy      t.some_string = get_user_input()
23:33 Wuzzy      minetest.serialize(t)
23:33 rubenwardy That's fine and not effected by this issue
23:33 Wuzzy      minetest.deserialize(previous_input_oops_i_forgot)
23:33 sfan5      except "return" and local" since serialize uses that for references
23:34 rubenwardy The only risk there is the size of the user input  which should probably be limited on the network or something
23:34 Wuzzy      assuming that get_user_input only outputs string
23:34 Wuzzy      I usually use (de)serialize for 2 things:
23:34 sfan5      Wuzzy: get_user_input can quite literally return anything, be it a table, string, int, float, function or _G
23:34 sfan5      <sfan5> again, core.deserialize(core.serialize(input)) will never lead to insecurities
23:35 Wuzzy      1) saving entity data un unload
23:35 Wuzzy      2) saving data to disk
23:35 Wuzzy      however, the 2nd thing can be avoided when using mod data
23:36 Wuzzy      mod storage*
23:36 Wuzzy      entity data is the main use case for me, so you have a neat string for get_staticdata()