Secure file upload example

Question

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?

1. 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

2. 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"))
   && ($_FILES["file"]["size"] < 20000)
   && in_array($extension, $allowedExts))
     {
     if ($_FILES["file"]["error"] > 0)
       {
       echo "Return Code: " . $_FILES["file"]["error"] . "<br>";
       }
     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.

3. So from what I understand now, all images uploaded cant do any harm to example.com, even if they contain injected code?

4. Do I not need to do a exif or Fileinfo check since I put the files on a subdomain?

5. I've assumed a lot of things. Did I miss anything?

Answer 1

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"]) && isAllowedType($file["type"]) && $file["size"] < 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"]);