Make debug.g/setmetatable respect the "__metatable_debug" metatable field

[Desour]

• Goal of the PR: Improvement of the SSM sandbox security.
• How does the PR work:
  • debug.getmetatable and debug.setmetatable are overwritten in builtin to use the "__metatable_debug" field like getmetatable and setmetatable use the "__metatable" field.
  • Userdata metatables have now the same value for "__metatable_debug" as for "__metatable". This prevents mods from doing nasty stuff like overwriting or replacing "__gc" metatable fields.
    (The "__metatable" fields had basically no effect.)
  • (We still have to leave debug.g/setmetatable in the sandbox because we want to allow to overwrite the string metatable.)
• Does it resolve any reported issue: Idk.
• If not a bug fix, why is this PR needed? What usecases does it solve? It can be considered a (security-related) bugfix.

To do

This PR is a Ready for Review.

How to test

• Check that your mods still work.
• Read the code.
• Read the doc changes.

[Desour]

Now they are not overwritten iff mod security is off. In that case, _G is the insecure env, and we don't want to restrict it.

[rubenwardy]

I find this change highly questionable. Shouldn't we just remove these functions from the sandbox if there's concern over security?

[Desour]

I didn't know how often these functions are used. IIRC there was a discussion in irc about this, where someone (you?) said that they're (almost?) nowhere used. (Ie. mesecons just uses the normal getmetatable on strings.) Hence, I'm fine with completely removing them. I kept this PR open as a reminder that the issue still exists. Also IIRC there was someone who wanted the functions to stay to do some weird stuff with functions.

[sfan5]

To properly discuss this it'd be useful if someone (@appgurueu?) could sum up on what types [gs]etmetable and debug\.[gs]etmetatable work.

[appgurueu]

To properly discuss this it'd be useful if someone (@appgurueu?) could sum up on what types [gs]etmetable and debug\.[gs]etmetatable work.

setmetatable only works on tables and respects the __metatable field, erroring if called on tables that have it set in their metatable. getmetatable also works on other types (strings, for example). It is possible to retrieve metatables set via debug.setmetatable using just getmetatable. getmetatable respects the __metatable field and returns its value instead if set, whereas debug.getmetatable and debug.setmetatable do not respect this field, operating on the raw metatable instead. The debug.* functions also work on pretty much all types and allow replacing/changing/setting string, number, boolean, nil metatables as well as function (for all functions). Full userdata, like tables, has individual metatables per instance, which can be set/get using the debug metatable funcs.

Example:

> t = setmetatable({}, {__metatable = 42})
> =getmetatable(t)
42
> =debug.getmetatable(t)
table: 0x55d51c5c4060
> setmetatable(t, {})
stdin:1: cannot change a protected metatable
stack traceback:
	[C]: in function 'setmetatable'
	stdin:1: in main chunk
	[C]: ?
> debug.setmetatable(t, {})

Further resources: pgimeno's metatable tutorial and the Lua reference manual

[appgurueu]

Why not respect the __metatable field?

[Desour]

To give more control. And theoretically there could be a mod that does some hacky, but valid, stuff with another mod's object.

If we want to keep debug.setmetatable just for non-table objects and nothing else, using __metatable might be better.

[Desour]

Underlying issue (#12216) was fixed, closing.