# Secure/Bulletproof Image Upload Using PHP

Here is my code that check and upload an image to my website.

Anything I have to add to this code to make it more secure?

I have also blocked execution of PHP in images folder using php_flag engine off in .htaccess of images folder, I can also move images folder out of the public_html (root) folder.

    <?php
if(isset($_POST['submit'])){ // permenant variabes for upload$target_dir = "./images/";
$allowed = array('jpg' => 'image/jpeg', 'png' => 'image/png', 'gif' => 'image/gif');$maximumsize=5000000000;
//variables form file
$filetempname=$_FILES["image"]["tmp_name"];
$fileinfo = new finfo(FILEINFO_MIME_TYPE);$filemime = $fileinfo->file($_FILES['image']['tmp_name']);
//check if error is set
if(!isset($_FILES['image']['error']) ){ echo 'Invalid upload.'; die(); } // check formultiple uploads and block elseif(is_array($_FILES['image']['error'])){
echo 'Only one file allowed.';
die();
}
elseif($_FILES['image']['error']===0){ // check image size if($_FILES["image"]["size"] > $maximumsize){ echo 'File size exceed maximum size.'; die(); }$ext = array_search($filemime,$allowed, true);
if(false !== $ext){ //upload file by giving new name$newfilename = md5(uniqid('', true));
$newfilename=$newfilename.'.'.array_search($filemime,$allowed);
$target_file =$target_dir .$newfilename; if (move_uploaded_file($_FILES["image"]["tmp_name"], $target_file)) { echo "uploaded."; } //upload failed message else{ echo "upload failed"; die(); } } // if file format is wrong else{ echo "invalid file format"; die(); } } // if there is any upload error else{ echo "error in upload"; die(); } } else{ echo "not submited form properly"; } ?>  Do you have any suggestions to make this more secure or can you spot any flaws in this script that could be used by hackers to upload shell or PHP scripts? • "Is this code really secure?" Secure against what? – Mast Jun 17 '20 at 8:06 • I want to know is there anyway someone can still use this upload feature to upload some php like scripts – markfiz Jun 17 '20 at 8:08 • Like this? – Mast Jun 17 '20 at 8:09 • I want to prevent uploading of shell scripts or something like that to my server. – markfiz Jun 17 '20 at 8:11 • This code is basically secure because it renames the file with a predefined set of extensions, thus making it impossible to upload a file with .php or any other harmful extension. You have to watch out for the other code though that may be tricked into including a file of the users choice. – Your Common Sense Mar 9 at 9:32 ## 1 Answer # Questions Is this code really secure against uploading of malicious codes ? Anything i have to add to this code to make it more secure? My initial thought is to ensure that the uploaded file is not an executable file or a file that may be run as a separate script- e.g. PHP or some other language the server may handle. The Mime type checking from finfo::file() should handle that. You could also : # Suggestions ### avoid excess else Whenever a conditional block contains a die statement then subsequent code does not need to be prefixed with else e.g. //check if error is set if(!isset($_FILES['image']['error']) ){
die();
}
// check formultiple uploads and block
elseif(is_array($_FILES['image']['error'])){ echo 'Only one file allowed.'; die(); }  When else is used it adds more complexity to the decision tree. ### Returning early Going along with the previous point, in some parts of the code when a precondition is not met then an error message is printed to the screen before die() is called. However this is not true in all cases - e.g. the first conditional: if(isset($_POST['submit'])){


Flipping the logic on that so the error message that is displayed when that condition is false and adding a call to die would allow the indentation levels to be decreased dramatically, which would likely make the code more readable. Further improvement would come from moving some logic to functions - e.g. a function to determine if the file can be uploaded - if not then return early.

With such changes the decision tree of the logic could go from something like this:

screenshot captured from source: Your code sucks, let's fix it - By Rafael Dohms at 15:17

To this:

screenshot captured from source: Your code sucks, let's fix it - By Rafael Dohms at 15:28

    $maximumsize=5000000000;  Since that value shouldn’t be reassigned during the runtime it is advisable to use a constant instead, and a common convention in many programming languages is to use all caps, which is in-line with the PHP Standards recommendation PSR-1 - e.g. const MAXIMUM_SIZE = 5000000000;  And define those near the top of the code or in a configuration file to allow quick access should you ever need to change the value - that way you won’t need to go hunting through the code to find it. ### Array syntax There isn't anything wrong with using array() but as of the time of writing, PHP has Active support for versions 8.0 and 7.4, and since PHP 5.4 arrays can be declared with short array syntax (PHP 5.4). $allowed = array('jpg' => 'image/jpeg', 'png' => 'image/png', 'gif' => 'image/gif');


Can be shortened slightly to

### Appending to $newfilename The extension is appended to the file name: $newfilename= $newfilename.'.'.array_search($filemime, $allowed);  The dot equals operator can be used to append to the variable: $newfilename .= '.' . array_search($filemime,$allowed);