Safe image upload

[Zephni]

I am rebuilding an area of one of our websites that needs to allow a user to upload images. The only way I have done this in the past is allowing 777 perms on the folder. Could I have some suggestions on the most efficient yet safe way of doing this.

 

I already check whether the file extension is a .jpg or .png but considering the file perms are open I'm guesing thats no where near good enough. What do the perms allow to outside users exactly, could they upload files to that folder from somewhere else and run php scripts to remove files from below that 777 folder?

 

Thanks for any help

[scootstah]

What do the perms allow to outside users exactly, could they upload files to that folder from somewhere else and run php scripts to remove files from below that 777 folder?

 

Eh, that's not exactly how permissions work - but sort of. If you use 777 it means that (mostly) any user or service on the machine can read/write/execute files in that directory. So, if some an exploit is found somewhere else in the application or server, it could potentially be used to place malicious files into that open directory. At least, that's my understanding. I'm not a sysadmin.

 

But aside from folder permissions, there are other concerns here.

 

First of all, how are you checking that the files are images? You cannot check on file extension alone, you have to rely on the mime type. In this case you can either use getimagesize or the fileinfo library to check the mime type.

 

Also, as a last line of defense, you can use .htaccess to disable script execution in the upload directory, so that even if someone manages to get a script in there, they can't do anything with it.

[Zephni]

Good answer thanks Sounds like I will check the mime type AND disallow execution of scripts with .htaccess to be safe. Tbh I think you've answered my question in one fell swoop.

[Zephni]

This is unfinished, but does this look safe enough to upload images?

 

<?php
class file_upload{
	public $err = array();
	public $msg;

	function image($field_title, $base_dir = "../images/", $force_title = false){
		//Set file path and filename
		if($force_title){
			$img = $force_title;
		}else{
			$img = basename($_FILES[$field_title]['name']);
		}
		$target_path = $base_dir.$img;

		$f_info = getimagesize($_FILES[$field_title]['tmp_name']);
		$mime = $f_info['mime'];

		if($mime == "image/jpg" || $mime == "image/png"){
			//Remove image if it already exists
			if(file_exists($target_path)){
				unlink($target_path);
			}else{
				$return = false;
				$this->err[] = "Error unlinking existing image (ER100)";	
			}

			//Upload
			if(move_uploaded_file($_FILES[$field_title]['tmp_name'], $target_path)){
				$this->msg .= "Image uploaded";
				$return = $img;
			}else{
				$this->err[] = "Error uploading image (ER101)";
				$return = false;	
			}

		}else{
			$this->err[] = "Cannot upload file of this type (ER102)";
			$return = false;	
		}

		$this->compile_errors();

		return $return;
	}

	function compile_errors(){
		$this->err = implode(", ", $this->err);
	}
}
?>

[scootstah]

You need code to handle when getimagesize() fails. It will fail and throw an error if the input is not a valid image. Because of this, you don't need to check the mime specifically with if($mime == "image/jpg" || $mime == "image/png"){ (unless, of course, you only want jpegs and png's).

 

Aside from that, your image() method is a wee bit generic, and has too much responsibility.