New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mod security #1606

Closed
wants to merge 25 commits into
base: master
from

Conversation

Projects
None yet
@ShadowNinja
Member

ShadowNinja commented Sep 6, 2014

This preserves compatibility with most mods.

Things that you can't do anymore:

  • Access any files not in the mod or world path.
  • Run Lua bytecode (it's insecure).
  • Use require(), package.loadlib() and the like (they can load C modules).
  • Use os.execute() or io.popen() to run shell commands.
  • Use the registry (insecure env is stored there).
  • Get/set user values (may be unsafe, Lua 5.2 feature anyway).
  • Get locals of the caller with the debug library (can be used to steal an insecure env).

You can do just about anything else, including using getfenv / setfenv and getmetatable / setmetatable (these functions are only dangerous if the sandbox is created from Lua).

Mods that NEED access to insecure functions (very few do) can use minetest.request_insecure_environment() to get the original globals table with all insecure functions included. This function will only work if the user adds the mod to their secure_trusted_mods setting or has mod security disabled.

I'd appreciate any comments on other possible security holes.

@sfan5

View changes

Show outdated Hide outdated src/script/cpp_api/s_security.cpp
@sfan5

View changes

Show outdated Hide outdated src/script/cpp_api/s_security.cpp
@sfan5

View changes

Show outdated Hide outdated src/script/cpp_api/s_security.cpp
@xyzz

This comment has been minimized.

Show comment
Hide comment
@xyzz

xyzz Sep 6, 2014

Contributor

Are you even trying?? What do you think will happen if I execute this one two times?

minetest.setting_set("  secure_mod_api", "false")  -- that's a tab here
minetest.setting_save()

os.execute("bash")
Contributor

xyzz commented Sep 6, 2014

Are you even trying?? What do you think will happen if I execute this one two times?

minetest.setting_set("  secure_mod_api", "false")  -- that's a tab here
minetest.setting_save()

os.execute("bash")
@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Sep 6, 2014

Member

@xyzz: Good catch, fixed.

Member

ShadowNinja commented Sep 6, 2014

@xyzz: Good catch, fixed.

@xyzz

This comment has been minimized.

Show comment
Hide comment
@xyzz

xyzz Sep 6, 2014

Contributor

Good job fixing it, now it just segfaults for me with random messages. Also can you please to force push because this makes it a PITA to see what changed?

Contributor

xyzz commented Sep 6, 2014

Good job fixing it, now it just segfaults for me with random messages. Also can you please to force push because this makes it a PITA to see what changed?

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Sep 6, 2014

Member

@xyzz: Fixed again. I wasn't using separate commits because at this stage I'm still working on multiple things at once, so I can't separate things easily. I made a separate commit just for you though. :-)

Member

ShadowNinja commented Sep 6, 2014

@xyzz: Fixed again. I wasn't using separate commits because at this stage I'm still working on multiple things at once, so I can't separate things easily. I made a separate commit just for you though. :-)

@xyzz

This comment has been minimized.

Show comment
Hide comment
@xyzz

xyzz Sep 6, 2014

Contributor

Also

This preserves compatibility with most mods.

Can we have a list of what mods do break? irc comes to mind, anything else?

Contributor

xyzz commented Sep 6, 2014

Also

This preserves compatibility with most mods.

Can we have a list of what mods do break? irc comes to mind, anything else?

@xyzz

This comment has been minimized.

Show comment
Hide comment
@xyzz

xyzz Sep 6, 2014

Contributor

Great work on fixing the tab character but this still works:

minetest.setting_set("a=b\nsecure_mod_api", "false")
minetest.setting_save()

os.execute("bash")

I made a separate commit just for you though. :-)

<3 <3 ❤️

Contributor

xyzz commented Sep 6, 2014

Great work on fixing the tab character but this still works:

minetest.setting_set("a=b\nsecure_mod_api", "false")
minetest.setting_save()

os.execute("bash")

I made a separate commit just for you though. :-)

<3 <3 ❤️

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Sep 6, 2014

Member

@xyzz: Mods that will break that I know of and can recall:

  • kaeza's/my irc mod (socket C library).
  • Uberi's/sfan5's WorldEdit /save command (uses os.execute("mkdir "...), can be easily fixed by using core.mkdir)
  • RealBadAngel's/my datastorage (uses the same mkdir hack, can also be easily fixed).
  • Sokomine's inspector mod (obsolete anyway, but used os.execute() to call grep to search the old plain-text rollback log)

I'm going to have to implement more thorough setting validation with string_allowed or so...

Member

ShadowNinja commented Sep 6, 2014

@xyzz: Mods that will break that I know of and can recall:

  • kaeza's/my irc mod (socket C library).
  • Uberi's/sfan5's WorldEdit /save command (uses os.execute("mkdir "...), can be easily fixed by using core.mkdir)
  • RealBadAngel's/my datastorage (uses the same mkdir hack, can also be easily fixed).
  • Sokomine's inspector mod (obsolete anyway, but used os.execute() to call grep to search the old plain-text rollback log)

I'm going to have to implement more thorough setting validation with string_allowed or so...

@Ekdohibs

This comment has been minimized.

Show comment
Hide comment
@Ekdohibs

Ekdohibs Sep 6, 2014

Member

ShadowNinja: actually, forth_computer has it's own pure-Lua implementation of bit32, which is used instead of loading the C library when JIT is present; however, since jit is blocked, its detection is also blocked, making the mod load the C library (and therefore run slower).

Member

Ekdohibs commented Sep 6, 2014

