pdinklag / MinecraftStats

A Minecraft player statistics browser for the web - supports 1.13 and later!
https://discord.gg/brH5PGG8By
Other
226 stars 53 forks source link

Security Issue #209

Closed TomLewis closed 1 year ago

TomLewis commented 1 year ago

Anyone can append /config.json to the end of the web stats URL and they can see your config.. this is insane, because in the footer it lists the software used, which brings them to your Github that explains how to see the config!

You need a public folder that is just for the public, and everything else needs to be lower down so it cant be loaded by anyone...

pdinklag commented 1 year ago

This is only an issue if you name the file config.json and put it next to index.html under the webserver's document root. For example, try it on the example installation and you'll see this doesn't work. Move the config anywhere else on your file system that the user running update.py can read and the issue is gone - it doesn't have to (and shouldn't) be in under your document root.

This is really the administrator's responsibility and out of scope of MinecraftStats on a technical level. I'm leaving this open as a reminder to state this in the readme.

TomLewis commented 1 year ago

I am using the example instillation and instructions.

To call the update script, simply go into the installation directory and execute the following

Which in turn creates the config in the root directory!

Screenshot 2022-07-27 14 20 55

I still think there should be an abstraction from your python generating code and your frontend public code.

You just need to move all HTML/CS/JS/Data into a public folder and everything else should be 1 level up out of reach.

I will manually move my config and edit my cron to adjust, but this still leaves the python files in the public web directory, I think this just downloads the python scripts once run, but they are not required to be hosted.

Edit: Maybe i'm missing an instillation step here? Because your crontab tells you to CD to the same directroy as the update.py which is also in the root with config:

*/10 * * * * cd /path/to/mcstats ; python3 update.py config.json

pdinklag commented 1 year ago

I am using the example instillation and instructions.

To call the update script, simply go into the installation directory and execute the following

Which in turn creates the config in the root directory!

Screenshot 2022-07-27 14 20 55

Well, it's a default. I agree about adding a hint to change the file name in said instructions, but that's really about it in my opinion.

I still think there should be an abstraction from your python generating code and your frontend public code.

You just need to move all HTML/CS/JS/Data into a public folder and everything else should be 1 level up out of reach.

I will manually move my config and edit my cron to adjust, but this still leaves the python files in the public web directory, I think this just downloads the python scripts once run, but they are not required to be hosted.

In principle, I agree, there is no need to serve those files. However, separating frontend and server code would add complication to the setup. Right now, no special webserver configuration (e.g. setting up some directory alias or moving around files from the repository) is needed to get MinecraftStats up and running. Updating the entire software - server and frontend - can be done by issuing one git pull. I prefer keeping it this way.

I'm not sure why you believe the Python scripts would be downloaded? Unless somebody deliberately issues a HTTP GET on, e.g., update.py, the file will never be downloaded by anybody or anything. The server executes the Python code only if asked to by a local user with the corresponding rights, and completely offline. Unless you explicitly tell your webserver to execute Python files when a client requests them, there's no added security risks to your system here (and even in that case, the script would simply exit right away due to no config file being stated).

We could argue about an adversary's information gain of reading out the contents of the __pycache__ folder, but other than probably revealing the path of your webserver's document root (which likely is /var/www/html anyway), I don't see any.

Edit: Maybe i'm missing an instillation step here? Because your crontab tells you to CD to the same directroy as the update.py which is also in the root with config:

*/10 * * * * cd /path/to/mcstats ; python3 update.py config.json

I'm not sure what you think you are missing. The cd is required to set the working directory - by default, a cronjob will always run in the executing user's home directory. Not doing the cd will put the generated data directory in the home directory, which has caused confusion in the past.

pdinklag commented 1 year ago

The workflow in v3.0.0 has changed so that this should no longer be an issue unless you deliberately put the CLI and config under the document root.

TomLewis commented 1 year ago

TIL there is a v3! Exciting! Where is best to follow updates/news about v3?

pdinklag commented 1 year ago

The best would be the Discord channel, where you can also ask questions more directly. Otherwise, simply watch this repository for new releases. 😃