Check secure image upload

Question

I want to upload images to server from browser window. I'm using this function for image upload. Images are uploaded by GD library.

What is your opinion of this code? Is there still a way to upload a malicious file?

<?php
function imageUpload($name, $thumb = false, $max_size = 5242880, $height = null, $width = null, $T__height = 100, $T__width = 100)
{
    if(!isset($_FILES[$name]))
    {
        return false;
    }

    $allowedTypes = array('image/gif', 'image/jpeg', 'image/png', 'image/wbmp');
    $image = &$_FILES[$name];

    if ($image['error'] == 0 && in_array($image['type'], $allowedTypes) && $image['size'] <= $max_size)
    {
        $in = '';
        switch ($image['type'])
        {
            case 'image/gif':
                $in = 'imagecreatefromgif';
                break;
            case 'image/jpeg':
                $in = 'imagecreatefromjpeg';
                break;
            case 'image/png':
                $in = 'imagecreatefrompng';
                break;
            case 'image/wbmp':
                $in = 'imagecreatefromwbmp';
                break;
        }

        if ($in == '')
        {
            return false;
        }

        $src = $in($image['tmp_name']);
        $height = ($height == null || $height <= 0 ? imagesy($src) : $height);
        $width = ($width == null || $width <= 0 ? imagesx($src) : $width);

        $dst = imagecreatetruecolor($width, $height);
        imagecopyresized($dst, $src, 0, 0, 0, 0, $width, $height, imagesx($src), imagesy($src));

        $fileName = '';
        do
        {
            $fileName = makeHash(mt_rand(0, mt_getrandmax()) . microtime(true), $image['tmp_name']);
        }
        while(file_exists('/image/' . $fileName . '.jpg') || ($thumb && file_exists('/thumb/' . $fileName . '.jpg')));
        if ($fileName == '')
        {
            return false;
        }

        imagejpeg($dst, '/image/' . $fileName . '.jpg', 75);

        if ($thumb)
        {
            $dst = imagecreatetruecolor($T__width, $T__height);
            imagecopyresized($dst, $src, 0, 0, 0, 0, $T__width, $T__height, imagesx($src), imagesy($src));
            imagejpeg($dst, '/thumb/' . $fileName . '.jpg', 75);
        }

        imagedestroy($src);
        imagedestroy($dst);

        return $fileName . '.jpg';
    }
}
?>

Answer 1

Is there still a way to upload a malicious file?

I think not, due to these elements:

• The check of $image['type'] against an array of various image/* mime types prevents uploading a script instead of an image
• The hashing of the filename with makeHash prevents uploading a script with a malicious path to get the file placed somewhere unintended
• The check of $image['size'] against a defined maximum prevents uploading an oversized file

What is your opinion of this code?

Instead of this:

    if ($in == '')
    {
        return false;
    }

It would be better to add a default statement in the switch above it:

    switch ($image['type'])
    {
        // ...
        case 'image/wbmp':
            $in = 'imagecreatefromwbmp';
            break;
        default:
            return false;
    }

This seems pointless to me:

    if ($fileName == '')
    {
        return false;
    }

It seems to me that $fileName will always be set.