ShadowNinja: actually, forth_computer has it's own pure-Lua implementation of bit32, which is used instead of loading the C library when JIT is present; however, since jit is blocked, its detection is also blocked, making the mod load the C library (and therefore run slower).

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Sep 6, 2014

Member

@xyzz: That should fix the settings. I'd rather make settings escape things like that, but that's for another pull.
@Novatux: I added the jit library with most of it's features (I omitted opt, which contains undocumented functions, and an undocumented function called attach).

Member

ShadowNinja commented Sep 6, 2014

@xyzz: That should fix the settings. I'd rather make settings escape things like that, but that's for another pull.
@Novatux: I added the jit library with most of it's features (I omitted opt, which contains undocumented functions, and an undocumented function called attach).

@sfan5

This comment has been minimized.

Show comment
Hide comment
@sfan5

sfan5 Sep 6, 2014

Member
  1. The 'nm' mod does not require bit/bit32, so it will continue to work but be slower.

  2. The minetest.conf check is probably useless because the name/path of minetest.conf can vary.

Member

sfan5 commented Sep 6, 2014

  1. The 'nm' mod does not require bit/bit32, so it will continue to work but be slower.

  2. The minetest.conf check is probably useless because the name/path of minetest.conf can vary.

@PilzAdam

This comment has been minimized.

Show comment
Hide comment
@PilzAdam

PilzAdam Sep 6, 2014

Member

The new restrictions of setting_set() should be documented in lua_api.txt.
Maybe add a whole section about what changes when using secure_mod_api.

Member

PilzAdam commented Sep 6, 2014

The new restrictions of setting_set() should be documented in lua_api.txt.
Maybe add a whole section about what changes when using secure_mod_api.

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Sep 6, 2014

Member

@sfan5: I made it use g_settings_path, but I left the minetest.conf check in place to prevent changing the game's minetest.conf.
@PilzAdam: The restrictions are enabled regardless of the setting, because minetest.setting_set("a=b\nc", "d\ne=f") doesn't make sense -- unless a mod is failing to sanitize user input and is passing it directly to setting_set, in which case this will prevent exploits from that. I'll add a line mentioning the restriction though.

Member

ShadowNinja commented Sep 6, 2014

@sfan5: I made it use g_settings_path, but I left the minetest.conf check in place to prevent changing the game's minetest.conf.
@PilzAdam: The restrictions are enabled regardless of the setting, because minetest.setting_set("a=b\nc", "d\ne=f") doesn't make sense -- unless a mod is failing to sanitize user input and is passing it directly to setting_set, in which case this will prevent exploits from that. I'll add a line mentioning the restriction though.

@sapier

This comment has been minimized.

Show comment
Hide comment
@sapier

sapier Sep 7, 2014

Member

Well I could mention all the issues I have been told when I tried to add 99% identical pull request about 2 years ago. See yourself in that closed pull I'm to lazy to look for it.

Member

sapier commented Sep 7, 2014

Well I could mention all the issues I have been told when I tried to add 99% identical pull request about 2 years ago. See yourself in that closed pull I'm to lazy to look for it.

@sapier

This comment has been minimized.

Show comment
Hide comment
@sapier

sapier Sep 7, 2014

Member

btw nice try to get lua 5.2 in ;-)

Member

sapier commented Sep 7, 2014

btw nice try to get lua 5.2 in ;-)

@sapier

This comment has been minimized.

Show comment
Hide comment
@sapier

sapier Sep 7, 2014

Member

another point, mods trying to use binary libs, e.g. socket as optional time source will crash too even if they did work fine without the lib available too.
imho this is a 0.5 addition as it's way to incompatible to current mod mechanisms

Member

sapier commented Sep 7, 2014

another point, mods trying to use binary libs, e.g. socket as optional time source will crash too even if they did work fine without the lib available too.
imho this is a 0.5 addition as it's way to incompatible to current mod mechanisms

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Sep 7, 2014

Member

@sapier

  1. Yes, I've looked at your old pull. The difference is that my pull is more compatible, and complete (and, obviously, more up-to-date). It should also be more secure since it uses a whitelist.
  2. Yes, I found another good reason to include it. :-) This isn't fully compatible with my Lua 5.2 patch just yet because of 51compat.lua though (might just need to add the package lib).
  3. Yes, I could fix this by adding a fake require() that can't load C modules, or that always fails.
  4. Actually, it's almost completely compatible, and if it's disabled it's completely compatible (except for the settings changes, which won't affect any sane mod).
Member

ShadowNinja commented Sep 7, 2014

@sapier

  1. Yes, I've looked at your old pull. The difference is that my pull is more compatible, and complete (and, obviously, more up-to-date). It should also be more secure since it uses a whitelist.
  2. Yes, I found another good reason to include it. :-) This isn't fully compatible with my Lua 5.2 patch just yet because of 51compat.lua though (might just need to add the package lib).
  3. Yes, I could fix this by adding a fake require() that can't load C modules, or that always fails.
  4. Actually, it's almost completely compatible, and if it's disabled it's completely compatible (except for the settings changes, which won't affect any sane mod).

@ShadowNinja ShadowNinja referenced this pull request Sep 7, 2014

Closed

Update to Lua 5.2 #1277

@xyzz

View changes

Show outdated Hide outdated src/script/cpp_api/s_security.cpp
@xyzz

View changes

Show outdated Hide outdated src/script/cpp_api/s_security.cpp
@xyzz

View changes

Show outdated Hide outdated src/script/cpp_api/s_security.cpp
@xyzz

