PHP image uploading security. (Advice from resident PHP gurus)

Factual Questions
1

Hi Dudes.
I had an uploader but I’ve decided, after reading this article…

http://www.scanit.be/uploads/php-file-upload.pdf

That it needs to be more secure. So I’ve disabled it.

The article goes into some quite complex details about what code to use. I have difficulty following it in such a way that I could end up producing working code. But I’ve been able to pick up enough to have an idea of what I need to do for an uploader…

Firstly, I want the files to be accessable from a URL directly. So my plan is that I will have personal control over the files that end up in the directory in question.

Secondly, I want people to be able to upload files without my direct supervision. So I want an uploader to do three four things…

Upload the file with a randomized file name to a folder outside the public html folder.

Add a record to a table called something like ‘pending_moderation’ which has the real filename, and the randomized filename.

Limit the filenames to ‘.jpg’ or ‘.jpeg’ (or possibly also ‘.gif’ if it is as secure as the first two) and the mime types to the same.

sanitize the data input.

The latter presents a problem with file uploading - It doesn’t work well with filenames. It has problems with the slashes and the dots. This would be a nightmare with the local filename as there could be any number of subdirs.

In the hope of illustrating my point. This…

<?php
$filename = "c:	hepictures\apicture.jpg";

$con = mysql_connect();
if (!$con)
  {
  die('Could not connect: ' . mysql_error());
  }

mysql_select_db();

$sanitized = mysql_real_escape_string($filename);

echo $sanitized;

?>

outputs this…

“c: hepictures\apicture.jpg”

the ’ ’ at has been removed.

It seems like any picture or folder that begins with a slash command will be broken.

Any advice on all this? Should I not even be attempting this if my php abilities aren’t up to that level?

2

Another question (to save using another thread).

I just moved my db connection string into a seperate file.

Is it possible and safer to move that file outside public_html and refer to it like this…
include(’…/includes/mydatabaseconnection.php’);
ETA: It does seem to work. Is it preferable? Or does it really not matter?

3

I’m going to give this just the one bump.

4

I put my connections (and some other includes and functions etc.) in a separate folder within the served folders. But I make sure that they are all .php and not .txt or .htm or something that could be served by the server and ultimately read by a user. I also add a file, “index.php” to all directories that I don’t want a user to view via the directory browser of the server, with code in it to redirect back to the homepage. That way if a user even tries to see what folders are in the “includes” directory, they will just get redirected back to the homepage. I believe this directory browsing feature can be turned off on most servers, but I don’t have much control over the host’s sever.

I am not saying this is the best approach, only what I have done in the past. I have not thought about or tried moving the connections outside the public folders.

Also, I am curious as to the answer for your first question. I am in a similar position. The goal I am trying to accomplish is to allow an admin user (so less security required) to upload a .zip file full of images. Upon upload the zip file will automatically be unzipped into a directory where the photo gallery is located. This would allow the admin users to add batches of photos for the gallery without getting the webmaster involved to FTP them over.

5

Thanks. FYI moving the file outside the public area does work. I do name all my files .php.

And funilly enough it did occur to me to add an index.php to the folders I don’t want people to see. But since I will have exclusive manual control over that folder (i.e. none of my code will put files there. I’ll be doing it myself from the ‘hidden’ destination of my eventually secure uploader) I don’t mind it being directory listable.

Unless there is still a valid reason for it not to be?
ETA: On second thoughts, for all I know a malicious person could find a way of putting files in the folder so I will make them ‘hidden’ with an index.php

6

You can also set your webserver to disallow “directory browsing” which will stop people from looking inside that folder without having to have an index.php file in there.

Personally I have my DB connection string in my webserver directory. It’s a file ending with .php so is pretty secure in that anyone who guesses the filename will only get a blank output back as that file does not echo anything. I wouldn’t say it’s 100% secure, but over the last 10 or 15 years, no-one has broken into anything of mine.

These are some snippets from code that I use for file uploading. Again, probably not 100% secure, but I haven’t been broken into nor had anything malicious uploaded. Technically someone could spoof the extension and the mime type and all bets are off, but I doubt they will be trying that on a small server somewhere.

If you are only wanting image files (and don’t care that animated GIFs get broken), then use the built in GD library to PHP and manipulate the image after you’ve initially verified it. i.e. Add a stamp to the bottom of the picture or something or make a thumbnail of it. You can always dispose of the thumbnail if it works and only keep the original file.

That way if the file is NOT what you expected from the extension and mime type, GD will probably fall over trying to open an invalid file to manipulate.

In the upload form:

"><input type="hidden" name="MAX_FILE_SIZE" value="1000000"><input type="file" name="userfile" size="50">

Note the name userfile.

Then in my code for the uploader I have this:

         $imtype      = $_FILES["userfile"]["type"];
         $imsize      = $_FILES["userfile"]["size"];
         $imname      = $_FILES["userfile"]["name"];
         $imtemp      = $_FILES["userfile"]["tmp_name"];
         $imerror     = $_FILES["userfile"]["error"];

Further on I use this:

            switch ($imtype)
              {
               case "image/pjpeg":
                  $filetype = ".jpg";
                  break;
               case "image/jpeg":
                  $filetype = ".jpg";
                  break;
               case "image/gif":
                  $filetype = ".gif";
                  break;
               default:
                  $error_message="You are restricted to image files only.<br>Please load images with a file type of .jpg or .jpeg or .gif only";
              }

