Mod security: Accessing mod paths in callbacks is broken

[sfan5]

2016-10-28 22:42:08: ACTION[Emerge-0]: lua_isstring(L, -1) -> false
2016-10-28 22:42:08: ERROR[Main]: ServerError: Lua: Runtime error from mod 'mg' in callback environment_OnGenerated(): Attempt to access external file minetest/bin/../mods/mg/schems/well.we with mod security on.

The -> false refers to this line.

[ShadowNinja]

It seems like this is because mg is accessing a file in its mod dir (which it should be able to do) after init (which it can't do because MT isn't sure what mod is running).
A workaround would be to preload the file in the init stage, or use the insecure API.

[sfan5]

which it can't do because MT isn't sure what mod is running

In the error message right after it seems to be no problem to find out what mod is currently running. ???

A workaround would be to preload the file in the init stage, or use the insecure API.

No, this severely limits mods in what they can do at runtime. Mod security is supposed to stop them from doing bad, not to limit their potential

[ShadowNinja]

In the error message right after it seems to be no problem to find out what mod is currently running.

That's an informational "best guess" value and not secure AFAIK. It's separate from the CURRENT_MOD_NAME field stored in the registry at startup which is intended to be secure.

No, this severely limits mods in what they can do at runtime. Mod security is supposed to stop them from doing bad, not to limit their potential

Yes, I don't like it either. But I don't know if there's anything to be done about it other than using one of those workarounds or disabling mod security when using that mod.

[sfan5]

But I don't know if there's anything to be done about it other than using one of those workarounds or disabling mod security when using that mod.

This is a dumb arbitrary limitation, if this isn't fixed before release we can't leave mod security enabled.

[Amaz1]

Something similar to this also causes this error in this code https://github.com/minetest-LOTR/Lord-of-the-Test/blob/master/mods/lottmapgen/schematics.lua#L34-L42:

2016-11-03 21:52:42: ERROR[Main]: ModError: Failed to load and run script from /home/amaz/.minetest/games/Lord-of-the-Test/mods/lottmapgen/init.lua:
2016-11-03 21:52:42: ERROR[Main]: ...ames/Lord-of-the-Test/mods/lottmapgen/schematics.lua:36: attempt to index local 'file' (a nil value)
2016-11-03 21:52:42: ERROR[Main]: stack traceback:
2016-11-03 21:52:42: ERROR[Main]:   ...ames/Lord-of-the-Test/mods/lottmapgen/schematics.lua:36: in main chunk
2016-11-03 21:52:42: ERROR[Main]:   [C]: in function 'dofile'
2016-11-03 21:52:42: ERROR[Main]:   ...test/games/Lord-of-the-Test/mods/lottmapgen/init.lua:96: in main chunk
2016-11-03 21:52:42: ERROR[Main]: Check debug.txt for details.

The same code works fine with mod security disabled.

(Small discussion on irc about this: http://irc.minetest.net/minetest/2016-11-03#i_4736790)

[ShadowNinja]

@Amaz1: Er, it looks like that runs at init time, so it should work fine. That seems to be a separate issue, in particular it looks like that issue might be caused by #4761, which I just fixed.

[Amaz1]

@ShadowNinja Okay. My specific issue has been fixed, but now I'm getting the same error as is given in the first post.

[ShadowNinja]

I thought of a possible way to fix this.
First, the reason that access to other mod dirs is blocked is because otherwise an untrusted mod could modify a trusted mod and gain trusted access on the next server start.
We could, however, allow read-only access to mod directories. This should block attacks while still not breaking this functionality.
Writing to mod directories isn't an issue since that can be considered a mod bug anyways, since there's no guarantee that they're writable at the filesystem level (and they aren't with system-wide installations).

[est31]

@ShadowNinja I agree with that proposal.

[est31]

Also, there could be a directory in the world dir that is only accessible to trusted mods, where those mods can store secrets, and maybe each mod could contain a directory private which only contains data private to the mod.

[ShadowNinja]

#4849

[paramat]

#4849 merged.

Testing appreciated since we are close to release.