Upload class | is_allowed_filetype() - possible bug and security issue

[eluser]Unknown[/eluser]
I think i found a bug and a possible security problem with file uploading.

I am sorry if somebody already mentioned this problem but I didn't have much time
- quick search was futile and I think this could be a serious problem (if I'm right)

This is the scenario:

Code:

$config['allowed_types'] = 'txt';

//In my $mimes I have (among other things)
array(
...
'txt'=>'text/plain',
...
);

This configuration allowed me to upload some_file.php!!! This was possible cause php file I managed to upload was text/plain (mime).


When you look into is_allowed_filetype() method:

Code:

function is_allowed_filetype()
{
    if (count($this->allowed_types) == 0 OR ! is_array($this->allowed_types))
    {
        $this->set_error('upload_no_file_types');
        return FALSE;
    }
            
    foreach ($this->allowed_types as $val)
    {
        $mime = $this->mimes_types(strtolower($val));
    
        if (is_array($mime))
        {
            if (in_array($this->file_type, $mime, TRUE))
            {
                return TRUE;
            }
        }
        else
        {
            if ($mime == $this->file_type)
            {
                return TRUE;
            }    
        }        
    }
    
    return FALSE;
}

You can see that there is NO file_ext checking!



I suggest following solution which gives us the ability to check both file extension and MIME type:

Code:

function is_allowed_filetype()
{
    if (count($this->allowed_types) == 0 OR ! is_array($this->allowed_types))
    {
        $this->set_error('upload_no_file_types');
        return FALSE;
    }
    
    //THESE LINES ADDED
    if( !in_array( trim($this->file_ext, ".") , $this->allowed_types ) )
    {
        return FALSE;
    }
    //END ADDITION
            
    foreach ($this->allowed_types as $val)
    {
        $mime = $this->mimes_types(strtolower($val));
    
        if (is_array($mime))
        {
            if (in_array($this->file_type, $mime, TRUE))
            {
                return TRUE;
            }
        }
        else
        {
            if ($mime == $this->file_type)
            {
                return TRUE;
            }    
        }        
    }
    
    return FALSE;
}

Another solution for this problem I found here. That is the replacement for Upload class that checks only file extension!



I would suggest that if you want to keep MIME checking in Upload class, you provide us with a switch. Something like:

Code:

$config['mime_check'] = FALSE;

This mime_check should be applied to following methods
- is_image()
- is_allowed_filetype()


Cheers!

Edit:
Changed erroneous code