Having a switch on the $imtype means I can check for the mime type of the file that was uploaded. If it’s a valid image mime type of one that I want, then I let it through, otherwise I error.

As far as the name of the file goes I don’t use any of the directory stuff that comes through (I’m not even sure if it comes through).

I set up my own filename to save to my webserver. As you have noted, some filenames will simply be invalid depending on what your OS is.

               $uploaddir        = $_SERVER["DOCUMENT_ROOT"]."/path/to/images/";
               $uploadfile       = $uploaddir.$MYNAME.$filetype;


               if (move_uploaded_file($_FILES["userfile"]["tmp_name"], $uploadfile)) {
...dostuff
               } else {
                  echo "An error has occured.<br>Please contact the webmaster with the diagnostic informtion:<br>";
                  echo "Diagnostic Information";
                  echo "<pre>";
                  print_r($_FILES);
                  echo "</pre>";
                 }

Unless you have a jolly good reason to keep the filename, don’t. This way you also avoid duplicates. i.e. If I upload a.jpg and someone later uploads a.jpg, are you going to overwrite it? If you have a DB with a table that has autoincrement, use the autoincrement number as the filename. Never have duplicates again. You can also be pretty sure that the filename will be valid.

If you want a way for the user to differentiate their file, use a description and store that in your DB in a new column. That way a.jpg = “Cute kitty picture”. No duplicates, valid filename, and if you check the mime type, pretty much a non-malicious file.

7

I will stick with the randomized names as you suggest.
I like the idea of putting the files outside the document root. I know it will probably be fine to store them within the DR but I like the idea of them waiting in some non-publicly-accessable place until I manually move them. So, would this work?..

uploaddir = _SERVER[“DOCUMENT_ROOT”]."…/images/";

Or would this make more sense…

$uploaddir = “images/”;
(Because I think that will default to a folder just outside of the DR called images).

I assume you got $MYNAME from a bit of string randomizing code not shown in your post?
I am using phpthumb to create thumbnails on the fly. it uses the GD (unless I have something called imagemagic installed, which I don’t.) But I’d like anigifs to be animated, so they bypass phpthumb.

8

Right. Here’s what I have so far. Feel free to comment. I have some questions at the end…

<form enctype="multipart/form-data" method="POST" action="<?=$_SERVER['PHP_SELF']?>" >

<input type="file" id="userfile" name="userfile" size="50" /></br>
<input type="hidden" name="MAX_FILE_SIZE" value="2000000" />
<input type="submit" name="submit" value="Upload this image"/>
</form>

<?php

include('../includes/db.php');
include('../includes/functions.php');

if (isset($_POST['submit']))
{
 $imtype = $_FILES["userfile"]["type"];
 $imsize = $_FILES["userfile"]["size"];
 $imname = $_FILES["userfile"]["name"];
 $imtemp = $_FILES["userfile"]["tmp_name"];
 $imerror = $_FILES["userfile"]["error"]; 
  
 switch ($imtype)
 {
  case "image/pjpeg":$filetype = ".jpg";break;
  case "image/jpeg":$filetype = ".jpg";break;
  case "image/gif":$filetype = ".gif";break;
  case "image/png":$filetype = ".png";break;
  default:die("You are restricted to image files only.<br>Please load images with a file type of .jpg or .jpeg or .gif only");  
 }  
 
 $randomizedname = random_string(16);
  
 $uploaddir = $_SERVER["DOCUMENT_ROOT"]."/../images/"; 
 $uploadfile = $uploaddir.$randomizedname.$filetype; 
 
 
 
 if (move_uploaded_file($_FILES["userfile"]["tmp_name"], $uploadfile)) 
 {
  echo "File has been uploaded and will be reviewed by the webmaster";
  
  
  mysql_select_db($whatsbetterdb, $con);
  
  $sql = "insert into pending (filename) values ('".$randomizedname.$filetype."')";
  
  //echo $sql;
  
  $result = mysql_query($sql) or die (mysql_error());
  
  
 } 
 else 
 {
  echo "An error has occured.<br>Please contact the webmaster with the diagnostic informtion:<br>";
  echo "Diagnostic Information";
  echo "<pre>";
  print_r($_FILES);
  echo "</pre>";
 }  
}

?>

Q1. From your code Caught@Work I’ve altered the behaviour of the ‘default:’ case from…

$error_message="You are restricted to image files only.<br>Please load images with a file type of .jpg or .jpeg or .gif only";

To…

default:die("You are restricted to image files only.<br>Please load images with a file type of .jpg or .jpeg or .gif only");

Is this an acceptable way of doing things?

Q2. Considering that no form input is part of the SQL query, do I no longer need to sanitize it? (all that is being passed is a filename and extension, both generated by the code)
And finally Thanks very much for your help with this. :slight_smile:

9

Doh. It still doesn’t work. I renamed a .doc file to .jpg, and the imtype thinks it’s image/jpeg.

10

I’ve fixed it by making the following change…

$imtype = mime_content_type($_FILES["userfile"]["tmp_name"]);
 $imsize = $_FILES["userfile"]["size"];
 $imname = $_FILES["userfile"]["name"];
 $imtemp = $_FILES["userfile"]["tmp_name"];
 $imerror = $_FILES["userfile"]["error"];