Securing an Image Upload Script

Question

How well does this code limit security problems when uploading images to the server?

<?php

$filename = $_FILE['inputfile']['name'];
$upload_path = 'images/';
//extract extension
$ext = strtolower(substr($filename, strrpos($filename, '.') + 1));

// return mime type ala mimetype extension
$finfo = finfo_open(FILEINFO_MIME_TYPE);

//get MIME type of the given file
$mime = finfo_file($finfo, $filename);
//close finfo
finfo_close($finfo);

if (is_uploaded_file($_FILES['inputfile']['tmp_name'])) {
    //first check: file extension and mime type
    if(!in_array($ext, array('jpeg', 'jpg', 'gif', 'png')) && !in_array($mime, array('image/jpeg', 'image/gif', 'image/png')) ){
     die("Error1: Invalid Image type");
    }

    //create image according to MIME type

    //first: JPEG and JPG
    if($ext == 'jpeg' || $ext == 'jpg'){
       //move_uploaded_file ignored because no need to use it
       $im = @imagecreatefromjpeg($_FILE['inputfile']['tmp_name']);
       if($im){
          $createimage = imagejpeg($im, $upload_path.$_FILE['inputfile']['name']);
          if(!$createimage){
            die("Error3: Can't create image!");
          }
          //last check
          $filecontent = file_get_contents($upload_path.$_FILE['inputfile']['name']);

          //clean the file from any php code
          //changed
          while(preg_match('#<\?php|\?>|<\?#i', $filecontent)){
            $filecontent = str_replace(array("<?php", "<?", "?>"), "", $filecontent);
          }
          $handle = fopen($upload_path.$_FILE['inputfile']['name'], "wb");
          fwrite($handle, $filecontent);
          fclose($handle);
       }
       else{

        die("Error2: Invalid Image Detected");

       }
    }

}
?>

Edit:

          while(preg_match('#<\?php|\?>|<\?#i', $filecontent)){
            $filecontent = str_replace(array("<?php", "<?", "?>"), "", $filecontent);
          }

Answer 1

This is a bad example of correcting user input:

$filecontent = str_replace(array("<?php", "<?", "?>"), "", $filecontent);

I could still upload an image with the following comment in it:

<?<?phpphp echo "this code gets executed"; ??>>

because after your str_replace the comment would be

<?php echo "this code gets executed"; ?>

Instead of correcting user input, you should simply search for these strings reject and the image if it contains <?. Why should you save the image anyway if you suspect an hacker attack? ;-)

Edit: This would be better

// test for opening php tags
// including asp-style opening tags (http://www.php.net/manual/en/ini.core.php#ini.asp-tags)
if(strpos($filecontent, "<?") !== false || strpos($filecontent, "<%") !== false) {
  // stop upload
}

Edit: And one other thing I forgot to mention. Never use the original filename of the upload without validating it!

Because you could receive a request like this:

POST /upload.php HTTP/1.1
Host: localhost
Content-Length: 318
Content-Type: multipart/form-data; boundary=----WebKitFormBoundaryePkpFF7tjBAqx29L

------WebKitFormBoundaryePkpFF7tjBAqx29L
Content-Disposition: form-data; name="uploadedfile"; filename="../hello.jpg"
Content-Type: image/jpeg

<?php echo "evil code"; ?>
------WebKitFormBoundaryePkpFF7tjBAqx29L

And the "image" file gets saved in the directory above.

Better generate your own file names.

Answer 2

The whitelist of file extensions is a pretty good way to keep it secure. There are a couple other things you can do to safeguard from someone uploading different file types though.

1. Add an .htaccess file that disables PHP files in the upload directory. This should protect from anyone injecting PHP code into an image.

   <IfModule mod_php5.c>
   php_flag engine off
   </IfModule>
   

2. Use the getimagesize() function to verify there is a size before doing something. getimagesize() will return false if it doesn't have a size. For example:

   if (getimagesize($image) === false) {
    die('This is an invalid file.');
   }