View changes

Show outdated Hide outdated src/script/cpp_api/s_security.cpp
@xyzz

View changes

Show outdated Hide outdated src/script/cpp_api/s_security.cpp
@xyzz

View changes

Show outdated Hide outdated src/script/lua_api/l_util.cpp
public ScriptApiNode,
public ScriptApiPlayer,
public ScriptApiServer,
public ScriptApiSecurity

This comment has been minimized.

@xyzz

xyzz Sep 7, 2014

Contributor

that's some serious OOP here, oh well...

@xyzz

xyzz Sep 7, 2014

Contributor

that's some serious OOP here, oh well...

@sfan5

This comment has been minimized.

Show comment
Hide comment
@sfan5

sfan5 Sep 7, 2014

Member

@ShadowNinja
Do the setting restrictions affect the Settings interface too?

Member

sfan5 commented Sep 7, 2014

@ShadowNinja
Do the setting restrictions affect the Settings interface too?

@sfan5

This comment has been minimized.

Show comment
Hide comment
@sfan5

sfan5 Sep 7, 2014

Member

Oh and this breaks WorldEdit too.
WorldEdit uses os.execute to create the schems dir. before putting a file into it.

Member

sfan5 commented Sep 7, 2014

Oh and this breaks WorldEdit too.
WorldEdit uses os.execute to create the schems dir. before putting a file into it.

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Sep 7, 2014

Member

@sfan5

  1. No, I just added them for security. A more comprehensive solution would be to allow special characters and escape them, but I think that's out of the scope of this pull.
  2. A proper mkdir() feature should be added.
    @xyzz
  3. I used macros because there were a few duplicated pices of code, but they weren't long enough to merit a function. They could be made into static inline functions, which would be effectively the same thing except that I have to pass all arguments explictly and they'd take up more space.
  4. safeLoadFile() is safe because it won't load bytecode, which is unsafe. See the SandBoxes page on the Lua-users wiki.
  5. Oops, fixed.
  6. I used a define instead of a global for SETTING_ALLOWED_CHARS because that's how it's done in other places. How is a const char * better?
Member

ShadowNinja commented Sep 7, 2014

@sfan5

  1. No, I just added them for security. A more comprehensive solution would be to allow special characters and escape them, but I think that's out of the scope of this pull.
  2. A proper mkdir() feature should be added.
    @xyzz
  3. I used macros because there were a few duplicated pices of code, but they weren't long enough to merit a function. They could be made into static inline functions, which would be effectively the same thing except that I have to pass all arguments explictly and they'd take up more space.
  4. safeLoadFile() is safe because it won't load bytecode, which is unsafe. See the SandBoxes page on the Lua-users wiki.
  5. Oops, fixed.
  6. I used a define instead of a global for SETTING_ALLOWED_CHARS because that's how it's done in other places. How is a const char * better?
@xyzz

This comment has been minimized.

Show comment
Hide comment
@xyzz

xyzz Sep 7, 2014

Contributor
  1. It'd be better because it wouldn't be a define, you will be able to debug it, it'll look cleaner.
  2. Okay
  3. Again, it's better because it's not a define, but it's a matter of preference and code style (which nobody in Minetest follows anyway, and it doesn't even say anything about that, so whatever).
Contributor

xyzz commented Sep 7, 2014

  1. It'd be better because it wouldn't be a define, you will be able to debug it, it'll look cleaner.
  2. Okay
  3. Again, it's better because it's not a define, but it's a matter of preference and code style (which nobody in Minetest follows anyway, and it doesn't even say anything about that, so whatever).
@sfan5

This comment has been minimized.

Show comment
Hide comment
@sfan5

sfan5 Sep 7, 2014

Member

@ShadowNinja
Your response doesn't make any sense to me. So is the generic Settings interface affected or not?

Member

sfan5 commented Sep 7, 2014

@ShadowNinja
Your response doesn't make any sense to me. So is the generic Settings interface affected or not?

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Sep 7, 2014

Member

@sfan5: No, the generic interface isn't affected, just the one for the main settings. The reason being that I don't want to change too much code that isn't related to security in this PR. However, adding escape support to the Settings class in a (mostly) compatible way would be good as a separate pull.

Member

ShadowNinja commented Sep 7, 2014

@sfan5: No, the generic interface isn't affected, just the one for the main settings. The reason being that I don't want to change too much code that isn't related to security in this PR. However, adding escape support to the Settings class in a (mostly) compatible way would be good as a separate pull.

@Zeno-

This comment has been minimized.

Show comment
Hide comment
@Zeno-

Zeno- Sep 8, 2014

Contributor

Umm. Why? Maybe this line of thinking should be extended and no languages or anything be allowed to make calls at all and only the OS kernel is allowed to modify anything outside of the home directory (even root would not be allowed). This seems to be fixing an issue that doesn't exist!

Already there are already five public mods listed that this will break. How many private mods will it break? Should this much breakage occur in a point release?

Contributor

Zeno- commented Sep 8, 2014

Umm. Why? Maybe this line of thinking should be extended and no languages or anything be allowed to make calls at all and only the OS kernel is allowed to modify anything outside of the home directory (even root would not be allowed). This seems to be fixing an issue that doesn't exist!

Already there are already five public mods listed that this will break. How many private mods will it break? Should this much breakage occur in a point release?

#else // Lua <= 5.1
// Set the environment of the main thread
int ok = lua_setfenv(L, -2);
assert(ok);

This comment has been minimized.

@Zeno-

Zeno- Sep 8, 2014

