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"]);

• Daniel: Hey Bobby :) Do you have any comments on the security aspect of it all?
• Bobby: Only that name could contain anything, otherwise it looks good to me (at least I don't see anything), as long as the server is correctly configured (does not allow uploads of 500MB or so). I don't know what randomName() returns, but better make sure that this never collides.
• Daniel: Ok, so yea.. I was thinking uploads of 5-50MB max. randomName() is just a function that returns a random name to put infront of the original name. But i might even just do a totally random name.
• Bobby: I don't see anything else, that doesn't meant that there isn't, though. Wait a moment, are you checking for the existence of /uploaded/filename.ext but then copying it to /uploads/randomstuffFilename.ext?
• Daniel: Ah the existence of a file on the subdomain.. forgot that :-) That will ofc have to be implemented :)