Image Upload Script... any security suggestions?

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
Citizen
Forum Contributor
Posts: 300
Joined: Wed Jul 20, 2005 10:23 am

Image Upload Script... any security suggestions?

Post by Citizen »

Code: Select all

if($_POST["action"] == "Upload Image"){
	unset($imagename);
	
	if(!isset($_FILES) && isset($HTTP_POST_FILES))
	$_FILES = $HTTP_POST_FILES;
	
	if(!isset($_FILES['image_file']))
	$error["image_file"] = "An image was not found.";
	
	
	$imagename = basename($_FILES['image_file']['name']);
	
	if(empty($imagename))
	$error["imagename"] = "The name of the image was not found.";
	
	if(empty($error))
	{
	$newimage = "avatar/" . $imagename;
	$result = @move_uploaded_file($_FILES['image_file']['tmp_name'], $newimage);
	if(empty($result)){
		$error["result"] = "There was an error moving the uploaded file.";
	}else{
		echo"<p>Your new avatar has been successfully entered.";
	}

	}
		

}

?>

<form method="POST" enctype="multipart/form-data" name="image_upload_form" action="<?$_SERVER["PHP_SELF"];?>">
<p><input type="file" name="image_file" size="20"></p>
<p><input type="submit" value="Upload Image" name="action"></p>
</form>

<?php
if(is_array($error))
{
while(list($key, $val) = each($error))
{
echo $val;
echo "<br>\n";
}
}
First time using image uploads, this is the code I implemented. Any suggestions as far as security goes?
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

It looks like you are doing the basics -- especially using move_uploaded_file(). You might also want to do a little more filtering on $imgname. but I don't know if it is necessary as PHP does some checks. Also see the manual:

http://www.php.net/manual/en/features.file-upload.php

Especially:

http://www.php.net/manual/en/features.f ... tfalls.php
(#10850)
Citizen
Forum Contributor
Posts: 300
Joined: Wed Jul 20, 2005 10:23 am

Post by Citizen »

So as far as security goes, its fine?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

  • There is a potential to divulge path information in the code. Specifically the access of potentially nonexistent indices.
  • The use of PHP_SELF is dangerous at best, although your script will not even echo it.
  • Your script relies on short_open_tag to be on.
  • Looking for a submission button is often inadvisable due to the ability for certain user-agents to submit without it.
Citizen
Forum Contributor
Posts: 300
Joined: Wed Jul 20, 2005 10:23 am

Post by Citizen »

Ok, got the other three, but what should i do about this one?
There is a potential to divulge path information in the code. Specifically the access of potentially nonexistent indices.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

Data missing should short circuit the rest of the code.
User avatar
nathanr
Forum Contributor
Posts: 200
Joined: Wed Jun 07, 2006 5:46 pm

Post by nathanr »

also check for the upload file size, many people simply type in random filenames in the file input thus making loads of dead files on your server.. check it has a filesize greater than X (atleast 0) and this alleviates that from happening :)
Citizen
Forum Contributor
Posts: 300
Joined: Wed Jul 20, 2005 10:23 am

Post by Citizen »

nathanr wrote:also check for the upload file size, many people simply type in random filenames in the file input thus making loads of dead files on your server.. check it has a filesize greater than X (atleast 0) and this alleviates that from happening :)
How do I check for the filesize, like making sure its not too big or too small?
User avatar
nathanr
Forum Contributor
Posts: 200
Joined: Wed Jun 07, 2006 5:46 pm

Post by nathanr »

Citizen wrote:
nathanr wrote:also check for the upload file size, many people simply type in random filenames in the file input thus making loads of dead files on your server.. check it has a filesize greater than X (atleast 0) and this alleviates that from happening :)
How do I check for the filesize, like making sure its not too big or too small?
the filesize for each uploaded file is available in the $_FILES['inputname']['size'] where inputname was the name of the <input type="file" name="inputname">

nice and simple :) you can also get all the info you need on file uploads in php from the php manual under http://uk.php.net/features.file-upload
d3ad1ysp0rk
Forum Donator
Posts: 1661
Joined: Mon Oct 20, 2003 8:31 pm
Location: Maine, USA

Post by d3ad1ysp0rk »

nathanr wrote:
Citizen wrote:
nathanr wrote:also check for the upload file size, many people simply type in random filenames in the file input thus making loads of dead files on your server.. check it has a filesize greater than X (atleast 0) and this alleviates that from happening :)
How do I check for the filesize, like making sure its not too big or too small?
the filesize for each uploaded file is available in the $_FILES['inputname']['size'] where inputname was the name of the <input type="file" name="inputname">

nice and simple :) you can also get all the info you need on file uploads in php from the php manual under http://uk.php.net/features.file-upload
I prefer to use the 'error' field of the file upload, as it contains much more info about what exactly happened.

http://us.php.net/manual/en/features.fi ... errors.php
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Image Upload Script... any security suggestions?

Post by Mordred »

That's not a security hole, but a security abyss. Try uploading a file called "backdoor.php" and see what I mean :)
Never, ever, use user-submitted data, such as $_FILES['image_file']['name'] for a target filename.

There are too many issues with securing uploads, but the simplest secure solution is to generate your own random name with a strictly image-only extension (randomname.jpg for example). You can use a graphic library to recognize the image type (gif/jpg/png etc.), and force the extension (after the randomly generated name) to the proper one.
Citizen
Forum Contributor
Posts: 300
Joined: Wed Jul 20, 2005 10:23 am

Re: Image Upload Script... any security suggestions?

Post by Citizen »

Mordred wrote:That's not a security hole, but a security abyss. Try uploading a file called "backdoor.php" and see what I mean :)
Never, ever, use user-submitted data, such as $_FILES['image_file']['name'] for a target filename.

There are too many issues with securing uploads, but the simplest secure solution is to generate your own random name with a strictly image-only extension (randomname.jpg for example). You can use a graphic library to recognize the image type (gif/jpg/png etc.), and force the extension (after the randomly generated name) to the proper one.
What code would I use to do the renaming and extension forcing stuff?
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

Instead of

Code: Select all

$imagename = basename($_FILES['image_file']['name']);
You need to:
1. Determine the type of the image (jpg/gif/png, whatever) - there are many ways, maybe getimagesize()/image_type_to_extension() will give you the proper extension to use.
2. Generate a random name, to properly do so - in a loop generate a name until a check if "random_filename.ext" exists says "no"
3. For full paranoia mode - upload the images in a separate folder, with a .htaccess file that disables the php engine.

Refer to the manual for details.
Post Reply