0

I am working for a paranoid client. The image uploader I've written must be able to 'survive' multiple penetration tests from some hired guys. Just for some background information: The images are for profile pictures only.

So, what I've got. I've got a HTML form to upload files, and the form can only be accessed by logged in users. The login system is safe, there's no worrying in that. Every user gets an unique user id, which is what I use to identify different users.

Whenever a file gets uploaded, I do the following checks on it:

  • The file must be larger than 128 bytes (stop ddossing)
  • The file must be smaller than 100 kb (the pictures will be displayed as 64x64 anyway)

Both of those checks are done with $_FILES["fileToUpload"]["size"]

  • The file's extension has to be '.png' (done with $imageFileType = pathinfo($target_file,PATHINFO_EXTENSION);)
  • The mime content must be "image/png" (done with mime_content_type($_FILES["fileToUpload"]["tmp_name"]))
  • The file will be renamed to $current_user->ID.".png"
  • All profile images are stored in /resources/img/profilepic/[userid].png
  • The file is moved with move_uploaded_file
  • Loading of the images is done within <img> tags. The code that gets the complete sentence (including the tags) is require'd in another .php file later.

So in short: The files are not named after the original but after [id].png. I haven't been able to find questions which address this too.

Is this bullet proof? Or is my current system just a nice challenge for someone willing to upload a web-shell? If there's a major vulnerability, how can I protect myself against it?

Ricky
  • 11
  • 3
  • 3
    *"The image uploader I've written must be able to 'survive' multiple penetration tests from some hired guys."* - You should ask the hired guys if it's "bullet proof". I think you should move this to code review. – Funk Forty Niner May 22 '17 at 14:52
  • 1
    It's also too broad. – Funk Forty Niner May 22 '17 at 14:54
  • Possible duplicate of [Full Secure Image Upload Script](https://stackoverflow.com/questions/38509334/full-secure-image-upload-script) – Reactgular May 22 '17 at 14:54
  • If the possible duplicate as marked by @ThinkingMedia answers your question, you (Ricky) should also flag it as such. – Funk Forty Niner May 22 '17 at 14:56
  • I wish I could ask them, but the problem is that I don't know who the pen-testers will be, and if it's entirely safe the first time I get a nice bonus. Don't wanna waste that, I hope you understand – Ricky May 22 '17 at 14:56
  • @Fred -ii- It does not answer my question fully sadly. Their code is mostly about dynamic filenames and extensions, whilst mine is only with preset filenames and the extension is always a .png – Ricky May 22 '17 at 14:58
  • I think most of what you've covered is good. The 100kb is very low as a limit, but I think you're failing to understand what security is all about. I would allow the user to upload ANYTHING. You should then tackle security on the web server, and sanitize HTML in your templates. Assume ALL data from users is UNSAFE. It's hard to explain in just a comment. – Reactgular May 22 '17 at 14:59
  • I would load the image and resize it to another image handle before saving it with new compression settings. Failure along any of these steps indicates a bad request by the user. Web server file permissions and server security are the key to success here. – Reactgular May 22 '17 at 15:03
  • Thank you @ThinkingMedia. I think I understand what you mean. I'll do more research on the security and files to broaden up the user's restrictions currently set in place. Resizing is a good idea. I'll do some research if it's duable with speeds. – Ricky May 22 '17 at 15:05
  • Here's another good post on this: https://stackoverflow.com/questions/2851976/block-upload-of-executable-images-php?rq=1 – Reactgular May 22 '17 at 15:07
  • Very helpfull. I'll be using this to increase the security. Thanks! – Ricky May 22 '17 at 15:08
  • Trying to resize it upon upload opens up another can of worms, see the remote code execution vulnerability in php-gd. I think in general what you have described is good but only one aspect, there is also the it security side to it (folder permission, etc.), and of course the actual code, where a one character typo can sometimes make the difference. – Gabor Lengyel May 22 '17 at 15:08
  • *"Their code is mostly about dynamic filenames and extensions, whilst mine is only with preset filenames"* - Dynamic or not, the procedures are still the same. @RickyvanSwaal – Funk Forty Niner May 22 '17 at 17:27

0 Answers0