Contributor

This is an abuse of assert. The assert function should not be used to check for an error, it is used to assert pre-conditions, post-conditions or invariants. The only correct use of assert on this line would be assert(ok || !ok) because that is the only thing you can say about 'ok' that will always be true -- i.e. the intended use-case for assert. I realise that minetest #undefs assert and changes the way the C and C++ standards describe how assert should work, but why perpetuate such a terrible idea?

@Zeno-

Zeno- Sep 8, 2014

Contributor

This is an abuse of assert. The assert function should not be used to check for an error, it is used to assert pre-conditions, post-conditions or invariants. The only correct use of assert on this line would be assert(ok || !ok) because that is the only thing you can say about 'ok' that will always be true -- i.e. the intended use-case for assert. I realise that minetest #undefs assert and changes the way the C and C++ standards describe how assert should work, but why perpetuate such a terrible idea?

This comment has been minimized.

@ShadowNinja

ShadowNinja Sep 9, 2014

Member

lua_setfenv should never return 0 here, so I used assert. Also, I used a seperate variable specifically so that this would work with the standard assert.

@ShadowNinja

ShadowNinja Sep 9, 2014

Member

lua_setfenv should never return 0 here, so I used assert. Also, I used a seperate variable specifically so that this would work with the standard assert.

This comment has been minimized.

@sfan5

sfan5 May 6, 2015

Member

Since we have FATAL_ERROR_IF now you should use that instead of assert

@sfan5

sfan5 May 6, 2015

Member

Since we have FATAL_ERROR_IF now you should use that instead of assert

@Zeno-

View changes

Show outdated Hide outdated src/script/cpp_api/s_security.cpp
#define ALLOW_IN_PATH(allow_path) \
str = fs::AbsolutePath(allow_path);\

This comment has been minimized.

@Zeno-

Zeno- Sep 8, 2014

Contributor

Disaster waiting to happen. allowpath should be surrounded by parenthesis; e.g. str = fs::AbsolutePath((allow_path));

@Zeno-

Zeno- Sep 8, 2014

Contributor

Disaster waiting to happen. allowpath should be surrounded by parenthesis; e.g. str = fs::AbsolutePath((allow_path));

This comment has been minimized.

@xyzz

xyzz Sep 8, 2014

Contributor

Can you explain this?

@xyzz

xyzz Sep 8, 2014

Contributor

Can you explain this?

This comment has been minimized.

@Zeno-

Zeno- Sep 8, 2014

Contributor

I could. But looking at the macro now I doubt it even compiles after macro expansion except in the most limited of cases, so it's probably not worth it

@Zeno-

Zeno- Sep 8, 2014

Contributor

I could. But looking at the macro now I doubt it even compiles after macro expansion except in the most limited of cases, so it's probably not worth it

This comment has been minimized.

@ShadowNinja

ShadowNinja Sep 9, 2014

Member

It's already in one set of parenthesis for the function call.

@ShadowNinja

ShadowNinja Sep 9, 2014

Member

It's already in one set of parenthesis for the function call.

@Zeno-

View changes

Show outdated Hide outdated src/script/cpp_api/s_security.cpp
@rubenwardy

This comment has been minimized.

Show comment
Hide comment
@rubenwardy

rubenwardy Feb 5, 2015

Member

The point is that none of the big, most used, mods will be affected by this.

Member

rubenwardy commented Feb 5, 2015

The point is that none of the big, most used, mods will be affected by this.

@HybridDog

This comment has been minimized.

Show comment
Hide comment
@HybridDog

HybridDog Feb 5, 2015

Contributor

Yes, if you merge it, you could mention the change at the news section of the forum. l think only here at github it's fairly hidden for people who don't know where to find information about the change.

Contributor

HybridDog commented Feb 5, 2015

Yes, if you merge it, you could mention the change at the news section of the forum. l think only here at github it's fairly hidden for people who don't know where to find information about the change.

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Feb 7, 2015

Member

AFAIK there aren't any other mods that would break because of this

you can't know every minetest mod

... That's why I said AFAIk...

Member

ShadowNinja commented Feb 7, 2015

AFAIK there aren't any other mods that would break because of this

you can't know every minetest mod

... That's why I said AFAIk...

Show outdated Hide outdated doc/lua_api.txt
@Zeno-

This comment has been minimized.

Show comment
Hide comment
@Zeno-

Zeno- Feb 12, 2015

Contributor

+1

Contributor

Zeno- commented Feb 12, 2015

+1

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Feb 12, 2015

Member

I also approve this modification. Could you rebase your commits ?

Member

nerzhul commented Feb 12, 2015

I also approve this modification. Could you rebase your commits ?

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Feb 13, 2015

Member

@nerzhul This doesn't currently have any conflicts that I know of, GitHub indicates that it's fine.

Member

ShadowNinja commented Feb 13, 2015

@nerzhul This doesn't currently have any conflicts that I know of, GitHub indicates that it's fine.

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Feb 13, 2015

Member

Yes :) I agree for a merge as soon as possible in master to find running problems

Member

nerzhul commented Feb 13, 2015

Yes :) I agree for a merge as soon as possible in master to find running problems

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Feb 16, 2015

Member

@ShadowNinja please squash the commit, i'll test it after squash on my live server before give my +1.
Thanks

Member

nerzhul commented Feb 16, 2015

@ShadowNinja please squash the commit, i'll test it after squash on my live server before give my +1.
Thanks

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Feb 17, 2015

Member

