HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Check the security of my image upload script

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scripttheimageuploadsecuritycheck

Problem

I am writing a basic image upload script in PHP, and am looking for critiques. The script below is the result of various suggestions I have found online... for now I am running it locally and it works fine, but I would love for someone more experienced to look over it.

Is this secure? Poorly implemented? Too redundant? Not redundant enough? What am I missing and/or doing incorrectly? Any suggestions/help/advice is GREATLY appreciated. Thank you.

PHP:

```
if (isset($_FILES['image']))
{
$files_array = $_FILES['image'];
$num_of_image_uploads = count($files_array['name']);

// Loop through array of file uploads
for ($i = 0; $i define, validate, and upload
if ($files_array['tmp_name'][$i] != "")
{
$errors = false; // Flag for errors
$allowed_ext = array('jpg', 'jpeg', 'png', 'gif');

$file_name = $files_array['name'][$i];
$file_ext = strtolower(end(explode('.', $file_name)));
$file_size = $files_array['size'][$i];
$file_tmp = $files_array['tmp_name'][$i];

// Extension check 1
if (in_array($file_ext, $allowed_ext) === false)
{
echo "Extension not allowed. ";
$errors = true;
}

// Extension check 2 - if file is not empty
if ($file_size == 0)
{
echo "File is empty. ";
$errors = true;
}
else if (!getimagesize($file_tmp))
{
echo "Not an image. ";
$errors = true;
}

// Extension check 3
$blacklist = array(".php", ".phtml", ".php3", ".php4");
foreach ($blacklist as $item)
{
if(preg_match("/$item\$/i", $file_name))
{
echo "File not allowed. ";
$errors = true;
}
}

// File size check

Solution

Your script has one underlying assumption that invalidates the whole set of security checks: That the value of $files_array['name'] is safe - it is not. This value is user supplied, and can very easily be spoofed to pass malicious data into your server that would get through all your security checks.

Neither $_FILES['control']['name'] nor $_FILES['control']['type'] are safe and you should not use either value for anything, ever.

In order to make your image uploads as safe as possible you must do all of the following:

  • Verify that the data you are working with is an uploaded file (move_uploaded_file() / is_uploaded_file() will take care of this for you)



  • Create the file name used to store the file on the file system entirely dynamically. You cannot use user input in any way to generate this. If your application requires that you store the original file name, you should keep this association in an external data store such as a database.



  • Verify that the image really is an image. The GD extension can verify the headers for you (use the getimagesize() function).



  • Copy the pixel data out of the image into a new one. Again, GD can do this for you (imagecopyresampled()). It is important to do this - it is possible to hide malicious code inside something with headers that make it appear to be a valid image. Resampling the image will destroy such malicious code.



  • I recommend that if you store the image on the file system, store it outside the document root and proxy the retrieval process with a script, so it cannot be directly accessed via the web server. The above measures should mitigate the implied security risk here, but better safe than sorry ;-)



  • Never include/require a file uploaded by a user. If you want to provide a file download, always use filesystem functions. readfile() is often the most appropriate choice.



Unfortunately, in order to comply with all these requirements you are basically going to need to throw your code out and start again.

If you want something for reference, this Gist contains something that's not a million miles away from the code I use in my real-life applications, and contains a basic usage example.

Context

StackExchange Code Review Q#24824, answer score: 4

Revisions (0)

No revisions yet.