Secure file upload example The overall question is:  I need feedback on the security of the idea and the script.  Do I need to do more in raw code, or is this ok, taking into account my other information?


Let's assume these following first:


All images are stored on a subdomain (e.g. images.example.com)
The subdomain images.example.com does not have execute rights
The upload script is named upload.php and is located at example.com
I run a virus scanner on images.example.com every day

A simple upload script written in PHP:

$allowedExts = array("gif", "jpeg", "jpg", "png");
$temp = explode(".", $_FILES["file"]["name"]);
$extension = end($temp);
if ((($_FILES["file"]["type"] == "image/gif")
|| ($_FILES["file"]["type"] == "image/jpeg")
|| ($_FILES["file"]["type"] == "image/jpg")
|| ($_FILES["file"]["type"] == "image/pjpeg")
|| ($_FILES["file"]["type"] == "image/x-png")
|| ($_FILES["file"]["type"] == "image/png"))
&amp;&amp; ($_FILES["file"]["size"] &lt; 20000)
&amp;&amp; in_array($extension, $allowedExts))
  {
  if ($_FILES["file"]["error"] &gt; 0)
    {
    echo "Return Code: " . $_FILES["file"]["error"] . "&lt;br&gt;";
    }
  else
    {  
    if (file_exists("upload/" . $_FILES["file"]["name"]))
      {
      echo $_FILES["file"]["name"] . " already exists. ";
      }
    else
      {
      move_uploaded_file($_FILES["file"]["tmp_name"],
      "images.example.com/" . randomName() .$_FILES["file"]["name"]);
      }
    }
  }
else
  {
  echo "Invalid file";
  }


The script is demonstrating my thoughts; nothing more.
So from what I understand now, all images uploaded cant do any harm to example.com, even if they contain injected code?
Do I not need to do a exif or Fileinfo check since I put the files on a subdomain?
I've assumed a lot of things.  Did I miss anything?
First thing I would consider is using functions so that it becomes easier readable.

function hasAllowedExtension($filename); // returns bool
function isAllowedType($filetype); // returns bool


That way you can shorten the first lines to

$file = $_FILES["file"];
if (hasAllowedExtension($file["name"]) &amp;&amp; isAllowedType($file["type"]) &amp;&amp; $file["size"] &lt; 20000) {


which is much more readable.



$allowedExts = array("gif", "jpeg", "jpg", "png");
$temp = explode(".", $_FILES["file"]["name"]);
$extension = end($temp);


There are better and easier ways:

$extension = pathinfo($_FILES["file"]["name"], PATHINFO_EXTENSION);




Also consider making all of this into a function which you only either hand the name of the file $_FILES[$filename] or the file (from $_FILES) directly.



echo "Invalid file";


A die might be appropriate here. die will also print a message but has the added bonus that it immediately stops execution and it is easier to understand that this is really a stopper.

die("Invalid file.");




I couldn't see any gaping holes in it except for only one thing: I can't remember and couldn't find out if $_FILES["file"]["name"] is safe or not. So check if this value can contain path separators or not. And by check I mean, read the documentation carefully.



Also it seems like you're checking for the file in a different location/under a different name than you actually save it as.

if (file_exists("upload/" . $_FILES["file"]["name"]))
...
move_uploaded_file($_FILES["file"]["tmp_name"],
  "images.example.com/" . randomName() .$_FILES["file"]["name"]);