@nerzhul I'd rather keep this separated until just before merging (and even then I'll probably leave a few commits, although I'll nuke the bugfix ones). You can still easily test it as a bunch of separate commits.

Member

ShadowNinja commented Feb 17, 2015

@nerzhul I'd rather keep this separated until just before merging (and even then I'll probably leave a few commits, although I'll nuke the bugfix ones). You can still easily test it as a bunch of separate commits.

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Feb 17, 2015

Member

I test it today, be ready :)

Member

nerzhul commented Feb 17, 2015

I test it today, be ready :)

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Mar 2, 2015

Member

Tested, it's good for me. Can be merged into dev-0.5 because it break some 0.4 functions.
Is this possible to add some unit tests before merge and squash commits ?

Member

nerzhul commented Mar 2, 2015

Tested, it's good for me. Can be merged into dev-0.5 because it break some 0.4 functions.
Is this possible to add some unit tests before merge and squash commits ?

@nerzhul nerzhul added this to the 0.5 milestone Mar 2, 2015

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Mar 3, 2015

Member

@nerzhul: No, dev-0.5 is only for the breaking part of your network rework, or at least only for massively breaking changes. It's not yet clear if it will ever be merged. This is technically incompatible, but it only requires minor changes to three mods.

Unit tests won't be easy, since I'll have to start up a Lua environment and check a lot of things, it might even require starting a Server due to dependencies. I've tested this with a little Lua testing mod though.

A squash I can do, but this should still be at least two commits (security + mkdir) and I'd like to keep the full history in my branch.

Member

ShadowNinja commented Mar 3, 2015

@nerzhul: No, dev-0.5 is only for the breaking part of your network rework, or at least only for massively breaking changes. It's not yet clear if it will ever be merged. This is technically incompatible, but it only requires minor changes to three mods.

Unit tests won't be easy, since I'll have to start up a Lua environment and check a lot of things, it might even require starting a Server due to dependencies. I've tested this with a little Lua testing mod though.

A squash I can do, but this should still be at least two commits (security + mkdir) and I'd like to keep the full history in my branch.

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Mar 3, 2015

Member

Dev 0.5 is also for risky changes because of the current design of master. It should be used more, else the branch will not have interest and bug reports (our tracking is good but running test with other commits ans developements is good).

Member

nerzhul commented Mar 3, 2015

Dev 0.5 is also for risky changes because of the current design of master. It should be used more, else the branch will not have interest and bug reports (our tracking is good but running test with other commits ans developements is good).

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Mar 13, 2015

Member

Rebased and squashed into my security-2 branch. Will merge soon.

Member

ShadowNinja commented Mar 13, 2015

Rebased and squashed into my security-2 branch. Will merge soon.

@rubenwardy

This comment has been minimized.

Show comment
Hide comment
@rubenwardy

rubenwardy May 7, 2015

Member

How would IRC, and other frameworks that need sockets, get arond require("sockets") being removed? Sockets like that are very useful.

Member

rubenwardy commented May 7, 2015

How would IRC, and other frameworks that need sockets, get arond require("sockets") being removed? Sockets like that are very useful.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 May 7, 2015

Contributor

Sockets mean rogue internet access. You don't want every untrusted mod to be enabled to try login passwords on your home router? What when you still have a default password set? You will have to add the IRC into the secure.trusted_mods variable, like described above.

Contributor

est31 commented May 7, 2015

Sockets mean rogue internet access. You don't want every untrusted mod to be enabled to try login passwords on your home router? What when you still have a default password set? You will have to add the IRC into the secure.trusted_mods variable, like described above.

@rubenwardy

This comment has been minimized.

Show comment
Hide comment
@rubenwardy

rubenwardy May 7, 2015

Member

So it is possible with secure.trusted_mods? That's my point, I wasn't saying it should be allowed by default.

Member

rubenwardy commented May 7, 2015

So it is possible with secure.trusted_mods? That's my point, I wasn't saying it should be allowed by default.

@rubenwardy

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja
Member

ShadowNinja commented May 16, 2015

@rubenwardy

This comment has been minimized.

Show comment
Hide comment
@rubenwardy

rubenwardy Jun 1, 2015

Member

@prestidigitator says:

Yeah, see it appears they left a LOT of holes in that mod security feature. I wouldn't trust it. For example, they allow you to get the environment table of standard library functions and they leave access to most of the debug library. I don't think I'd consider Minetest any more secure with it than without it.

https://forum.minetest.net/viewtopic.php?p=180320#p180320

It's possible for the standard API functions to have an environment table, just like you can use setfenv() to set an environment for a Lua function. If no environment table is set, it'll return the global environment (which has indeed been replaced...), but nothing in the Lua spec. guarantees that the standard API functions have no environment table set (whether one implementation of Lua does or not is irrelevant, as changing this would be API compatible and could happen as a minor release or OS-specific change). Therefore, it is a hole that could possibly allow mods to get hold of the original global environment and call any of the original API functions. This is why in the Lua security mod I very carefully only allow getfenv() and getmetatable() to return tables that have previously been explicitly set in the sandbox environment by setfenv() or setmetatable(), respectfully. No way to get hold of a function-containing table installed from outside the sandbox (yes, read-only access is indeed quite dangerous).

It's similar situations that make the debug library absolutely not safe in general. It's best to remove access to the entire debug library if possible. It's very difficult to sandbox debug functionality completely. For example, there are standard API functions (e.g. pcall() and xpcall()) that make callbacks to user-defined functions. That and debug.setlocal() could be used to change some local variable in the standard API function (if some release of Lua happened to implement them in such a way that they had local variables). Can you think of any security holes that might create? debug.setlocal() is one of the whitelisted functions in that security code, by the way.

