-
Notifications
You must be signed in to change notification settings - Fork 11k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Image rule is not secure #20390
Comments
Simple solution would probably be to just add a filename extension check to the image validation? We know the file extensions already That would probably solve it? |
Anyone can change the extension of a file. The issue has already been discussed here: #18299 The validation could be said for anything too with regards to mime, what's to stop someone uploading malicious PDF files etc? It's very difficult to guarantee a file is what it's meant to be. If you don't trust the uploaded files, don't store them in the public web folder - store them in a storage folder and provide access to them via a script? |
But the extension has meaning - apart from the contents. If the file extension is But when the extension is
Nothing - you are right. But the arbitrary ability to upload files that turn out to be executable PHP code is a different issue to a general malicious files of other types and how you might otherwise interact with them.
Whilst I do agree with this - I think there is probably an opportunity for the framework to put in a sensible default here... |
An alternative solution - any file validation on files could include the automatic blocking of a file that physically ends in Is there realistically ever a time that you want to allow a If you feel you are smart enough to allow users to upload Seems like a better sensible default to prevent people from all sorts of issues... |
👍 for that solution, apache checks extension + mime when executing php however nginx usually checks only the file extension so this should be a good fix for it.
There's a bigger list than that (Webp, Tiff, etc) however that's pretty much it. |
@Numline1 @fernandobandeira - I've done a PR which you can see. We can probably close this issue for now, and discuss further on the PR... |
Description:
I'm not sure if this is obvious to many people or just something that wasn't noticed before, however it seems that the "image" validator only validates mime types. Our project has recently been audited and it turns out it's dead simple to upload a malicious file when only image validation is used.
Steps To Reproduce:
Set up an environment with file upload and validation similar to this:
Upload your malicious file named
picture.php
, which, among some binary image content, contains this:Once the file is uploaded, you're able to control the system easily through commands like
http://compromisedsite.dev/files/picture.php?cmd='rm -rf /'.
Proposed solutions:
Several things come to mind on how to fix this. This doesn't seem to be much of a framework issue, so setting up the server to disable code execution on upload folders seems as the most reasonable option, however, this is not mentioned in the documentation. Second recommendation is to disable shell execution upon the user under which the code is executed, however, this poses some implications to cron and queues. This second option also doesn't fix issues with "php-only" malicious code.
I am not aware of any validation rule, that'd be capable of file extension validation ("mimes" only seems to care about mime types).
Outro:
I'd like to hear other people opinions on this issue to see how this could be improved, since from the quick internet search I did, many people (and some major open source projects built on Laravel) use simple "image" rule validation, which may expose them to an attack.
The text was updated successfully, but these errors were encountered: