{"action":"created","issue":{"url":"https://api.github.com/repos/comur/ComurImageBundle/issues/80","repository_url":"https://api.github.com/repos/comur/ComurImageBundle","labels_url":"https://api.github.com/repos/comur/ComurImageBundle/issues/80/labels{/name}","comments_url":"https://api.github.com/repos/comur/ComurImageBundle/issues/80/comments","events_url":"https://api.github.com/repos/comur/ComurImageBundle/issues/80/events","html_url":"https://github.com/comur/ComurImageBundle/issues/80","id":172764070,"number":80,"title":"Security Vulnerability: Browser-Side config params","user":{"login":"antonskv","id":1985362,"avatar_url":"https://avatars.githubusercontent.com/u/1985362?v=3","gravatar_id":"","url":"https://api.github.com/users/antonskv","html_url":"https://github.com/antonskv","followers_url":"https://api.github.com/users/antonskv/followers","following_url":"https://api.github.com/users/antonskv/following{/other_user}","gists_url":"https://api.github.com/users/antonskv/gists{/gist_id}","starred_url":"https://api.github.com/users/antonskv/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/antonskv/subscriptions","organizations_url":"https://api.github.com/users/antonskv/orgs","repos_url":"https://api.github.com/users/antonskv/repos","events_url":"https://api.github.com/users/antonskv/events{/privacy}","received_events_url":"https://api.github.com/users/antonskv/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2016-08-23T17:53:18Z","updated_at":"2016-08-23T18:33:03Z","closed_at":null,"body":"Hi Contributors, \r\n\r\nI 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. \r\n\r\n**Part 1: Upload config from client-side post parameters**\r\n\r\nInside UploadController.php there are lines which take configuration from client-side: \r\n\r\n```\r\n$config = json_decode($request->request->get('config'),true);\r\n```\r\n\r\nSince 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. \r\n\r\nI think this should take such parameters as uploadDir and webDir from server-side configuration inside config.yml file and not from client-side request.\r\n\r\nI quickly fixed it by overriding the array values in beginning of upload and cropping actions after the JSON array gets parsed, like this: \r\n\r\n```\r\n$config = json_decode($request->request->get('config'),true);\r\n\r\n$config['uploadConfig']['uploadUrl'] = \"<forced_folder_name>\";\r\n$config['uploadConfig']['webDir'] = \"<forced_folder_name>\";\r\n$config['uploadConfig']['libraryDir'] = \"<forced_folder_name>\";\r\n```\r\n\r\nBut there it should probably get it like $this->container->getParameter(\"<name of comur param inside config.yml>\") from the server-side configuration. I just needed something quick.\r\n\r\n**Part 2: Controller not checking file being an image**\r\n\r\nSecond 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:\r\n\r\n```\r\nif( !\\getimagesize($request->files->get('image_upload_file')) )\r\n            throw new \\Exception(\"File not an image\");\r\n```\r\n\r\nAnd inside cropping action: \r\n\r\n```\r\n$src = $uploadUrl.'/'.$imageName;\r\n\r\nif( \\getimagesize($src) )\r\n  throw new \\Exception(\"Source file is not an image\");\r\n```\r\n\r\nI hope this is useful. Sorry i didn't have time to fork and just fix it myself.\r\n\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/comur/ComurImageBundle/issues/comments/241830225","html_url":"https://github.com/comur/ComurImageBundle/issues/80#issuecomment-241830225","issue_url":"https://api.github.com/repos/comur/ComurImageBundle/issues/80","id":241830225,"user":{"login":"antonskv","id":1985362,"avatar_url":"https://avatars.githubusercontent.com/u/1985362?v=3","gravatar_id":"","url":"https://api.github.com/users/antonskv","html_url":"https://github.com/antonskv","followers_url":"https://api.github.com/users/antonskv/followers","following_url":"https://api.github.com/users/antonskv/following{/other_user}","gists_url":"https://api.github.com/users/antonskv/gists{/gist_id}","starred_url":"https://api.github.com/users/antonskv/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/antonskv/subscriptions","organizations_url":"https://api.github.com/users/antonskv/orgs","repos_url":"https://api.github.com/users/antonskv/repos","events_url":"https://api.github.com/users/antonskv/events{/privacy}","received_events_url":"https://api.github.com/users/antonskv/received_events","type":"User","site_admin":false},"created_at":"2016-08-23T18:33:03Z","updated_at":"2016-08-23T18:33:03Z","body":"Oh small addition, it probably better to remove the file in same spot in case it turned out not to be an image: \r\n\r\n```\r\nif( !\\getimagesize($request->files->get('image_upload_file')) )\r\n{\r\n            \\unlink( $request->files->get('image_upload_file') );\r\n            throw new \\Exception(\"File not an image\");\r\n}\r\n```\r\n\r\nAnd inside crop action: \r\n\r\n```\r\nif( \\getimagesize($src) )\r\n{\r\n            \\unlink( $src );\r\n            throw new \\Exception(\"Source file is not an image\");\r\n}\r\n```\r\n\r\nNOTE: 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. \r\n"}}
{"actor":{"display_login":"antonskv"}}