Mod security: Allow read-only access to all mod paths #4849

ShadowNinja · 2016-12-05T23:04:33Z

This allows mods to read from their mod dir after init time. However, there isn't a reliable "current mod" after init time, so this access has to be granted for all mods.
Fixes #4692.

sofar · 2016-12-06T00:51:55Z

Euh, this allows any mod to read any file in any mod folder? Is that right?

ShadowNinja · 2016-12-06T02:06:01Z

@sofar: Correct. After init time (what this primarily fixes) there is no definitive "current mod", so it can't be limited by mod.

sofar · 2016-12-06T05:30:20Z

Ok, I just wanted to make sure that that was the case, before I start explaining that granting explicit read permission to all other mod content is a gross security issue.

If we really must have a "mod X must be able to read files in mod B" type of thing, can we please have an API function call that mod B can use to declare mod A is allowed to read my files (or, any mod is, if you want to simplify it)?

Maybe a mod.conf setting can provide this setting, so that lua can't even change this bit.at runtime.

ghost · 2016-12-06T12:00:58Z

Granting all mods to directly read all other mod's files is by far the worst idea I've read so far in this issue tracker. Heavy dislike from me.

sfan5 · 2016-12-06T12:51:33Z

Can anyone enlighten me why allowing this is a security issue?

@sofar also having mods explicitly grant permission to read their files is not possible (apparently) because of the limitation @ShadowNinja mentioned already:

After init time (what this primarily fixes) there is no definitive "current mod", so it can't be limited by mod.

nerzhul · 2016-12-06T13:10:14Z

Sorry but no. mod is code, if you want a shared area for mods create /shared/ somewhere to put shared things between mods

ghost · 2016-12-06T16:42:14Z

if you want a shared area for mods create /shared/ somewhere to put shared things between mods

If mod creators wanted their code (or functions/variables/etc.) to be accessed they would create a properly prefixed global table for that.

sofar · 2016-12-06T16:44:22Z

@sfan5 it could be possible if during initialization (not after) there is a way to declare that a mod's files are accessible for reading, for instance if the module folder has a mod.conf file that has a line allow_global_mods_read = true...

That way it's not limited "by mod", it's only limited "to what it accesses", which avoids the stated problem.

edit: even better would be a specific path, that can be (relative) inside the mod:

allow_global_read_access = "/schematics/"

