Securely upload multiple image files with PHP

Question

I am working on script to a upload image files Securely. can anyone please review my code and suggest improvements on security.

<input type="file" name="images[]" id="image" multiple="multiple">

check if request method is POST and request was made from same domain and form was submitted with submit btn

if (is_post_request() && is_request_same_domain() && isset($_POST['add-list-submit'])) {

Convert the $_FILES array to a cleaner version

$images = rearrange_files_array($_FILES['images']);

function rearrange_files_array($file)
{

    $file_array = [];
    $file_count = count($file['name']);
    $file_keys = array_keys($file);

    for ($i = 0; $i < $file_count; $i++) {
        foreach ($file_keys as $key) {
            $file_array[$i][$key] = $file[$key][$i];
        }
    }
    return $file_array;
}

Loop through the images

foreach ($images as $image) {

Check if image was upload, image mime type, extension, and size

if (!isset($image) || $image['error'] !== UPLOAD_ERR_OK) {
    $errors['form'] = 'Please upload the images.';
} else {
    if (!is_file_valid_image($image) || !has_valid_file_extension($image, ['jpeg', 'png', 'jpg'])) {
        $errors['form'] = 'Only PNG and JPG file formats are allowed.';
    }

    if (has_max_file_size($image)) {
        $errors['form'] = 'Max file size is 2.5 MB.';
    }
}

function has_valid_file_extension($file, $allowed_file_extensions)
{
  $file_extension = pathinfo($file['name'], PATHINFO_EXTENSION);
  return in_array($file_extension, $allowed_file_extensions);
}

function is_file_valid_image($file)
{
  $imageTypes = [IMAGETYPE_GIF, IMAGETYPE_JPEG, IMAGETYPE_PNG, IMAGETYPE_SWF, IMAGETYPE_PSD, IMAGETYPE_BMP, IMAGETYPE_TIFF_II, IMAGETYPE_TIFF_MM, IMAGETYPE_JPC, IMAGETYPE_JP2, IMAGETYPE_JPX, IMAGETYPE_JB2, IMAGETYPE_SWC, IMAGETYPE_IFF, IMAGETYPE_WBMP, IMAGETYPE_XBM, IMAGETYPE_ICO];
  $info = getimagesize($file['tmp_name']);
  $width = $info[0];
  $height = $info[1];
  $type = $info[2];
  $bits = $info['bits'];
  $mime = $info['mime'];

  if (!in_array($type, $imageTypes)) {
    return false; // Invalid Image Type
  } else if ($width <= 1 && $height <= 1) {
    return false; // Invalid Image Size
  } else if ($bits === 1) {
    return false; // One Bit Image
  } else if ($mime !== 'image/gif' || $mime !== 'image/jpg' || $mime !== 'image/jpeg' || $mime !== 'image/png') {
    return false; // Invalid Image Type
  } else {
    return true;
  }
}

function has_max_file_size($file)
{
  return $file['size'] > 2500000; // 2.5 MB
}

If there are no errors move the image to the uploads directory

$filename = bin2hex(random_bytes(12));
$file_ext = pathinfo($image['name'])['extension'];
$upload_path = UPLOAD_DIRECTORY . '/' . $filename . '.' . $file_ext;
move_uploaded_file($image['tmp_name'], $upload_path);

Answer 1

If you are talking about a "secure" version then please state what you mean with it. What are you protecting against and which security requirements do you have extracted?

if (is_post_request() && is_request_same_domain() && isset($_POST['add-list-submit'])) {

This only indicates what it is doing. But it is doing too much on a line. If this is some particular check then please implement it in a well named function.

Convert the $_FILES array to a cleaner version

What does that even mean, a "cleaner version"?

function rearrange_files_array($file)

So it is apparently a rearranged array, but why? Please document your functions if it is not clear what they are doing. Furthermore, here even the "how" is missing: how is the array rearranged?

$errors['form'] = 'Please upload the images.';

Is this something that a user could do? I just select the images. If they don't get loaded into variables then I cannot fix that myself.

$errors['form'] = 'Only PNG and JPG file formats are allowed.';
...
$errors['form'] = 'Max file size is 2.5 MB.';

You have run into a common problem: putting all possible information in the error message while keeping the parameters generic. Please fix the error message so that the extensions and max file size are dynamic. Currently you would have to adjust both the function and the content of the error message separately to achieve one thing.

You did let the extensions be the 2nd parameter of the has_valid_file_extension method. That's a good thing and you should perform the same for checking the file size. It will also bring the error and value of the file size closer together.

It seems that you fall through the if statements when error checking; I'd expect the script to perform some kind of action, right? Currently only an error is set, but the script seems to continue.

$imageTypes = [IMAGETYPE_GIF, IMAGETYPE_JPEG, IMAGETYPE_PNG, IMAGETYPE_SWF, IMAGETYPE_PSD, IMAGETYPE_BMP, IMAGETYPE_TIFF_II, IMAGETYPE_TIFF_MM, IMAGETYPE_JPC, IMAGETYPE_JP2, IMAGETYPE_JPX, IMAGETYPE_JB2, IMAGETYPE_SWC, IMAGETYPE_IFF, IMAGETYPE_WBMP, IMAGETYPE_XBM, IMAGETYPE_ICO];

That's strange, I thought only JPEG and PNG were allowed. This is probably a more generic method. But even in that case: check the file extension first before running this method. You don't want to be checking some exotic file type only to find out that it is not allowed anyway; always bring down the number of options / states of a program as soon as possible.

You may also wan to check that the extension is correct for the given image type, I don't see that happening.

As an aside: Obviously the method getimagesize is named terribly badly as it returns all kinds of information, but that's something to blame on PHP, not your script.

return false; // Invalid Image Type

This wipes out the actual error message, which means it will be impossible to inform the user correctly. You'd better return result which contains an error string or use a similar construct. I applaud you for at least including a comment, but if you have to type such comments then the code itself is often not verbose enough.

return false; // One Bit Image

Huh? No, that seems to be the bit depth. So it would be B/W or rather monochrome image.

else {
    return true;
}

Just return true is enough here, the application has nowhere to go after all.

---

All in all, it's not a bad attempt to me, but a few repairs and a lot of polishing seems in order.