Only allowing images to be uploaded in image upload script

[lagc]

Over the weekend I was hacked and my site was used to upload a php file through an image upload area, and subsequently thousands of spam emails where sent out.

I can recognise the script that I'm supposed to be workign with, but I'm not 100% with it.

What my hosting company wants me to do is change it so only image files can be uploaded.

The code below is the upload button, and there are 4 of them.

<input type="file"  name="file1" size="8" >
<? if($i1!=""){
if (file_exists("../admin1212".$i1)) {
if(is_file("../admin1212".$i1))
{	
?>
<A onClick="MM_openBrWindow('<? echo "../admin1212".$i1; ?>');" class="hand"><b><u>View</u></b></A><? }}}	?>

I think the code that deals with the image upload is below.
If you think its not the lot, I will try and find the rest.

$src1=$_FILES['file1']['tmp_name'];
$fname1=rand(0,9999).$_FILES['file1']['name'];
$src2=$_FILES['file2']['tmp_name'];
$fname2=rand(0,9999).$_FILES['file2']['name'];
$src3=$_FILES['file3']['tmp_name'];
$fname3=rand(0,9999).$_FILES['file3']['name'];		
$src4=$_FILES['file4']['tmp_name'];
$fname4=rand(0,9999).$_FILES['file4']['name'];				
					
copy("$src1","imgdata/hotel/$fname1");		
copy("$src2","imgdata/hotel/$fname2");		
copy("$src3","imgdata/hotel/$fname3");		
copy("$src4","imgdata/hotel/$fname4");								
			
$Rate=$_POST['txtrtd'];
			
$len=strlen($Rate);

$ratenum=$_POST['txtrtd'];

if(strlen($fname1)<5)
{
$f1=$_POST['f1'];

}
else
{
$f1="/imgdata/hotel/$fname1";
}
						
if(strlen($fname2)<5)
{
$f2=$_POST['f2'];

}
else
{
$f2="/imgdata/hotel/$fname2";
}
						
if(strlen($fname3)<5)
{
$f3=$_POST['f3'];
}
else
{
$f3="/imgdata/hotel/$fname3";
}
						
if(strlen($fname4)<5)
{
$f4=$_POST['f4'];
						
}
else
{
$f4="/imgdata/hotel/$fname4";
}

So just to re-cap, what I need to fix is that only images can be uploaded, and not any type of file.

Be great too, if we can work with this code, as I've inherited this website, and I'm not a solid code developer.

Thanks guys

 

[lagc]

My bad, I missed a little bit of code for the first code section. It should be this.

<td bgcolor="#eeeeee" valign="top" width="27%"><input type="file"  name="file1" size="8" >
<? if($i1!=""){
if (file_exists("../admin1212".$i1)) {				
if(is_file("../admin1212".$i1))
{	
?>
<A onClick="MM_openBrWindow('<? echo "../admin1212".$i1; ?>');" class="hand"><b><u>View</u></b></A><? }}}	?>	
							
[b]<input type="hidden" name="f1" value="<?=$i1;?>">[/b]
</td>

The highlighted bit is the added code.

Sorry about that

 

[lagc]

Something I need to add also, is that the image upload option is only 1 part of a bigger form.

So if the file uploaded is not an image file it needs an alert or something to stop the rest of the data being uploaded.

If somebody wanted to take a look at the whole workings, which I wont to add isnt mine, although I couldnt do that code, I know its not the best, it maybe better if you saw the whole page with all the code.

As you can tell, Im pretty desperate to sort this out, as if it happens again my host will block my site and also charge me for the subsequent clear up.

Thanks

 

[lagc]

Ok Ive made a little bit of progress..

I added this to the part of the code that I think deals with the first upload file.

if ($_FILES['file1']['type'] == "image/gif")
{
$src1=$_FILES['file1']['tmp_name'];
$fname1=rand(0,9999).$_FILES['file1']['name'];
}
else
{	
echo "Invalid file";
}

So I can get it to come back and say Invalid file if the file isnt a gif, but the problem is using echo doesnt stop the upload, as you would expect. But the other trouble I have is that when it is a gif file, I get an error as below:

Warning: copy(imgdata/hotel/4523buy.gif) [function.copy]: failed to open stream: Permission denied in /home/checksaf/public_html/en/admin1212/admin_hotels.php on line 296

which relates to this

copy("$src1","imgdata/hotel/$fname1");		
copy("$src2","imgdata/hotel/$fname2");		
copy("$src3","imgdata/hotel/$fname3");		
copy("$src4","imgdata/hotel/$fname4");

What would be good, is if whilst recognising it isnt a gif file the upload is stopped and an alert is triggered and it will keep on alerting them until a gif file is uploaded.

Thanks

 

[lagc]

Sorry to keep posting.

Im making small steps, but i know im going to not go any further soon.

