1. Advertising
    y u no do it?

    Advertising (learn more)

    Advertise virtually anything here, with CPM banner ads, CPM email ads and CPC contextual links. You can target relevant areas of the site and show ads based on geographical location of the user if you wish.

    Starts at just $1 per CPM or $0.10 per CPC.

I need a review for my secure PHP file uploading script

Discussion in 'PHP' started by eritrea1, Nov 30, 2013.

  1. #1
    I am new to code review, and to PHP also.
    Last night I made the below image uploading class. It was the first time I made a file uploader to be used on a real site, so I thought I would share it here and on Github https://github.com/simon-eQ/BulletProof/blob/master/BulletProof.php to get some reviews, hoping if I can perfect it with your help, I will use the class on all my future projects.
    <?php
        /**
        * A small, secure & fast image uploader class written in all-static
        * class to give an extra boost in performance.
        * @author    Simon _eQ <https://github.com/simon-eQ>
        * @license    Public domain. Do anything you want with it.
        * Don't ever forget to use at your own risk.
        */
    
    
        class BulletProof
        {
            /**
            * User defined, allowed image extensions to upload. ex: 'jpg, png, gif'
            * @var
            */
            static $allowedMimeTypes;
    
            /**
            * Set a max. height and Width of image
            * @var
            */
            static $allowedFileDimensions;
    
            /**
            * Max file size to upload. Must be less than server/browser
            * MAX_FILE_UPLOAD directives
            * @var
            */
            static $allowedMaxFileSize;
    
            /**
            * Set the new directory / folder to upload new image into.
            * @var
            */
            static $directoryToUpload;
    
            /**
            * MIME type of the upload image/file
            * @var
            */
            static $fileMimeType;
    
            /**
            * A new name you have chosen to give the image
            * @var
            */
            static $newFileName;
    
    
            /**
            * Set of rules passed by user to choose what/where/how to upload.
            * @param array $allowedMimeTypes
            * @param array $allowedFileDimensions
            * @param $allowedMaxFileSize
            * @param $directoryToUpload
            */
            static function options(array $allowedMimeTypes,
                                    array $allowedFileDimensions,
                                          $allowedMaxFileSize,
                                          $directoryToUpload)
            {
                /**
                * Globalize the score of the directives, so we can call them from another method.
                */
                self::$allowedMimeTypes = $allowedMimeTypes;
                self::$allowedFileDimensions = $allowedFileDimensions;
                self::$allowedMaxFileSize = $allowedMaxFileSize;
                self::$directoryToUpload = $directoryToUpload;
            }
    
    
            /**
            * Native and possible PHP errors provided by the $_FILES[] super-global.
            * @return array
            */
            static function commonFileUploadErrors()
            {
                /**
                * We can use the key identifier from $_FILES['error'] to match this array's key and
                * output the corresponding errors. Damn I'm good!
                */
                return array(
                    UPLOAD_ERR_OK          => "...",
                    UPLOAD_ERR_INI_SIZE    => "File is larger than the specified amount set by the server",
                    UPLOAD_ERR_FORM_SIZE    => "Files is larger than the specified amount specified by browser",
                    UPLOAD_ERR_PARTIAL        => "File could not be fully uploaded. Please try again later",
                    UPLOAD_ERR_NO_FILE        => "File is not found",
                    UPLOAD_ERR_NO_TMP_DIR    => "Can't write to disk, as per server configuration",
                    UPLOAD_ERR_EXTENSION    => "A PHP extension has halted this file upload process"
                );
            }
    
    
            /**
            * There are many reasons for a file upload not work, other than from the information we
            * can get of the $_FILES array. So, this function tends to debug server environment for
            * a possible cause of an error (if any)
            * @param null $newDirectory optional directory, if not specified this class will use tmp_name
            * @return string
            */
            static function checkServerPermissions($newDirectory = null)
            {
                $uploadFileTo = $newDirectory ? $newDirectory : init_get("file_uploads");
    
                /**
                * if the directory (if) specified by user is indeed dir or not
                */
                if(!is_dir($uploadFileTo))
                {
                    return "Please make sure this is a valid directory, or php 'file_uploads' is turned on";
                }
    
                /**
                * Check http header to see if server accepts images
                */
                if(stripos('image', $_SERVER['HTTP_ACCEPT']) !== false)
                {
                    return "This evil server does not seem to accept images";
                }
    
    
                /**
                * Check if given directory has write permissions
                */
              //  if(!substr(sprintf('%o', fileperms($uploadFileTo)), -4) != 0777)
              //  {
              //      return "Sorry, you don't have her majesty's permission to upload files on this server";
              //  }
    
            }
    
    
            /**
            * Upload given files, after a series of validations
            * @param $fileToUpload
            * @param $newFileName
            * @return bool|string
            */
            static function upload($fileToUpload, $newFileName = null)
            {
    
                /**
                * check if file's MIME type is specified in the allowed MIME types
                */
                $fileMimeType = substr($fileToUpload['type'], 6);
                if(!in_array($fileMimeType, self::$allowedMimeTypes))
                {
                    return "This file type is not allowed.";
                }
    
                self::$fileMimeType = $fileMimeType;
    
                /**
                * show if there is any error
                */
                if($fileToUpload['error'])
                {
                    $errors = self::commonFileUploadErrors();
                    return $errors[$fileToUpload['error']];
                }
    
                /**
                * Check if size of the file is greater than specified
                */
                if($fileToUpload['size'] > self::$allowedMaxFileSize)
                {
                    return "File size must be less than ".(self::$allowedMaxFileSize / 100)." Kbytes";
                }
    
                /**
                * Checking image dimension is an enhancement as a feature but a must & wise check from
                * a security point of view.
                */
                list($width, $height, $type, $attr) = getimagesize($fileToUpload['tmp_name']);
    
                if($width > self::$allowedFileDimensions['max-width'] ||
                  $height > self::$allowedFileDimensions['max-height'])
                {
                    return "Image must be less than ". self::$allowedFileDimensions['max-width']."pixels wide
                            and ". self::$allowedFileDimensions['max-height']."pixels in height";
                }
    
                /**
                * No monkey business
                */
                if($height <= 1 || $width <= 1)
                {
                    return "This is invalid Image type";
                }
    
                /**
                * If user has provided a new file name, assign it. Otherwise,
                * use a default uniqid id, to avoid name collision
                */
                if($newFileName)
                {
                    self::$newFileName = $newFileName;
                }
                else
                {
                    self::$newFileName = uniqid();
                }
    
                /**
                * Upload file.
                */
                $newUploadDir = self::$directoryToUpload;
                $upload = move_uploaded_file($fileToUpload['tmp_name'], $newUploadDir.'/'.self::$newFileName.'.'.self::$fileMimeType);
    
                if($upload)
                {
                  return true;
                }
                else
                {
                    /**
                    * If file upload fails, the debug server enviroment as a last resort before giving 'Unknown error'
                    */
                    $systemErrorCheck = self::checkServerPermissions($newUploadDir);
                    return $systemErrorCheck ? $systemErrorCheck : "Unknown error occured. Please try again later";
                }
    
    
            }
    
        } 
    PHP:
    In-between the time I finished the it, and right now. I have began to have doubts about somethings I did/should have done like:

    - Is making the methods static really appropriate for this class? There is no need for
    unit testing, so imho, I would say, there is nothing with using static class here.

    - Should I create costume exceptions to handle the errors instead of returning error messages (note, I am also trying to make the script faster)
    - Should I be extending the `SplFileInfo` class handle verification the file types better
    - Is it better to use `finfo_open(FILEINFO_MIME_TYPE);` to check file type, or is `$_FILES['file']['type']` or SplFileInfo's getFileExtension http://www.php.net/manual/en/splfileinfo.getextension.php) Which one is safer, or better..
    This is the usage of the class.

    BulletProof::set(array('png', 'jpeg', 'gif', 'jpg'),
                        array('max-width'=>150, 'max-height'=>150),
                        30000,
                        'pictures/'
                        );
        if($_FILES){
          $upload =  BulletProof::upload($_FILES['profile_pic'], 'simon');
          if($upload !== true){
            echo "ERROR: ".$upload;
          }
        }
    PHP:
    I am still thinking if `BulletProof::upload()` should take in all the augments, if user wants to upload different files with different types/sizes/dimensions instead of creating a global option style.

    I have a lot of questions/doubts, but I am in a need of any type of review.

    Thanks.
     
    eritrea1, Nov 30, 2013 IP
  2. nico_swd

    nico_swd Prominent Member

    Messages:
    4,153
    Likes Received:
    344
    Best Answers:
    18
    Trophy Points:
    375
    #2
    It's not bulletproof. ;)

    Mainly because you're relying on the MIME type rather than the extension. The MIME type (if taken from the $_FILES array) comes from the user, and can never ever be trusted. It's easy to fake it in order to bypass your security checks.

    Rely on the file extension instead, it's a much more secure approach.

    uniqid() alone is not a good way of generating random file names. No security concerns here, but I would implements a custom method.

    Well it's said that accessing static methods is a little slower than regular ones, but other than that it's up to the programmer. (Unit testing hasn't anything to do with this, though). Although I don't see how the static methods add any value here, but it's up to you really. Not sure why you're storing the "new file name" in a static variable even though you're not accessing it anywhere outside this one method.

    It's probably better to lose a few milliseconds if that means your script gains a bit in usability.

    That doesn't sound like a bad idea. Just don't forget to validate the file extensions, that's the most critical part.

    Anything is better than the "type" in the $_FILES array.

    Well you could have a set of default values in your class, and then merge the array passed to "set" with the defaults. This way can change single values without having to pass the entire array with all wanted options. Or, you could add a third (optional) argument to the method that allows people to change settings.
     
    Last edited: Nov 30, 2013
    nico_swd, Nov 30, 2013 IP
    eritrea1 and ryan_uk like this.
  3. nico_swd

    nico_swd Prominent Member

    Messages:
    4,153
    Likes Received:
    344
    Best Answers:
    18
    Trophy Points:
    375
    #3
    Actually the only things that prevents your script from being vulnerable is the fact that you're appending the MIME type back as extension. But it still looks a little sketchy. I would take the extension directly from the file name.

    On a further note, your script won't work on IE6 (not that anyone cares), because it doesn't use the standard MIME type for JPGs. It uses FindMimeFromData which returns image/pjpeg.
     
    nico_swd, Nov 30, 2013 IP
  4. eritrea1

    eritrea1 Active Member

    Messages:
    182
    Likes Received:
    9
    Best Answers:
    2
    Trophy Points:
    70
    #4
    Thanks for the quick feedback. Really appreciated.

    I thought 'MIMEtype' and 'extension' pointed to the same source -- the header of the image file. Anyway, I will dig more on that.

    As far as the IE Issue, I haven't heard about that before. There is a susbstr function on line 146
     $fileMimeType = substr($fileToUpload['type'], 6);
    PHP:
    that basically strips the `image/` part so user can pass the image type as is. But, will check this one too.

    uniqid() is just a default value, if the user does not assign a name for the image (which isn't supposed to happen) but, better upload the image with a unique name rather than halt the whole thing. Still does not make much sense, since the user it only uploads the file and does not return that unique id. Anyway, this is not important to focus, because user has to specify a name before uploading file.

    Last but not least, even though FILES['type'] may be unreliable, the file dimension check:
    if($height <= 1 || $width <= 1){
                return "This is invalid Image type";
            }
    PHP:
    Will not upload the file, if the width/height is less or equal to 1.

    But, I will see what I can do tomorrow
    thanks.
     
    eritrea1, Nov 30, 2013 IP
  5. ThePHPMaster

    ThePHPMaster Well-Known Member

    Messages:
    737
    Likes Received:
    52
    Best Answers:
    33
    Trophy Points:
    150
    #5
    It will suffice for general purposes, it all depends on what you need to accomplish and your requirements are (for me, most of the file uploads I worked with contained no file restrictions, so I end up renaming the file to a random string and store the original name for dispatch, the most I did was to scan the file for known viruses/trojans).

    Being new to PHP, I would say you are doing an awesome job at learning it. Keep it up.

    I love static variables, and I always try to stop myself from using them since most times I want to use them its not really necessary. Generally speaking, I use static properties only when I need to access those properties without creating an object of that class that it is in. If you are going to initialize the object, why use them?

    I believe that unit tests are much more important than the code itself. Whenever I avoid unit tests it is only because I have no time to do them. I won't go into details about unit testing, but from my personal experience it helps me catch those bugs that I wouldn't catch with manual testing. It is also great for someone who wants to make changes to your code, that those changes won't break existing functionality of your code.

    That is also a good thing. Throwing exceptions whenever there exists a possibility of errors is not only professional, but avoids notices/warnings shown to the user. For this case scenario, you probably can get away with it since anyone who will use this class would probably have enough experience to turn them off, but when working with general scripts, I try to adhere to that practice.

    I wouldn't also worry about making things faster with not using exceptions, it doesn't even take .05 seconds to throw 10,000 exceptions on a shared hosting -worst case scenario, if all your calls get thrown.

    These questions are typically answers after you answer the following question: What PHP versions do you want your object to support? finfo_* is a PECL extension prior to PHP 5.3. Splfileinfo only works with PHP >= 5.1.2. Personally, I try to have everything I create compatible with PHP version 4 and up to increase usage base, then again it is just a personal preference.

    Good thinking, but what if the user is uploading 10 images with same settings? If you want it to be easier to use, I would:

    1) Keep the set method with only 1 param (I think would make it easier, the same way you had the max-width/max-height).
    2) Enable settings in both set (global) and custom on upload.

    
    $settings = array (
    'allowed_ext' => array('png', 'jpeg', 'gif', 'jpg'),
    'max-width'=>150, 
    'max-height'=>150
    'max-file-size' => 30000,
    'upload-dir' => 'pictures/'
    )
    
    BulletProof::set($settings);
    
    if($_FILES){
          $upload =  BulletProof::upload($_FILES['profile_pic'], 'simon', $settings);
          // or $upload =  BulletProof::upload($_FILES['profile_pic'], 'simon');
          if($upload !== true){
            echo "ERROR: ".$upload;
          }
    
    PHP:

    That's an almost perfect check, but expect that it would fail seldom for valid images. As stated on the manual:

     
    ThePHPMaster, Nov 30, 2013 IP
    eritrea1 likes this.
  6. NetStar

    NetStar Notable Member

    Messages:
    2,471
    Likes Received:
    541
    Best Answers:
    21
    Trophy Points:
    245
    #6
    What makes it secure?
    What makes it fast?
    You named the class BulletProof?

    Just by browsing the class it looks like a very basic uploader. In fact, I'm not even sure if it was worth the efforts to create a class for it. If needed, there are basic uploader classes out there already.

    I don't see any code in there that suggests it's a secure script nor do I see a reason to name the class bullet proof. I also don't see anything in there that would make it fast.... seems like the client/server bandwidth really would determine the speed since this is just simple PHP execution.
     
    NetStar, Dec 1, 2013 IP
  7. nico_swd

    nico_swd Prominent Member

    Messages:
    4,153
    Likes Received:
    344
    Best Answers:
    18
    Trophy Points:
    375
    #7
    Geez, calm down, the guy is still learning, and he's making quite a good progress. There's nothing wrong with working on small projects and asking for suggestions.
     
    nico_swd, Dec 2, 2013 IP
    ryan_uk and eritrea1 like this.
  8. eritrea1

    eritrea1 Active Member

    Messages:
    182
    Likes Received:
    9
    Best Answers:
    2
    Trophy Points:
    70
    #8
    I was going to reply back to him, but opted to change the class instead. So, I objectified it :D and added the splFileInfo::getExtension and some other ways to validate the image. Here https://github.com/simon-eQ/BulletProof the README should be a through guide, please feel free to highlight any, even the slightest thing you don't agree about.


    On second thoughts, I need you to see the class, in a way what you would change about it, if you wanted to use it. I won't settle for anything less :)
     
    eritrea1, Dec 2, 2013 IP
  9. MakZF

    MakZF Well-Known Member

    Messages:
    390
    Likes Received:
    9
    Best Answers:
    1
    Trophy Points:
    140
    #9
    Seems straightforward enough. As others have said, you should check the file extension. The naming is somewhat unconventional as well. Perhaps your class name should hint at what it actually does.

    Anyway to comment on your questions:
    1. There is no reason at all for the class to be static, in fact your class is very inflexible. Why force users to use the global scope to access your class as well as deny them the ability to create multiple instances of it ? Your code could be like this:
    class FileUploader {
        // Some default values
        private $maxSize = 10000;
        private $allowedExtensions = array('jpg', 'png', 'gif');
        private $uploadFolder = '/pictures';
      
        public function setMaxFileSize($fileSize) {
            $this->maxSize = (int) $fileSize;
        }
      
        public function setExtensions(array $extensions) {
            $this->extensions = $extensions;
        }
      
        public function setFolder($folder) {
            $this->uploadFolder = $folder;
        }
      
        /* Methods relating to file upload go here */
    }
    PHP:
    2. Throwing normal PHP exceptions should work just fine.
    3. Probably not. Why would you want to extend the SplFileInfo class?
    4. Use finfo_open or SplFileInfo. They are built into PHP, so why not use them instead of recreating them?
     
    MakZF, Dec 2, 2013 IP
    eritrea1 likes this.
  10. NetStar

    NetStar Notable Member

    Messages:
    2,471
    Likes Received:
    541
    Best Answers:
    21
    Trophy Points:
    245
    #10
    There is something terribly wrong with creating a programming class named "Bullet Proof" for others to use and claiming it's "fast and secure" when it's not.. He even has it on github for version control...
     
    NetStar, Dec 2, 2013 IP
  11. eritrea1

    eritrea1 Active Member

    Messages:
    182
    Likes Received:
    9
    Best Answers:
    2
    Trophy Points:
    70
    #11
    Hey, thanks. I actually had updated the code in github https://github.com/simon-eQ/BulletProof 5 or more hours before your wrote your answer.
    Meaning, I had changed the class from static to dynamic, I didn't extend the SplFileInfo class, I just instantiated in side the class to get Extension.
    And my next plan was to throw simple exceptions from the class as you suggested. But, I realized one thing in your answer. That even with the new class I have created, I need to create private properties with default values, to be overridden later by the user. I think that is a very good idea. It is always better to have a default value, but I was letting the user make a default value and then override them
    later. It's a good point. But, you should take a look at the class as it is different from the one you read.


    I am really bad with names, I had name my PdoWrapper NoodlePiece before :(
    I will change that now too.
     
    eritrea1, Dec 2, 2013 IP
  12. eritrea1

    eritrea1 Active Member

    Messages:
    182
    Likes Received:
    9
    Best Answers:
    2
    Trophy Points:
    70
    #12
    Why don't you then help me then, huh?, I didn't come here to sell the script, I came for advise. So tell me how to improve it instead of saying the same thing over and over again. To learn and improve the code. What you are saying doesn't "help" me
     
    eritrea1, Dec 2, 2013 IP
    ryan_uk likes this.
  13. eritrea1

    eritrea1 Active Member

    Messages:
    182
    Likes Received:
    9
    Best Answers:
    2
    Trophy Points:
    70
    #13
    I renamed to "Suploader" I am really bad with naming things. That is all I could come up with :)
     
    eritrea1, Dec 2, 2013 IP
  14. NetStar

    NetStar Notable Member

    Messages:
    2,471
    Likes Received:
    541
    Best Answers:
    21
    Trophy Points:
    245
    #14
    I advised you not to name the class "Bullet Proof" because it has nothing to do with the class and it's misleading. I also stated that your code isn't Fast & Secure as you claim. In fact, there's nothing in there that would accelerate an upload (speed is determined by the servers execution time and the client/server bandwidth). There's also no code enforcing security for you to even suggest it's secure.

    That's my statement and my advice. As for telling you how to improve your code, I don't have any suggestions because I don't feel the class is needed for it's basic functionality. There are classes out there that do a lot more. I'm not so sure why you chose to create a class for public use that obviously you aren't going to maintain.
     
    NetStar, Dec 2, 2013 IP
  15. eritrea1

    eritrea1 Active Member

    Messages:
    182
    Likes Received:
    9
    Best Answers:
    2
    Trophy Points:
    70
    #15

    Hey, I applied your suggestion. There is much more clarity in the code now, It looks neat. Thanks. I will implement the exception tomorrow and maybe improve it a bit if I can. Then, I will start using this class in my projects.
    thanks for the feedbacks @ll.
     
    eritrea1, Dec 2, 2013 IP