69

I am programming a script to upload images to my application. Are the following security steps enough to make the application safe from the script side?

  • Disable PHP from running inside the upload folder using .httaccess.
  • Do not allow upload if the file name contains string "php".
  • Allow only extensions: jpg,jpeg,gif and png.
  • Allow only image file type.
  • Disallow image with two file type.
  • Change the image name.
  • Upload to a sub-directory not root directory.

This is my script:

 $filename=$_FILES['my_files']['name'];
 $filetype=$_FILES['my_files']['type'];
 $filename = strtolower($filename);
 $filetype = strtolower($filetype);

 //check if contain php and kill it 
 $pos = strpos($filename,'php');
 if(!($pos === false)) {
  die('error');
 }




 //get the file ext

 $file_ext = strrchr($filename, '.');


 //check if its allowed or not
 $whitelist = array(".jpg",".jpeg",".gif",".png"); 
 if (!(in_array($file_ext, $whitelist))) {
    die('not allowed extension,please upload images only');
 }


 //check upload type
 $pos = strpos($filetype,'image');
 if($pos === false) {
  die('error 1');
 }
 $imageinfo = getimagesize($_FILES['my_files']['tmp_name']);
 if($imageinfo['mime'] != 'image/gif' && $imageinfo['mime'] != 'image/jpeg'&& $imageinfo['mime']      != 'image/jpg'&& $imageinfo['mime'] != 'image/png') {
   die('error 2');
 }
//check double file type (image with comment)
if(substr_count($filetype, '/')>1){
die('error 3')
}

 // upload to upload direcory 
 $uploaddir = 'upload/'.date("Y-m-d").'/' ;

if (file_exists($uploaddir)) {  
} else {  
    mkdir( $uploaddir, 0777);  
}  
  //change the image name
 $uploadfile = $uploaddir . md5(basename($_FILES['my_files']['name'])).$file_ext;



  if (move_uploaded_file($_FILES['my_files']['tmp_name'], $uploadfile)) {
 echo "<img id=\"upload_id\" src=\"".$uploadfile."\"><br />";
  } else {
   echo "error";
  }

Any new tips are welcome :)

usef_ksa
  • 1,669
  • 3
  • 23
  • 40
  • 13
    I would remove the following rule: Do not allow upload if the file name contains string "php". It is not needed because you are renaming the file. – troynt Nov 12 '10 at 18:26
  • 1
    You can download [**Secure Image upload**](https://github.com/simon-eQ/ImageUploader) from github. It is the most secure PHP script alive. It supports image re-sizing/croping too. – samayo Dec 05 '13 at 06:37
  • 1
    @Alez From a quick glance at that class the only security I can see is an extension check. Please, PLEASE say it ain't so! – sousdev Jan 20 '14 at 17:39
  • @Fricker It ain't so. In what sense? the `pathinfo(, PATHINFO_EXTENSION)` is a very reliable way to get the most accurate file extension, actually there is nothing more reliable than that. read where it says [**"note"**](http://www.php.net/manual/en/splfileinfo.getextension.php) – samayo Jan 20 '14 at 17:54
  • @Alez If I left the security of my server down to an extension check i wouldn't be able to sleep at night is what I'm saying – sousdev Jan 21 '14 at 10:44
  • @Fricker Well, if you are not smart enough not to give your folders permission to execute files, the I agree. You should not sleep :) – samayo Jan 21 '14 at 13:33
  • @Alez I am smart enough however I'm not conformable leaving such a massive risk down to such a simple check that can let executable code into your server which could be a very important link in a chain that enables leakage of things such as account details, most real-world hacks do not come from one vulnerability but a few used in conjunction. EDIT: I see your Swiss, good :) – sousdev Jan 21 '14 at 14:12
  • @Fricker I am not Swiss :). Why don't you download the classes and try your best to beat it? I am really interested to know, how it can be exploited. I'm not 100% sure, as I am PHP newbie. But, I want to use the class in future projects so, I would like to find out – samayo Jan 21 '14 at 14:35
  • @Alez Fair enough :) I'm actually currently developing a secure image upload, storage, and serving class in PHP so I'm very paranoid while reading how all these checks can be circumvented. If you do use the script please make sure you store the images out side of the document root and display them using PHP, just encase something gets by – sousdev Jan 21 '14 at 15:23
  • Sure, let me check out your code when you are done. I need to learn about this as much as possible. I think my script is secure, but it can be very improved. So, I need to compare it with others – samayo Jan 21 '14 at 16:03
  • 3
    @Alez ofc & btw I like the Luke 3:11 license :) – sousdev Jan 22 '14 at 00:19

