Potential Issue Found: File Upload

[bgeesaman]

I've been doing source code analysis of certain types of public repos for a specific classes of problems, and I found a something in your repo from my research that you may want to take a look at.

Specifically: https://github.com/area17/twill/blob/90da826b0f317098479b6761c3b6071c68477ef0/src/Http/Controllers/Admin/MediaLibraryController.php#L190

Laravel's file upload storeAs() function can be vulnerable to an arbitrary file upload flaw if the file name and path arguments are not properly validated and sanitized. This means and attacker can potentially write files to places outside the intended location or even overwrite critical files on the server. Depending on permissions, this may provide a path for remote code execution.

See: https://cheatsheetseries.owasp.org/cheatsheets/Laravel_Cheat_Sheet.html#unrestricted-file-uploads for more details on how to improve validation of file names and paths.

Note: This research has taken some time to complete, so the commit I'm referencing is a few weeks old. You may have already fixed this issue in a later commit. If so, feel free to ignore/close. Just wanted to give you a heads up as a courtesy in case you found it helpful.

[edgrosvenor]

I don't think this is likely to be an issue here for a few reasons:

1. This method is within an admin controller, so authorization would ensure that only administrators of the site have access to this particular upload form. Obviously when creating user-facing upload forms, robust validation would be added.
2. The file path is generated within the code and not derived from user input.
3. The filename is sanitized to remove all non-alphanumeric characters except for . and without being able to pass a directory separator in the filename, I can't envision an scenario in which a user might be able to cause a file to be written outside of the defined directory.

[Tofandel]

The file path is generated within the code and not derived from user input.

That's not the point of concern, there is both filename (which is derived from user input btw) and directory name, the upload directory if you look at the source is derived directly from request->input with no additional validation and passed to storeAs

Meaning path traversal is possible there and could be used to overwrite any file on the server (not just within the application, if chown permits it)

This method is within an admin controller, so authorization would ensure that only administrators of the site have access to this particular upload form

While true, this still allows privilege escalation to lower capabilities of users (for example editors and that's still it's own category of CVE), or privilege escalation to the file system/server.

An editor with this vulnerability could gain access to the server's filesystem and use that to either upload malicious files in places that will self execute and extract information this way or maybe upload a new .ssh/authorized_keys file and login to ssh

So I would still rate it a 5/10 medium CVE: it requires a trusted user but it is then very easy to exploit (just edit the request to use "../.." as the directory name input and replay it)

But maybe this should be disclosed to laravel and fixed there in addition, I see absolutely no reason that files outside a configured disk should be accessible when explicitely using a disk