Security Vulnerability: Browser-Side config params

[antonskv]

Hi Contributors,

I checked 1.3 version branch to make sure it wasn't already fixed. But our penetration test team has discovered a vulnerability which exposes the server to good amount of security concerns. I fixed it by just quickly modifying the source code. I will describe it here including the quick fix. It will need to be done more elegantly, but it will give the general idea. Hope it's useful.

Part 1: Upload config from client-side post parameters

Inside UploadController.php there are lines which take configuration from client-side:

$config = json_decode($request->request->get('config'),true);

Since the directories are passed inside this JSON array, you can substitute them to anything you want, and files get uploaded to where webuser has access to write. This opens a door to massive security issues.

I think this should take such parameters as uploadDir and webDir from server-side configuration inside config.yml file and not from client-side request.

I quickly fixed it by overriding the array values in beginning of upload and cropping actions after the JSON array gets parsed, like this:

$config = json_decode($request->request->get('config'),true);

$config['uploadConfig']['uploadUrl'] = "<forced_folder_name>";
$config['uploadConfig']['webDir'] = "<forced_folder_name>";
$config['uploadConfig']['libraryDir'] = "<forced_folder_name>";

But there it should probably get it like $this->container->getParameter("") from the server-side configuration. I just needed something quick.

Part 2: Controller not checking file being an image

Second part is that there is no verifications to make sure that file being uploaded is in fact an actual image and not something else. This can be fixed by native PHP function before processing anything, as that function returns nothing in case file is not an image, just for sake of an example:

if( !\getimagesize($request->files->get('image_upload_file')) )
            throw new \Exception("File not an image");

I hope this is useful. Sorry i didn't have time to fork and just fix it myself.

[antonskv]

Oh small addition, it probably better to remove the file in same spot in case it turned out not to be an image and quickly check for MIME type as well:

if( !\getimagesize($request->files->get('image_upload_file')) || !\in_array( \mime_content_type($request->files->get('image_upload_file')), ['image/gif', 'image/png', 'image/jpg', 'image/jpeg']) )
{
            \unlink( $request->files->get('image_upload_file') );
            throw new \Exception("File not an image");
}

NOTE: There is a way around this function as well. Maybe there should be more extensive MIME type checking and so on to make it more secure.

[comur]

Hi @antonskv

Big issue in fact, I did never think about it, we should change this ASAP. I will try to find a way to get these urls from entity or form type (and not only from config because it can be different than default values). Tell me if you see a way to do it quickly

Thanks

[antonskv]

@comur No, you can use config file params. Default values are overridden inside main symfony config.yml, no? So if person wants to change the defaults they override the comur_image configs in their own configuration.

So i have web dir override inside my own symfony's config.yml:

comur_image:
    config:        
        web_dirname: '/imgs'

So anywhere you pull "comur_image.config.web_dirname" parameter will return the value i set with the override, and not the default one from original bundle.

What you think?

[ignasdamunskis]

Any news on this issue?

[ofeugret]

Hello and thank you for this bundle, I would like to use the 1.3.*@dev version for a Symfony3 app, is it possible for you to fix these critical issues?