ctf0 / Laravel-Media-Manager

A "Vuejs & Laravel" Media Manager With Tons of Features
MIT License
829 stars 179 forks source link

[Question] Are unallowed_mimes checked by file extension or content? #131

Closed jasperf closed 4 years ago

jasperf commented 4 years ago

Do know we have unallowed_mimes in config/mediaManager.php to add mime types we do not want to allow to be uploaded

/*
* disallow uploading files with the following mimetypes
* https://www.iana.org/assignments/media-types/media-types.xhtml
*/
 'unallowed_mimes' => ['php', 'java'],

Good to have .phar by default as well by the way. Could be security hazard for many.. But my main question is here, does this check just check file extension or also its content to determine its mime type?

ctf0 commented 4 years ago

the check is done through https://github.com/ctf0/Laravel-Media-Manager/blob/ac7af4440329d41ec13053fea61fbef6e453562a/src/App/Controllers/Modules/Upload.php#L39

https://github.com/ctf0/Laravel-Media-Manager/blob/ac7af4440329d41ec13053fea61fbef6e453562a/src/App/Controllers/Modules/Upload.php#L44

ctf0 commented 4 years ago

for the extra mime, can u send a PR ?

jasperf commented 4 years ago

I checked Media Manager Upload.php . Should it not be

namespace ctf0\MediaManager\App\Controllers\Modules;

instead of

namespace ctf0\MediaManager\App\Controllers\Moduels;

with modules instead of moduels?

GetMimeType

So I assume the getMimeType is based on Laravel's Mime Type (see Laravel Mime Type Validation) for example, which in turn is based on Symfony's Mime Type which does state not only the extension but content is tested as well. At least this SO thread confirms this:

For a trusted mime type, use getMimeType() instead (which guesses the mime type based on the file content).

Update in https://github.com/symfony/symfony/blob/e60a876201b5b306d0c81a24d9a3db997192079c/src/Symfony/Component/HttpFoundation/File/UploadedFile.php added by Media Manager https://github.com/ctf0/Laravel-Media-Manager/blob/ac7af4440329d41ec13053fea61fbef6e453562a/src/App/Controllers/Modules/Upload.php#L9 with use Symfony\Component\HttpFoundation\File\UploadedFile; is shows it checks content

unallowedMimes

Media Manager unallowed mime types: https://github.com/ctf0/Laravel-Media-Manager/blob/ac7af4440329d41ec13053fea61fbef6e453562a/src/App/Controllers/Modules/Upload.php#L44 shows you can disallow with the list I mentioned to start with. I am only not sure this also checks on content of file or only extension. I think only by extension?

Mime types List

Mime Types List at Symfony Mime Types does not show .phar as option, but show all php versions

'application/x-php' => ['php', 'php3', 'php4', 'php5', 'phps'],

Guess mimeType / Add new mimeType

So perhaps extra code to guess this mime Type needs to be added. Or an extension to mimTypes list. I am looking into this some more.

NB Guzzle also does check. See upgrading README on this

jasperf commented 4 years ago

It is possible to add a new Mime guesser that really checks content as well as long as PHP Fileinfo or another tool can check for .phar . See https://symfony.com/doc/current/components/mime.html#guessing-the-mime-type

ctf0 commented 4 years ago

with modules instead of moduels?

fixed, thanks for the headsup

ctf0 commented 4 years ago

for the .phar am not sure how to check for it, https://laravel.com/api/6.x/Illuminate/Filesystem/Filesystem.html#method_mimeType already does the check by content because there is another one https://laravel.com/api/6.x/Illuminate/Http/Testing/MimeType.html#method_getMimeTypeFromExtension that checks by file ext which is ineffective

jasperf commented 4 years ago

https://laravel.com/api/6.x/Illuminate/Filesystem/Filesystem.html#method_mimeType already does the check by content because there is another one

I see now yes, so that is done well already. Good.

Still doing research on checking for .phar files. Perhaps adding a new Mime Type to guess is still possible. It is in fact an extension of a .tar or .zip archive (wikipedia article). Surprised there is not code for this out there already.

However, as .phars can be embedded in jpeg files as well a content check would be needed for sure. And that also means checking or the .phar extension or .phar mime type only won't suffice. You also need to make sure your application is up to date to deal with possible vulnerabilities. Like some in older Guzzle versions using the __destruct vector - see PHP GCC that allow .phar files to run and execute code dressed up like images that can take over your system. And that your input disallows .phar execution on input.

See https://knasmueller.net/5-answers-about-php-phar-exploitation

ctf0 commented 4 years ago

try https://github.com/ctf0/Laravel-Media-Manager/wiki/Restrict-Access-To-Path which connects to https://www.dropzonejs.com/#config-acceptedFiles

and see if dz will catch it or not, if yes then we dont need the php side, also part of the article

But I’m safe if I do not allow upload of .phar files, right? Not necessarily. When parsing files, PHP does not care about the file extension as long as the uploaded file has a valid Phar structure. Sam Thomas has demonstrated in his whitepaper that it is possible to embed a phar archive in a valid JPG file, so even if you are filtering your uploads for MIME type etc, you won’t be safe from this attack.

so no 100% coverage against phar even if we found away to check for mime-types

jasperf commented 4 years ago

@ctf0 Did check https://github.com/ctf0/Laravel-Media-Manager/wiki/Restrict-Access-To-Path and do see that this is interesting. Could at least filter right away, by what should be allowed in with something like

@include('MediaManager::_manager', ['restrict' => [
    'path' => 'users/admin/pdfs', 
    'uploadTypes' => ['image/*', 'application/pdf'], // mime-types
    'uploadSize' => 5 // in MB
]])

Thank you for pointing me to this option. Will be testing this week! As for the .phar issue. Yes, no real cure besides patching security holes that the .phar file or embedded .phar code can take advantage of.