Implementing save data

[grump]

If I understood you right, sending in the 'savedata' blank table will disable access to modules such as 'os' that's needed for the attack that Pgimeno showed?

It assigns an empty environment to the loaded code - no predefined globals are available to the code, and no potentially dangerous functions that one could abuse to modify the caller's state. Only the basic language constructs are usable.

The executed code assigns all values to global variables, which will end up in caller's savedata table. No need to return anything at all.

And the `t` argument to loadfile makes sure the file can't contain bytecode.

[RNavega]

@grump thank you, very interesting. That makes it a great solution.

[pgimeno]

Yeah, it certainly won't protect against hangups. Disabling the string metatable might help mitigating this well-known attack vector:

Code: Select all

x = ("a"):rep(100):match(("a*"):rep(100).."b")

[GVovkiv]

Yeah, it certainly won't protect against hangups. Disabling the string metatable might help mitigating this well-known attack vector:

Code: Select all

x = ("a"):rep(100):match(("a*"):rep(100).."b")

But what to do with infinity loops?

[grump]

You could use pattern matching or debug hooks to mitigate those. Or don't do this at all in scenarios where user generated data is shared between players. Crafting defects in data that crashes a game is always a possibility regardless of the data format, anyway. Does not really matter if it's "repeat until false" in a Lua file or a fake huge length indicator in a binary file - you have to decide if you should care or not in any case.

[pgimeno]

But what to do with infinity loops?

There are debug hooks as grump has mentioned, but they are only reliable if JIT is disabled; for example they don't work on compiled tail calls. So if you want to go that way, you need to disable the string metatable, disable JIT for the loaded chunk, and set up appropriate debug hooks to watch for OOM or infinite loops.

You could use pattern matching or debug hooks to mitigate those.

Lua patterns are a joke, far too powerless for proper tokenization of a Lua file. Imagine something like

Code: Select all

x = ("")["\109\97t\099h"](("")["r"..("")["c\104a\114"](101).."p"]("a",100),("")["r"..("")["c\104a\114"](101).."p"]("a\42",100).."b")

Debug hooks won't help if the infinite loop (actually finite but many years long) is in a C function like string.match().

I agree that in general, it's difficult to protect yourself against a specially crafted save file. But there are probably more attack vectors that I haven't considered beyond OOM and lockups. Don't underestimate the imagination and time spent by those with enough motivation.

For that reason, I advocate the use of non-Lua savefiles. They're much easier to protect in comparison. Granted, the existing serialization libraries are not written with security in mind, so they won't control things such as maximum memory used, therefore you will probably have to edit them to add those checks.

[grump]

For that reason, I advocate the use of non-Lua savefiles.

Yeah me too, in case I wasn't clear on that point. Use Lua for configuration/data files instead of JSON, but not for saving data.

Granted, the existing serialization libraries are not written with security in mind, so they won't control things such as maximum memory used, therefore you will probably have to edit them to add those checks.

It's probably not wise to talk shit about established and successful libraries, but if you have to do that, don't choose bitser. It works, but the code is... unmaintainable.

[pgimeno]

It's probably not wise to talk shit about established and successful libraries, but if you have to do that, don't choose bitser. It works, but the code is... unmaintainable.

To be clear, I mean deserialization. Most libraries generate Lua code and leave deserialization up to the user; them being established and successful does not make them secure. Windows is established and successful and has a tradition of being insecure.

You may not like bitser's code, but it's pretty well written. The ability of saving and restoring self-references is intrinsically complicated, and the author handles it well.

[grump]

You may not like bitser's code, but it's pretty well written. The ability of saving and restoring self-references is intrinsically complicated, and the author handles it well.

It works well enough. It's just not maintainable and impossible to reason about. What was the meaning of 241 again? Why is there some magic number subtracted from some other magic number?

It looks like an entry to a code golfing competition.

[zorg]

bitser (and iirc binser as well) kinda arbitrarily defined its tokens to save data in various forms of compression... there's still the missing library that just uses love.data.un/pack to use C types directly without arbitrary tokens.