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 used by hackers to upload shell or PHP scripts?

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 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}

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.