Adding image file check security to Valums Ajax Uploader

Question

I've been doing research on image file upload security for a while but can't seem to crack it. I want to add some security to the fantastic Valums Ajax Uploader by checking whether or not a file is actually an image before saving to a server. The only extensions allowed are .jpg, .png and .gif, which of course the extensions are checked but I'm looking to validate it's an image file via GD processing. I know very little about this subject so I'm taking cues from this and this post. As it stands now, I can easily save a random file with an image extension and it will be saved the server, which I want to prevent. Here is the script I've come up with thus far, which I've added to the handleUpload function in the php.php file. Unfortunately the result is that it returns an error no matter what file I upload, valid image or not. Please excuse my utter newbishness.

            $newIm = @imagecreatefromjpeg($uploadDirectory . $filename . '.' . $ext);
            $newIm2 = @imagecreatefrompng($uploadDirectory . $filename . '.' . $ext);
            $newIm3 = @imagecreatefromgif($uploadDirectory . $filename . '.' . $ext);
            if (!$newIm && !$newIm2 && !$newIm3) {
                return array('error' => 'File is not an image.  Please try again');
            }else{ 
                imagedestroy($newIm);
                imagedestroy($newim2);
                imagedestroy($newIm3);
}

Here's most of my php.php file. By the way, my files are not being submitted via a regular form but by the default uploader:

    class qqUploadedFileXhr {
        /**
         * Save the file to the specified path
         * @return boolean TRUE on success
         */
        function save($path) {    
            $input = fopen("php://input", "r");
            $temp = tmpfile();
            $realSize = stream_copy_to_stream($input, $temp);
            fclose($input);

            if ($realSize != $this->getSize()){            
                return false;
            }

            $target = fopen($path, "w");        
            fseek($temp, 0, SEEK_SET);
            stream_copy_to_stream($temp, $target);
            fclose($target);

            return true;
        }
        function getName() {
            return $_GET['qqfile'];
        }
        function getSize() {
            if (isset($_SERVER["CONTENT_LENGTH"])){
                return (int)$_SERVER["CONTENT_LENGTH"];            
            } else {
                throw new Exception('Getting content length is not supported.');
            }      
        }   
    }

    /**
     * Handle file uploads via regular form post (uses the $_FILES array)
     */
    class qqUploadedFileForm {  
        /**
         * Save the file to the specified path
         * @return boolean TRUE on success
         */  
        function save($path) {
            if(!move_uploaded_file($_FILES['qqfile']['tmp_name'], $path)){
                return false;
            }
            return true;
        }
        function getName() {
            return $_FILES['qqfile']['name'];
        }
        function getSize() {
            return $_FILES['qqfile']['size'];
        }

    }

    class qqFileUploader {
        private $allowedExtensions = array();
        private $sizeLimit = 2097152;
        private $file;

        function __construct(array $allowedExtensions = array(), $sizeLimit = 2097152){        
            $allowedExtensions = array_map("strtolower", $allowedExtensions);

            $this->allowedExtensions = $allowedExtensions;        
            $this->sizeLimit = $sizeLimit;

            $this->checkServerSettings();       

            if (isset($_GET['qqfile'])) {
                $this->file = new qqUploadedFileXhr();
            } elseif (isset($_FILES['qqfile'])) {
                $this->file = new qqUploadedFileForm();
            } else {
                $this->file = false; 
            }
        }

        private function checkServerSettings(){        
            $postSize = $this->toBytes(ini_get('post_max_size'));
            $uploadSize = $this->toBytes(ini_get('upload_max_filesize'));        

            if ($postSize < $this->sizeLimit || $uploadSize < $this->sizeLimit){
                $size = max(1, $this->sizeLimit / 1024 / 1024) . 'M';             
                die("{'error':'increase post_max_size and upload_max_filesize to $size'}");    
            }        
        }

        private function toBytes($str){
            $val = trim($str);
            $last = strtolower($str[strlen($str)-1]);
            switch($last) {
                case 'g': $val *= (1024 * 1024 * 1024);
                case 'm': $val *= (1024 * 1024);
                case 'k': $val *= 1024;        
            }
            return $val;
        }

        /**
         * Returns array('success'=>true) or array('error'=>'error message')
         */
        function handleUpload($uploadDirectory, $replaceOldFile = FALSE){
            if (!is_writable($uploadDirectory)){
                return array('error' => "Server error. Upload directory isn't writable.");
            }

            if (!$this->file){
                return array('error' => 'No files were uploaded.');
            }

            $size = $this->file->getSize();

            if ($size == 0) {
                return array('error' => 'File is empty');
            }

            if ($size > $this->sizeLimit) {
                return array('error' => 'File is too large, please upload files that are less than 2MB');
            }

            $pathinfo = pathinfo($this->file->getName());
            $filename = $pathinfo['filename'];
            //$filename = md5(uniqid());
            $ext = $pathinfo['extension'];

            if($this->allowedExtensions && !in_array(strtolower($ext), $this->allowedExtensions)){
                $these = implode(', ', $this->allowedExtensions);
                return array('error' => 'File has an invalid extension, it should be one of '. $these . '.');
            }

            if(!$replaceOldFile){
                /// don't overwrite previous files that were uploaded
                while (file_exists($uploadDirectory . $filename . '.' . $ext)) {
                    $filename .= rand(10, 99);
                }
            }
            $newIm = @imagecreatefromjpeg($uploadDirectory . $filename . '.' . $ext);
            $newIm2 = @imagecreatefrompng($uploadDirectory . $filename . '.' . $ext);
            $newIm3 = @imagecreatefromgif($uploadDirectory . $filename . '.' . $ext);
            if (!$newIm && !$newIm2 && !$newIm3) {
                return array('error' => 'File is not an image.  Please try again');
            }else{ 
                imagedestroy($newIm);
                imagedestroy($newim2);
                imagedestroy($newIm3);
}   

            if ($this->file->save($uploadDirectory . $filename . '.' . $ext)){
                // At this point you could use $result to do resizing of images or similar operations
            if(strtolower($ext) == 'jpg' || strtolower($ext) == 'jpeg' || strtolower($ext) == 'gif' || strtolower($ext) == 'png'){

                $imgSize=getimagesize($uploadDirectory . $filename . '.' . $ext);

                if($imgSize[0] > 100 || $imgSize[1]> 100){  
                $thumbcheck = make_thumb($uploadDirectory . $filename . '.' . $ext,$uploadDirectory . "thumbs/" . $filename ."_thmb" . '.' . $ext,100,100);
                    if($thumbcheck == "true"){
                        $thumbnailPath = $uploadDirectory . "thumbs/" . $filename ."_thmb". '.' . $ext;
                    }
                }else{
                $this->file->save($uploadDirectory . "thumbs/" . $filename ."_thmb" . '.' . $ext);  
                $thumbnailPath = $uploadDirectory . "thumbs/" . $filename ."_thmb". '.' . $ext;
                }

                if($imgSize[0] > 500 || $imgSize[1] > 500){
                resize_orig($uploadDirectory . $filename . '.' . $ext,$uploadDirectory . $filename .  '.' . $ext,500,500);
                $imgPath = $uploadDirectory . $filename . '.' . $ext;
                $newsize = getimagesize($imgPath);
                $imgWidth = ($newsize[0]+30);
                $imgHeight = ($newsize[1]+50);
                }else{
                $imgPath = $uploadDirectory . $filename . '.' . $ext;
                $newsize = getimagesize($imgPath);
                $imgWidth = ($newsize[0]+30);
                $imgHeight = ($newsize[1]+50);
                }

            }
                return array('success'=>true,
                'thumbnailPath'=>$thumbnailPath,
                'imgPath'=>$imgPath,
                'imgWidth'=>$imgWidth,
                'imgHeight'=>$imgHeight
                );
            } else {
                return array('error'=> 'Could not save uploaded file.' .
                    'The upload was cancelled, or server error encountered');
            }

        }    
    }

    // list of valid extensions, ex. array("jpeg", "xml", "bmp")
    $allowedExtensions = array();
    // max file size in bytes
    $sizeLimit = 2097152;

    $uploader = new qqFileUploader($allowedExtensions, $sizeLimit);
    $result = $uploader->handleUpload('uploads/');

    // to pass data through iframe you will need to encode all html tags
    echo htmlspecialchars(json_encode($result), ENT_NOQUOTES);

