[BMR777]

Hello,

I have written a file upload script in PHP as part of an application I am working on currently. Basically I want logged in users to be able to upload .gif and .jpg images ONLY to the server. I have made a file uploaded and I have taken the following steps to make sure that only safe .gif and .jpg files can be uploaded and that everything else is rejected:

1. I do a check of the submitted file name and only upload files with .gif or .jpg extentions

2. I force the file extension on the file as well as change the filename, so if you upload picture.gif I change the name to somethingrandom.gif and force the .gif.

3. I use getimagesize() on the file to determine the mime type server side, rather than just trusting the mime type provided by the browser.

4. I have a .htaccess file in the upload directories that turns off the PHP engine for that directory.

Below is my code. Please think like a hacker and tell me if you see any way that an attacker could possibly bypass any of the security with the end result of uploading a malicious file to the server and executing it server side. I want to make sure this is secure as possible before going live with it. Also, if there is any additional check you would run on the uploaded files, please let me know.

Thanks,
Brandon

<?php

// Wake the sleeping giant
include("inc/functions.php");
connect();
$themeurl = themeurl();
$site_title = sitetitle();
$site_name = sitename();
$slogan = slogan();
$newsbar = newsbar();


if($newsbar != ""){
$shownews = "<div class='subheader'><p>".$newsbar."</p></div>";
}
else{
$shownews = "<div class='subheader'></div>";
}

// **********************************************************************
// Check if user is logged in
// **********************************************************************

$userdata = logincheck();
$isloggedin = $userdata[loginstatus];
$loggedinname = $userdata[username];

// **********************************************************************
// We do all our prepwork here
// **********************************************************************

// If we're not logged in, we cannot access this page...

if($isloggedin != "yes"){
$article_title = "403 Forbidden";
$article_content = "You do not have permission to access the file uploads page.  Only artists may access this page.
Are you an artist?  <a href='login.php'>Log in</a> or <a href='register.php'>register</a> to upload files.";

}
else{

	// BEGIN FILE UPLOAD
	
	$flag = 0; // Safety net, if this gets to 1 at any point in the process, we don't upload.

$filesize = $_FILES['uploadedfile']['size'];
$mimetype = $_FILES['uploadedfile']['type'];
$filename = $_FILES['uploadedfile']['name'];


$filename = htmlentities($filename);
$filesize = htmlentities($filesize);
$mimetype = htmlentities($mimetype);

//Default upload directory
$uploaddir = "picuploads/gif";


//***************************************************************************
//First we determine if the file is a gif or a jpg by checking the extension

//First check and see if the file is a .gif file
$isgif = "no";

$whitelist = array(".gif");
foreach ($whitelist as $ending) {

if(substr($filename, -(strlen($ending))) != $ending) {

//File is not a gif file, so let's do nothing right now
//When we check for if it is a jpg we will flag the file

}
else{
//The file IS a gif file, so we set the isgif to true
$isgif = "yes";

}
}

// Now we check if it is a .jpg file or not, because it is not a gif

if($isgif != "yes"){

$whitelist = array(".jpg");
foreach ($whitelist as $ending) {

if(substr($filename, -(strlen($ending))) != $ending) {

	if($flag == 0){
	$error = "The file type or extention you are trying to upload is not allowed!  
	You can only upload gif or jpg files to the server!";
	}

$flag++;
}
}

}

//***************************************************************************


/*
if($filename != ""){

echo "Beginning upload process for file named: ".$filename."<br>";
echo "Filesize: ".$filesize."<br>";
echo "Type: ".$mimetype."<br><br>";

}

*/

//First generate a MD5 hash of what the new file name will be
//Force a file extention on the file we are uploading

//Now we create a hashed file name of the file and set the upload directory...

if($isgif == "yes"){

$date = date('Y-m-d');
$hashstring = $filename."_".$date;

$hashedfilename = md5($hashstring);
$hashedfilename = $hashedfilename.".gif";
$target_path = "picuploads/gif/";
$uploaddir = "picuploads/gif";

}
else if ($isgif == "no" and $flag == 0){

//File is a jpg

$date = date('Y-m-d');
$hashstring = $filename."_".$date;

$hashedfilename = md5($hashstring);
$hashedfilename = $hashedfilename.".jpg";
$target_path = "picuploads/jpg/";
$uploaddir = "picuploads/jpg";

}


//SET TARGET PATH?
$target_path = $target_path . basename( $filename );


//Check for empty file
if($hashedfilename == ""){

	if($error == ""){
	$error = "No File Exists!";
	}

$flag = $flag + 1;

}

//Now we check that the file doesn't already exist.
$existname = $uploaddir."/".$hashedfilename;

if(file_exists($existname)){

if($flag == 0){
$error = "Your file already exists on the server!  
Please choose another file to upload or rename the file on your 
computer and try uploading it again!";
}

$flag = $flag + 1;
}

////////////////////////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////////////////////////


//Now we check the filesize.  If it is too big then we reject it


if($filesize > 153600){
//File is too large

if($flag == 0){
$error = "The file you are trying to upload is too large!  Files must be under 150 KB.";
}

$flag = $flag + 1;
}

////////////////////////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////////////////////////

//Check the mimetype of the file
if($mimetype != "image/gif" and $mimetype != "image/jpeg"){

if($flag == 0){
$error = "The file you are trying to upload does not contain expected data.
Are you sure that the file is a .gif or .jpg file?";
}

$flag = $flag + 1;
}

////////////////////////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////////////////////////

//Now that we checked the mime type client side, let's check it again server side...

$imageInfo = getimagesize($_FILES["uploadedfile"]["tmp_name"]); // note that we need to use the temporal name since it has not yet been moved  
if($imageInfo["mime"] != "image/gif" and $imageInfo["mime"] != "image/jpeg")  
{  
	if($error == ""){
	$error = "The file you are trying to upload does not contain expected data.
	Are you sure that the file is a .gif or .jpg file?";
	}

$flag++;

} 


////////////////////////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////////////////////////

//All checks are done, actually move the file...

if($flag == 0){

if(move_uploaded_file($_FILES['uploadedfile']['tmp_name'], $uploaddir."/".$hashedfilename)) {
	

	if(@file_exists($uploaddir."/".$hashedfilename)){

	$article_title = "Success!";
	/*

	$article_content = "The file ".  basename( $filename ). " 
	  has been uploaded.  Your file is <a href='uploads/$uploaddir/$hashedfilename'>here</a>.";

	*/

	$article_content = "The file ".  basename( $filename ). " 
	  has been uploaded successfully!  It will now appear on your profile and in your photo gallery.";
	
	}	
	else{
	  $article_title = "ERROR!";
	$article_content = "There was an error uploading the file, please try again!";

	}


} else{
	$article_title = "ERROR!";
	$article_content = "There was an error uploading the file, please try again!";

	
	
}

}
else {
$article_title = "ERROR!";
if($error != ""){
$article_content = $error;
}
else{
$article_content = "File Upload Failed!";
}
}

//More code here
//Done with upload, so insert data into database



$origfilename = secure(basename( $filename ));
$hashedfilename = secure($hashedfilename);
$location = $uploaddir."/".$hashedfilename;

//*******************************************************

	$info = $_POST['info'];
	$info = secure($info);
	
//*******************************************************

if($flag == 0){

$crdate = date('Y-m-d');
mysql_query("INSERT INTO picsmap VALUES ('', '$loggedinname', '','$location','$location', 'profileimage', '$crdate', '$info')");

}


$article_content = $article_content."<br><br><u>What do you want to do now?<br><br>
<a href='uploadpicform.php'>Upload another picture file</a><br>
<a href='managepicuploads.php'>Manage uploads or change / delete file info</a><br>
<a href='account.php'>Manage My Account</a>";


}





// **********************************************************************
// End Prepwork - Output the page to the user
// **********************************************************************

//Define our current theme
$file = $themeurl;

