Page 1 of 1
Image Upload Script... any security suggestions?
Posted: Thu Jul 05, 2007 8:46 pm
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?
Posted: Thu Jul 05, 2007 10:18 pm
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
Posted: Fri Jul 06, 2007 11:38 am
by Citizen
So as far as security goes, its fine?
Posted: Fri Jul 06, 2007 12:29 pm
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.
Posted: Fri Jul 06, 2007 1:37 pm
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.
Posted: Fri Jul 06, 2007 1:43 pm
by feyd
Data missing should short circuit the rest of the code.
Posted: Fri Jul 06, 2007 7:52 pm
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

Posted: Sun Jul 08, 2007 11:05 am
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?
Posted: Sun Jul 08, 2007 12:18 pm
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
Posted: Sun Jul 08, 2007 3:52 pm
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
Re: Image Upload Script... any security suggestions?
Posted: Mon Jul 09, 2007 8:35 am
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.
Re: Image Upload Script... any security suggestions?
Posted: Mon Jul 09, 2007 3:46 pm
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?
Posted: Tue Jul 10, 2007 3:00 am
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.