12 Answers12

36

Re-process the image using GD (or Imagick) and save the processed image. All others are just fun boring for hackers.

Edit: And as rr pointed out, use move_uploaded_file() for any upload.

Late Edit: By the way, you'd want to be very restrictive about your upload folder. Those places are one of the dark corners where many exploits happen. This is valid for any type of upload and any programming language/server. Check https://www.owasp.org/index.php/Unrestricted_File_Upload

Halil Özgür
  • 15,731
  • 6
  • 49
  • 56
  • 2
    +1. I don't see how any of the checks from the list (in the question) can be useful, if *it is an image* and contains a valid extension. – Arseni Mourzenko Nov 12 '10 at 18:28
  • +1 I think this is the most secure solution, but would image quality or size suffer? Is there a secure way GD/Imagick can validate it is an image without altering it? – troynt Nov 12 '10 at 18:39
  • White-listing is nearly always better than black-listing. So is processing rather than checking. You will be altering it but you can have the resulting image with the exact same properties and virtually no quality loss. Of course, if you want to store the original image as exactly as possible (like flickr) you may want to dig deeper in this area. – Halil Özgür Nov 12 '10 at 18:46
  • 1
    I do not add this because I think it need a lot of CPU processing. – usef_ksa Nov 12 '10 at 21:39
  • 7
    Mark my words: most of the time, CPU is the most redundant resource. Just watch the graph of any given machine/server (except Google, Facebook, etc servers maybe). You will see that it is dusting there most of the time. Projects like SETI@Home tries to use that free time. At any rate, it would be more than worth the effort for the added security. – Halil Özgür Nov 12 '10 at 22:41
  • There's some interesting feedback on this question (http://stackoverflow.com/questions/7349473/php-file-upload-mime-or-extention-based-varification) about the importances of re-processing images. – Lachlan McDonald Jan 13 '12 at 01:53
  • 2
    Would you care to give example of 're-process'? Lets say with imagick (or GD at worse) ? – Andrew Oct 02 '13 at 11:54
  • @Andrew A simple "imagick php example" Google search returns many results, but at a bare minimum it would be something like: `try { $image = new Imagick($_FILES['my_files']['tmp_name']); $image->writeImage($outFile); } catch (ImagickException) { //invalid image or something }` – Halil Özgür Oct 02 '13 at 13:07
  • 1
    Would merely checking the result of getimagesize() be efficient for "processing the image"? getimagesize() would return false if its not a valid image correct? Just for clarity for everyone else, its pretty simple to spoof mime types and extensions by just posting a false mime type and/or extension to bypass that security check. – thorne51 Nov 22 '13 at 12:09
  • 2
    @thorne51, I believe no function is enough for checking. So simply don't check, just create a new image. Example: PHP code can be hidden in EXIF, XMP etc parts of the image, and functions/programs may fail (inadvertently or through the use of an exploit code) to report that code. – Halil Özgür Nov 22 '13 at 12:55
  • A note from the future: So how real is the threat? I've found a couple of articles on the issue [here](http://ha.ckers.org/blog/20070604/passing-malicious-php-through-getimagesize/) and [here](http://blog.insicdesigns.com/2009/01/secure-file-upload-in-php-web-applications/). Especially the second one goes through securing image uploads in depth. They are quite old, but IMO still relevant today. – Halil Özgür Apr 06 '14 at 15:46
  • Re-process the image using GD sounds lovely. Why make me Google it though? Give an example/link so I can go do what you prescribe. Here: https://stackoverflow.com/questions/9754047/image-upload-security-reprocess-with-gd – blogo Sep 25 '19 at 15:43
  • @blogo good point, and thanks for the link! Unfortunately, I haven't been using PHP for quite a while, so I hope that scanning through relevant answers is better than whatever I'd try to come up with :) – Halil Özgür Sep 26 '19 at 16:19
