Secure/Bulletproof Image Upload Using PHP

Question

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

Is this code really secure against uploading of malicious codes?

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();
 }
 //check for any upload error
 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 exploited by hackers to upload shell or PHP scripts?

• Mast: "Is this code really secure?" Secure against what?
• markfiz: I want to know is there anyway someone can still use this upload feature to upload some php like scripts
• Mast: Like this?
• markfiz: I want to prevent uploading of shell scripts or something like that to my server.
• Your Common Sense: 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.

Answer 1

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 :

• check that a GD function like imagecreatefromgif(), imagecreatefromjpeg(), etc. does not return FALSE
• check the permissions on the file after it is uploaded to ensure anything that shouldn’t be executed cannot be executed - perhaps use is_executable(), fileperms and chmod
• like you say: "move images folder out of public_html (root) folder."- Ensure that any uploaded files are not stored in a web accessible directory unless that is your desire. This may lead to needing a (PHP/other) script or web server module to read files.

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']) ){
 echo 'Invalid upload.';
 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 of wider decision tree _{screenshot captured from source: Your code sucks, let's fix it - By Rafael Dohms at 15:17}

To this:

enter image description here _{screenshot captured from source: Your code sucks, let's fix it - By Rafael Dohms at 15:28}

storing max size

 $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

$allowed = ['jpg' => 'image/jpeg', 'png' => 'image/png', 'gif' => 'image/gif'];

checking mimetype

$filemime could either be string or Boolean - check for Boolean, then if it isn’t then in_array() could be used to determine if it exists in the array instead of using array_search()

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);

Spaces added for readability.

• Your Common Sense: It is not a mime-type checking that makes this code secure, but the mime-type based renaming. the checking is not enough, as a php file can simply bypass the checking. but renaming a .php file to .jpg will make it at least directly harmless.
• Your Common Sense: the mime type checking literally "scans the first couple of bytes", so it's already done. From the security standpoint, we don't care whether uploaded file is an image or not. We only care whether a file can be executed as a PHP script.