would e.g. grant read access to `mods/modname/schematics/

sfan5 · 2016-12-06T17:12:39Z

@nerzhul mods are more than just their code: schematic, translation files

@sofar that'd work but you didn't answer why it is a security issue

sofar · 2016-12-06T17:33:11Z

@sfan5 e.g a mod could read all of datastorageand get knowledge about stuff other players have hidden, player attributes or misc content. A mod also could attempt to steal proprietary lua mod code from private servers and disclose it to players.

Example: a bug in the protection mod allows a player to duplicate a command block. Without permissions, the player can now grant himself any item, or manipulate protected areas. But if he can read mod code, he can get a hold of copyrighted code that is not open source, or API tokens that a developer has put into code so that his IRC mod can interface with an IRC daemon. Oops.

sfan5 · 2016-12-06T17:36:56Z

Storing configuration in mod directories seems like a valid problem.
I'm not sure what you mean by datastorage, player-related files should not be stored in the mod directories.

sofar · 2016-12-06T17:44:38Z

@sfan5 the alternative is to hack an invisible inventory like hud does to store hunger values, which is terrible.

sofar · 2016-12-06T17:45:21Z

Offtopic: having a proper player metadata storage API would remove half the problem.

ShadowNinja · 2016-12-07T04:15:23Z

Sorry but no. mod is code, if you want a shared area for mods create /shared/ somewhere to put shared things between mods.

This isn't about sharing mod files. I really only want to add read access to mod directories after init time, but after init time I don't have a reliable mod name, so it has to be available to all mods. This seems to be the basis of a lot of misunderstanding in this thread. I titled the patch that read access was added to all mods, because that's what actually changes, but the main intent is just adding access after init time for mods, and then limiting it to read-only access since it has to be for all mods due to limitations of the API.

This could be limited by a setting in the config, although it would have to be named something like allow_global_read_after_init to be sufficiently descriptive.

Rant i just wrote about mod security.

e.g a mod could read all of datastorage...

datastorage uses the world dir. All mods can already read (and write) to datastorage files.

A mod also could attempt to steal proprietary Lua mod code from private servers and disclose it to players.

Eh... I'm not too concerned about this.

Example: a bug in the protection mod allows a player to duplicate a command block. Without permissions, the player can now grant himself any item, or manipulate protected areas.

What? How does infinite command blocks lead to infinite everything and manipulation of mod data?

But if he can read mod code, he can get a hold of copyrighted code that is not open source, or API tokens that a developer has put into code so that his IRC mod can interface with an IRC daemon. Oops.

I'm not going to worry too much about the proprietary code aspect. API tokens shouldn't be hardcoded anyways, although I don't know of anywhere that you could store them that would be private to that mod. This is a legitimate shortcoming.

EDIT:

Offtopic: having a proper player metadata storage API would remove half the problem.

This is what datastorage is (although you could argue about whether it's "proper").

raymoo · 2016-12-07T06:01:42Z

Why not have a "mod folder environment" thing you can create at init time that has file functions that can read the mod dir? Similar to the insecure environments we already have, except it just grants read access to the folder of the creating mod. Mods that want to be able to read their folders after init can create one and keep it secret, and mods that want to be readable from other mods can export it in its table.

ghost · 2016-12-07T06:33:40Z

datastorage uses the world dir. All mods can already read (and write) to datastorage files.

Then I don't see a problem here. Mods that want their stuff being read by other mods place that stuff there.

And I agree with @sfan5 saying that user-generated content should not be in mod folders (separating data and functionality).

sfan5 · 2016-12-07T09:14:10Z

@raymoo that is an alternative way of solving this problem, however this PR is specially designed to fix (at least that's my impression) mods that want to read their own mod dir at runtime without requiring those mods to be fixed in any way

ShadowNinja · 2016-12-09T03:02:53Z

@raymoo: That could work, but it's a very ugly hack. The insecure env is bad enough, but generating an environment for every mod would be even worse.

Also, as sfan5 stated, it wouldn't fix compatibility.

paramat · 2016-12-13T06:26:37Z

src/script/cpp_api/s_security.cpp

@@ -380,32 +384,50 @@ bool ScriptApiSecurity::checkPath(lua_State *L, const char *path)

 		// Builtin can access anything
 		if (mod_name == BUILTIN_MOD_NAME) {
+			if (write_allowed) *write_allowed = true;


Should be split into 2 lines.

paramat · 2016-12-13T06:26:55Z

src/script/cpp_api/s_security.cpp

+			if (mod) {
+				str = fs::AbsolutePath(mod->path);
+				if (!str.empty() && fs::PathStartsWith(abs_path, str)) {
+					if (write_allowed) *write_allowed = true;


Should be split into 2 lines.

paramat · 2016-12-13T06:27:11Z

src/script/cpp_api/s_security.cpp

+		}
+		// Allow all other paths in world path
+		if (fs::PathStartsWith(abs_path, str)) {
+			if (write_allowed) *write_allowed = true;


Should be split into 2 lines.

paramat · 2016-12-14T06:09:10Z

Added line comments, looks like doing this is strictly prohibited.

ShadowNinja · 2016-12-17T03:37:33Z

@paramat: Done.

est31 · 2016-12-17T03:46:15Z

src/script/cpp_api/s_security.cpp

-	if (fs::PathStartsWith(abs_path, str)) {
-		return true;
+	if (!str.empty()) {
+		// Don't allow access to world mods.  We add to the absolute path


Shouldn't read access be allowed?

Yes, but if it is a mod path and read-only access is acceptable we will have already returned true.

So what will it do then? If its dead code it should be removed to be not confusing

~~Er, you're right, this needs to be changed...~~
EDIT: Er, nevermind, it's actually fine. This is how it works:

If path is in mod's path return rw-access.

If path is in any other mod path ro-access.

If path is in worldmods (but is not a current mod path) return no-access. (we don't want to allow mods to create a new dir such as worldmods/trusted and override a trusted mod stored in a lower-priority mod directory like ~/.minetest/mods/trusted)

If path is in world return rw-access.

Return no-access.

See edit, I've updated the commit with a better comment explaining why this still needs to be here.

So we basically are banning any access to the worldmods dir outside of actual mod directories. That's good and actually required, as you explain.

est31 · 2016-12-17T03:49:06Z

👍 to concept and implementation, if my comment is addressed.

est31 · 2016-12-18T02:02:46Z

@ShadowNinja I haven't approved yet, the comment is not yet addressed.

ghost · 2016-12-18T21:58:03Z

👎 to the whole thing. Mods should use the API other mods provide instead of reading their files directly. And mods should not have to read their own directory after loading it (nor write there).

ShadowNinja · 2016-12-18T22:49:46Z

@dsohler: Whether you like it or not, some mods access things in the mod folder after startup and this is needed for compatibility with those mods. Whether those mods should be doing that is another question (e.g., player_textures does this, but it doesn't have to and really shouldn't; other mods may have good reasons for this though, for example you may want to place a schematic without having to keep it loaded always).

est31 · 2016-12-19T00:08:01Z

👍

ghost · 2016-12-19T07:33:47Z

Whether you like it or not, some mods access things in the mod folder after startup and this is needed for compatibility with those mods.

So instead of changing the mods to be compatible with the API we change the API to compensate the mods flaws? Wouldn't it be better to extend the API in a way that this security hole isn't necessary?

for example you may want to place a schematic without having to keep it loaded always

If it's a schematic created by the user it should not be stored in the mod's folder at all because it is user date. And user data and application date should not be mixed. if it's a predefined schematic without loading it at load time, well, that's an edge case (see first answer).

... speaking of security: Since this will likely make it into core even if it's unsafe by design what are the mod authors ways to prevent other mods reading the mod directory?

sfan5 · 2016-12-19T21:40:20Z

if it's a predefined schematic without loading it at load time, well, that's an edge case

Precisely this thing you are calling an "edge case" is the most common case for schematic usage in mods.

what are the mod authors ways to prevent other mods reading the mod directory?

none, why would you want to do that even?

paramat · 2016-12-20T06:40:40Z

59f84ca

Testing appreciated since we are close to release.

ghost · 2016-12-20T11:26:57Z

Precisely this thing you are calling an "edge case" is the most common case for schematic usage in mods.

An if it is such a common thing why isn't there a shared folder where mods can read and write to? A mod could copy schematics there on initialization.

paramat · 2016-12-20T11:46:09Z

That's unnecessary complexity, it's useful and essential for a mod to just grab a texture or schematic from another mod, MTGame does this a lot.

sfan5 · 2016-12-20T12:58:42Z

An if it is such a common thing why isn't there a shared folder where mods can read and write to? A mod could copy schematics there on initialization.

Why should mods bother copying their files somewhere? It takes both time and additional disk space to do that.

ghost · 2016-12-20T15:21:01Z

Adding not declinable full read access to everywhere is better?

sfan5 · 2016-12-20T15:59:54Z

Yes

ghost · 2016-12-21T08:22:08Z

Does it at least check if a mod was actually loaded or not? For example having World Edit mod in the mod folder but not using it but not deleting it either because it's used in another world?

sfan5 · 2016-12-21T09:01:34Z

I've quickly checked the code and it looks like it only allows access to mod paths that are actually loaded.

est31 · 2016-12-21T09:02:43Z

Yes, it should only give access to actually loaded mods. And even there its read-only (read-write would be bad and something I'd be against).

ShadowNinja added @ Script API High priority labels Dec 5, 2016

ShadowNinja mentioned this pull request Dec 5, 2016

Mod security: Accessing mod paths in callbacks is broken #4692

Closed

est31 added this to the 0.4.15 milestone Dec 5, 2016

paramat reviewed Dec 13, 2016

View reviewed changes

ShadowNinja force-pushed the secure-modpath-ro branch from ae16f22 to 7c9cded Compare December 17, 2016 03:36

est31 reviewed Dec 17, 2016

View reviewed changes

ShadowNinja added the One approval ✅ ◻️ label Dec 17, 2016

paramat mentioned this pull request Dec 17, 2016

Fix RemoveRelativePathComponents (again) and fix mod security with paths starting with ".." #4919

Closed

est31 removed the One approval ✅ ◻️ label Dec 18, 2016

Mod security: Allow read-only access to all mod paths

f5d8911

ShadowNinja force-pushed the secure-modpath-ro branch from 7c9cded to f5d8911 Compare December 18, 2016 21:29

est31 added the One approval ✅ ◻️ label Dec 19, 2016

paramat closed this Dec 20, 2016

Mod security: Allow read-only access to all mod paths #4849

Mod security: Allow read-only access to all mod paths #4849

Conversation

ShadowNinja commented Dec 5, 2016 • edited Loading

sofar commented Dec 6, 2016

ShadowNinja commented Dec 6, 2016 • edited Loading

sofar commented Dec 6, 2016

ghost commented Dec 6, 2016

sfan5 commented Dec 6, 2016

nerzhul commented Dec 6, 2016

ghost commented Dec 6, 2016

sofar commented Dec 6, 2016 • edited Loading

sfan5 commented Dec 6, 2016

sofar commented Dec 6, 2016

sfan5 commented Dec 6, 2016

sofar commented Dec 6, 2016

sofar commented Dec 6, 2016

ShadowNinja commented Dec 7, 2016 • edited Loading

raymoo commented Dec 7, 2016 • edited Loading

ghost commented Dec 7, 2016

sfan5 commented Dec 7, 2016

ShadowNinja commented Dec 9, 2016

paramat Dec 13, 2016

Choose a reason for hiding this comment

paramat Dec 13, 2016

Choose a reason for hiding this comment

paramat Dec 13, 2016

Choose a reason for hiding this comment

paramat commented Dec 14, 2016 • edited Loading

ShadowNinja commented Dec 17, 2016

est31 Dec 17, 2016

Choose a reason for hiding this comment

ShadowNinja Dec 17, 2016

Choose a reason for hiding this comment

est31 Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

ShadowNinja Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

ShadowNinja Dec 18, 2016

Choose a reason for hiding this comment

est31 Dec 19, 2016

Choose a reason for hiding this comment

est31 commented Dec 17, 2016

est31 commented Dec 18, 2016

ghost commented Dec 18, 2016

ShadowNinja commented Dec 18, 2016

est31 commented Dec 19, 2016

ghost commented Dec 19, 2016 • edited by ghost Loading

sfan5 commented Dec 19, 2016

paramat commented Dec 20, 2016 • edited Loading

ghost commented Dec 20, 2016

paramat commented Dec 20, 2016

sfan5 commented Dec 20, 2016

ghost commented Dec 20, 2016

sfan5 commented Dec 20, 2016

ghost commented Dec 21, 2016 • edited by ghost Loading

sfan5 commented Dec 21, 2016

est31 commented Dec 21, 2016

ShadowNinja commented Dec 5, 2016 •

edited

Loading

ShadowNinja commented Dec 6, 2016 •

edited

Loading

sofar commented Dec 6, 2016 •

edited

Loading

ShadowNinja commented Dec 7, 2016 •

edited

Loading

raymoo commented Dec 7, 2016 •

edited

Loading

paramat commented Dec 14, 2016 •

edited

Loading

est31 Dec 17, 2016 •

edited

Loading

ShadowNinja Dec 18, 2016 •

edited

Loading

ghost commented Dec 19, 2016 •

edited by ghost

Loading

paramat commented Dec 20, 2016 •

edited

Loading

ghost commented Dec 21, 2016 •

edited by ghost

Loading