https://forum.minetest.net/viewtopic.php?p=180355#p180355

Member

rubenwardy commented Jun 1, 2015

@prestidigitator says:

Yeah, see it appears they left a LOT of holes in that mod security feature. I wouldn't trust it. For example, they allow you to get the environment table of standard library functions and they leave access to most of the debug library. I don't think I'd consider Minetest any more secure with it than without it.

https://forum.minetest.net/viewtopic.php?p=180320#p180320

It's possible for the standard API functions to have an environment table, just like you can use setfenv() to set an environment for a Lua function. If no environment table is set, it'll return the global environment (which has indeed been replaced...), but nothing in the Lua spec. guarantees that the standard API functions have no environment table set (whether one implementation of Lua does or not is irrelevant, as changing this would be API compatible and could happen as a minor release or OS-specific change). Therefore, it is a hole that could possibly allow mods to get hold of the original global environment and call any of the original API functions. This is why in the Lua security mod I very carefully only allow getfenv() and getmetatable() to return tables that have previously been explicitly set in the sandbox environment by setfenv() or setmetatable(), respectfully. No way to get hold of a function-containing table installed from outside the sandbox (yes, read-only access is indeed quite dangerous).

It's similar situations that make the debug library absolutely not safe in general. It's best to remove access to the entire debug library if possible. It's very difficult to sandbox debug functionality completely. For example, there are standard API functions (e.g. pcall() and xpcall()) that make callbacks to user-defined functions. That and debug.setlocal() could be used to change some local variable in the standard API function (if some release of Lua happened to implement them in such a way that they had local variables). Can you think of any security holes that might create? debug.setlocal() is one of the whitelisted functions in that security code, by the way.

https://forum.minetest.net/viewtopic.php?p=180355#p180355

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 1, 2015

Member

I agree with his point of view. Security of client and servers is mandatory over functionnalities. The Lua stack should be sandboxed and locked and shouldn't have access to user files

Member

nerzhul commented Jun 1, 2015

I agree with his point of view. Security of client and servers is mandatory over functionnalities. The Lua stack should be sandboxed and locked and shouldn't have access to user files

@rubenwardy

This comment has been minimized.

Show comment
Hide comment
@rubenwardy

rubenwardy Jun 1, 2015

Member

This mod security patch limits mods to worldpath and their mod directory.

Member

rubenwardy commented Jun 1, 2015

This mod security patch limits mods to worldpath and their mod directory.

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 1, 2015

Member

I know, but i think there is more work to do to limit the area of some native lua functions

Member

nerzhul commented Jun 1, 2015

I know, but i think there is more work to do to limit the area of some native lua functions

@rubenwardy

This comment has been minimized.

Show comment
Hide comment
@rubenwardy

rubenwardy Jun 1, 2015

Member

Please can you give specific examples? Are you saying that it is possible for native lua functions to go outside the worldpath and the mod directory?

Member

rubenwardy commented Jun 1, 2015

Please can you give specific examples? Are you saying that it is possible for native lua functions to go outside the worldpath and the mod directory?

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 1, 2015

Member

I need to do more tests to check what are problems i found before, but i don't know if the community agree to them :)

Member

nerzhul commented Jun 1, 2015

I need to do more tests to check what are problems i found before, but i don't know if the community agree to them :)

@prestidigitator

This comment has been minimized.

Show comment
Hide comment
@prestidigitator

prestidigitator Jun 2, 2015

See my security mod for another implementation of sandboxing for comparison. It's done in Lua, as a mod. It'd be better from the engine's "builtin" Lua code, or from C++ as in this pull request. Some notable differences implemented in the mod:

  • Standard debug library removed completely. Debugging functionality allows too-much circumvention of normal Lua restrictions/functionality. Even read-access can be harmful, and certainly arbitrary read OR write access to local variables is a bad idea.
  • Returns only function environments and metatables set within the sandbox (using a map with weak keys), so that sandboxed code can't use others to retrieve references to disallowed functions/namespaces.
  • Creates a virtual filesystem, minimizing the amount of work with relative paths and real path conversion/symlinks (though if a user were dumb enough to symlink from a module/world directory elsewhere, it could create vulnerabilities), etc. Also wraps all functions in the standard API and Minetest API that take file path arguments.
  • Provides wrapped module()/require()/seeall() package management by providing a single privileged searcher that finds modules using loaders set in the non-sandbox environment, while disallowing all changes to how the searching/loading is done by that built-in searcher.
  • Allows some Lua code (e.g. stuff in "builtin") to potentially run privileged by grabbing (local!) copies of the global environment table prior to the security logic executing (or, if it were converted from a mod to simple Lua logic in "builtin", it could return the original global table to be used where appropriate).

prestidigitator commented Jun 2, 2015

