Jump to content
  • Checkout
  • Login
  • Get in touch

osCommerce

The e-commerce.

Secured image upload


Juto

Recommended Posts

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

Link to comment
Share on other sites

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

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

Link to comment
Share on other sites

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

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

Link to comment
Share on other sites

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

Archived

This topic is now archived and is closed to further replies.

×
×
  • Create New...