1

currently i am saving user uploaded image files as follows:

public_html/img/user/$category/$username/$imagename

however, is this bad practice? Why is it bad to store in document root and where would a better place to store the files be?

i filter extensions as follows:

    // Check to see if the type of file uploaded is a valid image type
function is_valid_type($file)
{
    // This is an array that holds all the valid image MIME types
    $valid_types = array("image/jpg", "image/JPG", "image/jpeg", "image/bmp", "image/gif", "image/png");

    if (in_array($file['type'], $valid_types))
        return 1;
    return 0;
}
neeko
  • 1,930
  • 8
  • 44
  • 67
  • 2
    it's not bad pratice if you practice safe uploading. But if you've got bad/non-existent file validation, putting user-uploaded files into a publically accessible directory can lead to a total compromise of your server, or your server becoming a malware source. So if you're a security rock star, feel free. Otherwise, stuff them OUTSIDE of the document root and provide other means of access. – Marc B Oct 06 '12 at 15:40
  • @Marc B thankyou for your reply, i dont think im a security rockstar, could you explain what you mean by outside the document root? and what you mean by providing other means of access? – neeko Oct 06 '12 at 15:43

2 Answers2

2

Contrary to what some believe is bad practice, the location itself is not the problem; but you have to take care of a few things:

  • Filter by extensions; accept a few image formats (.jpg, .gif, .png) only and reject the rest; definitely don't accept .php extension

  • Don't trust the mime-type sent by the browsers to determine if an image is valid. Use getimagesize() to do this yourself; this can be fooled by hiding a PHP script inside an image, but that's why we have one more important step.

  • Important - Make sure that images are NOT served with the PHP engine; some images can be crafted in a way that it looks like an image but hides a script inside. Use the web server's configuration for this.

  • Other issues you need to be aware about when you're handling uploads

See also: Secure User Image Upload Capabilities in PHP

Btw, to test if the PHP engine is not being used to serve images, make sure the expose_php is On (you can tell from a phpinfo() page. Then download an image with your browser, inspect the response headers and check whether you see the X-Powered-By header; if it's not there, you should be safe.

Community
  • 1
  • 1
Ja͢ck
  • 170,779
  • 38
  • 263
  • 309
  • thankyou for your reply, if i add my code showing how i filter extensions could you check it for me? I will add it to the question, also how would i turn expose_php on? – neeko Oct 06 '12 at 15:51
  • 1
    @neeko You shouldn't trust what the browser sends you as mime type. Updated answer. – Ja͢ck Oct 06 '12 at 15:55
  • i have checked my php.ini file and expose_php = on, how would check the the x-power-by header? also could you give me a code example of a good way to filter extensions, thankyou for all your help! – neeko Oct 06 '12 at 15:58
  • @neeko use the debug facility of your browser; search for Firebug if you're not sure. – Ja͢ck Oct 06 '12 at 16:01
  • thankyou again, i am still unsure of how to inspect the response headers, i have firebug? – neeko Oct 06 '12 at 16:10
  • @Jack: You should at least link one of the more detailed questions, because you just scratched the little top of the mountain what to take into account. And also please link OWASP. – hakre Oct 06 '12 at 16:45
1

Its better to keep publicly visible images in document root. But store only the images you want to show. Not the other one. And make sure these are image files as there are some gotchas.

I have a website where user stores image. But I keep the file name, category, username, imagename into database and the image files in a single directory.

Community
  • 1
  • 1
Shiplu Mokaddim
  • 56,364
  • 17
  • 141
  • 187
  • thankyou for your reply, i store the filename, category, username and imagename in a database and my image files are in the directory as shown above and yes all these images are publicly shown – neeko Oct 06 '12 at 15:50