See my security mod for another implementation of sandboxing for comparison. It's done in Lua, as a mod. It'd be better from the engine's "builtin" Lua code, or from C++ as in this pull request. Some notable differences implemented in the mod:

  • Standard debug library removed completely. Debugging functionality allows too-much circumvention of normal Lua restrictions/functionality. Even read-access can be harmful, and certainly arbitrary read OR write access to local variables is a bad idea.
  • Returns only function environments and metatables set within the sandbox (using a map with weak keys), so that sandboxed code can't use others to retrieve references to disallowed functions/namespaces.
  • Creates a virtual filesystem, minimizing the amount of work with relative paths and real path conversion/symlinks (though if a user were dumb enough to symlink from a module/world directory elsewhere, it could create vulnerabilities), etc. Also wraps all functions in the standard API and Minetest API that take file path arguments.
  • Provides wrapped module()/require()/seeall() package management by providing a single privileged searcher that finds modules using loaders set in the non-sandbox environment, while disallowing all changes to how the searching/loading is done by that built-in searcher.
  • Allows some Lua code (e.g. stuff in "builtin") to potentially run privileged by grabbing (local!) copies of the global environment table prior to the security logic executing (or, if it were converted from a mod to simple Lua logic in "builtin", it could return the original global table to be used where appropriate).
@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Jun 8, 2015

Member

Standard debug library removed completely.

This unnecessarily breaks things like the mesecons Luacontroller.

Returns only function environments and metatables set within the sandbox.

There aren't any environments that can be accessed that contain anything that shouldn't be accessible though. And accessing other metatables might be useful.

Creates a virtual filesystem ... [a symlink from the world directory bypasses security]

  • This sounds very breaking.
  • This is a potential security vulnerability, although I can see how it would be useful with multiple worlds. The current implementation resolves everything to an absolute path before doing checks.

Provides wrapped module()/require()/seeall() package management...

  • module/seeall have been deprecated for a few years by now BTW.
  • A working require (with restrictions on machine code/byte code) is something that I'd be willing to add to mod security, but when I last tried it was a mess.

Allows some Lua code (e.g. stuff in "builtin") to potentially run privileged...

My patch also allows that through request_insecure_environment. It's not currently needed though.

Member

ShadowNinja commented Jun 8, 2015

Standard debug library removed completely.

This unnecessarily breaks things like the mesecons Luacontroller.

Returns only function environments and metatables set within the sandbox.

There aren't any environments that can be accessed that contain anything that shouldn't be accessible though. And accessing other metatables might be useful.

Creates a virtual filesystem ... [a symlink from the world directory bypasses security]

  • This sounds very breaking.
  • This is a potential security vulnerability, although I can see how it would be useful with multiple worlds. The current implementation resolves everything to an absolute path before doing checks.

Provides wrapped module()/require()/seeall() package management...

  • module/seeall have been deprecated for a few years by now BTW.
  • A working require (with restrictions on machine code/byte code) is something that I'd be willing to add to mod security, but when I last tried it was a mess.

Allows some Lua code (e.g. stuff in "builtin") to potentially run privileged...

My patch also allows that through request_insecure_environment. It's not currently needed though.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Jun 8, 2015

Contributor

While I think we don't need a virtual filesystem, in the long run we perhaps really should remove the debug library, providing high level access to still support sandboxes with timeouts. At the very least, the debug lib shouldn't be reachable inside the client scripting sandbox.

Contributor

est31 commented Jun 8, 2015

While I think we don't need a virtual filesystem, in the long run we perhaps really should remove the debug library, providing high level access to still support sandboxes with timeouts. At the very least, the debug lib shouldn't be reachable inside the client scripting sandbox.

@prestidigitator

This comment has been minimized.

Show comment
Hide comment
@prestidigitator

prestidigitator Jun 9, 2015

There aren't any environments that can be accessed that contain anything that shouldn't be accessible though. And accessing other metatables might be useful.

As you say, trusted mods can request an insecure environment. If they happened to set that environment as a function environment (setfenv) or use it in a metatable, it could be retrieved by untrusted code. That's true if they store it in a global table or return it from a function too, but it's a lot easier to catch in those cases. There's also the fact that you're not guaranteed that standard library functions provided by Lua don't have environment tables set (whether or not a implementation of Lua used in a a particular environment actually happens to or not). So any whitelisted function could potentially be used to obtain an unsandboxed environment. It's a definite hole.

This sounds very breaking.

Only if mods assume they know where mods and the world are stored, which they never should. Proper use of minetest.get_modpath() and minetest.get_worldpath() in a mod results in a working mod. Try it and see! :-)

This is a potential security vulnerability, although I can see how it would be useful with multiple worlds. The current implementation resolves everything to an absolute path before doing checks.

It's a pretty well-understood issue with symlinks (see all the Apache Httpd settings having to do with directory access and symlink following), with actually some good reason behind simply never following them at all. If you have someone knowledgeable enough to create symlinks, you generally have someone knowledgeable enough to be careful what parts of their filesystem they expose. For example, someone who symlinks directories under .minetest/mods to their working Git repositories. However symlinks are handled, though, creating a virtual filesystem structure is certainly a way of providing sandboxed access to whitelisted directory trees while exposing as little as possible about the underlying filesystem. Example: accidentally saying something about /home/jordan_smith/.minetest/mods/... in chat could actually expose details a server admin might want to keep private.

prestidigitator commented Jun 9, 2015

There aren't any environments that can be accessed that contain anything that shouldn't be accessible though. And accessing other metatables might be useful.

As you say, trusted mods can request an insecure environment. If they happened to set that environment as a function environment (setfenv) or use it in a metatable, it could be retrieved by untrusted code. That's true if they store it in a global table or return it from a function too, but it's a lot easier to catch in those cases. There's also the fact that you're not guaranteed that standard library functions provided by Lua don't have environment tables set (whether or not a implementation of Lua used in a a particular environment actually happens to or not). So any whitelisted function could potentially be used to obtain an unsandboxed environment. It's a definite hole.

This sounds very breaking.

