Path traversal vulnerability

[yaci]

Let's consider an example url: https://domain.com/srv/?id=iqrcodes&key=someKey&fn=qrc_hash.png

The fn parameter takes a file name, but it can also take a path. I.e. it may be possible to execute the following https://domain.com/srv/?id=iqrcodes&key=someKey&fn=../../../somefile.png

This may result in a situation where attacker gains an access to a file outside public_html directory. You can read more about this kind of attack here: https://www.owasp.org/index.php/Path_Traversal

Currently it's mostly mitigated by the fact, that srv.php only processes requests for image files and other file types are rejected. Also my server configuration did not allow access to files outside public_html. Nevertheless I feel it would be good to address this.

The approach that I would suggest:

1. Do not send file names in url, only hash. Append qrc_ prefix and file extension in php script.
2. Remove slashes, backslashes and dots from the input
3. Only accept inputs that are exactly 32 chars long (output of md5 is always 32 chars)

Another approach would be to send the short url and do the hashing on the server. In this way we would end up with a hash string regardless of user input.

[joshp23]

I will address this as soon as I have the time, thank you for pointing this out.

[joshp23]

Thanks again for bringing this to my attention, currently this issue, as you said, is mostly managed by restricting file-types, so an attacker will be met by this:
file type not supported, please check your configuration

This issue is also mitigated by the fact that srv.php serves up time sensitive links, but this will likely be replaced by a cookie based exchange due to an annoyance in #13 in the near-ish future. The path traversal issue will be dealt with before that happens. But it seems pretty secure right now.

[joshp23]

as for now, I have

1. included a regex_replace to strip anything resembling ../../../ from the beginning of the fn key
2. added a directory comparison to make sure that the file being served up is in the store set in config.

These should manage the problem fine, actually, just the first one should be enough... as for the rest of it, I like the idea of adding the prefix and file extension after transmission, checking the length there sounds good as well. It will have to wait, however. Thanks for the suggestions, I'm closing this as the vulnerability has been dealt with.