12

I develop a php script to replace a current one, that will have a lot of exposure to various markets/countries. This script between others offers an photo upload functionality .

After a lot of reading about the issue, I followed the approach described below. I would deeply appreciate your comments on its security.

  1. The photo is uploaded in a private 777 folder outside web root.
  2. A check for white listed extensions is performed (allow only jpgs, gifs, pngs) everything else is deleted.
  3. Use of getimagesize to check of min-max dimensions and photo validity.
  4. Check of mimetype and file extension match.
  5. Resizing of uploaded photo to std dimensions (using imagecopyresampled).
  6. Saving the created files as jpg.
  7. Removal of original file.
  8. Save photos with a new (not random name) ie img51244.jpg.
  9. Move the new photos to variable subdirectories of a public folder (777 permissions) according to a non predictable algorithm. I.e., img10000.jpg will be stored at photos/a/f/0/img10000.jpg while img10001.jpg will be stored at photos/0/9/3/img10001.jpg. This is done for other reasons (use of subdomains for static content serve or use of a CDN).

The script will run on a linux dedicated server.

Sam
  • 7,252
  • 16
  • 46
  • 65
Alex
  • 121
  • 3
  • nothing jumps out at me here. apart from the 777 permission on the folder - as i understand it, if it has 777 permissions, it isn't private. but as far as i can tell this would only really matter if your server was compromised (which doesn't look likely to me through this script atleast) – totallyNotLizards Sep 06 '11 at 08:44
  • 2
    777 does not sounds secure. Possible related to http://stackoverflow.com/questions/3644138/secure-user-image-upload-capabilities-in-php – ajreal Sep 06 '11 at 08:46
  • Sounds very good to me, except maybe for very liberal permissions. Maybe they can be limited somewhat? Otherwise, this does everything necessary I can think of, including the removal of EXIF data – Pekka Sep 06 '11 at 08:47
  • I call the upload folder "private" as opposite to public (under the web root folder) – Alex Sep 06 '11 at 08:47

3 Answers3

4
  1. A directory with chmod 0777 is, by definition, public to other users logged into your server, not private. The correct permissions would be 700 and being owned by apache (or whatever user your webserver runs at). I'm not sure why you wouldn't use php's default temporary directory here, since it tends to be outside of the web root too.
  2. A white-list is a good idea. Be careful to have a correct implementation. For example, the regexp /.png/ actually matches apng.php.
  3. This step is a great idea. It basically checks the file magic.
  4. Is not strictly necessary. In the two previous steps, we have determined that extension and file format are correct. If you require a correct MIME type to be specified by the client, you should also check that the given MIME type and the one determined above are equivalent.

Steps 5 to 8 are not security-related.

Step 9: I'm assuming that your site allows everyone to see every photo. If that isn't the case, you should have a URL scheme with substantially longer URLs (say, the hashsum of the image).