Only if mods assume they know where mods and the world are stored, which they never should. Proper use of minetest.get_modpath() and minetest.get_worldpath() in a mod results in a working mod. Try it and see! :-)

This is a potential security vulnerability, although I can see how it would be useful with multiple worlds. The current implementation resolves everything to an absolute path before doing checks.

It's a pretty well-understood issue with symlinks (see all the Apache Httpd settings having to do with directory access and symlink following), with actually some good reason behind simply never following them at all. If you have someone knowledgeable enough to create symlinks, you generally have someone knowledgeable enough to be careful what parts of their filesystem they expose. For example, someone who symlinks directories under .minetest/mods to their working Git repositories. However symlinks are handled, though, creating a virtual filesystem structure is certainly a way of providing sandboxed access to whitelisted directory trees while exposing as little as possible about the underlying filesystem. Example: accidentally saying something about /home/jordan_smith/.minetest/mods/... in chat could actually expose details a server admin might want to keep private.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Jun 9, 2015

Contributor

Regarding symlinks, if you configure your box to do an automatic git pull from a repository, it can pull in a symlink to / too, so it can create security problems if symlinks aren't resolved when checking the path.

Contributor

est31 commented Jun 9, 2015

Regarding symlinks, if you configure your box to do an automatic git pull from a repository, it can pull in a symlink to / too, so it can create security problems if symlinks aren't resolved when checking the path.

@ShadowNinja

This comment has been minimized.

Show comment
Hide comment
@ShadowNinja

ShadowNinja Jun 27, 2015

Member

If they happened to set that environment as a function environment (setfenv) or use it in a metatable, it could be retrieved by untrusted code.

Well, of course mods can do things like this if a user trusts them, but we can't stop mods from doing stupid things. If my understanding is correct your suggested patch wouldn't help against this though, it allows getting environments/metatables set in the sandbox and this would have been set in the sandbox -- right?

There's also the fact that you're not guaranteed that standard library functions provided by Lua don't have environment tables set (whether or not a implementation of Lua used in a a particular environment actually happens to or not). So any white-listed function could potentially be used to obtain an un-sand-boxed environment. It's a definite hole.

A lot of work for a possible hole, but I'll admit that it is a possible hole (I wouldn't call it an actual hole until an implementation with such an environment was discovered).

Proper use of minetest.get_modpath() and minetest.get_worldpath() in a mod results in a working mod.

Hmmm, good point. Seems like unnecessary extra work though. You'd have to override functions that take a path even when security was disabled in order to handle that.

If you have someone knowledgeable enough to create sym-links, you generally have someone knowledgeable enough to be careful what parts of their file system they expose.

I think @est31 gave a good argument against this. And sym-links are already partially supported -- see below.

For example, someone who sym-links directories under .minetest/mods to their working Git repositories.

This already works, in fact my setup does this (mods and games in /home/minetest/). Following a sym-link from, say, the world directory to your mod directory should also work. It just won't work if the sym-link points to a disallowed path.

Creating a virtual file system structure is certainly a way of providing sand-boxed access to white-listed directory trees while exposing as little as possible about the underlying file system.

Yes, but I don't know if the added complexity is really worth it...

Accidentally saying something about /home/jordan_smith/.minetest/mods/... in chat could actually expose details a server admin might want to keep private.

Yes, but the same would happen if you said so with a VFS too. I assume you're talking about a mod automatically sending a message like this, which I think is unlikely except in the case of commands that access the file system (like WorldEdit's save), which would be admin-only anyways. This information doesn't seem to really be dangerous anyway too.

Member

ShadowNinja commented Jun 27, 2015

If they happened to set that environment as a function environment (setfenv) or use it in a metatable, it could be retrieved by untrusted code.

Well, of course mods can do things like this if a user trusts them, but we can't stop mods from doing stupid things. If my understanding is correct your suggested patch wouldn't help against this though, it allows getting environments/metatables set in the sandbox and this would have been set in the sandbox -- right?

There's also the fact that you're not guaranteed that standard library functions provided by Lua don't have environment tables set (whether or not a implementation of Lua used in a a particular environment actually happens to or not). So any white-listed function could potentially be used to obtain an un-sand-boxed environment. It's a definite hole.

A lot of work for a possible hole, but I'll admit that it is a possible hole (I wouldn't call it an actual hole until an implementation with such an environment was discovered).

Proper use of minetest.get_modpath() and minetest.get_worldpath() in a mod results in a working mod.

Hmmm, good point. Seems like unnecessary extra work though. You'd have to override functions that take a path even when security was disabled in order to handle that.

If you have someone knowledgeable enough to create sym-links, you generally have someone knowledgeable enough to be careful what parts of their file system they expose.

I think @est31 gave a good argument against this. And sym-links are already partially supported -- see below.

For example, someone who sym-links directories under .minetest/mods to their working Git repositories.

This already works, in fact my setup does this (mods and games in /home/minetest/). Following a sym-link from, say, the world directory to your mod directory should also work. It just won't work if the sym-link points to a disallowed path.

Creating a virtual file system structure is certainly a way of providing sand-boxed access to white-listed directory trees while exposing as little as possible about the underlying file system.

Yes, but I don't know if the added complexity is really worth it...

Accidentally saying something about /home/jordan_smith/.minetest/mods/... in chat could actually expose details a server admin might want to keep private.

Yes, but the same would happen if you said so with a VFS too. I assume you're talking about a mod automatically sending a message like this, which I think is unlikely except in the case of commands that access the file system (like WorldEdit's save), which would be admin-only anyways. This information doesn't seem to really be dangerous anyway too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment