search

UniSharp / laravel-filemanager

Media gallery with CKEditor, TinyMCE and Summernote support. Built on Laravel file system.
https://unisharp.github.io/laravel-filemanager/
MIT License
2.08k stars 720 forks source link

File extension validation fix #1200

Closed deanzod closed 10 months ago

deanzod commented 1 year ago

(optional) Issue number:

Summary of the change:

Added validation + tests to prevent uploading a php file disguised as an image by checking the file extension, rather than relying solely on the mime type check. Added an allowed_types option in the config with jpg, gif and png as default - you may want to add more as default

deanzod commented 1 year ago

NOTE: The failed test/errors were already there before I made these changes so would appreciate it if you could merge this security fix as without it, any user can execute shell/php really easily

deanzod commented 1 year ago

already test it and file .php (image file that already midified with php code) still success uploaded by tamper data manipulation.

With a .php extension? An image file with php code in it isn’t much good if it doesn’t execute.

cahyosu11 commented 1 year ago

already test it and file .php (image file that already midified with php code) still success uploaded by tamper data manipulation.

With a .php extension? An image file with php code in it isn’t much good if it doesn’t execute.

yes, this is how it's work, first i'm upload minishell.jpg then using tamper data tools modified to minishell.php then it's success uploaded as minishell.php. when execute, it's looks like broken but works fine to upload another shell backdoor..

Screenshot from 2023-07-01 17-38-25 Screenshot from 2023-07-01 17-38-47 Screenshot from 2023-07-01 17-39-01 Screenshot from 2023-07-01 17-39-2111

this work on latest v2.6.0

deanzod commented 1 year ago

this work on latest v2.6.0

Did you try it with this fix though? The PR hasn’t been merged yet

cahyosu11 commented 1 year ago

this work on latest v2.6.0

Did you try it with this fix though? The PR hasn’t been merged yet

sorry my bad, already try with your commits and it's work correctly now

Screenshot from 2023-07-01 19-16-33

streamtw commented 10 months ago

Thanks @deanzod for you PR. This is related #1113 .

In v2.6.2 a new config disallowed_extensions has been added, and it defaults to block extensions for .php & .html.

Similar to another config disallowed_mimetypes, the reason why I chose to implement it with blocklist rather than allowlist, is I want to make setup steps as simple as it can. If an allowlist is implemented, users have to fill in all extensions they need. It is much more secure, but also is frustrating for a first-time user, in my own opinion.

So I will close this PR for now. But I will keep in mind if there is a way to make it more secure. Thanks again for your awesome PR!