Upload image with secure code

Question

I have written [PHP code][ to upload image/s to server.

(To see the result, open the JavaScript console of your browser, look at the Network tab or the Console tab.)

The code is also available and runnable on paste.ofcode.org.

(Upload images)

I would like to know if this code is secure for uploading valid images.

<?php
$IMAGE_SIZE = 2097152; // 2mb ht tp:/ /ww w.convertunits.com/from/MB/to/byte
$MAX_IMAGES = 30;
if(!empty($_FILES['fileToUpload']['name'][0])) {
    foreach($_FILES["fileToUpload"]['tmp_name'] as $key=>$imgLocation){}
    if($key<$MAX_IMAGES){
        foreach($_FILES["fileToUpload"]['tmp_name'] as $key=>$imgLocation){     
            if($imgLocation){//filename exist
                $imageName = $_FILES["fileToUpload"]['name'][$key];
                if(checkMime($imgLocation)&&
                    checkExtension($imageName)&&
                    checkByExif($imgLocation)&&
                    checkSize(filesize($imgLocation))&&
                    checkImageName($imageName)){
                    echo $usr_dir = "img/".changeImageName($imageName).".".getExtension($imageName);
                    if(move_uploaded_file($imgLocation, $usr_dir)){
                        echo "<br/>success full  uploaded<br/>";
                    }else{
                        echo "<br/> File Not uploaded<br/>";
                    }
                }else{  echo "<br/>Invalid File<br/>";}
            }else{echo " No Image uploaded";}
        }
    }else echo "<br/> - Only 30 Images are allowed -<br/>";
}else{
    echo "No Image Is selected";
}
function handleError($msg){
    //return json_encode();
}
function checkImageName($imagename){
    $imagename = pathinfo($imagename, PATHINFO_FILENAME);
    if(strpbrk($imagename, "\\/?%*' :|\"<>") === FALSE&&$imagename){
        if (strlen($imagename) >= 1 && strlen($imagename)<= 30){
            return true;
        }else{
            return false;
        //HandleError('Invalid file name');
        }
    }else{
        return false;
    }
}
function issetImage($image_tmpName){
    return (file_exists($image_tmpName) || is_uploaded_file($image_tmpName));//return ture and false
}
function checkMime($image_tmpName){
    if(issetImage($image_tmpName)){
        $whitelist = array("image/gif","image/png","image/jpeg","image/jpg","image/bmp");
        $imageinfos = getimagesize($image_tmpName);
        $imageTypes = $imageinfos['mime'];
        if(in_array($imageTypes, $whitelist)){
            return true;
        }else{
            return false;
        }
    }
    else{
        return false;
    }
}
function checkExtension($imageName){
    $ext = getExtension($imageName);
    if(strtolower($ext) == "jpg" || strtolower($ext) == "png" || strtolower($ext) == "jpeg" || strtolower($ext) == "gif" || strtolower($ext) == "bmp"){
        return true;
    }else{
        return false;
    }
}
function checkByExif($image_tmpName){
    if(issetImage($image_tmpName)){
        $image_type_allowed = array(IMAGETYPE_GIF, IMAGETYPE_JPEG, IMAGETYPE_PNG, IMAGETYPE_BMP);
        $filetype =exif_imagetype($image_tmpName);
        if (in_array($filetype, $image_type_allowed)){
            return true;
        }else{
            return false;
        }
    }else{
        return false;
    }
}
function checkSize($imageSize){
    return ($imageSize<= $GLOBALS["IMAGE_SIZE"] && $imageSize>=1) ? true : false;
}
function changeImageName($imgName){
    return md5(pathinfo($imgName, PATHINFO_FILENAME))."_".time();
}
function getExtension($fullImageName){  
    if($fullImageName)
        return pathinfo($fullImageName,PATHINFO_EXTENSION);
}
?>

Answer 1

Security

The echo should be vulnerable to XSS via the file extension (but it's not exploitable - at least not easily; Still, I would HTML encode it when echoing).

Otherwise it looks good to me. Regarding the various checks:

• checkMime: Can easily be bypassed. Checking the mimetype is generally not very secure, so it doesn't matter that much, but as defense in depth it's not a bad idea. getimagesize isn't a good function for this though, finfo_file generally gives better results. [see eg here]
• checkExtension: This is the important check. There are no known vulnerabilities for pathinfo, so this should be secure.
• checkByExif: Can very easily be bypassed, as exif_imagetype only checks the first bytes. It's not a security check in any way, but may help with usability (although in this case I don't see the use for it).
• checkSize: important check, should be secure.
• checkImageName: Not needed as you change the image name anyways. The function itself looks pretty bad though; strpbrk($imagename, "\\/?%*' :|\"<>") === FALSE&&$imagename isn't very readable.

To add additional security as defense in depth, you could move the upload dir outside the web root and check the content of the file (eg that it doesn't contain opening PHP tags).

Returning Early

Your ifs are quite nested. If you return early, you can avoid this, and make it more obvious what condition returns what error. Eg:

if(empty($_FILES['fileToUpload']['name'][0])) {
    return "No Image Is selected";
}
if(too many images){
    return "Only 30 Images are allowed";
}
foreach($_FILES["fileToUpload"]['tmp_name'] as $key=>$imgLocation){ 
    uploadImage($_FILES["fileToUpload"]['tmp_name'][$key], $imgLocation);
}

checkImageName

Instead of this:

function checkImageName($imagename){
    $imagename = pathinfo($imagename, PATHINFO_FILENAME);
    if(strpbrk($imagename, "\\/?%*' :|\"<>") === FALSE&&$imagename){
        if (strlen($imagename) >= 1 && strlen($imagename)<= 30){
            return true;
        }else{
            return false;
        //HandleError('Invalid file name');
        }
    }else{
        return false;
    }
}

You can use this:

function checkImageName($imagename){
    if (preg_match("/[\\\\\/?%*' :|\"<>]/", $imagename)) {
       return false;
    }
    return strlen($imagename) >= 1 && strlen($imagename) <= 30;
}

preg_match is a more fitting function, as you want to see if the string matches certain characters, and this also gets rid of the nested if structure.

Personally, I would prefer a whitelist check on a-zA-Z0-9 here.

Misc

• why do you have an empty foreach loop? Seems unnecessary.
• I would add a function for the upload of a single image.
• Your formatting could use some work. Always use curly brackets, always put statements on their own line, be more consistent with your spacing, etc. Any IDE can fix these issues for you.
• be more consistent with your variable names. Eg imagename vs imageName vs image_tmpName, imageName vs imgLocation, etc.