possible path traversal issue

[johannestaas]

Sorry, I'm not familiar with the code base but I love the game so I was skimming through out of curiosity, and I noticed what might be a path traversal bug so I figure I'd create an issue.

In upload_scores.php it seems there's an escaped shell command that passes user input from$filepath to parse_game, and in parse_game.cpp it seems it just takes whatever path from the filename that was passed from the php and treats it as a gzipped archive and iterates over it.

You might possibly want to make sure the filename is just a filename, and wasn't manipulated in traffic through a proxy like burp so that you couldn't pass ../../../../../../var/log/some_log.gz and try to guess arbitrary paths to log files or other gzipped resources that might be on the server that serves upload_scores.php. Might leak data from the system in some high scores display, or maybe it will just crash.

Again, I'm really not familiar with this codebase but if this is running on a server you own and users can hit your site's upload_scores.php, then path traversal might be possible.

[miki151]

In this particular case it seems that it's safe - I use "tmp_name" which is a file in /tmp/ generated by PHP. But the issue that you describe can happen in other scripts. Thanks for reporting this!

[johannestaas]

no problem! Thanks for keeperRL!

[miki151]

I went through the file uploading logic in the php scripts, and here are cases of using user-supplied data:

upload_scores.php:$filepath = $_FILES["fileToUpload"]["tmp_name"];
upload_scores.php:if ($_FILES["fileToUpload"]["size"] > 100000) {
upload_site.php:$filename = basename($_FILES["fileToUpload"]["name"]);
upload_site.php:if ($_FILES["fileToUpload"]["size"] > 5000000) {
upload_site.php:if (move_uploaded_file($_FILES["fileToUpload"]["tmp_name"], $target_file))

From what I understand "tmp_name" and "size" elements are generated by php and safe to use without sanitizing, and the only case where "name" is used, the input is sanitized using the 'basename' function.

Let me know if I'm wrong on anything or if the issue you've raised concerns other points in code. Thanks again.

[johannestaas]

I think you got it! I looked it up and it seems tmp_name and size aren't user input like you mention. basename seems to be a good fix from the PHP docs to get rid of path traversal issues from name as well. It can be any filename the user specifies, but as long as they can't get out of the directory you're fine in terms of path traversal, but might want to make sure you have error handling to handle whether they upload something with a name that might overwrite another file.

But there might be other things you want to double check: https://www.owasp.org/index.php/Unrestricted_File_Upload

This link has a lot of good info: https://www.acunetix.com/websitesecurity/upload-forms-threat/

There's some really bad vulnerabilities involving file uploads where a user might be able to drop a file with whatever name they want. One thing mentioned in that second link is an attacker uploading a file named .htaccess with the contents AddType application/x-httpd-php .jpg, which instructs the webserver to treat files with extension .jpg as actual executable PHP, then they can write their own php script and rename it evil.jpg and upload it, and if they can link to it, they basically have their own code running on your server which has the privileges as the webserver. You'll often run into hacked sites where an attacker was able to upload their own PHP script where the server might expect something else, and since it's PHP it's executed by the webserver when someone visits a link to it. They can basically take over a webserver that way. One common webshell that's uploaded is the c99 shell.

If you can I would definitely try to use your own generated names for files and not trust filenames from the name field you get. One technique in python is to use the tempfile module to create a temporary file with a temporary name it generates in a directory you specify, then you just write whatever file they uploaded to that path. It looks like PHP's tempnam might be the equivalent:

http://php.net/manual/en/function.tempnam.php

I wouldn't worry too much about it since someone literally has to target you and do it manually, but it's something to consider whenever taking files users upload. There's weird little tricks like that where suddenly your server thinks it's code it should execute.