2
\$\begingroup\$

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);
}
?>
\$\endgroup\$
  • 1
    \$\begingroup\$ Maybe take a look at this SO post concerning the upload size. Otherwise someone might be tempted to DOS that script with a couple of gigabytes of uploads. \$\endgroup\$ – ferada May 31 '16 at 21:24
3
\$\begingroup\$

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.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for review my code. echoi used for test that script and for returning value I will use json_encode()amd in client side i parse it. : ) That for loop count images.. i use count(), it doesn't work sm times in return gave 1 and sometime 5 even i do not upload file \$\endgroup\$ – Phoenix Jun 1 '16 at 4:48
  • \$\begingroup\$ and Also which pattern will be go better for verify name ? I used strpbrk($fileName, "\\/?%*' :|\"<>") === FALSE because it checks for every char in name. \$\endgroup\$ – Phoenix Jun 1 '16 at 4:49
  • \$\begingroup\$ what u suggest me more to improve my code ? \$\endgroup\$ – Phoenix Jun 1 '16 at 10:58
  • \$\begingroup\$ @Phoenix: Hm, you are right, the loop is indeed needed. But I don't think that it's very nice. There would be two other possibilities, which may work for your situation: using the multiple attribute (which may hurt usability), or dynamically adding additional image fields via JavaScript once they are actually needed, so that you submit the exact number of images you actually need and do not have empty values (which probably makes a lot of sense in your situation). Regarding strpbrk, I updated my answer. \$\endgroup\$ – tim Jun 1 '16 at 15:26
  • \$\begingroup\$ i tried to avoid nested if and write as you said me but when i do like this return "No image"; in server side and in client side(via $.ajax) does not show anything.. it reurns empty does not show any result in object.responceText \$\endgroup\$ – Phoenix Jun 2 '16 at 10:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.