Juto Posted March 3, 2012 Share Posted March 3, 2012 Hi, I am trying to develope a snippet for secured image upload. So far I have this snippet just after the line: function upload($file = '', $destination = '', $permissions = '644', $extensions = '') { //Begin Security addition: http://code.google.com/p/doctype/wiki/Articles $file = stripslashes($file); //getimagesize($file)...will return false if the file is not an image or is not accessable, otherwise it will return an array... // is_executable($file) ...Returns TRUE if the filename exists and is executable, or FALSE on error. $file_extension = pathinfo($file, PATHINFO_EXTENSION); if ((is_executable($file) == true) || (getimagesize($file) == false && ($file_extension != 'pdf' || $file_extension != 'zip'))) { $new_extension = 'cya'; //The function replace_extension is defined at the bottom replace_extension($file, $new_extension); fclose($file); //Close file unlink($file); //Clear cache } //End Security addition With: function replace_extension($filename, $new_extension) { $info = pathinfo($filename); return $info['filename'] . '.' . $new_extension; } Sorry to say I've got these errors: Date / Time: 02-03-2012 20:08:06 Error Type: [E_WARNING] getimagesize(products_image) [<a href='function.getimagesize'>function.getimagesize</a>]: failed to open stream: No such file or directory On line 23 File includes/classes/upload.php -+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ Date / Time: 02-03-2012 20:08:06 Error Type: [E_WARNING] fclose() expects parameter 1 to be resource, string given On line 27 File includes/classes/upload.php -+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ Date / Time: 02-03-2012 20:08:06 Error Type: [E_WARNING] unlink(products_image) [<a href='function.unlink'>function.unlink</a>]: No such file or directory On line 28 File includes/classes/upload.php -+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ So, what's wrong? Sara Contributions: http://addons.oscommerce.com/info/8010 http://addons.oscommerce.com/info/8204 http://addons.oscommerce.com/info/8681 Link to comment Share on other sites More sharing options...
MrPhil Posted March 4, 2012 Share Posted March 4, 2012 fclose() does not want a file name, it wants a handle. See http://us.php.net/manual/en/function.fclose.php getimagesize() and unlink() want a file name, are you sure you're feeding them a complete and properly formed and valid filename? See http://us.php.net/manual/en/function.getimagesize.php and http://us.php.net/manual/en/function.unlink.php Link to comment Share on other sites More sharing options...
Juto Posted March 4, 2012 Author Share Posted March 4, 2012 Hi Phil, thanks for your answer. I have changed the snippet and moved it into the parse function like so: function parse() { global $HTTP_POST_FILES, $messageStack; $file = array(); if (isset($_FILES[$this->file])) { $file = array('name' => $_FILES[$this->file]['name'], 'type' => $_FILES[$this->file]['type'], 'size' => $_FILES[$this->file]['size'], 'tmp_name' => $_FILES[$this->file]['tmp_name']); } elseif (isset($HTTP_POST_FILES[$this->file])) { $file = array('name' => $HTTP_POST_FILES[$this->file]['name'], 'type' => $HTTP_POST_FILES[$this->file]['type'], 'size' => $HTTP_POST_FILES[$this->file]['size'], 'tmp_name' => $HTTP_POST_FILES[$this->file]['tmp_name']); } //Begin Security addition: http://code.google.com/p/doctype/wiki/Articles $file_tmp = stripslashes($file['tmp_name']); if ( $file_tmp != '') { //getimagesize($file)...will return false if the file is not an image or is not accessable, otherwise it will return an array... // is_executable($file) ...Returns TRUE if the filename exists and is executable, or FALSE on error. $file_extension = pathinfo($file_tmp, PATHINFO_EXTENSION); if ((is_executable($file_tmp) == true) || (getimagesize($file_tmp) == false && ($file_extension != 'pdf' || $file_extension != 'zip'))) { $new_extension = 'cya'; //The function replace_extension is defined at the bottom replace_extension($file_tmp, $new_extension); // http://us.php.net/manual/en/function.pathinfo.php $handle = fopen($file_tmp, 'r'); fclose($handle); unlink($file_tmp); //Clear cache } } //End Security addition if ( tep_not_null($file['tmp_name']) && ($file['tmp_name'] != 'none') && is_uploaded_file($file['tmp_name']) ) { if (sizeof($this->extensions) > 0) { if (!in_array(strtolower(substr($file['name'], strrpos($file['name'], '.')+1)), $this->extensions)) { if ($this->message_location == 'direct') { $messageStack->add(ERROR_FILETYPE_NOT_ALLOWED, 'error'); } else { $messageStack->add_session(ERROR_FILETYPE_NOT_ALLOWED, 'error'); } return false; } } //We have an image. Now, remove all exif data: //Code to be written: // $this->set_file($file); //mb_strtolower() can be useful: $this->set_filename($file['name']);// $this->set_filename(mb_strtolower($file['name'])); $this->set_tmp_filename($file['tmp_name']);// $this->set_tmp_filename(mb_strtolower($file['tmp_name'])); return $this->check_destination(); // Bugfix // } else { } elseif ($file['name'] != '') { if ($this->message_location == 'direct') { $messageStack->add(WARNING_NO_FILE_UPLOADED, 'warning'); } else { $messageStack->add_session(WARNING_NO_FILE_UPLOADED, 'warning'); } return false; } } The snippet wasn't in the right place previously. I haven't tested the above yet, I would like to have your opinion first. Kind regards Sara Contributions: http://addons.oscommerce.com/info/8010 http://addons.oscommerce.com/info/8204 http://addons.oscommerce.com/info/8681 Link to comment Share on other sites More sharing options...
MrPhil Posted March 5, 2012 Share Posted March 5, 2012 Your code looks very strange to me. //Begin Security addition: http://code.google.com/p/doctype/wiki/Articles $file_tmp = stripslashes($file['tmp_name']); if ( $file_tmp != '') { //getimagesize($file)...will return false if the file is not an image or is not accessable, otherwise it will return an array... // is_executable($file) ...Returns TRUE if the filename exists and is executable, or FALSE on error. $file_extension = pathinfo($file_tmp, PATHINFO_EXTENSION); if ((is_executable($file_tmp) == true) || (getimagesize($file_tmp) == false && ($file_extension != 'pdf' || $file_extension != 'zip'))) { $new_extension = 'cya'; //The function replace_extension is defined at the bottom replace_extension($file_tmp, $new_extension); // http://us.php.net/manual/en/function.pathinfo.php $handle = fopen($file_tmp, 'r'); fclose($handle); unlink($file_tmp); //Clear cache } } //End Security addition Line by line: $file_tmp = stripslashes($file['tmp_name']); get the full file name, including path and extension if ( $file_tmp != '') { so long as the file name is not an empty string, process the rest of it $file_extension = pathinfo($file_tmp, PATHINFO_EXTENSION); grabs the file extension (e.g., 'pdf') if ((is_executable($file_tmp) == true) || (getimagesize($file_tmp) == false && ($file_extension != 'pdf' || $file_extension != 'zip') ) ) { Why would an image file be marked executable? If getimagesize() is false, it was not a valid image file. Your $file_extension test will always be TRUE, as the extension can't be both pdf and zip, but could be neither. I don't understand what you're trying to test here. As written, the file needs to be marked "executable" AND/OR not be an image file. $new_extension = 'cya'; replace_extension($file_tmp, $new_extension); presumably changes the extension to .cya $handle = fopen($file_tmp, 'r'); fclose($handle); unlink($file_tmp); //Clear cache You open the file and then close it without doing any I/O, and then you erase the file. Nothing is accomplished here. You don't seem to use the file under $file_tmp any longer, so no harm done in the following code. If all you want to do is erase the .cya file, the unlink() call should be enough. Note that the file needs to be writable to be erased. Link to comment Share on other sites More sharing options...
Juto Posted March 5, 2012 Author Share Posted March 5, 2012 Hi Phil and thanks for your opinion. I have changed the snippet per your advice. :) What I'm trying to do for image files is to assure that the image EXIF data doesn't contain viruses and the like. So, a better way would perhaps be to simply remove all exif data, "on the fly"? I have searched for a solution for this without succes, so far. Is there a way to do it? Regards Sara Contributions: http://addons.oscommerce.com/info/8010 http://addons.oscommerce.com/info/8204 http://addons.oscommerce.com/info/8681 Link to comment Share on other sites More sharing options...
MrPhil Posted March 5, 2012 Share Posted March 5, 2012 I would think that there's some way to zap an image's EXIF data, if you're afraid that something might be lurking there. These are images that are supplied by members or customers, and therefore might have something nasty in them? I have seen in forum software a three-pronged attack: 1) scan image files for various keywords that indicate that there's script code inside them (and bar uploading them if found), 2) use .htaccess codes on your image directory to ensure that no file can be executed (run) in that directory, and 3) "Attached" files also have their names changed (scrambled) so that a hacker can't easily run them directly in the image directory (also prevents collisions by same-named files). You can look at simplemachines.org (SMF) if you want the details. A possible alternative would be to use the GD library to read in the file and then write it back out under a different extension (type) that doesn't save EXIF data, but I can't say that I'm familiar with the details of this. Link to comment Share on other sites More sharing options...
Juto Posted March 6, 2012 Author Share Posted March 6, 2012 Hi Phil, thanks for the info. I will have a look around for a way to remove exif data. Regards Sara Contributions: http://addons.oscommerce.com/info/8010 http://addons.oscommerce.com/info/8204 http://addons.oscommerce.com/info/8681 Link to comment Share on other sites More sharing options...
Recommended Posts
Archived
This topic is now archived and is closed to further replies.