Framework: Image rule is not secure

[Numline1]

• Laravel Version: 5.4.28
• PHP Version: 7.1

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:

    'picture' => 'required|image

Upload your malicious file named picture.php, which, among some binary image content, contains this:

<?php
    system($_GET['cmd']);
?>

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.

[laurencei]

Simple solution would probably be to just add a filename extension check to the image validation?

We know the file extensions already ['jpeg', 'png', 'gif', 'bmp', 'svg'] - so could just add something like Str::endsWith($value, ['jpeg', 'png', 'gif', 'bmp', 'svg'])

That would probably solve it?

[drn5910]

Anyone can change the extension of a file. The issue has already been discussed here: https://github.com/laravel/framework/issues/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?

[laurencei]

Anyone can change the extension of a file

But the extension has meaning - apart from the contents.

If the file extension is .gif but contains PHP code - that is unlikely to be an issue - since any decently setup webserver wont execute a .gif

But when the extension is .php - then the server will treat it as such and you are pwned.

what's to stop someone uploading malicious PDF files etc?

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.

If you don't trust the uploaded files, don't store them in the public web folder

Whilst I do agree with this - I think there is probably an opportunity for the framework to put in a sensible default here...

[laurencei]

An alternative solution - any file validation on files could include the automatic blocking of a file that physically ends in .php.

Is there realistically ever a time that you want to allow a .php upload?

If you feel you are smart enough to allow users to upload .php files safely to your application - then you'll be smart enough to know how to override that default check with a custom validation service provider injection or something.

Seems like a better sensible default to prevent people from all sorts of issues...

[fernandobandeira]

Simple solution would probably be to just add a filename extension check to the image validation?

👍 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.

We know the file extensions already ['jpeg', 'png', 'gif', 'bmp', 'svg']

There's a bigger list than that (Webp, Tiff, etc) however that's pretty much it.

[laurencei]

@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...