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

Question

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

Answer 1

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! =)

Answer 2

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.