// Do the template changes and echo the ready template
$template = file_get_contents($file);
//$template = replace(':SITETITLE:',$site_title,$template);
$template = replace(':SITENAME:',$site_name,$template);
$template = replace(':SLOGAN:',$slogan,$template);
$template = replace(':ARTICLETITLE:',$article_title,$template);
$template = replace(':ARTICLEDATE:',$article_date,$template);
$template = replace(':ARTICLECONTENT:',$article_content,$template);
$template = replace(':LINK1:',$link1,$template);
$template = replace(':LINK2:',$link2,$template);
$template = replace(':LINK3:',$link3,$template);
$template = replace(':NEWSBAR:',$shownews,$template);

/*

//Ad Management
$header = @file_get_contents("ads/header.txt");
$footer = @file_get_contents("ads/footer.txt");
$tower = @file_get_contents("ads/tower.txt");

$header = stripslashes($header);
$footer = stripslashes($footer);
$tower = stripslashes($tower);

$template = replace(':HEADERAD:',$header,$template);
$template = replace(':FOOTERAD:',$footer,$template);
$template = replace(':TOWERAD:',$tower,$template);

*/

//**************************************************************

//Custom template replacement to allow the javascript to work properly

$oldtext = "<head>
	<meta name=\"author\" content=\"Luka Cvrk (www.solucija.com)\" />
	<meta http-equiv=\"content-type\" content=\"text/html;charset=iso-8859-2\" />
	<link rel=\"stylesheet\" href=\"templates/default/images/style.css\" type=\"text/css\" />
	<title>:SITETITLE:</title>
</head>";

