[thelist] PHP file upload: how to make it safe?

[Steve Lewis]

m u i n a r wrote:

> How dangerous is a public file upload?
That depends on
1) the audience who has access to upload
2) how you limit what can be uploaded

> I'm checking for file type, size and image dimensions (see following PHP
> code snippet). If it has a width, it should be an image, and image files
> can't be self-executing and harm the server - ?

You are using three mechanisms for this:
1) type
2) size
3) image dimensions

Based on this, I presume that you are allowing ONLY images to be
uploaded.  My greatest concerns is that according to the PHP Website
(http://www.php.net/manual/en/function.getimagesize.php)

"If accessing the filename image is impossible, or if it isn't a valid
picture, getimagesize()  will return NULL and generate a warning."

You perform NO error-checking here.  Yes the code will return an error
when you try to /$img1Width = $imageDim[ 0 ];/ when $imageDim is NULL,
however, this is poor coding practice.

First, be sure there is a valid uploaded file using /is_uploaded_file()/
.  You then want to instead check /if (IsArray($imageDim))/ and trap
this error.  Otherwise use an error throw/catch mechanism with
/trigger_error()/ and /user_error()/ to trap this.

>         $imageDim = getimagesize( $img1 );
>         $img1Width = $imageDim[ 0 ];
>         $img1Height = $imageDim[ 1 ];
>         $type1 = $HTTP_POST_FILES['img1']['type'];
>         if( substr_count( $type1,"jpeg" ) != 0 and $img1Width *
> $img1Height <= 5000 )
>                 {
>                 $newFile = "cat_" . $id . ".jpg";
>                 copy( "$img1", "$dirName/$newFile" );
>                 $textJpg = "Image uploaded.";
>                 }
>
More concerns:
- How is $id generated?  This needs to be generated by an atomic
thread-safe operation (a primary key using identity or auto_increment
for instance).
- Are /registered globals/ off?  (If they are on, you should explicitly
scope your variables whenever possible)
- Which version of PHP are you running?  You should be using $_FILES
instead of $HTTP_POST_FILES as of PHP 4.1.0!
- you CAN get the file type from $imageDim[3] as well, and this method
is probably prefered.
- Where is $newFile coming from?  Don't trust user-generated filenames.
  Consider URLEncoding them as '%20' is less troublesome than ' ' to
your filesystem.
- don't use copy, use move_uploaded_file() instead... again with error
checking!
- why use $img1Width * $img1Height again?  You can check
$_FILES['img1']['size'] to get the actual file size in bytes, which is
probably more meaningful than it's pixel-area.

> Is it still possible to fake all three criteria and upload something
> dangerous? Or, generally asked: Is it less secure than a normal website
> without file upload?

To the latter question: Yes, but there is not much you can do.  You have
no option but to trust that there are no security flaws in PHP itself
(such as http://www.php.net/release_4_2_2.php), or your web server (see
http://www.cert.org/advisories/CA-2002-17.html), and the file-upload
management system will not itself compromise the server when you have
carefully coded the file upload mechanism.

P.S. I do have an article on handling File Uploads in ColdFusion written
which was submitted to Evolt about the time everyone got busy with the
server relocation, but is presumably waiting on technical/editorial
matters before publication.  Stay tuned.

-- Steve