Skip to content
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

[no squash] Remove insecure environment from async and emerge environment #14370

Merged
merged 2 commits into from Feb 15, 2024

Conversation

sfan5
Copy link
Member

@sfan5 sfan5 commented Feb 14, 2024

fixes #13092 (comment)
since this affects the async environment too we should create a security advisory (done)

this situation does not have an obvious fix:
At the time register_mapgen_script is called the engine trusts the mod name. But this isn't safe because mods can (intentionally or unintentionally) call into each other's code.

  • restricting register_mapgen_script to the top level (like request_insecure_environment) would be inconvenient
  • in addition that still doesn't work if the mod registers a script at a global-writable location (e.g. world path)
  • alternatively:
  • the engine can tell which mod folder a file originated from
  • but then the internal code needs to be reworked to differentiate between
    • "we know which mod is probably running" (get_current_modname would use this because it needs to stay working)
    • "we know which mod is running and trust this value" (requirement for request_insecure_environment)
  • also quite inconvenient

So I chose to just remove the feature.

To do

This PR is Ready for Review.

How to test

  1. make sure lua still works in general
  2. make sure request_insecure_environment still works

Copy link
Member

@Desour Desour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

src/script/lua_api/l_client.cpp Show resolved Hide resolved
This is to prevent future mistakes and make it clearer whether
the mod name can be trusted depending on how it is retrieved.
@sfan5 sfan5 merged commit ce97210 into minetest:master Feb 15, 2024
13 checks passed
@sfan5 sfan5 deleted the ieproblem branch February 15, 2024 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants