2

I know that you can never be sure that you have done enough to be secure, and I also know that file uploading is hard to make correct. Before asking this question I read some of the related posts here like risk of php image upload, what steps should be taken to validate... and security risks of uploading.

So I think that I have done everything that is needed, but I would be very grateful if someone can take a look at what I have and tell if I have done enough. So I am running the latest PHP 5.5.9-1+sury.org~saucy+1 on Apache/2.4.7. And my uploading method looks as follows:

public static function uploadTemp($number, $file){
    //check if the filename exist and upload was without an error
    if (!$file['name'] || $file['error']){
        return false;
    }

    // check if extension is valid
    if (!Helper::validExtension($file['name'])){
        return false;
    }

    // check is the size of file is valid
    if ($file['size'] > (1024 * 1024 * 6) || $file['size'] < 1024 * 10){
        return false;
    }

    // no need to upload images less than 50x50. Also $file['size'] can be spoofed
    $imageSize = getimagesize($file['tmp_name']);
    if ($imageSize === false || $imageSize[0] < 50 || $imageSize[1] < 50){
        return false;
    }

    require_once('SimpleImage.php');
    $image = new SimpleImage();
    $image->load($file['tmp_name']);

    // saving a file to a temporary directory and renaming it.
    $image->save(Image::$tempDir.$number.'.jpg');
    return true;
}

SimpleImage is an open source tool for manipulating an image, inside of a tool I changed only one thing (function save to save every file with 644 permissions). My $number is a string which is a concatenation of random number and a current timestamp, $file = $_FILES['fileToUpload'] and validExtension looks the following way:

public static function validExtension($filename){
    $extensions = array('jpg', 'jpeg', 'png');

    $arr = explode('.', $filename);
    if ( in_array(strtolower(end($arr)), $extensions) ){
        return true;
    } else return false;
}

My temporary folder has 755 permissions.

So my question is: am I missing something here or is there I way I can improve things:

  • may be restrict further permissions (I do not need to do anything with images except to view them by the client. The folder is used only for image uploading)
  • may be changing some parameters in php.ini or apache
Community
  • 1
Salvador Dali
  • 1,745
  • 1
  • 19
  • 32

1 Answers1

7
// check if extension is valid

is largely pointless. You (wisely) create the filesystem name without any input from the original filename, so the extension is discarded anyway.

Filename extensions are not a globally-observed convention. Even on Windows not everyone uses the same file extension mappings. So it's not usually a good idea to rely on particular extensions.

// no need to upload images less than 50x50

You will want to check that the image dimensions are not too big. Uploading an enormous image (especially a super-compressed one) is a good way to DoS a server.

My temporary folder has 755 permissions.

Ideally you would want to have the web server user and the file owner user in a group, so you could give it 750 permissions, allowing the web user to write files without giving the same privileges to any other users on the server.

I changed only one thing (function save to save every file with 644 permissions)

And similarly 640 here, or more likely 660 if you want your owner user to be able to write to the web server user's files.

The folder is used only for image uploading

Make sure you enforce that on the server level, so that if someone does manage to upload a .php or .htaccess file into that directory, it doesn't do anything. It doesn't look like this code would allow that, but there might be some other flaw somewhere that does.

In general every file/directory that is writable to the web server user needs to be made completely non-executable, either by locking down the web server to do only static file serving from there, or not serving anything from there at all.

If you are having the web server serve the picture out of the upload directory you have a range of potential cross-site-scripting problems:

  • IE sniffing the contents of the file to find HTML embedded in it and treating it as a web page (with scripting) - can be prevented in more up to date versions using X-Content-Type-Options header;
  • Java treating the file as an applet, ignoring the real type; Flash treating the file as a SWF, ignoring the real type; the Same Origin Policies of these plugins not matching JavaScript's;
  • Flash loadPolicyFile treating the file as a crossdomain policy file, ignoring the real type and, in older versions of Flash, malformed content.

You've greatly mitigated these XSS-content attacks by loading and re-saving the image as JPEG, which would disturb the contents of the file, so an attacker can't just include the active content as-is inside the file.

It's not totally impossible for an attacker to submit an image that they know, when PHP re-saves it, will result in chosen malicious content appearing somewhere in file. This would be pretty difficult, especially for a lossy encoder like JPEG, and I've not heard of anyone actually using that in a real attack, but it's conceivable. The JPEG format is not specifically designed to provide hard guarantees about the difficulty of crafting input to generate a chosen output (as a cryptographic hash would).

If you want to be sure there are no possible XSS upload attacks the usual solution is to serve your untrusted data files from a separate domain, so that cross-site-scripting into it doesn't get the attacker into your main site.

bobince
  • 12,494
  • 1
  • 26
  • 42