-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Mod security: Allow read-only access to all mod paths #4849
Conversation
Euh, this allows any mod to read any file in any mod folder? Is that right? |
@sofar: Correct. After init time (what this primarily fixes) there is no definitive "current mod", so it can't be limited by mod. |
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 |
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. |
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:
|
Sorry but no. mod is code, 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. |
@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 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:
would e.g. grant read access to `mods/modname/schematics/ |
@sfan5 e.g a mod could read all of 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. |
Storing configuration in mod directories seems like a valid problem. |
@sfan5 the alternative is to hack an invisible inventory like |
Offtopic: having a proper player metadata storage API would remove half the problem. |
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 Rant i just wrote about mod security.
Eh... I'm not too concerned about this.
What? How does infinite command blocks lead to infinite everything and manipulation of mod data?
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:
This is what |
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. |
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). |
@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 |
@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. |
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be split into 2 lines.
if (mod) { | ||
str = fs::AbsolutePath(mod->path); | ||
if (!str.empty() && fs::PathStartsWith(abs_path, str)) { | ||
if (write_allowed) *write_allowed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be split into 2 lines.
} | ||
// Allow all other paths in world path | ||
if (fs::PathStartsWith(abs_path, str)) { | ||
if (write_allowed) *write_allowed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be split into 2 lines.
Added line comments, looks like doing this is strictly prohibited. |
ae16f22
to
7c9cded
Compare
@paramat: Done. |
if (fs::PathStartsWith(abs_path, str)) { | ||
return true; | ||
if (!str.empty()) { | ||
// Don't allow access to world mods. We add to the absolute path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't read access be allowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but if it is a mod path and read-only access is acceptable we will have already returned true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what will it do then? If its dead code it should be removed to be not confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See edit, I've updated the commit with a better comment explaining why this still needs to be here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
👍 to concept and implementation, if my comment is addressed. |
@ShadowNinja I haven't approved yet, the comment is not yet addressed. |
7c9cded
to
f5d8911
Compare
👎 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). |
@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., |
👍 |
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?
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? |
Precisely this thing you are calling an "edge case" is the most common case for schematic usage in mods.
none, why would you want to do that even? |
Testing appreciated since we are close to release. |
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. |
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. |
Why should mods bother copying their files somewhere? It takes both time and additional disk space to do that. |
Adding not declinable full read access to everywhere is better? |
Yes |
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? |
I've quickly checked the code and it looks like it only allows access to mod paths that are actually loaded. |
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). |
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.