If someone can point me in the right direction about what I'm doing wrong, it would be most appreciated.

Edit:

Thanks Mark B for the help and clarification. So the new code (which I pasted in the same location as the old, the handleUpload function) is still throwing an error no matter what I upload, so I wonder if it needs to be in a different place -- I'm just not sure where to put it.

$newIm = getimagesize($uploadDirectory . $filename . '.' . $ext);
            if ($newIm === FALSE) {
               return array('error' => 'File is not an image.  Please try again');

        }

Second Edit:

I believe the answer is here, still trying to implement it.

Check what the filepath you're trying to access is. getimagesize will also return false if the specified path/file doesn't exist, or is unreadable. However, you don't destroy the newimg. getimagesize just returns an array of info about the image, not a GD handle. — Marc B, Mar 21 '12 at 20:31
The mistakes I'm making are insane, I'm pretty sleep deprived at the moment. With the second part removed, it's still throwing an error. The whole point is I want to do this before it is saved to the server, so the path doesn't exist yet. I want to check the file that's been uploaded, not the file that's saved. I don't know how to do that/where to put that in the code. Does it have to be saved before I can check the image size? — seaofinformation, Mar 21 '12 at 22:17
Figured out the answer from this link: http://stackoverflow.com/questions/8209646/getimagesize-on-stream-instead-of-string . Will post it once I'm able (in a few hours it will let me answer my own q). Thanks again for your help. :) — seaofinformation, Mar 21 '12 at 23:36

