1
\$\begingroup\$

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?

\$\endgroup\$
1
\$\begingroup\$

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"]);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hey Bobby :) Do you have any comments on the security aspect of it all? \$\endgroup\$ – Daniel Nov 14 '13 at 14:21
  • \$\begingroup\$ 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. \$\endgroup\$ – Bobby Nov 14 '13 at 14:46
  • \$\begingroup\$ 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. \$\endgroup\$ – Daniel Nov 14 '13 at 14:50
  • 1
    \$\begingroup\$ 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? \$\endgroup\$ – Bobby Nov 14 '13 at 15:05
  • \$\begingroup\$ Ah the existence of a file on the subdomain.. forgot that :-) That will ofc have to be implemented :) \$\endgroup\$ – Daniel Nov 14 '13 at 15:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.