search

MinecraftServerControl / mscs

Powerful command-line control for UNIX and Linux powered Minecraft servers
https://minecraftservercontrol.github.io
BSD 2-Clause "Simplified" License
489 stars 62 forks source link

Usage of non-local environment variables #309

Open xathon opened 2 years ago

xathon commented 2 years ago

Currently, the command-handling logic is not placed inside a function, making the usage of local environment variables impossible. This can cause previously set environment variables to get overwritten.

To mitigate the above issue, environmental variables get cleared before usage or directly overwritten. However, in some cases environment variables get appended, but not checked if they are empty before. This can cause external code to change the environmental variable at a specific time, causing injection of unwanted codes into the options of mscs/msctl. This can have multiple effects ranging from a confusing and bloated output of status to a spam of created folders when using start; however, as far as I have seen, there are no security implications, since variables are always enclosed in double quotes and thus not interpreted.

An example of the above: https://github.com/MinecraftServerControl/mscs/blob/8118d107471d09a73416d45f834d6aae034c6204/msctl#L3053-L3058

An example where it's done correctly (note L2668): https://github.com/MinecraftServerControl/mscs/blob/8118d107471d09a73416d45f834d6aae034c6204/msctl#L2667-L2676

xathon commented 2 years ago

310 fixes the mitigation, however, rewriting the command handling in a function might still be useful.

sandain commented 2 years ago

Nice catch @xathon. I've looked over your PR, and it looks correct to me. Thanks for noting the issue and sending in a patch to correct it!

sandain commented 2 years ago

I don't have much time to work on this code, but would be happy to review patches if you are interested in moving the command handling into a function.

xathon commented 2 years ago

No worries! This was causing me all sorts of headaches, wondering how in the world I had injected an array into my status command 😄

I don't have much time to work on this code, but would be happy to review patches if you are interested in moving the command handling into a function.

I'll have a look. I did a short attempt earlier, which failed due to being unable to pass the correct variables to the handlers. It might be a bigger rewrite, but if I find some time I'll try my hand at it.