Welcome Guest, Not a member yet? Register   Sign In
Upload class | is_allowed_filetype() - possible bug and security issue
#1

[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
#2

[eluser]dreamynsx[/eluser]
what if someone uploads a php file as script.php.jpg and passes in mime-time of image/jpeg?
#3

[eluser]dimethroxy[/eluser]
[quote author="dreamynsx" date="1230143602"]what if someone uploads a php file as script.php.jpg and passes in mime-time of image/jpeg?[/quote]

It doesn't matter, .php.jpg will not get executed by the server.
The problem is that .php can be uploaded and possibly EXECUTED.
The is a serious security risk that had been said in a lots of thread.
The upload class is totally unsafe to use and the codeigniter team should do something about it.
#4

[eluser]jpi[/eluser]
I am worried about this issue.

Can an admin confirm it please ? (I am too lazy to test by myself :p)




Theme © iAndrew 2016 - Forum software by © MyBB