Code check: file upload

PHP
1

I put together this code from my research. It’s the file used to accept a file upload. Is this correct? I am attempting to upload to the page but it is failing. Not sure if the problem is on the server side (this script) or not, at this point. (This code is being used to accept camera shot uploads o smartphones and tablets. I’m using Cordova PhoneGap software.)

I’m uncertain of the line that goes: “/srv/www/uploads/” This is from an example online. Is /srv/ common to servers, or am I supposed to change it? My server reflects the /www/uploads/ part correctly.

<?php
define("UPLOAD_DIR", "/srv/www/uploads/"); // UNCERTAIN OF THIS PART: /SRV/

print_r($_FILES);

$allowed_exts = array("gif", "jpeg", "jpg", "png");
$temp = explode(".", $_FILES["file"]["name"]);
$extension = end($temp);
if ((($_FILES["file"]["type"] == "image/gif")
|| ($_FILES["file"]["type"] == "image/jpeg")
|| ($_FILES["file"]["type"] == "image/jpg")
|| ($_FILES["file"]["type"] == "image/pjpeg")
|| ($_FILES["file"]["type"] == "image/x-png")
|| ($_FILES["file"]["type"] == "image/png"))
&& ($_FILES["file"]["size"] < 20000)
&& in_array($extension, $allowed_exts)) {
  if ($_FILES["file"]["error"] > 0) { ?>
document.getElementById('camera_status').innerHTML = "Return Code: "
<?php . $_FILES["file"]["error"] . ?>
document.getElementById('camera_status').innerHTML = "<br/>";
<?php
  }
  else {
  ?>

document.getElementById('camera_status').innerHTML = "Upload: "
<?php . $_FILES["file"]["name"] . ?>
document.getElementById('camera_status').innerHTML =+ "<br/>Type: "
<?php . $_FILES["file"]["type"] . ?>
document.getElementById('camera_status').innerHTML =+ "<br/>Size: "
<?php . ($_FILES["file"]["size"] / 2048) . ?>
document.getElementById('camera_status').innerHTML =+ " kB<br/>Temp file: "
<?php . $_FILES["file"]["tmp_name"] . ?>
document.getElementById('camera_status').innerHTML =+ "<br/>";
<?php
    if (file_exists(UPLOAD_DIR . $_FILES["file"]["name"])) { ?>
document.getElementById('camera_status').innerHTML =+ <?php $_FILES["file"]["name"] . ?>
document.getElementById('camera_status').innerHTML =+ " already exists. ";
<?php
    }
    else {
      var now = new Date();
      $new_image_name = now + ".jpg";
      move_uploaded_file($_FILES["file"]["tmp_name"], UPLOAD_DIR . $new_image_name);
    }
  }
}
else { ?>
document.getElementById('camera_status').innerHTML =+ "Invalid file";
<?php
}
?>
2

I’m surprised you’re not getting any error messages that might help, but I think you’re probably right about the upload directory.

I don’t know how many servers have this structure, but if you don’t I think you should create it or change the CONSTANT value.
It looks to be “outside the root” which might mean the script relies on that for security and it may be vunerable if you change to one under the root.

3

My guess is also that the directory must be changed to reflect the directory on your server. Other than that there are two quite crucial mistakes in the code. You have a few lines like


document.getElementById('camera_status').innerHTML [color=red]=+[/color] "<br/>Type: "

that must be


document.getElementById('camera_status').innerHTML [color=red]+=[/color] "<br/>Type: "

Also, to go from bytes to kB, you need to divide by 1024, not 2048 !

Lastly, the script isn’t very secure, since the file type in $_FILES isn’t very reliable. You’d better haul the file through fileinfo and check the information from that, or even better open and save again with GD to make sure that 1) it is in image and 2) all extra data the user may try to piggy back in via the image will be removed.

4

Thank you for the valuable info! I am using BlueHost, so I don’t think I can do anything with fileinfo or GD. But I made the other changes you pointed out.

Also, I’ll research to figure out how to make it more secure. Ideas welcome!

5

Looks like I can replace this …

$allowed_exts = array("jpeg", "jpg");
$temp = explode(".", $_FILES["file"]["name"]);
$extension = end($temp);
if ((($_FILES["file"]["type"] == "image/jpeg")
|| ($_FILES["file"]["type"] == "image/jpg"))
&& ($_FILES["file"]["size"] < 20000)
&& in_array($extension, $allowed_exts)) {
  if ($_FILES["file"]["error"] > 0) { ?>document.getElementById('camera_status').innerHTML += "Return Code: "
<?php . $_FILES["file"]["error"] . ?>document.getElementById('camera_status').innerHTML += "<br/>";

… with this …

$allowedTypes = array(IMAGETYPE_PNG, IMAGETYPE_JPEG, IMAGETYPE_GIF);
$detectedType = exif_imagetype($_FILES['file']['name']);
$error = !in_array($detectedType, $allowedTypes);
  ?>
  document.getElementById('camera_status').innerHTML += "Upload failed. Try again."

… for more security

6

Yup, that would work! :slight_smile:

7

Finally got console output from Eclipse. At fileTransfer, it insists that the upload.php page is not found:

E/FileTransfer(1013): java.io.FileNotFoundException: <URL here>

The URL is gives is correct, and if I use that URL in a browser, there is no 404 error. What’s odd is that I have this at the very bottom of the php page, and it doesn’t show the <h2> text:

}
?>

<!DOCTYPE html>
<html>
    <head>
        <meta charset="utf-8" />
		<title>upload page</title>
	</head>
<body style="background-color:yellow;">
	<h2>From HTML: This is an upload page.</h2> 
    </body>
</html>

And at the top of the page I have the following, and it doesn’t display that text either:

<?php
print("From PHP: This is an upload page.");

So now I know the problem is my php page somewhere. When I put this in a php page and access it, it gives me all kinds of info on PHP 5.4.24:

<?php phpinfo(); ?>
8

I tried this …

define(“UPLOAD_DIR”, “/www/uploads/”);

and this…

define(“UPLOAD_DIR”, $_SERVER[‘DOCUMENT_ROOT’] . “/www/uploads/”);

No difference in the error.