CODEWITHSUNDEEP
php
q0987
q0987

Reputation: 35982

Should we sanitize $_FILES['filename']['name']?

After the user uploads an image to the server, should we sanitize $_FILES['filename']['name']?

I do check file size/file type etc. But I don't check other things. Is there a potential security hole?

Thank you

Upvotes: 10

Views: 5623

Answers (3)

Alix Axel
Alix Axel

Reputation: 154553

Absolutely! As @Bob has already mentioned it's too easy for common file names to be overwritten.

There are also some issues that you might want to cover, for instance not all the allowed chars in Windows are allowed in *nix, and vice versa. A filename may also contain a relative path and could potentially overwrite other non-uploaded files.

Here is the Upload() method I wrote for the phunction PHP framework:

function Upload($source, $destination, $chmod = null)
{
    $result = array();
    $destination = self::Path($destination);

    if ((is_dir($destination) === true) && (array_key_exists($source, $_FILES) === true))
    {
        if (count($_FILES[$source], COUNT_RECURSIVE) == 5)
        {
            foreach ($_FILES[$source] as $key => $value)
            {
                $_FILES[$source][$key] = array($value);
            }
        }

        foreach (array_map('basename', $_FILES[$source]['name']) as $key => $value)
        {
            $result[$value] = false;

            if ($_FILES[$source]['error'][$key] == UPLOAD_ERR_OK)
            {
                $file = ph()->Text->Slug($value, '_', '.');

                if (file_exists($destination . $file) === true)
                {
                    $file = substr_replace($file, '_' . md5_file($_FILES[$source]['tmp_name'][$key]), strrpos($value, '.'), 0);
                }

                if (move_uploaded_file($_FILES[$source]['tmp_name'][$key], $destination . $file) === true)
                {
                    if (self::Chmod($destination . $file, $chmod) === true)
                    {
                        $result[$value] = $destination . $file;
                    }
                }
            }
        }
    }

    return $result;
}

The important parts are:

  1. array_map('basename', ...), this makes sure that the file doesn't contain any relative paths.
  2. ph()->Text->Slug(), this makes sure only .0-9a-zA-Z are allowed in the filename, all the other chars are replaced by underscores (_)
  3. md5_file(), this is added to the filename iff another file with the same name already exists

I prefer to use the user supplied name since search engines can use that to deliver better results, but if that is not important to you a simple microtime(true) or md5_file() could simplify things a bit.

Hope this helps! =)

Upvotes: 7

deceze
deceze

Reputation: 522091

The filename is an arbitrary user supplied string. As a general rule, never trust arbitrary user supplied values.

You should never use the user supplied filename as the name to save the file under on the server, always create your own filename. The only thing you may want to do with it is to save it as metadata for informational purposes. When outputting that metadata, take the usual precautions like sanitation and escaping.

Upvotes: 5

Bob Baddeley
Bob Baddeley

Reputation: 2262

you also need to check for duplicate names. It's too easy for multiple people to upload an image called 'mycat.jpg', which if uploaded to the same folder would overwrite a previously uploaded file by the same name. You can do this by putting a unique id in the file name (as Prix suggests). Also verify that the file type doesn't just end with an image extension but also is an actual image; you don't want your server acting as a blind host for random files.

Upvotes: 1

Related Questions