Upload script not working

[Chanpa]

Hey,

I'm creating a script so my clients can upload pictures to their site and I had it working but then I think I broke it somehow. This is what it's looks like:

The Form:

<form enctype='multipart/form-data' id='uploadPic' action='../php-bin/uploader.php' method='post'>
Namn: <input type='text' name='name' value=''><br />
Text: <input type='text' name='text' value=''><br />
<input name='uploadedfile' type='file' /><br />
<input type='hidden' name='location' value='/images/slideshow/'>
<input type='hidden' name='dbtable' value='Slideshow'>"<input type='submit' value='Submit'></form>

 

And this is the uploader.php script:

<?php
include $_SERVER['DOCUMENT_ROOT']."/php-bin/var.php";

$name=mysql_real_escape_string($_POST['name']);
$text=mysql_real_escape_string($_POST['text']);
$dbtable=mysql_real_escape_string($_POST['dbtable']);
$location=mysql_real_escape_string($_POST['location']);
$location.=basename($_FILES['uploadedfile']['name']); 

// Check if it's an image:
if (!getimagesize($_FILES['uploadedfile']['tmp_name'])){
echo "Bara bilder kan laddas upp.";
exit();
}

// Restrict width and height of the image is for slideshow:
if($dbtable == "Slideshow"){ 
list($width, $height, $type, $attr) = getimagesize($_FILES['uploadedfile']['tmp_name']);
if (!($width == 710)){
	echo "Bilder till bildspelet måste vara exakt 710 pixlar breda. Filen laddades INTE upp.";
	exit();
}
if (!($height == 420)){
	echo "Bilder till bildspelet måste vara exakt 420 pixlar höga. Filen laddades INTE upp.";
	exit();
}
}

// The file is a picture. (With both correct height and width if its for the slideshow)
// Upload it:
if(move_uploaded_file($_FILES['uploadedfile']['tmp_name'], $location)){
echo "The file ".  basename( $_FILES['uploadedfile']['name']). " has been uploaded";
}
else {
echo "Bilden laddades INTE upp.<br />".
	 "location = $location<br />".
	 "tmp_name = ".$_FILES['uploadedfile']['tmp_name']."<br />".
	 "Error: ".$_FILES['uploadedfile']['error'];
exit();
}

$query="INSERT INTO $dbtable VALUES ('$name','$location','$text','')";
mysql_query($query) or die("Could not execute query: $query." . mysql_error());
mysql_close();
?>

 

The script works to a certain degree, but the picture doesn't actually upload, it goes into the last else statement;

if(move_uploaded_file($_FILES['uploadedfile']['tmp_name'], $location)){
echo "The file ".  basename( $_FILES['uploadedfile']['name']). " has been uploaded";
}
[b]else {
echo "Bilden laddades INTE upp.<br />".
	 "location = $location<br />".
	 "tmp_name = ".$_FILES['uploadedfile']['tmp_name']."<br />".
	 "Error: ".$_FILES['uploadedfile']['error'];
exit();
}[/b]

 

This is what the page says when I run:

Bilden laddades INTE upp.

location = /images/slideshow/slideShow_1.jpg

tmp_name = C:\mowes_portable\php5\temp\php7049.tmp

Error: 0

So I looked up error: 0 and it found:

UPLOAD_ERR_OK

Value: 0; There is no error, the file uploaded with success.

And since the else-statement only triggers when the move fails, shouldn't this be impossible?

 

As I said it worked awhile then it stopped, so I though I had reached max_file_uploads so I raised that from 20 to 1000 and it still doesn't work.

 

If I try to upload other filetypes, it does into the correct if-statement and exits, same for the height and width if I try to upload for the slideshow, so those two checks are working, so the file must be uploaded to the temp location but then it doesn't get moved to the $location.

 

Any light on this is greatly appreciated.

[PFMaBiSmAd]

The leading slash / on your location value refers to the root of the current hard disk. I doubt that is where you intended the uplaoded files to be put and the /images/slideshow/ folder probably does not exist starting in the root of the disk.

 

You should have php's error_reporting set to E_ALL and display_errors set to ON in your master php.ini on your development system so that all the php detected errors will be reported and displayed. You would be getting a 'directory or file does not exist' error message.

 

Also, you should not accept the location through a form field, without completely validating it, since that would allow someone to upload a file to ANY location on your server and do things like replace your main index.php file. You also need to validate the filename in $_FILES['uploadedfile']['name'] to only allow files of a particular file extension to be uploaded (you would not want .php files to be uploaded and you can fool getimagesize with image data in a file that also contains php code.)

[Chanpa]

Thanks for the reply! I updated the first post with some new info, basically that the error code for the $_FILES returned 0 which means ok.

 

Your post really helped! I solved it so now it works. What I did was:

$target_path=$_SERVER['DOCUMENT_ROOT'];

$name=mysql_real_escape_string($_POST['name']);
$text=mysql_real_escape_string($_POST['text']);
$dbtable=mysql_real_escape_string($_POST['dbtable']);
$location=mysql_real_escape_string($_POST['location']);
$location.=basename($_FILES['uploadedfile']['name']);

$target_path.=$location;

 

And then changed the if-statement to use the $target_path variable.

Again your post was extremely helpful. Now I want to ask you about your other pointers

you should not accept the location through a form field, without completely validating it

How do I "completely" validate it?

 

You also need to validate the filename in $_FILES['uploadedfile']['name'] to only allow files of a particular file extension

Ok, I added this after the if(!getimagesize)-statement:

$blacklist = array(".php", ".phtml", ".php3", ".php4", ".js", ".shtml", ".pl" ,".py");
foreach ($blacklist as $file){
if(preg_match("/$file\$/i", $_FILES['uploadedfile']['name'])){
	echo "ERROR: That filetype is NOT allowed.\n";
	exit;
}
}

Is that sort of what you mean?

Again thanks for all your input!

[PFMaBiSmAd]

How do I "completely" validate it?

 

Why do you need to pass the path through the form? If it is always images/slideshow/, just use that value in the php code. If you do need it to be one of several choices, either -

 

1) User a numerical index to path relationship and only pass the numerical index through the form. That way someone (hacker) can only pick one of the possible values by suppling the index number. Your php code would get the actual path from the supplied index number.

 

2) If you still want to pass the actual path, such as images/slideshow/, through the form, completely validating it means that you test if the submitted value is only and exactly one of the possible paths.

 

For your extension validation, you should use a white list of permitted values, rather than try to use a black list, since you can miss some possible values and they could change over time or be different on different servers.

 

 

[Chanpa]

I want to use the same script to upload pictures to other places aswell. images/articles for example.

So doing it the 2nd way I would add something like this at the start of the file?:

if ($location != "images/slideshow"){
      echo "That location isn't allowed";
      exit();
}
// The script

 

And I changed from using blacklist to using a whitelist, don't even know why I used blacklist to start with.

 

Really big thanks for all your help