ZipArchive class PHP security issue

Question

I have the following PHP code

// Check if the upload is setted
if
(
    isset($_FILES['file']['name']) && !empty($_FILES['file']['name']) && 
    isset($_FILES['file']['type']) && !empty($_FILES['file']['type']) &&
    isset($_FILES['file']['size']) && !empty($_FILES['file']['size'])
)
{
    $UploadIsSetted = true;
    $UploadIsBad = false;

    $UploadExtension = pathinfo($_FILES['file']['name'], PATHINFO_EXTENSION);

    // Check if the upload is good
    require "../xdata/php/website_config/website.php";
    $RandomFoo = rand(1000999999,9999999999);

    if (($_FILES["file"]["size"] < ($MaxAvatarPictureSize*1000000)))
    {
        if ($_FILES["file"]["error"] > 0)
        {
            $UploadIsBad = true;
            $hrefs->item(0)->setAttribute("Error","true");
            $hrefs->item(0)->setAttribute("SomethingWrong","true");
        }
        else
        {
            move_uploaded_file($_FILES["file"]["tmp_name"],"../upload/tmp/".$RandomFoo.".file");    
        }
    }
    else
    {
        // The file is too big
        $UploadIsBad = true;
        $hrefs->item(0)->setAttribute("Error","true");
        $hrefs->item(0)->setAttribute("UploadTooBig","true");
    }
}
else
{
    $UploadIsSetted = false;
}

$ZipFile = new ZipArchive;
$ZipFile->open('../upload/tmp/'.$LastFilename.'.zip',ZIPARCHIVE::CREATE);
$ZipFile->addFile('../upload/tmp/'.$RandomFoo.'.file',$RandomFoo.".".$UploadExtension);
$ZipFile->close();

now my big concern is that user can upload anything so how can i prevent :

uploading 2GB 3GB files
floading
uploading some kind of twisted exploit that would eventually alter my server security
buffer overflow
filenames that have arbitrary code injections

i mean, how secure is this script?

i'm running windows for now, i will switch to linux

That's too many subquestions at once. Ask for the most important. Else consider [Codereview.SE](http://codereview.stackexchange.com/) please. — mario, Jul 02 '12 at 17:51

score 1 · Accepted Answer · edited May 23 '17 at 12:04

Four your other questions:

floading

That's the complex part. Let me google you some ideas:

Prevent PHP script from being flooded
Quick and easy flood protection? - use a nonce, time+tie it onto a session
Use a captcha, if it doesn't impair usability too much.

uploading some kind of twisted exploit that would eventually alter my server security

Use a commandline virus scanner (f-prot or clamav) to scan uploaded files. You might use a naive regex scanner in PHP itself (probe for HTMLish content in image files, e.g.), but that's not a factual security feature; don't reinvent the wheel.

buffer overflow

PHP in general is not susceptible to buffer overflows.

Okay, joking. But you can't do anything in userland about it. But pushing strings around isn't much of a problem. That's pretty reliable and unexploitable in scripting languages, as long as you know how to escape what in which context.

filenames that have arbitrary code injections

At the very leat you should most always use basename() to avoid path traversal exploits. If you want to keep user-specified filenames, a regex whitelist is in order. =preg_replace('/[^\w\s.]/', '', $fn) as crude example.

thank you very much, very useful :) glad to read all of those — Master345, Jul 02 '12 at 18:10

score 0 · Answer 2 · answered Jul 02 '12 at 17:51

0

Your line if (($_FILES["file"]["size"] < ($MaxAvatarPictureSize*1000000))) already limits the size of file acceptable to $MaxAvatarPictureSize megabytes. Though $MaxAvatarPictureSize doesn't appear to be set in the code you provided. My guess that should be 1 or 2 max. Also not set is $LastFilename and probably some others too.

Also place an if($UploadIsBad === false) { /* do zipping */ } around the Zipping part to avoid zipping up files which are too large or otherwise invalid.

answered Jul 02 '12 at 17:51

Bob Davies

2,229
1
19
28

and couldn't $_FILES["file"]["size"] be fake? injecting some code? i don't know, i'm not sec expert, but i should become, i'm developing an software ... – Master345 Jul 02 '12 at 17:53
1

He might as well use [`upload_max_filesize`](http://www.php.net/manual/en/ini.core.php#ini.upload-max-filesize) as that avoids PHP processing bloated requests already. (Though with non-userfriendly message.) – mario Jul 02 '12 at 17:54
Additional check would be to call `filesize($_FILES["file"]["tmp_name"])` to check actual size instead of relying on `$_FILES["file"]["size"]` as @RowMinds said. – Bob Davies Jul 02 '12 at 17:55

ZipArchive class PHP security issue

2 Answers2