if (($_FILES['file1']['type'] == "image/gif")
|| ($_FILES['file1']['type'] == "image/jpeg")
|| ($_FILES['file1']['type'] == "image/pjpeg"))
{
$src1=$_FILES['file1']['tmp_name'];
$fname1=rand(0,9999).$_FILES['file1']['name'];  						
}
else
{
$msg = "Please upload image files only."; 
confirm($msg);
//echo "Invalid file";
}

What I cant seem to work out, is how to stop the upload going ahead after the user clicks the alert message.

Ideally it doesnt go any further than there. The other problem can wait I suppose, to see if I can sort this bit out.

 

[vacunita]

The way you are doing it requires the file to be uploaded first.

That is you can't check the type of file until you actually have it.

Once you know the file is not what you want you can ignore it and just not do anything with it.

You either wrap your code in a conditional so that if its not the correct type the rest of the code is not run, or you just exit the execution of the code if its not.

I guess the easier albeit less elegant way is to just output the error and issue an exit statement to terminate code execution.

You should really think about adding an extra layer of checking prior to upload as well. JS should be able to issue and alert if the file is not what's expected, and you should be able to reset the input if that's the case.





----------------------------------
Phil AKA Vacunita
----------------------------------
Ignorance is not necessarily Bliss, case in point:
Unknown has caused an Unknown Error on Unknown and must be shutdown to prevent damage to Unknown.

 

[lagc]

Hi Vacunita,

Would you be able to show me what you mean with code, using my code.

Thanks for getting back to me though, what you said does make sense and I would be grateful for some help.

I did inherit this code, and using google etc I have worked at it a bit.

I was trying to find an exit code, but couldnt find what I was looking for.

Cheers

 

[vacunita]

don;t know what your confirm() functions does there, but just stick an exit there instead to terminate script execution.

if (($_FILES['file1']['type'] == "image/gif")
|| ($_FILES['file1']['type'] == "image/jpeg")
|| ($_FILES['file1']['type'] == "image/pjpeg"))
{
$src1=$_FILES['file1']['tmp_name'];
$fname1=rand(0,9999).$_FILES['file1']['name'];                          
}
else
{
$msg = "Please upload image files only.";
[red]exit($msg);[/red]
}

----------------------------------
Phil AKA Vacunita
----------------------------------
Ignorance is not necessarily Bliss, case in point:
Unknown has caused an Unknown Error on Unknown and must be shutdown to prevent damage to Unknown.

 

[lagc]

Hi,

confirm($msg); is calling some java script to allow an alert to appear which contains the alert message $msg.

So changing that to exit($msg); will cancel the java script alert pop up, and so will that exit script still work.

I'm cant do anything until I get back into work to try it, thats why I'm not trying it straight away.

 

[vacunita]

You can mix and match Javascript and PHP like that.

They run separately, and Javascript must either be inside an element event or surrounded by <script> tags.


If you want to get a Js box I would suggest using an alert instead of a confirm, since you don't really need an answer from the user, and stick it in the message variable like this:

$msg = "[red]<script> alert('[/red]Please upload image files only.[red]'); </script>[/red]";
exit($msg);

----------------------------------
Phil AKA Vacunita
----------------------------------
Ignorance is not necessarily Bliss, case in point:
Unknown has caused an Unknown Error on Unknown and must be shutdown to prevent damage to Unknown.

 

[lagc]

Ok Ive managed to connect to my computer in work now.

I just added your code, and it works as in it didnt recognise a .doc file and the alert message appeared which is great.

I clicked OK and the rest of the script didnt run, which again is great.

But is there then anyway to refresh the page and so the page loads again, ready to try and upload again.

I did try header location today and it didnt allow me for some reason.

So its great that unless its an image file it wont work, but I need to make it abit more user firendly for the person uploading file, so they can try again, although the main issue is to stop a hacker uploading files to that resulted in thousands of emails being sent.

Thanks for the help too

 

[lagc]

On second thoughts, maybe its not that important.

The good thing is that they cannot upload any file other than a jpg, gif or png, which is good.

But I have a slightly different problem now, when the file is one of those image files.

The script uploads the file, but i get an error for the image Im trying to upload, so would you maybe be able to cast your eye over it for me.

Here is the error:

Warning: copy(imgdata/hotel/4918St-Davids-Day-2010-1.jpg) [function.copy]: failed to open stream: Permission denied in /home/checksaf/public_html/en/admin1212/admin_hotels.php on line 310

That code is referring to the code below:

copy("$src1","imgdata/hotel/$fname1");
{/code]

Which is the next bit of code in the upload of the images, this line dealing specifically with what I changed. I havent changed naythng here, and cant see what the problem could be.

 

[vacunita]

Did you change the folder to which you are copying the images? Looks like the PHP process can't read the files in the imgdata/hotel/ folder. If you can give everyone permission to access the folder. You webhost/server admin should be able to tell you how to do this.

Maybe a file with that name already exists and can't overwrite it.

http://php.net/manual/en/function.copy.php

----------------------------------
Phil AKA Vacunita
----------------------------------
Ignorance is not necessarily Bliss, case in point:
Unknown has caused an Unknown Error on Unknown and must be shutdown to prevent damage to Unknown.