[eluser]reconstrukt[/eluser]
Hi,
The upload class has what appears to be a potential [script execution] security issue with the way the extension is handled.
In the main do_upload() method, the flow goes:
1. do some checks, confirm file's legit
2. set class vars, specifically:
a. set file name (passing $_FILES[field]['name'] to _prep_filename())
b. set file extension (passing $_FILES[field]['name'] to get_extension())
3. clean the file name, removing bad chars - via clean_file_name()
In a nutshell: filename is cleansed in step 2a, but the cleansed filename is not used when retrieving the extension in 2b. Further, when bad characters are removed in step 3, they're also removed from the filename's extension; however, the file extension is never reset to the newly-cleansed string.
The extension is therefore passed, uncleansed, back out of the Upload class.
We could therefore envision an attack that injects bad characters into the file extension (string after the last "."), which would be passed uncleansed to an outside script. If this outside script does any further manipulation on the file that utilizies the uncleansed extension, the attack would manifest itself.
So,
I would propose a change to the main do_upload method that focuses specifically on filename cleansing, the sole purpose being to set a fully cleansed filename from which all other properties (i.e. extension) are derived.
Code in question starts on line 186:
Code:
// Set the uploaded data as class variables
$this->file_temp = $_FILES[$field]['tmp_name'];
$this->file_name = $this->_prep_filename($_FILES[$field]['name']);
$this->file_size = $_FILES[$field]['size'];
$this->file_type = preg_replace("/^(.+?);.*$/", "\\1", $_FILES[$field]['type']);
$this->file_type = strtolower($this->file_type);
$this->file_ext = $this->get_extension($_FILES[$field]['name']);
// ... some other stuff here, then
// Sanitize the file name for security
$this->file_name = $this->clean_file_name($this->file_name);
Proposed fix:
Code:
// cleanse the filename 1st
$this->file_name = $this->clean_file_name( $this->_prep_filename($_FILES[$field]['name']) );
// Now set uploaded data as class variables
$this->file_ext = $this->get_extension( $this->file_name );
$this->file_temp = $_FILES[$field]['tmp_name'];
$this->file_size = $_FILES[$field]['size'];
$this->file_type = preg_replace("/^(.+?);.*$/", "\\1", $_FILES[$field]['type']);
$this->file_type = strtolower($this->file_type);
// ... continue
Simple: by passing get_extension() a cleansed filename, you assure that the extension derived is also cleansed.
Additionally:
clean_file_name() should have a hashmark '#' added to the bad char bag, as a file with '#' in the name will break the URL in the file/image src.
I do this now by extending the Upload class, and overriding the clean_file_name() method to also pass the fname thru url_title() -- but, at the very least, '#' chars should already be stripped out by the parent method.
Any thoughts would be great, cheers.
Matt