phihag
  • 278,196
  • 72
  • 453
  • 469
  • 1
    (-1) 777 doesn't mean public in terms of accessabilty through the web - this is what he is talking about. And your regex example implies that this is of any imortance anyway ... and BTW exe is a windows extension ... :-/ – Raffael Sep 06 '11 at 08:49
  • @Raffael1984 I think phihag knows that. It's still public in terms of *accessibility to other users on the same web server*, which *is* an issue. But I agree that step 2) is unnecessary if you do 3) properly – Pekka Sep 06 '11 at 08:51
  • @Raffael1984 True. I'm not certain why that step is necessary at all. Updated the answer. – phihag Sep 06 '11 at 08:53
  • @Raffael1984 Updated the ending in the example to `.php`, although it's just an example of something that should not slip through. Updated the answer again to clarfiy HTTP availability and system-wide public directories. – phihag Sep 06 '11 at 08:55
  • @Rook `image` is not a file, but a directory. As such, +x is necessary to allow the user to [traverse subdirectories](http://en.wikipedia.org/wiki/Filesystem_permissions#Permissions). – phihag Sep 06 '11 at 20:30
3

You should also check uploaded file size, as getimagesize can sometimes exceed available RAM memory. It's also good to assume that your script can crash at any point (for example when the electricity go down), so you should implement some clean up procedures to remove left, unneeded files.

Sebastian Nowak
  • 5,607
  • 8
  • 67
  • 107
  • 1
    The maximum uploadable filesize is already limited in php.ini. – Maerlyn Sep 06 '11 at 08:45
  • Yes it is, but it doesn't mean it's enough to avoid exceeding RAM limit. – Sebastian Nowak Sep 06 '11 at 08:47
  • If it exceeds the size in php.ini you don't get a filename in `$_FILES` - so you cannot check the size of it. – Maerlyn Sep 06 '11 at 08:49
  • A minimum and maximum filesize check also exists. There is also a check for php post_max_size ini set – Alex Sep 06 '11 at 08:49
  • @Maerlyn: Yes genius, it won't. But what if the limit in php.ini is set to 128mb and 32mb is enough to crash the script? – Sebastian Nowak Sep 06 '11 at 08:51
  • Sebastian has a point: I could upload a 100000 x 100000 pixel JPG that is 500 kilobytes small – Pekka Sep 06 '11 at 08:52
  • It's the sysadmin's responsibility to keep the setting at a reasonable value. Also: as Pekka says, there's no direct correllation between file size and image size, so I don't think you can avoid calling getimagesize. – Maerlyn Sep 06 '11 at 09:03
  • I'm sorry for people who work with you, if you really think you can rely on sysadmin. There's an old good saying "A good programmer looks both ways when passing one-way street". And yes, we can't really avoid getimagesize without involving external scripts so that's why I've mentioned clean up procedures. – Sebastian Nowak Sep 06 '11 at 09:09
  • I'm sorry for people who work with you, if you really think you can rely on sysadmin. There's an old good saying "A good programmer looks both ways when passing one-way street". And yes, we can't really avoid getimagesize without involving external scripts so that's why I've mentioned clean up procedures. – Sebastian Nowak Sep 06 '11 at 09:10
  • @Sebastian Nowak. The defaults of the script are max upload size 5,5MB (an average of a 10MPixel photo) and 128MB php memory-limit – Alex Sep 06 '11 at 09:17
-1

That's a quite complete approach, but I do not see any code execution prevention mechanism.

You should make sure that the content of the image is never included (with an include or require call) or executed through eval().

Otherwise, php code included at the end of the file could be executed.

You can also try to detect php code inside the image content (with file_get_contents, and then a regex searching for " < ? php " for instance ) but I could not find a 100% secure way to eliminate suspicious code without destroying some (valid)images.

Benjamin Dubois
  • 971
  • 7
  • 11
  • I don't see how PHP code would be executed in a JPG file unless your server is misconfigured? – Pekka Sep 06 '11 at 08:47
  • It has no point, as it's obvious nobody is gonna call eval or include on the image. I can't think of any scenario where someone would ever think of doing it (unless he just started using PHP). – Sebastian Nowak Sep 06 '11 at 08:49
  • The original file is deleted as soon as the resampled photos occur. it is used only with following php functions in that order: move_uploaded_file, filesize, getimagesize, imagecreatefromjpeg, imagecopyresampled. – Alex Sep 06 '11 at 09:04
  • @Sebastian Nowak : You can't assume that developers won't make mistakes. There have been known attacks on widely spread systems such as phpBB with code injection through image uploading (avatars in their case). Saying that nobody will never make this mistake again will just cause the flaw to be forgotten, until someone uses it again. I found this post with interesting ways to exploit such vulnerabilities (comments are good too, with an example of null-char injection for keeping the .php ext on the filename) : http://ha.ckers.org/blog/20070604/passing-malicious-php-through-getimagesize/ – Benjamin Dubois Sep 06 '11 at 12:12