Security and efficiency for profile picture upload

Question

I am trying to allow users to upload profile pics on my site. Basically I am following this code: Allow users to upload a pic for their profile.

I don't know if the code is secure enough. I am worried that users may upload malicious files.

Also, I am concerned that if there is many users, I will have many pictures in a single folder and it may slow down the retrieving process. I wonder what is the best practice in storing the profile pics. How many photos maximum should there be in a folder for better efficiency?

if(is_uploaded_file($_FILES['image']['tmp_name']))
{ 
 uploadImage($_FILES['image']['tmp_name'], 100, 100, "image/users/test2.jpeg");
} 

and

function uploadImage($source, $max_width, $max_height, $destination) {
list($width, $height) = getimagesize($source);
if ($width > 150 || $height > 150) {
 $ratioh = $max_height / $height;
 $ratiow = $max_width / $width;
 $ratio = max($ratioh, $ratiow);
 // New dimensions
 $newwidth = intval($ratio * $width);
 $newheight = intval($ratio * $height);
 $newImage = imagecreatetruecolor($newwidth, $newheight);
 $ext = trim(strtolower($_FILES['image']['type']));
 $sourceImage = null;
 // Generate source image depending on file type
 switch ($ext) { 
 case "image/jpg":
 case "image/jpeg":
 $sourceImage = imagecreatefromjpeg($source);
 break;
 case "image/gif":
 $sourceImage = imagecreatefromgif($source);
 break;
 case "image/png":
 $sourceImage = imagecreatefrompng($source);
 break;
 }
 imagecopyresampled($newImage, $sourceImage, 0, 0, 0, 0, $newwidth, $newheight, $width, $height);
 // Output file depending on type
 switch ($ext) {
 case "image/jpg":
 case "image/jpeg":
 imagejpeg($newImage, $destination);
 break;
 case "image/gif":
 imagegif($newImage, $destination);
 break;
 case "image/png":
 imagepng($newImage, $destination);
 break;
 }
 // Destroy resources
 imagedestroy($newImage);
 imagedestroy($sourceImage);
}
}

Answer 1

what happens if I upload a .exe file? or even a .txt file? Your application will give errors. Always have a fallback for if some hack0r uploads a non-picture to your application. Use the default: statement in your switch cases for that and hand back an error when they try to upload a non-image or a image you don't support (e.g. svg).

You talk about security and number of files in one directory, why is there a correlation between these two? The only restriction you have on the number of files per directory depends on the Filestructure format.

I would however create a directory per user instead of storing all the images in the same directory.

Answer 2

This won't help with security, but if you want to shorten your code a little, you can replace this

// Generate source image depending on file type
switch ($ext) { 
case "image/jpg":
case "image/jpeg":
 $sourceImage = imagecreatefromjpeg($source);
 break;
case "image/gif":
 $sourceImage = imagecreatefromgif($source);
 break;
case "image/png":
 $sourceImage = imagecreatefrompng($source);
 break;
}

with this

$sourceImage = imagecreatefromstring(file_get_contents($source)); 

Also why don't you just store the whole lot as jpg, why is it necessary to save them as different types

• Alex L: Always make sure there is a default. Especially in a case like this where the program has a chance of breaking if the conditions aren't met.