Securing file uploading

Question

I'm working on this snippet of code within a PHP file. The upload function is working but I suspect it is not secure. I move all the files into an upload folder within the site folder. I think that some of this could be tricked into uploading malicious files but I'm not sure how it would be done and more importantly, how to adjust this code to be more secure.

Basically what I'm trying to do is:

1. Allow Word and PDF documents to be uploaded to a sitefolder's upload folder.
2. Append the date to the filename.
3. Move the file from the PHP temporary directory into the destination direction mentioned in step 1.

Other note: I've read on some similar threads that files should not be uploaded to the wwwroot folder, but I'm not sure why that is the case. I understand that if a hacker gains access to some folder X and Y resides within X, that the hacker will have access to X and Y, but would the hacker necessarily be able to gain access to files up the www root directories?

<?php

$allowedExts = array("docx", "DOCX", "DOC", "doc","pdf","PDF");
$temp = explode(".", $_FILES["file".$x]["name"]);
$extension = end($temp);
$destinationdirectory= "C:\inetpub\wwwroot\sitefolder\upload\\";
$date=date("m-d-Y-his", time());
$originalname=(string)$_FILES["file".$x]["name"];
$newfilename=$date." ".$originalname;
$newfilename=preg_replace('/\s+/', '', $newfilename);//Need to remove all white space to make url friendly
//Validation of files types
if ((($_FILES["file".$x]["type"] == "application/pdf") || ($_FILES["file".$x]["type"] == "application/msword")) && in_array($extension, $allowedExts)) {
      if ($_FILES["file".$x]["error"] > 0) {
            echo "Return Code: " . $_FILES["file".$x]["error"] . "<br>";
      } else {
          if (file_exists($destinationdirectory . $_FILES["file".$x]["name"])) {
              echo $_FILES["file".$x]["name"] . " already exists. ";
          } else {
              move_uploaded_file($_FILES["file".$x]["tmp_name"], destinationdirectory . $newfilename);
              //Now that the file is validated, renamed and uploaded to the server, get the vars together to store them inside of the array.
              $filelocation="http://mywebsite.com/upload/".$newfilename;
          }//File moving success loop.    
      }
} //IF FILETYPE IS CORRECT AND ALLOWED.
else {
    //Upload error or no file chosen for upload, add it to the array.
    array_push($uploadserrors,$actiondoctypesindex[$x]);
}

?>

Answer 1

Readability

It's a bit difficult to comment on the security aspects of code that's hard to read. There are several readability issues here:

• The indentation is very messy and inconsistent throughout. It would be good to clean that up and use consistent indenting, for example use 4 spaces inside every code block (inside { ... })

• $_FILES["file".$x] is duplicated in many places, which makes the code look very noisy. You could save it in a local variable and the whole thing will look a lot simpler.

• It's good to put a space around operators, for example:

  • instead of: $newfilename=$date." ".$originalname;
  • write like this: $newfilename = $date . " " . $originalname;

Simplify

Other simplifications are possible.

Instead of listing up all the lowercase and uppercase variants of extensions like this:

$allowedExts = array("docx", "DOCX", "DOC", "doc","pdf","PDF");

You could use list just in lowercase, and when you need to check the extension, just wrap that in strtolower, like this:

in_array(strtolower($extension), $allowedExts)

---

You could reduce one nesting level in the conditions. Instead of this:

  if ($_FILES["file".$x]["error"] > 0) {
    // ...
  } 
  else {
    if (file_exists($destinationdirectory . $_FILES["file".$x]["name"])) {
      // ...
    } else {
      // ...
    }
  }

You could use an elseif:

  if ($_FILES["file".$x]["error"] > 0) {
      // ...
  } elseif (file_exists($destinationdirectory . $_FILES["file".$x]["name"])) {
      // ...
  } else {
      // ...
  }