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.

Improve this question
4
  • 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) Commented Sep 6, 2011 at 8:44
  • 2
    777 does not sounds secure. Possible related to stackoverflow.com/questions/3644138/… Commented Sep 6, 2011 at 8: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 Commented Sep 6, 2011 at 8:47
  • I call the upload folder "private" as opposite to public (under the web root folder) Commented Sep 6, 2011 at 8:47

3 Answers 3

Reset to default
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).

Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

(-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 ... :-/
@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
@Raffael1984 True. I'm not certain why that step is necessary at all. Updated the answer.
@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.
@Rook image is not a file, but a directory. As such, +x is necessary to allow the user to traverse subdirectories.
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.

Improve this answer

10 Comments

The maximum uploadable filesize is already limited in php.ini.
Yes it is, but it doesn't mean it's enough to avoid exceeding RAM limit.
If it exceeds the size in php.ini you don't get a filename in $_FILES - so you cannot check the size of it.
A minimum and maximum filesize check also exists. There is also a check for php post_max_size ini set
@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?
|
-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.

Improve this answer

4 Comments

I don't see how PHP code would be executed in a JPG file unless your server is misconfigured?
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).
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.
@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) : ha.ckers.org/blog/20070604/…

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.