Improve file upload validation

[axel]

Improve validation on uploaded files.

http://www.exploit-db.com/exploits/14315/

[axel]

Fixed, see joomla security forum for details:

http://forum.joomla.org/viewtopic.php?f=432&p=2214671#p2214671

Revision: 261
Author: axel
Date: 20:11:18, Donnerstag, 29. Juli 2010
Message:
Added validation of uploaded files.

---

Modified : /branch/1.5.x/administrator/components/com_mysms/mysms.phonebook.php
Modified : /branch/1.5.x/components/com_mysms/mysms.frontend.php

[axel]

hi @all,

here is my mysms exploit analysis. Many thanks to mandville for the fast replies today !

At first it is only possible to upload files if a user is registerd and the admin has granted to use the MySMS component. So no anaonymous attacks ( file uploads ) can be done.
I tested some popular php shells ( clim.txt, r57.txt ) wihtout any problems. It was not possible to execute the shell code. This is a good news for me. But I found some problems by the phonebook import.

The exploit "http://www.exploit-db.com/exploits/14315/" propergate that the mysms component does not validate the uploaded data. This is correct. Lets have a short look on the original php code.

Code:
-- snip --
function DoImportphonebook? ()
{

if( !isset( $_FILESphonebook? ) )
{
MySMSRedirect( $this->CreateRedirectUrl?( 'phoneBook' ) , JText::_( 'MYSMS_PHONEBOOKIMPORT_FAILED' ) );
}

$data = $_FILESphonebook?;

//try to open tmp file
$handle = fopen( $datatmp_name?, 'r' );

if( !$handle )
{
MySMSRedirect( $this->CreateRedirectUrl?( 'phoneBook' ) , JText::_( 'MYSMS_PHONEBOOKIMPORT_FAILED' ) );
}

while( ( $data = fgetcsv ( $handle, 1000, ";") ) !== false )
{
list( $name, $number ) = $data;

if( $this->user->_phoneBook->addEntry( $name, $number ) == false )
{
$this->errorHandler->Alert( JText::_( 'MYSMS_SQLQUERY_ERROR' ) , $this->db->getErrorMsg() );
}
}

MySMSRedirect( $this->CreateRedirectUrl?( 'phoneBook' ) , JText::_( 'MYSMS_PHONEBOOKIMPORT_SUCCESSFULLY' ) );
}
-- end snip --

It is correct that the mysms component does not validate the uploaded file, we see that in the function above. This maybe causes "trash" entries in the database.
$this->user->_phoneBook->AddEntry? only checks interal if the given $name and $number is not empty.

If i tried to import the clim.txt I got the following database entries:

Code:
-- database entries --
| 138 | 62 | | $sh_mainurl = "http://coretan-recky.blogspot.com/" |
| 139 | 62 | | $html_start = '. |
| 137 | 62 | | $sh_name = base64_decode($sh_id).$sh_ver |
| 135 | 62 | | $sh_id = "" |
| 136 | 62 | | $sh_ver = "Hacked by bogel ReloaD-X @ DAL.net" |
| 133 | 62 | | /* ReloaD-X@… */ |
| 134 | 62 | | /*******************************************/ |
| 131 | 62 | | /* [ * ] PHP Shell by bogel [ * ] */ |
| 132 | 62 | | /* Re-coded and modified By bogel */ |
| 128 | 62 | ys?? | PK??¶ |
| 129 | 62 | | <?php |
-- end database entries --

So in my opinion the risk of this exploit is very low. I also did a fix, lets have a look to the new code.

Code:
-- snip --
/**
* Import to users phonebook by a csv file
*/
function DoImportphonebook? ()
{

if( !isset( $_FILESphonebook? ) )
{
MySMSRedirect( $this->CreateRedirectUrl?( 'phoneBook' ) , JText::_( 'MYSMS_PHONEBOOKIMPORT_FAILED' ) );
}

$data = $_FILESphonebook?;

[b]* //Is really uploaded file
if( false == is_uploaded_file( $datatmp_name? ) )
{
MySMSRedirect( $this->CreateRedirectUrl?( 'phoneBook' ) , JText::_( 'MYSMS_PHONEBOOKIMPORT_FAILED' ) );
}/b

//Check file type

$type = $datatype?;

if( $type != 'text/comma-separated-values' )
{
MySMSRedirect( $this->CreateRedirectUrl?( 'phoneBook' ) , JText::_( 'MYSMS_PHONEBOOKIMPORT_FAILED' ) );
}*

//try to open tmp file
$handle = fopen( $datatmp_name?, 'r' );

if( !$handle )
{
MySMSRedirect( $this->CreateRedirectUrl?( 'phoneBook' ) , JText::_( 'MYSMS_PHONEBOOKIMPORT_FAILED' ) );
}

while( ( $data = fgetcsv ( $handle, 1000, ";") ) !== false )
{
list( $name, $number ) = $data;

* $this->Filter( $name );
$this->Filter( $number );*

if( $this->user->_phoneBook->addEntry( $name, $number ) == false )
{
$this->errorHandler->Alert( JText::_( 'MYSMS_SQLQUERY_ERROR' ) , $this->db->getErrorMsg() );
}
}

MySMSRedirect( $this->CreateRedirectUrl?( 'phoneBook' ) , JText::_( 'MYSMS_PHONEBOOKIMPORT_SUCCESSFULLY' ) );
}

function Filter( &$param )
{
$param = htmlentities( strip_tags( $param ) , ENT_QUOTES );
}
-- end snip --

At first i check if the entry in $_FILES is really a file upload by using is_uploaded_file api, after that i validate the type.
I also added $this->Filter calls, which strip all tags from the given variable.

I released the version 1.5.12.

Quote:
Revision: 261
Author: axel
Date: 20:11:18, Donnerstag, 29. Juli 2010
Message:
Added validation of uploaded files.

---

Modified : /branch/1.5.x/administrator/components/com_mysms/mysms.phonebook.php
Modified : /branch/1.5.x/components/com_mysms/mysms.frontend.php

Feedback welcome.

best regards,

axel