1
\$\begingroup\$

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);

}
}
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Alex L Feb 21 '14 at 21:21

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.