PHP Simple Image Upload Function Class

Ok this is a very quick question but what am i doing wrong? I have a simple image file upload script that does everything I need it to do apart from showing the error messages when they occur! Am i doing something stupid or am i missing something that prevents it from echoing the message? Currently i’m simply echoing $error; Thanks all help much appreciated!

Here is my function:

<?php
/**
 * Upload an Image Function
 * @param  string $tag       the name of the input field that was used to upload the file
 * @param  int $size         the allowed size of the file in bytes
 * @param  array $format     the allowed formats for the file
 * @param  string $filename  the base name of the file after upload
 * @param  string $directory the directory in which the file will be stored
 * @param  string $error	 the error generated from the function
 * @return boolean           returns true if the file was uploaded successfully, otherwise false
 */
 
function uploadFile($tag,$size,$format,$filename,$directory,$error=null) {
	@$imageFileType = array_pop(explode(".", strtolower($_FILES[$tag]["name"])));
	$target_file =  $directory.'/'.$filename.'.'.$imageFileType;
	// Check if image file is a actual image or fake image
	if(getimagesize($_FILES[$tag]["tmp_name"]) !== false) {
		// Check if file already exists
		if (!file_exists($target_file)) {
			// Check file size
			if ($_FILES[$tag]["size"] <= $size) {
				// Allow certain file formats
				$check = false;
				foreach ($format as $key => $value) {
					if ($imageFileType == $value) {
						$check = true;
					}
				}
				if ($check) {
					// if everything is ok, try to upload file
					if (move_uploaded_file($_FILES[$tag]["tmp_name"], $target_file)) {
						return true;
					} else {
						$error = 'There was an error uploading your file';
					}
				} else {
					$error = 'The allowed formats are ';
					foreach ($format as $key => $value) {
						$error .= $value.', ';
					}
					chop($error, ', ');
				}
			} else {
				$error = 'Your file is larger than '.($size/1000).' KBs';
			}
		} else {
			$error = 'File already exists';
		}
	} else {
		$error = 'Upload a valid file';
	}
	return false;
}

Here is my php code validation:

$ext = strtolower(end(explode('.', $_FILES['photo']['name'])));
if(uploadFile('photo',10000000,array('jpeg','jpg','png','gif'),$data['photo_name'],'uploads/users/') === TRUE){
						if( $this->user->updateUserPicture($data, $ext) ){
							flash('message', 'Your profile image has been changed!', 'alert alert-success mb-0');
							redirect('user/settings/#tab3');
						} else{
							die(LANG['somethingwentwrong']);
						}
					} else{
						echo $error;
					}

Thanks very much

A class that does everything is NOT simple. The purpose of OOP is break things down to simple functions/methods so that it is versatile.

