How to make image uploads safe.

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
condoravenue1
Forum Commoner
Posts: 30
Joined: Fri Dec 03, 2010 10:24 pm

How to make image uploads safe.

Post by condoravenue1 »

Here is what I use to upload profile pictures to a folder. I know this isn't secure yet. The folder's permission is set to 777 because that is the only way I could make it work.

Let me know what needs to be added/changed to keep hackers away.

Thanks!

Code: Select all

<?php
session_start();
if(!isset($_SESSION['username'])){header("location: ../index.php"); exit();}

$con = mysql_connect("**","**","**");
mysql_select_db("**", $con);

$username = mysql_real_escape_string($_SESSION['username']);

// get extension
if ($HTTP_POST_FILES['uploadedfile']["type"] == "image/gif") {$ext = ".gif";}
if ($HTTP_POST_FILES['uploadedfile']["type"] == "image/png") {$ext = ".png";}
if ($HTTP_POST_FILES['uploadedfile']["type"] == "image/jpeg") {$ext = ".jpg";}

// validate
if ($HTTP_POST_FILES['uploadedfile']["type"] != "image/gif" && $HTTP_POST_FILES['uploadedfile']["type"] != "image/png" && $HTTP_POST_FILES['uploadedfile']["type"] != "image/jpeg") {$valid = "false"; $errors = $errors . "1";}
if ($HTTP_POST_FILES['uploadedfile']['size'] > 3000000) {$valid = "false"; $errors = $errors . "2";}

// if invalid, go to error page
if ($valid == "false"){
header("location: ../error_pages/upload_picture_error.php?why=$errors");
exit();}

// resize, rename, copy to folder
$path = "../profile_pictures/".$username . $ext;
$file_name = $HTTP_POST_FILES['uploadedfile']['tmp_name'];

copy($file_name, $path);
   include('SimpleImage.php');
   $image = new SimpleImage();
   $image->load($path);
   $image->resizeToWidth(100);
   $image->save($path);
$picture = $username . $ext;

$result = mysql_query("SELECT * FROM perm WHERE username = '$username'");
while($row = mysql_fetch_array($result)) 
{$old_picture = $row['picture'];}

// delete old default picture
unlink("../profile_pictures/".$old_picture);

mysql_query("UPDATE perm SET picture = '$picture' WHERE username = '$username'");

echo '<script type="text/javascript">window.location = "../account.php";</script>';

?>
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: How to make image uploads safe.

Post by Christopher »

The first think is to use move_uploaded_file() instead of copy() because it does security checks. See the manual for that function for more info about uploads. There is also a whole section of the manual about Handling file uploads - http://us.php.net/manual/en/features.file-upload.php
(#10850)
condoravenue1
Forum Commoner
Posts: 30
Joined: Fri Dec 03, 2010 10:24 pm

Re: How to make image uploads safe.

Post by condoravenue1 »

I had to set the permissions of the folder that the files are uploaded into to 777. Is this what it should be?
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: How to make image uploads safe.

Post by Christopher »

I would set the directory to the user that the web server runs as ... and then 700.
(#10850)
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: How to make image uploads safe.

Post by social_experiment »

“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
condoravenue1
Forum Commoner
Posts: 30
Joined: Fri Dec 03, 2010 10:24 pm

Re: How to make image uploads safe.

Post by condoravenue1 »

As I said before, the folder's permission is 777. If I change it, not only does my uploading code not work, but the pictures don't display on the website. The way it is now, can someone upload files to that folder without using my website? My main concern is that someone will upload a bunch of junk to the folder, or overwrite someone else's picture with an unwanted one. Or perhaps they can delete other's photos.

Btw, the site is here: http://bible-help.com/

New Code:

Code: Select all

<?php
session_start();
if(!isset($_SESSION['username'])){header("location: ../index.php"); exit();}

$con = mysql_connect("*","*","*");
mysql_select_db("*", $con);

$username = mysql_real_escape_string($_SESSION['username']);


// get extension
if ($HTTP_POST_FILES['uploadedfile']["type"] == "image/gif") {$ext = ".gif";}
if ($HTTP_POST_FILES['uploadedfile']["type"] == "image/png") {$ext = ".png";}
if ($HTTP_POST_FILES['uploadedfile']["type"] == "image/jpeg") {$ext = ".jpg";}

// validate
if ($HTTP_POST_FILES['uploadedfile']["type"] != "image/gif" && $HTTP_POST_FILES['uploadedfile']["type"] != "image/png" && $HTTP_POST_FILES['uploadedfile']["type"] != "image/jpeg") {$valid = "false"; $errors = $errors . "1";}
if ($HTTP_POST_FILES['uploadedfile']['size'] > 3000000) {$valid = "false"; $errors = $errors . "2";}
if (strpos(strtolower($HTTP_POST_FILES['uploadedfile']['name']), '.php') !== false || strpos(strtolower($HTTP_POST_FILES['uploadedfile']['name']), '.phtml') !== false) {$valid = "false"; $errors = $errors . "1";}

// if invalid, go to error page
if ($valid == "false")
{
header("location: ../error_pages/upload_picture_error.php?why=$errors");

exit();
}

$result = mysql_query("SELECT * FROM perm WHERE username = '$username'");
while($row = mysql_fetch_array($result)) 
{$old_picture = $row['picture'];}
// delete old default picture
unlink("../profile_pictures/".$old_picture);

// resize, rename, copy to folder
$file_name = $HTTP_POST_FILES['uploadedfile']['tmp_name'];
$path = "../profile_pictures/".$username . $ext;

move_uploaded_file($file_name, $path);

   include('SimpleImage.php');
   $image = new SimpleImage();
   $image->load($path);
   $image->resizeToWidth(100);
   $image->save($path);

$picture = $username . $ext;

mysql_query("UPDATE perm SET picture = '$picture' WHERE username = '$username'");

echo '<script type="text/javascript">window.location = "../account.php";</script>';

?>
Christopher wrote:The first think is to use move_uploaded_file() instead of copy() because it does security checks.
Did that.
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: How to make image uploads safe.

Post by social_experiment »

condoravenue1 wrote:The way it is now, can someone upload files to that folder without using my website? My main concern is that someone will upload a bunch of junk to the folder, or overwrite someone else's picture with an unwanted one. Or perhaps they can delete other's photos.
No, you need access to a file that allows for file uploading to your server (or access to the webserver like an FTP account). Same goes for deleting / overwriting. Just a point on overwriting, it will be better to create new file names, should there be an upload ability on your site, in that way, the overwriting issue is pretty much resolved.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
Post Reply