score 0 · Answer 1 · answered Mar 21 '12 at 18:45

0

Just use getimagesize():

$newIm = getimagesize$uploadDirectory . $filename . '.' . $ext');
if ($newIm === FALSE) {
    die("Hey! what are you trying to pull?");
}

The problem with your code is the 3 imagecreate calls and subsequent if() statement. It should have been written as an OR clause. "if any of the image load attempts fail, then complain". Yours is written as "if all image attempts fail", which is not possible if a gif/jpg/png actually was uploaded.

answered Mar 21 '12 at 18:45

Marc B

356,200
43
426
500

I figured I was doing some noob code mistake. :) Thanks for that. I thought about doing getimagesize but I read this - http://ha.ckers.org/blog/20070604/passing-malicious-php-through-getimagesize/. Should I not worry about what that article talks about? – seaofinformation Mar 21 '12 at 18:50
It's a valid concern, but if you don't provide direct access to the uploaded files, and/or make sure the file's saved with proper filenames then it shouldn't be a matter. if you save it as 'pic.gif', then it won't be run through the php interpreter to begin with, so the most malicious code in the universe couldn't run anyways. – Marc B Mar 21 '12 at 18:52
Thanks so much. Updated my question, still throwing an error every time. – seaofinformation Mar 21 '12 at 19:07

score 0 · Accepted Answer · edited May 23 '17 at 12:10

The answer detailed here works like a charm, I wish I had seen it before posting. I hope I implemented it correctly...as I have it, it works!

class qqUploadedFileXhr {
    /**
     * Save the file to the specified path
     * @return boolean TRUE on success
     */
function save($path) { 
// Store the file in tmp dir, to validate it before storing it in destination dir
$input = fopen('php://input', 'r');
$tmpPath = tempnam(sys_get_temp_dir(), 'upload'); // upl is 3-letter prefix for upload
$tmpStream = fopen($tmpPath, 'w'); // For writing it to tmp dir
$realSize = stream_copy_to_stream($input, $tmpStream);
fclose($input);
fclose($tmpStream);

if ($realSize != $this->getSize()){            
            return false;
        }
$newIm = getimagesize($tmpPath);
if ($newIm === FALSE) {
    return false;

}else{      
// Store the file in destination dir, after validation
$pathToFile = $path . $filename;
$destination = fopen($pathToFile, 'w');
$tmpStream = fopen($tmpPath, 'r'); // For reading it from tmp dir
stream_copy_to_stream($tmpStream, $destination);
fclose($destination);
fclose($tmpStream);

        return true;
}
    }
    function getName() {
        return $_GET['qqfile'];
    }
    function getSize() {
        if (isset($_SERVER["CONTENT_LENGTH"])){
            return (int)$_SERVER["CONTENT_LENGTH"];            
        } else {
            throw new Exception('Getting content length is not supported.');
        }      
    }   
}

Adding image file check security to Valums Ajax Uploader

2 Answers2