Security for file upload

[Itshim]

Greetings Coders,

First I wanted to thank all the usual suspects who answer everyone’s questions. I very rarely post on these boards, because after a search of the FAQs and boards I usually find the answer I’m looking for, that I I’m not quite advanced enough to help with others questions. Any way on to my question…

I recently wrote a function to upload a file to the server. The function works beautifully and I don’t have any issues with it, but it is going to be used on a website that does not want to force users to register or be authenticated in anyway. I understand that this can be a huge security risk and I have explained that to the owners, but they will not budge on this issue. In the function I took as many precautions as I could think of, I just wanted to know if there is anything more I can do to make it secure.

I know the indenting is screwy, but I was trying to make it easier to read.

//$strName = the user name submitted through $_POST
function Get_File($strName) 
{	
//Check if uploaded file exists
if ($_FILES['Ad']['tmp_name'] == "none") 		
	$strMessage = "No Ad file uploaded";
else {
	//Check uploaded has content
	if ($_FILES['Ad']['size'] == 0)				
		$strMessage = "Uploaded Ad file is zero length";
	else {
		//Check uploaded file is not too large
		if ($_FILES['Ad']['size'] > 1000000)  		
			$strMessage = "Uploaded Ad file is too large.";
		else {
		//Check that we are working with the uploaded file
		if (!is_uploaded_file($_FILES['Ad']['tmp_name']))		
		$strMessage = "There was a problem when uploading the Ad file";
		else {
				//Ensure file type is valid, and set file extention
				switch ($_FILES['Ad']['type']) {				
					case "image/gif":
						$Extention = ".gif";					
						break;
					case "image/pjpeg":
						$Extention = ".jpg";
						break;
					case "image/bmp":
						$Extention = ".bmp";
						break;
					//Error Trap
					default:
						$strMessage = "Ad file is not a valid type.";	
				}
			}
		//if all went well in previous section $strMessage will not be set
		if (!isset($strMessage)) { 			
		//$i is used to exit the loop when unique name is achieved	
			$i = 0;						
		//$n is used to append to the file name to achieve unique name					
			$n = 0;							
			//Loop to create unique file name
			while ($i<1)										
			{
			//Creation of path with file name
			$file_upfile = "C:\xxx\Uploads\\".$strName.$n.$fl_extention;	
			//Check to see if file name already exists		
			if (file_exists($file_upfile))							
				$n++;	//if so increment $n and loop again													
			else 
				$i++;	//if not increment $i to exit loop													
			}
		//move the uploaded file to correct directory						
		if ( !move_uploaded_file($_FILES['Ad']['tmp_name'], $file_upfile))	
			$strMessage = "Could not move Ad file into directory";
		else 
			$strMessage = "Ad file uploaded.";
		}	
		}		
	}
}
return $strMessage;
}

The function takes care of the upload, and returns a string, containing either an error, or ‘file uploaded successfully’. If the function returns an error it is spit out to the user, if successful I then send an email to the webmaster letting them know someone submitted an Advertisement. The file is never displayed to the user or accessed in anyway through the website; there is only one link to it in the email to the webmaster.

Thank you in advance for any insight provided,
Itshim

What is displayed?
echo “quack”;

 

[sleipnir214]

Some general comments.

At this point:

if ($_FILES['Ad']['tmp_name'] == "none")

it looks like your code is assuming that when no file is uploaded, $_FILES['Ad']['tmp_name'] will be "none". On my system, when there is no file uploaded, that array element is blank, not "none"


Keep in mind, too, as you're doing your error-checking that $_FILES['Ad']['error'] can tell you a lot, too. The errors are explained here.


Is there any possibility that this code could be invoked when the "Ad" file input was not submitted? If so, you will want to check for the existence of $_FILES['Ad'] before you do anything.


Other than that, it looks to me like you're in pretty good shape.


Want the best answers? Ask the best questions!

TANSTAAFL!!

 

[sleipnir214]

CORRECTION:

This sentence:

Other than that, it looks to me like you're in pretty good shape.

should have read:

Other than that, it looks to me like you're in about the best shape you can be in, considering the constraints of the project.


Want the best answers? Ask the best questions!

TANSTAAFL!!