Image upload filter for PHP

Question

I am trying to improve my file upload handler filter.

Environment

Since any client-side validation is easily circumvented, there is none in this code, for ease of testing. Not even an accept="image/*" on the <input> element. Production code will include some client side validation.

The upload handler is set up to first check that the standard php file upload criteria are met, and then goes on to validate that the upload file is an image and can be saved.

The Error handler is simplified

Inspiration

I realized that my handler was vulnerable after completing the upload vulnerabilities room in tryhackme since I originally only checked the extension and the mime type.

Known possible pitfalls:

• non-image files like.php can still be uploaded with null char in name .php%00.png with a spoofed header (e.g. image/png instead of application/x-php), but must have correct magic number.
• No check of whether the header matches the extension.
  • Fix1: scan for strings like /bin/bash and <?php?
  • check for null bytes in file names?

I put the code on github as well...

function is_image($filename) {
    // PNG, GIF, JFIF JPEG, EXIF JPEF (respectively)
    $allowed = array('89504E47', '47494638', 'FFD8FFE0', 'FFD8FFE1');

    $handle = fopen($filename, 'r');
    $bytes = strtoupper(bin2hex(fread($handle, 4)));
    fclose($handle);

    return in_array($bytes, $allowed);
}


$resize = filter_input(INPUT_POST, "resize", FILTER_SANITIZE_STRING);
$resize = (boolean) $resize;
//flag to indicate whether we would have saved the file...
$file_ok = false;
$preliminary_check = true;

//Preliminary check of basic PHP file handling
if ($_FILES["inageFile"]["error"] != 0) {
    switch ($_FILES["inageFile"]["error"]) {
        case UPLOAD_ERR_INI_SIZE: //1
            error_log("File is too big. uploaded: " . $_FILES["inageFile"]["size"] . ")");
            $preliminary_check = false;
            break;
        case UPLOAD_ERR_FORM_SIZE: //2
            error_log("Form is too big, " . $_FILES["inageFile"]["size"] . ")");
            $preliminary_check = false;
            break;
        case UPLOAD_ERR_PARTIAL: //3
            error_log("Partial Error");
            $preliminary_check = false;
            break;
        case UPLOAD_ERR_NO_FILE: //4
            error_log("No file uploaded");
            $preliminary_check = false;
            break;
        default:
            error_log("Unknown error " . $_FILES["inageFile"]["error"]);
            $preliminary_check = false;
            break;
    }
}

//if no errors, continue
if ($preliminary_check) {
    echo "PASSED preliminary check\n";
    if (substr($_FILES["inageFile"]["type"], 0, 6) == "image/") {
        if (is_image($_FILES["inageFile"]["tmp_name"])) {
            $im["filename"] = str_replace(" ", "-", $_FILES["inageFile"]["name"]); //replace spaces with hyphens
            $im["size"] = $_FILES["inageFile"]["size"];
            list($im["width"], $im["height"], $im["type"], $tempattr) = getimagesize($_FILES["inageFile"]["tmp_name"]);
            $im["type"] = $_FILES["inageFile"]["type"];
            if (!file_exists($uploadfile)) {
                error_log($_FILES["inageFile"]["name"] . " will not overwrite existing Logo. Would be saved.");
                //if (move_uploaded_file($_FILES["inageFile"]["tmp_name"], $uploadfile)) {
                error_log($_FILES["inageFile"]["name"] . " has been confirmed to be an image and would have been saved on the server.");
                $file_ok = true;
            //} else {
            //    error_log("Error in file upload. " . $_FILES["inageFile"]["tmp_name"] . " cannot be moved to $uploadfile");
            //}
            } else {
                error_log("filename exists already");
            }
        } else{
            error_log($_FILES["inageFile"]["type"] . " does not match magic number");    
        } 
    } else {
        error_log("Mime/type is not an image - received: " . $_FILES["inageFile"]["type"]);
    }

    if($file_ok) {
        echo "FILE WOULD HAVE BEEN UPLOADED AND SAVED";
    }else {
        echo "FAILED image check";
    }
} else {
    echo "FAILED preliminary check";
}

Answer 1

Avoid pitfalls using PHP extensions

It is good to be aware of the pitfalls like users attempting to upload non-image files. In order to avoid them PHP extensions like fileinfo offers finfo_open() and the Exif extension offers exif_imagetype() - used in this code to check if an uploaded file is an image.

Other review points

boolean type casting can be done when assigning initial value

$resize = filter_input(INPUT_POST, "resize", FILTER_SANITIZE_STRING);
$resize = (boolean) $resize;

The cast can be done during the assignment from the function call:

$resize = (boolean) filter_input(INPUT_POST, "resize", FILTER_SANITIZE_STRING);

Also note that boolean is an alias of bool¹ so it could be simplified to not use the alias:

$resize = (bool) filter_input(INPUT_POST, "resize", FILTER_SANITIZE_STRING);

Variable assignment is repetitive across cases in switch statement.

Inside the switch statement every case as well as the default includes:

        $preliminary_check = false;

Thus that line can be pulled out of the switch statement and placed anywhere inside the conditional block if ($_FILES["inageFile"]["error"] != 0) {. Then all cases within the switch statement just have a call to error_log() before break. One could simplify that block using a match statement.

Prefer strict equality checks

As explained near the end of this review it is wise to use strict equality operators like === instead of == which uses type juggling.

This conditional:

if (substr($_FILES["inageFile"]["type"], 0, 6) == "image/") {

Can be updated to

if (substr($_FILES["inageFile"]["type"], 0, 6) === "image/") {

because substr() returns a string¹.

Answer 2

One way to be almost 100% secure against upload vulnerabilities is to convert the image yourself, maybe using a libary like ImageMagick.

That way it would be very difficult to add code to the file that actually could be executed.