Maybe this will help you spot the problem

	return false;
}
...
if(uploadFile( ... ) === TRUE){ 

* @return boolean returns true if the file was uploaded successfully, otherwise false

Nope!

				// if everything is ok, try to upload file
				if (move_uploaded_file($_FILES[$tag]["tmp_name"], $target_file)) {
					****return true;
				} else {
					$error = 'There was an error uploading your file';
					echo $error;
				}

Please stop relying on this. This can easily get exploited with ease.

This doesn’t help with anything. What is it doing? What are you expecting it to do?

1 Like

I’m simply trying to get the error messages to show! An i built a simple function that can be easily integrated with my mvc but if its bad how do i improve it?

Thanks

Don’t rely on file types and don’t rely on file extensions. Check the mime content type of the temp file. The temp file consists of the original mime. And since temporary files don’t have file extensions, no one can spoof them. You can then safely check the original mime of the temporary file. Each file has something called a Mime Content Type. The Mime Content Type is the original type for that file. For instance, if we create an image from say Adobe Photoshops and export it to JPG, that image will contain a Mime Content Type of image/jpg. Now if we create an image via a text editor like Adobe Dreamweaver or Notepad++ and add a file extension of .jpg, the original Mime Content Type will always be text/plain or of the sort. But if rely on the file extension, the file does indeed have a .jpg extension. The extension really has no say in what the actual file is. A malicious PHP file can be disguised in a .jpg extension if they really wanted to. So relying on the ['type'] or file extensions really just leads to a huge security hole. The Mime Content Type will always remain the same no matter how much you change the file extension. The ['type'] may hold image/jpg, but the original Mime Content Type will always hold text/plain. Therefore, someone can bypass your upload system and they can pretty much try to get the malicious file to execute.

Wow thankyou very much, i’ll update my bit of code to support this and I got the error messages to work by adding echo $error;

Thanks

1 Like

Checking the mime type is part one of the story. Part two is making sure that the file has the extension that goes with the detected mime type, as mime types can be spoofed as well (it’s basically just putting the right bits in the right place).

So you would need to do the following:

  1. Check the mime type of the uploaded file
  2. Check the extension of the uploaded file
  3. If the mime type and extension of the uploaded file do not match, add the detected file extension to the filename

So for example

  1. We detect a mime type of application/pdf
  2. The filename is haxxor.exe, so the extension is exe
  3. application/pdf should have extension pdf but the file has extension exe so we rename the file to haxxor.exe.pdf

If someone now opens the malicious file they will just get an error from their PDF reader, instead of getting a virus/ransomwhere/whatever installed.

Also, your function could do with some early returns:

<?php

/**
 * Upload an Image Function
 * @param  string $tag       the name of the input field that was used to upload the file
 * @param  int $size         the allowed size of the file in bytes
 * @param  array $format     the allowed formats for the file
 * @param  string $filename  the base name of the file after upload
 * @param  string $directory the directory in which the file will be stored
 * @param  string $error	   the error generated from the function
 * @return boolean           returns true if the file was uploaded successfully, otherwise false
 */
function uploadFile($tag, $size, $format, $filename, $directory, &$error=null) {
  if (getimagesize($_FILES[$tag]["tmp_name"]) === false) {
    $error = 'Upload a valid file';

    return;
  }
  
  // Check if file already exists
  if (file_exists($target_file)) {
    $error = 'File already exists';

    return;
  }
    
  // Check file size
  if ($_FILES[$tag]["size"] > $size) {
    $error = 'Your file is larger than '.($size/1024).' KBs';

    return;
  }
  
  // Allow certain file formats
  @$imageFileType = array_pop(explode(".", strtolower($_FILES[$tag]["name"])));
  $target_file =  $directory.'/'.$filename.'.'.$imageFileType;

  if (!in_array($imageFileType, $format)) {
    $error = 'The allowed formats are '.implode(', ', $format);

    return;
  }
  
  // if everything is ok, try to upload file
  if (move_uploaded_file($_FILES[$tag]["tmp_name"], $target_file)) {
    return true;
  }
  
  $error = 'There was an error uploading your file';

  return false;
}

I’ve also changed $error to &$error so you can obtain the value outside of the function once it’s set

$ext = strtolower(end(explode('.', $_FILES['photo']['name'])));
if (uploadFile('photo', 10000000, array('jpeg','jpg','png','gif'), $data['photo_name'], 'uploads/users/', $error) === TRUE) {
  if ($this->user->updateUserPicture($data, $ext)) {
    flash('message', 'Your profile image has been changed!', 'alert alert-success mb-0');
    redirect('user/settings/#tab3');
  } else {
    die(LANG['somethingwentwrong']);
  }
} else{
  echo $error;
}

Although a better design would use a result object. Something like this:

class UploadResult
{
  private $successful;
  private $error;
  
  public function __construct($successful, $error)
  {
    $this->successful = $successful;
    $this->error = $error;
  }

  public function isSuccessful()
  {
    return $this->successful;
  }

  public function getError()
  {
    return $this->error;
  }
}

I’ll leave further implemention of the usage of such a class as an exercise to the reader.

Where have the early returns gone?

Sorry the code above was just an experimental one regarding the mime types.

This is the function code where i’m adding mime security now (thankyou for your changes they work so much better and a really helpful):

<?php
/**
 * Upload an Image Function
 * @param  string $tag       the name of the input field that was used to upload the file
 * @param  int $size         the allowed size of the file in bytes
 * @param  array $format     the allowed formats for the file
 * @param  array $type       the allowed file mime types for the file (extra security, never trust the user)
 * @param  string $filename  the base name of the file after upload
 * @param  string $directory the directory in which the file will be stored
 * @param  string $error	   the error generated from the function
 * @return boolean           returns true if the file was uploaded successfully, otherwise false
 */
function uploadFile($tag, $size, $format, $type, $filename, $directory, &$error=null) {
  if (getimagesize($_FILES[$tag]["tmp_name"]) === false) {
    $error = 'Upload a valid file';

    return;
  }
  
  // Check if file already exists
  if (file_exists($target_file)) {
    $error = 'File already exists';

    return;
  }
    
  // Check file size
  if ($_FILES[$tag]["size"] > $size) {
    $error = 'Your file is larger than '.($size/1024).' KBs';

    return;
  }
  
  // Allow certain file formats
  @$imageFileType = array_pop(explode(".", strtolower($_FILES[$tag]["name"])));
  $target_file =  $directory.'/'.$filename.'.'.$imageFileType;

  if (!in_array($imageFileType, $format)) {
    $error = 'The allowed formats are '.implode(', ', $format);

    return;
  }
  
  // Allow certain file types
  $mimetype = explode(",",$type);

  if (!in_array($_FILES[$tag]["type"], $mimetype)) {
    $error = 'The allowed mime formats are '.implode(', ', $format);

    return;
  }
  
  // if everything is ok, try to upload file
  if (move_uploaded_file($_FILES[$tag]["tmp_name"], $target_file)) {
    return true;
  }
  
  $error = 'There was an error uploading your file';

  return false;
}

How best do you suggest i check where the mime + ext match and then add the correct extention on the end if they do not?

  1. We detect a mime type of application/pdf
  2. The filename is haxxor.exe , so the extension is exe
  3. application/pdf should have extension pdf but the file has extension exe so we rename the file to haxxor.exe.pdf

Thanks

Here is an newer version that checks mime type and also enables and disables the getimagesize incase you want different files.

<?php
/**
 * @param $tag			| the name of the input field that was used to upload the file
 * @param $size			| the allowed size of the file in bytes
 * @param $format		| the allowed formats for the file
 * @param $type			| the allowed file mime types for the file (extra security, never trust the user)
 * @param $filename		| the base name of the file after upload
 * @param $directory	| the directory in which the file will be stored
 * @param $error		| the error generated from the function
 * @param $checkimg		| true for only image / false for other files 
 * @return boolean      | returns true if the file was uploaded successfully, otherwise false
 *
 * Simple Upload File Function
 */
function uploadFile($tag, $size, $format, $type, $filename, $directory, $checkimg, $checkdim, $w=null, $h=null, &$error=null)
{
	// Added Image Security (Enable/Disable)
	if($checkimg) {
		if(getimagesize($_FILES[$tag]["tmp_name"]) === false) {
			$error = 'Upload a valid image file';
			return;
		}
	}
	
	// If Image Enabled  - Check Width & Height
	if($checkdim) {
		list($width, $height) = getimagesize($_FILES[$tag]["tmp_name"]);
		if($width > $w || $height > $h) {
			die("This image does not meet the image dimension limits.");
		}
	}
	
	// Check if File Exists
	if(file_exists($target_file)) {
		$error = 'File already exists';
		return;
	}
	
	// Check File Size
	if($_FILES[$tag]["size"] > $size) {
		$error = 'Your file is larger than '.($size/1024).' KBs';
		return;
	}
	
	// Allow Certain File Extentions
	@$imageFileType = array_pop(explode(".", strtolower($_FILES[$tag]["name"])));
	$target_file =  $directory.'/'.$filename.'.'.$imageFileType;

	if(!in_array($imageFileType, $format)) {
		$error = 'The allowed formats are '.implode(', ', $format);
		return;
	}

	// Allow Certain File Mime Types
	if (!in_array($_FILES[$tag]["type"], $type)) {
		$error = 'The allowed mime formats are '.implode(', ', $type);
		return;
	}

	// If Validation Passed Try Upload
	if (move_uploaded_file($_FILES[$tag]["tmp_name"], $target_file)) {
		return true;
	}

	$error = 'There was an error uploading your file';
	return false;
}

					$ext = strtolower(end(explode('.', $_FILES['photo']['name'])));
					if(uploadFile('photo',10000000,array('jpeg','jpg','png','gif'),array('image/jpeg', 'image/png','image/gif'),$data['photo_name'],'uploads/users/', false, $error) === TRUE){
						if( $this->user->updateUserPicture($data, $ext) ){
							flash('message', 'Your profile image has been changed!', 'alert alert-success mb-0');
							redirect('user/settings/#tab3');
						} else{
							die(LANG['somethingwentwrong']);
						}
					} else{
						echo $error;
					}

or not, it doesnt like any zip files even tho I have specified the mime type! Any suggestions?

					$ext = strtolower(end(explode('.', $_FILES['photo']['name'])));
					if(uploadFile('photo', 10000000, array('jpeg','jpg','png','gif','zip'), array('image/jpeg','image/png','image/gif','application/zip','application/octet-stream'), $data['photo_name'], 'uploads/users/', false, $error) === TRUE){
						if( $this->user->updateUserPicture($data, $ext) ){
							flash('message', 'Your profile image has been changed!', 'alert alert-success mb-0');
							redirect('user/settings/#tab3');
						} else{
							die(LANG['somethingwentwrong']);
						}
					} else{
						echo $error;
					}

Can you see what is wrong with this? The file upload process works great if there are no errors but when there are the error messages below are displayed! Thanks very much

<?php
/**
 * @param $tag			| the name of the input field that was used to upload the file
 * @param $size			| the allowed size of the file in bytes
 * @param $format		| the allowed formats for the file
 * @param $type			| the allowed file mime types for the file (extra security, never trust the user)
 * @param $filename		| the base name of the file after upload
 * @param $directory	| the directory in which the file will be stored
 * @param $checkimg		| true for only image / false for other files 
 * @param $checkdim		| if true will check uploaded image dimentions with $w & $h
 * @param $w		    | if not null sets the maximum width of uploaded images
 * @param $h		    | if not null sets the maximum height of uploaded images
 * @param $error		| the error generated from the function
 * @return boolean      | returns true if the file was uploaded successfully, otherwise false
 *
 * Simple Upload File Function
 */
function uploadFile($tag, $size, $format, $type, $filename, $directory, $checkimg, $checkdim, $w=null, $h=null, &$error=null)
{
	// Added Image Security (Enable/Disable)
	if($checkimg) {
		if(getimagesize($_FILES[$tag]["tmp_name"]) === false) {
			$error = 'Upload a valid image file';
			return;
		}
	}
	
	// If Image Enabled  - Check Width & Height
	if($checkimg && $checkdim) {
		list($width, $height) = getimagesize($_FILES[$tag]["tmp_name"]);
		if($width > $w || $height > $h) {
			die("This image does not meet the image dimension limits.");
		}
	}
	
	// Check if File Exists
	if(file_exists($target_file)) {
		$error = 'File already exists';
		return;
	}
	
	// Check File Size
	if($_FILES[$tag]["size"] > $size) {
		$error = 'Your file is larger than '.($size/1024).' KBs';
		return;
	}
	
	// Allow Certain File Extentions
	@$imageFileType = array_pop(explode(".", strtolower($_FILES[$tag]["name"])));
	$target_file =  $directory.'/'.$filename.'.'.$imageFileType;

	if(!in_array($imageFileType, $format)) {
		$error = 'The allowed formats are '.implode(', ', $format);
		return;
	}

	// Allow Certain File Mime Types
	if (!in_array($_FILES[$tag]["type"], $type)) {
		$error = 'The allowed mime formats are '.implode(', ', $type);
		return;
	}

	// If Validation Passed Try Upload
	if (move_uploaded_file($_FILES[$tag]["tmp_name"], $target_file)) {
		return true;
	}

	$error = 'There was an error uploading your file';
	return false;
}

I get an error that says this:

**Notice** : Only variables should be passed by reference in **C:\xampp\htdocs\justchewi\app\controllers\Help.php** on line **247**

**Notice** : Undefined variable: error in **C:\xampp\htdocs\justchewi\app\controllers\Help.php** on line **247**

My PHP code is:

			if(empty($data['formData']['photo_err'])) {
					if(uploadFile('fileUpload', 10000000, array('jpeg','jpg','png','gif','zip'), array('image/jpeg','image/png','image/gif','application/zip', 'application/octet-stream', 'application/x-zip-compressed', 'multipart/x-zip'), array_pop(array_reverse(explode(".", $formData['filename']))), 'uploads/suploads/', false, false, $error) === TRUE) {
						if($this->support->insertFile($formData)) {
							flash('message', 'Your file was uploaded!', 'alert alert-success mb-0');
							redirect('user/ticket/'.$id);
						} else {
							die(LANG['something-went-wrong']);
						}
					} else {
						echo $error;
					}
				} else {
					$this->view('support/ticket', $data);
				}

Do you have to define $error before you do the call, as you’re passing it by reference now? Just add

$error = "";

prior to calling the upload function. Without that, you’re passing a reference to a variable that does not exist.

I took that to be responsible for the undefined error.

I have a feeling the “by reference” error has something to do with

some_function( array('one', 'two') ) { 

vs.

$some_array = array('one', 'two'); 
some_function($some_array) { 

Ah ok so what you are saying is I should do all of them variables and then put the variables in the function whereas now i’m doing the array in the function? Thanks

That did not work! Really odd but I can’t see why it’s not working! Thanks for your help

                $newname = array_pop(array_reverse(explode(".", $formData['filename'])));
                $extentions = array('jpeg','jpg','png','gif','zip');
                $mime = array('image/jpeg','image/png','image/gif','application/zip', 'application/octet-stream', 'application/x-zip-compressed', 'multipart/x-zip');
				if(uploadFile('fileUpload', 10000000, $extentions, $mime, $newname, 'uploads/suploads/', false, false, $error) === TRUE) {
					if($this->support->insertFile($formData)) {
						flash('message', 'Your file was uploaded!', 'alert alert-success mb-0');
						redirect('user/ticket/'.$id);
					} else {
						die(LANG['something-went-wrong']);
					}
				} else {
					echo $error;
				}

Can you expand on “did not work”? Same error messages? I notice you still don’t define $error before you call the function. Or are there now different error messages?

Do you need to define variables for the places where you’ve used quoted strings and numbers in your function call? I read a description that PHP now points directly at variable contents when passing a variable in a function, and it can’t do that if some of your parameters are not variables.

$p1 = "fileUpload";
$p2 = 10000000;
$p3 = "uploads/uploads/";
$error = "";
if(uploadFile($p1, $p2, $extentions, $mime, $newname, $p3, false, false, $error) === TRUE) {

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.