13

For security test of the image files, I can think of 4 level of securities. They would be:

  • Level 1: Check the extension (extension file ends with)
  • Level 2: Check the MIME type ($file_info = getimagesize($_FILES['image_file']; $file_mime = $file_info['mime'];)
  • Level 3: Read first 100 bytes and check if they have any bytes in the following range: ASCII 0-8, 12-31 (decimal).
  • Level 4: Check for magic numbers in the header (first 10-20 bytes of the file). You can find some of the files header bytes from here: http://en.wikipedia.org/wiki/Magic_number_%28programming%29#Examples

Note: Loading entire image would be slow.

tanjir
  • 1,294
  • 13
  • 22
9

XSS Warning

One more very important remark. Do not serve/upload anything that could be interpreted as HTML in the browser.

Since the files are on your domain, javascript contained in that HTML document will have access to all your cookies, enabling some sort of XSS attack.

Attack scenario:

  1. The attacker uploads HTML file with JS code that sends all the cookies to his server.

  2. The attacker sends the link to your users via mail, PM, or simply via iframe on his or any other site.

Most secure solution:

Make uploaded content available only on the subdomain or on the another domain. This way cookies are not going to be accessible. This is also one of the google's performance tips:

https://developers.google.com/speed/docs/best-practices/request#ServeFromCookielessDomain

Rok Kralj
  • 46,826
  • 10
  • 71
  • 80
7

Create a new .htaccess file in the uploads dir and paste this code:

php_flag engine 0
RemoveHandler .phtml .php .php3 .php4 .php5 .php6 .phps .cgi .exe .pl .asp .aspx .shtml .shtm .fcgi .fpl .jsp .htm .html .wml
AddType application/x-httpd-php-source .phtml .php .php3 .php4 .php5 .php6 .phps .cgi .exe .pl .asp .aspx .shtml .shtm .fcgi .fpl .jsp .htm .html .wml

Just be sure to rename the files u upload + forget about checking types, contents etc

jloa
  • 71
  • 1
  • 1
6

You might want to run "is_uploaded_file" on the $_FILES['my_files']['tmp_name'] as well. See http://php.net/manual/en/function.is-uploaded-file.php

rr.
  • 6,484
  • 9
  • 40
  • 48
5

I will repeat something I posted in related question.

You may detect content type using Fileinfo functions (mime_content_type() in previous versions of PHP).

An excerpt form PHP manual on older Mimetype extension, which is now replaced by Fileinfo:

The functions in this module try to guess the content type and encoding of a file by looking for certain magic byte sequences at specific positions within the file. While this is not a bullet proof approach the heuristics used do a very good job.

getimagesize() may also do a good job, but most of the other checks you are performing are nonsense. For example why string php is not allowed in filename. You are not going to include image file within PHP script, just because its name contains php string, are you?


When it comes to re-creating images, in most cases it will improve security... until library you use is not vulnerable.

So which PHP extension suits best for secure image re-creation? I've checked CVE details website. I think the applicable trio are those extensions:

  1. GD (6 vulnerabilities)
  2. ImageMagick (44 vulnerabilities)
  3. Gmagick (12 vulnerabilities)

From the comparison I think GD suits best, because it has smallest number of security issues and they are quite old. Three of them are critical, but ImagMagick and Gmagick do not perform any better... ImageMagick seems to be very buggy (at least when it comes to security), so I choose Gmagick as the second option.

mip
  • 8,355
  • 6
  • 53
  • 72
2

if security is very important use database for save file name and renamed file name and here you can change extension of file to somthing like .myfile and make a php file for send image with headers . php can be more secure and you can use it in the img tag like blow :

<img src="send_img.php?id=555" alt="">

also check file extension with EXIF before upload.

M Rostami
  • 4,035
  • 1
  • 35
  • 39
2

The simplest answer to allow users to securely upload files in PHP is: Always save files outside of your document root.

For example: If your document root is /home/example/public_html, save files to /home/example/uploaded.

With your files safely out of the confines of being directly executed by your web server, there are a couple of ways you can still make them accessible to your visitors:

  1. Set up a separate virtual host for serving static content which never executes PHP, Perl, etc. scripts.
  2. Upload the files to another server (e.g. a cheap VPS, Amazon S3, etc).
  3. Keep them on the same server, and use a PHP script to proxy requests to ensure the file is only ever readable, not executable.

However, if you go with options 1 or 3 on this list and you have a local file inclusion vulnerability in your application, your file upload form can still be an attack vector.

Scott Arciszewski
  • 33,610
  • 16
  • 89
  • 206
2

The best way to keep your website secure when user upload image is to do this steps :

  • check the image extension
  • check the image size with this function "getimagesize()"
  • after this you can use the function "file_get_contents()"
  • In the end you should insert file_Content to your database i think that's the best way ! nd what's your opinion ?
HamzaO
  • 37
  • 5
1

I looked yesterday for best secure solution and it seems using extra app as GD is the one of them - but if somebody has lack of money for good server it may cause huge delay in server response. However i come up with some trick but possible only with one scenario if you do not have gallery or some image preview and you only giving an option to download uploaded file. So the thing to : 1st. change name on the fly and after it zip the file and then delete sorurce file - so there would bo no chance anybody can cause execution of the hidden code - offcours this solution is secure for server only - cause user which download the file my open some infected one etc -but in my case its not the problem.

br

drcwycior
  • 59
  • 1
  • 4
0

For an image file, you could also change file permission after renaming to be sure it never executes (rw-r--r--)

Jesus
  • 25
  • 1
  • 5
    -1. First: The X (execute) flag is not controlled by the client, it is not set anyways. Second: The PHP engine doesn't require files to have X flag for them to get executed. – Rok Kralj Feb 07 '13 at 13:13
0

I'm using a php-upload-script that creates a new random 4-byte-number for each uploaded file, then XORs the content of the file with these 4 bytes (repeating them as often as necessary), and finally attaches the 4 bytes to the file before saving it.

For downloading, the 4 bytes have to be cut off of the file again, the contents will be XORed with them again and the result is sent to the client.

This way, I can be sure that the files I save on the server will not be executable or have any potential meaning whatsoever for any application. Plus I don't need any extra database to store filenames in.

Here is the code I'm using for this:

Upload:

        <?php
            $outputfilename = $_POST['filename'];
            $inputfile = $_FILES["myblob"]["tmp_name"];
            $tempfilename="temp.tmp";

            if( move_uploaded_file($inputfile, $tempfilename) ) {
                $XORstring = random_bytes(4);

                $tempfile=fopen($tempfilename, "r");
                $outputfile=fopen($outputfilename, "w+");
                flock($outputfilename, LOCK_EX);

                fwrite($outputfilename, $XORbytes1);

                while ( $buffer = fread($tempfile, 4) ) {
                    $buffer = $buffer ^ $XORstring;
                    fwrite($outputfilename, $buffer);
                }

                flock($outputfilename, LOCK_UN);

                fclose($tempfile);
                fclose($outputfile);

                unlink($tempfilename);
            }

            exit(0);
        ?>

Download:

        <?php
            $inputfilename = $_POST['filename'];
            $tempfilename = "temp.tmp";

            $inputfile=fopen($inputfilename, "r");
            $tempfile=fopen($tempfilename, "w+");
            flock($tempfile, LOCK_EX);

            $XORstring = fread($inputfile, 4);

            while ( $buffer = fread($inputfile, 4) ) {
                $buffer = $buffer ^ $XORstring;
                fwrite($tempfile, $buffer);
            }

            flock($tempfile, LOCK_UN);

            fclose($inputfile);
            fclose($tempfile);

            readfile($tempfile);
            unlink($tempfile);

            exit(0);
        ?>
Max M.
  • 51
  • 5