4
\$\begingroup\$

I am writing a basic image upload script in PHP, and am looking for critiques. The script below is the result of various suggestions I have found online... for now I am running it locally and it works fine, but I would love for someone more experienced to look over it.

Is this secure? Poorly implemented? Too redundant? Not redundant enough? What am I missing and/or doing incorrectly? Any suggestions/help/advice is GREATLY appreciated. Thank you.

PHP:

if (isset($_FILES['image']))
{
    $files_array = $_FILES['image'];
    $num_of_image_uploads = count($files_array['name']);

    // Loop through array of file uploads
    for ($i = 0; $i < $num_of_image_uploads; $i++)
    {
        // Check if there is a file to upload
        // If so -> define, validate, and upload
        if ($files_array['tmp_name'][$i] != "")
        {
            $errors = false;    // Flag for errors
            $allowed_ext = array('jpg', 'jpeg', 'png', 'gif');

            $file_name = $files_array['name'][$i];
            $file_ext = strtolower(end(explode('.', $file_name)));
            $file_size = $files_array['size'][$i];
            $file_tmp = $files_array['tmp_name'][$i];

            // Extension check 1
            if (in_array($file_ext, $allowed_ext) === false)
            {
                echo "Extension not allowed. <br/>";
                $errors = true;
            }

            // Extension check 2 - if file is not empty
            if ($file_size == 0)
            {
                echo "File is empty. <br/>";
                $errors = true;
            }
            else if (!getimagesize($file_tmp))
            {
                echo "Not an image. <br/>";
                $errors = true;
            }

            // Extension check 3
            $blacklist = array(".php", ".phtml", ".php3", ".php4");
            foreach ($blacklist as $item) 
            {
                if(preg_match("/$item\$/i", $file_name)) 
                {
                    echo "File not allowed. <br/>";
                    $errors = true;
                }
            }

            // File size check
            if ($file_size > 4718592)
            {
                echo "File size must be under 4.5MB. <br/>";
                $errors = true;
            }

            // Rename file
            $file_name = preg_replace("/[^A-Z0-9_]/i", "_", $file_name);
            $file_name = md5($i.$file_name.time());

            // Insert if no errors
            if ($errors == false)
            {
                if (move_uploaded_file($file_tmp, 'uploads/'.$file_name.'.'.$file_ext))
                {
                    echo "Success! File uploaded. <br/>";
                }
            }
        }
    }       
}

HTML:

<body>
  <form enctype="multipart/form-data" action="" method="post">

    <input name="image[]" type="file" />
    <input name="image[]" type="file" />
    <input name="image[]" type="file" />
    <input name="image[]" type="file" />

    <input type="submit" value="Upload" />

  </form> 
</body>
Improve this question
\$\endgroup\$

2 Answers 2

Reset to default
4
\$\begingroup\$

Your script has one underlying assumption that invalidates the whole set of security checks: That the value of $files_array['name'] is safe - it is not. This value is user supplied, and can very easily be spoofed to pass malicious data into your server that would get through all your security checks.

Neither $_FILES['control']['name'] nor $_FILES['control']['type'] are safe and you should not use either value for anything, ever.

In order to make your image uploads as safe as possible you must do all of the following:

  • Verify that the data you are working with is an uploaded file (move_uploaded_file() / is_uploaded_file() will take care of this for you)
  • Create the file name used to store the file on the file system entirely dynamically. You cannot use user input in any way to generate this. If your application requires that you store the original file name, you should keep this association in an external data store such as a database.
  • Verify that the image really is an image. The GD extension can verify the headers for you (use the getimagesize() function).
  • Copy the pixel data out of the image into a new one. Again, GD can do this for you (imagecopyresampled()). It is important to do this - it is possible to hide malicious code inside something with headers that make it appear to be a valid image. Resampling the image will destroy such malicious code.
  • I recommend that if you store the image on the file system, store it outside the document root and proxy the retrieval process with a script, so it cannot be directly accessed via the web server. The above measures should mitigate the implied security risk here, but better safe than sorry ;-)
  • Never include/require a file uploaded by a user. If you want to provide a file download, always use filesystem functions. readfile() is often the most appropriate choice.

Unfortunately, in order to comply with all these requirements you are basically going to need to throw your code out and start again.

If you want something for reference, this Gist contains something that's not a million miles away from the code I use in my real-life applications, and contains a basic usage example.

Improve this answer
\$\endgroup\$
9
  • \$\begingroup\$ i disagree that you need to resample the picture, if you always serve it with a safe content-type, the risk is minimal. even big sites like imgur do not resample your images. \$\endgroup\$
    – Janus Troelsen
    Apr 8, 2013 at 13:39
  • 1
    \$\begingroup\$ @Ysangkok It's not about how you serve it, it's about what's theoretically possible. If you can trick another script on the server into includeing the file, and you can manage to hide (the classic example) <?php exec('rm -rf /'); ?> in the pixel data, you're screwed. Never trust anyone, not even yourself ;-) \$\endgroup\$
    – DaveRandom
    Apr 8, 2013 at 13:46
  • 1
    \$\begingroup\$ @Ysangkok Big difference between include and eval() though. Probably 99.9% of PHP apps ever made use an include statement somewhere, but I know I'm not in the habit of eval()ing data from the database. It's quite feasible that somewhere on the server (may not even be my app) there is a script with an include hole in it. The most dangerous and harmful attacks often utilise more than one attack vector, via multiple routes into the server. \$\endgroup\$
    – DaveRandom
    Apr 8, 2013 at 21:13
  • 3
    \$\begingroup\$ The "oh, it'll probably be fine" approach to security has historically not worked out well for most people... \$\endgroup\$
    – DaveRandom
    Apr 8, 2013 at 21:22
  • 1
    \$\begingroup\$ The argument here seems to be against taking an inexpensive one-shot security measure simply because the exploit it avoids is complex and difficult to achieve. That argument just doesn't hold water for me - it costs me maybe 1/10 of a second of CPU cycles to resample the image, to make it impossible for a known attack vector to be exploited. I cannot see the sense in not doing it. \$\endgroup\$
    – DaveRandom
    Apr 8, 2013 at 23:50
0
\$\begingroup\$

Why are you using preg_match if you don't need it? The unescaped dot in a regex will match any char. Even if the dot wasn't there, your code is still vulnerable to regex injection.

Also, why are you sanitizing the filename if you're hashing it anyway? Why use hashing at all? Is it so important that the original filename is lost? What about hash collisions? Why not just base-16 it?

Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the response. I am a beginner when it comes to security, and I am not really sure what I'm doing. I tried to implement security measures I have found in various forums/tutorials, but I don't fully understand all the threats I need to protect against. Sanitizing and hashing the name was just my attempt to rename the file something safe, unique, and random. It sounds like I am going about that wrong. Any suggestions on a better solution? Also, can you elaborate on regex injection, why I am vulnerable, and how I can prevent it? Thanks! \$\endgroup\$
    – Mark Miller
    Apr 7, 2013 at 22:21
  • \$\begingroup\$ just use php.net/preg_quote and hex (base-16) the user supplied data instead of hashing it. and if the file already exists, refuse the upload. \$\endgroup\$
    – Janus Troelsen
    Apr 8, 2013 at 13:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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