Does this PHP function protect against file traversal?

Question

I have a URL which will serve a protected file to my user.

The file name is re-written by my application when the file is uploaded, regardless of the name, and stored in my database. So I know it will never contain a "/" or ".."

The filename is: "USER_ID"_"RANDOMMD5".FILE_EXT With "USER_ID" = current logged in user ID and "RANDOM MD5" exactly that.

i.e. 5_ji3uc237uckj92d0jf3932t09ut93f2.pdf

This is my function to serve the file:

function user_file($file_name = "")
{
    if ($file_name)
    {
         // Ensure no funny business names:
         $file_name = str_replace ('..', '', $file_name);
         $file_name = str_replace ('/', '', $file_name);

         // Check if the user is allowed to access this file
     $user_id = substr($file_name, 0, strpos($file_name, "_"));

         // now do the logic to check user is logged in, and user is owner of file
         (($this->ion_auth->logged_in()) && (($user_id === $this->user->id))
         {
                // Serve file via readfile()
         }
    }
}

Question: Is this a secure way to ensure that there is no other way for the person to transverse directories, gain access to other files etc?

edit 1: ion_auth is my authentication library, and "$this->user->id" is the user id stored in my construct

edit 2: The user files are stored outside public_html - and thus only accessible via my application AFAIK

edit 3: My improved code, using the ideas from Amber below, taking into account the fact I need to accomodate for different file extensions, and I'm going to try and avoid a database hit:

function user_files($file_name = "")
{
    // for security reasons, check the filename is correct
    // This checks for a 32bit md5 value, followed by a single "." followed by a 3-4 extension (of only a-z)
    if (preg_match('^[A-Za-z0-9]{32}+[.]{1}[A-Za-z]{3,4}$^', $file_name))
    {
        // Now check the user is logged in
        if ($this->ion_auth->logged_in())
        {
            // Rewrite the request to the path on my server - and append the user_id onto the filename
            // This ensures that users can only ever access their own file which they uploaded
            // As their userid was appended to the filename during the upload!
            $file = MY_SECURE_FOLDER.$this->user->id.'_'.$file_name;

            // Now check if file exists
            if (file_exists($file))
            {
                // Serve the file
                header('Content-Type: '.get_mime_by_extension($file));
                readfile($file);
            }
        }
    }
}

Is the file still in a web-accessible directory? If so, the answer to your question is **NO**. — , May 08 '12 at 05:18
Hi rdlowrey - thanks - I should have mentioned the file is stored outside Public_html - and thus only accessible via the application - thanks for thinking of that - I'll edit my question — Laurence, May 08 '12 at 05:23
Looks ok. The only thing I would be concerned about with a system like this is the possibility that you will have a lot of files all in one directory. On a linux server with ext3 or a windows fat32 file directory you could have performance issues, if the routines involve the use of the readdir() libc call. — gview, May 08 '12 at 05:39

score 2 · Accepted Answer · answered May 08 '12 at 05:19

2

Better idea: have the user supply the MD5, and construct the filename yourself. That way you don't have to do all sorts of crazy checking for user input versus filenames - you can just ensure that the MD5 is a 40-character string of [0-9a-f] only, and then you're good.

answered May 08 '12 at 05:19

Amber

507,862
82
626
550

thanks Amber - the function above is to retrieve the filename - I have already constructed it. I'm trying to making sure that when a user requests a filename, they cant do anything to get other files, i.e. by some sort of dodgy name such as "test../../../php.ini" or something? – Laurence May 08 '12 at 05:22
1

@Laurencei I believe Amber has already explained that part here: `you can just ensure that the MD5 is a 40-character string of [0-9a-f] only` – Andreas Wong May 08 '12 at 05:26
1

@Laurencei by "construct" I don't mean creating the file; I mean putting together the string for the file path the user is requesting. Your function currently takes the entire filename. That's bad, because you have to validate it as a file path which is more complex. Better would be to have it take an MD5 - you already know the user's ID, and you already know the pattern of the filename, so just take an MD5, make sure it's really an MD5, and then see if the file named `"{$this->user->id}_$md5.pdf"` exists. – Amber May 08 '12 at 05:32
Arh... ok Amber - I see what you are saying - that makes perfect sense and is really clean! Except; the extension might be different - i.e. jpg|png|pdf|doc etc - how would you solve that? – Laurence May 08 '12 at 05:33
actually... i dont need to accept the file extension - i can store that myself in my db - so if the md5 matches - i can pull the extension? – Laurence May 08 '12 at 05:36
Sure - though at that point, you should really just store the entire file path in the DB. – Amber May 08 '12 at 06:43

score 2 · Answer 2 · answered May 08 '12 at 05:46

I could traverse to any directory, but I would be limited to file names which start with my user id.

consider

$file_name = '/./.\\1234_anything.anything';
         $file_name = str_replace ('..', '', $file_name);
         echo $file_name = str_replace ('/', '', $file_name);

and as far as file path seperators go, / and \ are generally equivalent.

Does this PHP function protect against file traversal?

2 Answers2

Linked