$newtext = "<head><title>File Upload</title>
<script language=\"Javascript\">
<!-- Copyright 2001 Bontrager Connection, LLC
function WorkingMessage() {
   var url=\"\";	   // Blank for thankyou page.
   var height = 100; // Height of popup
   var width = 450;  // Width of popup
   var att='width=' + width + ',height=' + height;
   WorkingMessagePopup=window.open(url,\"wmp\",att);
}
function KillWorkingMessagePopup(){
   WorkingMessage();
   WorkingMessagePopup.close();
} // -->
</script>	<meta name=\"author\" content=\"Luka Cvrk (www.solucija.com)\" />
	<meta http-equiv=\"content-type\" content=\"text/html;charset=iso-8859-2\" />
	<link rel=\"stylesheet\" href=\"templates/default/images/style.css\" type=\"text/css\" /></head>";

$template = replace($oldtext,$newtext,$template);


$oldtext = "<body>";

$newtext = "<body onload=\"KillWorkingMessagePopup();\">";

$template = replace($oldtext,$newtext,$template);

//**************************************************************

//Is the user logged in?

if ($isloggedin == "yes"){

$logincontent = logincontent($loggedinname);

$template = replace(':LOGINBAR:',$logincontent[loginbar],$template);
$template = replace(':WELCOMEORREGISTER:',$logincontent[welcome],$template);
$template = replace(':LOGINORACCT:', $logincontent[content] ,$template);
$friends = minifriends();
$template = replace(':FRIENDS:',$friends,$template);
}
else{

//User is not logged in
$template = replace(':LOGINBAR:','<b>You are not Logged in!</b> <a href="login.php">Log in</a>  or <a href="register.php">register</a> to start downloading music!',$template);
$template = replace(':WELCOMEORREGISTER:','<u>Member Login:</u>',$template);
$loginform = loginform();
$template = replace(':LOGINORACCT:', $loginform ,$template);
$friends = "";
$template = replace(':FRIENDS:',$friends,$template);
}

$morecontent = morecontent();
$template = replace(':MORECONTENT:',$morecontent,$template);

// **********************************************************************
// THIS IS THE LAST THING WE DO!
// **********************************************************************

echo $template;



?>

[CTphpnwb]

You might want to keep track of the number of times that a user attempts to upload a file that doesn't fit into your criteria, and remove the ability to upload if that number gets too high. Reducing the number of attempts a hacker can make should reduce their odds of success.

[BMR777]

CTphpnwb, on 1 Feb, 2009 - 01:23 PM, said:

You might want to keep track of the number of times that a user attempts to upload a file that doesn't fit into your criteria, and remove the ability to upload if that number gets too high. Reducing the number of attempts a hacker can make should reduce their odds of success.

I like that idea, thanks. The only issue I see with that is experienced hackers who want to really get in can simply fake the IP address or use a proxy or register for a new username, thus bypassing any check I could put in place. For the script-kiddies though that is a good idea. Thanks.

This post has been edited by BMR777: 01 February 2009 - 02:27 PM

[CTphpnwb]

BMR777, on 1 Feb, 2009 - 01:26 PM, said:

The only issue I see with that is experienced hackers who want to really get in can simply fake the IP address or use a proxy or register for a new username, thus bypassing any check I could put in place.

Yes, but even for them it raises the level of difficulty. All that extra effort just to see if a new hack will work should discourage all but the most determined, and slow them down quite a bit!

[Martyr2]

You have some real spaghetti code here and really the only thing saving your bacon here is that you do a good job at checking for a file extension and then setting a flag. Because everything hinges on that flag being 0 to prevent moving files, updating databases etc.

Personally if it was me if it didn't pass the gif/jpg test right at the top, end the script. Don't go any further. Instead of flagging and then continue with the process, kill the script right there. If they don't match gif/jpg then there is no point to continue, it is a bad file.


But an overall design stand point and maintenance, I would look at possibly refactoring some of this code and simplify it because if there is an error somewhere in it, it will be hard for you to track down. So then you will be sitting there with a hole in security for a long time while you try to track down the error.

[BMR777]

Thanks Martyr2 for the advise. The main reason I have the code such as it is is so that any error I get is outputted through the template system at the end, because I want to be able to show a "friendly" message for users who say try to upload a png or bmp rather than a gif, using the site's template. I couldn't do that with a die() statement.

I will certainly take a second look at the code though. I think though I have been very careful to make sure that flag is set at any failure point and that was my strategy all along, but I see what you are saying with killing it right away. It gives me something to think about at least.

BMR777

[CTphpnwb]

I have to agree with Martyr2 and admit that I "cheated" a bit in my answer. I was feeling too lazy to go through the code so I only provided a suggestion without looking for flaws. That should have been a clue that the code needs to be streamlined.

[BMR777]

CTphpnwb, on 1 Feb, 2009 - 01:44 PM, said:

I have to agree with Martyr2 and admit that I "cheated" a bit in my answer. I was feeling too lazy to go through the code so I only provided a suggestion without looking for flaws. That should have been a clue that the code needs to be streamlined.

Yeah, the code is rather bulky I guess. I'd rather hit the file with too many checks though than not enough. Aside from the sheer mass of code though being an issue does anyone see any other attack vector or logic flaw in the current code that would allow something to slip by?

This post has been edited by BMR777: 01 February 2009 - 02:49 PM

[CTphpnwb]

This doesn't fix any security issue, but is shortens the code a bit:

// First check to see if the file is of the correct type
$whitelist = array(".gif",".jpg");
$type = false;
foreach($whitelist as $ending) {
	if(substr($filename, -(strlen($ending))) == $ending) {
		$type = true;
		break;
	}
}

[BMR777]

Thanks, CTphpnwb, however the code being long as it is has a reason to it. I want to place the .gif files in a "gif" directory and the jpgs in a "jpg" directory, which is why I go through all the trouble of running the check for each file type separately, so I can assign each file to the appropriate directory.

Thanks.
BMR777

This post has been edited by BMR777: 01 February 2009 - 03:05 PM

[CTphpnwb]

In that case:

// First check to see if the file is of the correct type
$whitelist = array(".gif",".jpg");
$directory = array("The gif path","The jpg path");
$type = false;
foreach($whitelist as $key => $ending) {
	if(substr($filename, -(strlen($ending))) == $ending) {
		$type = true;
		echo $directory[$key];
		$the_directory_path_is = $directory[$key];
		break;
	}
}

The echo statement is just there so you can see it works. You don't even really need to assign "$the_directory_path_is"

[BMR777]

Thanks