120

I'm working on a website — right now it's in early stages of testing, not yet launched and just has test data - thank goodness.

First of all, a hacker figured out the password to log onto the websites 'administration' pages*. I think they used a key logger on a friend's computer who logged into the site to give me feedback.

Secondly, they used a picture upload box to upload a PHP file. I have put in strict checking so that only .jpg and .png files are accepted — everything else should have been rejected. Surely there is no way to upload a .jpg file and then change the extension once the file is stored?

Thankfully I also generate new file names when a file is sent to me, so I don't think they were able to locate the file to execute the code.

I just can't seem to figure out how the website let a PHP file through. What's wrong with my security? The validation function code is below:

function ValidateChange_Logo(theForm)
{
   var regexp;
   if (theForm.FileUpload1.value == "")
   {
      alert("You have not chosen a new logo file, or your file is not supported.");
      theForm.FileUpload1.focus();
      return false;
   }
   var extension = theForm.FileUpload1.value.substr(theForm.FileUpload1.value.lastIndexOf('.'));
   if ((extension.toLowerCase() != ".jpg") &&
       (extension.toLowerCase() != ".png"))
   {
      alert("You have not chosen a new logo file, or your file is not supported.");
      theForm.FileUpload1.focus();
      return false;
   }
   return true;
}

Once the file gets to the server, I use the following code to retain the extension, and generate a new random name. It is a bit messy, but it works well.

// Process and Retain File Extension
$fileExt = $_FILES[logo][name];
$reversed = strrev($fileExt);
$extension0 = substr($reversed, 0, 1);
$extension1 = substr($reversed, 1, 1);
$extension2 = substr($reversed, 2, 1);
$fileExtension = ".".$extension2.$extension1.$extension0;
$newName = rand(1000000, 9999999) . $fileExtension;

I've just tested with a name such as logo.php;.jpg and although the picture cannot be opened by the website, it correctly changed the name to 123456.jpg. As for logo.php/.jpg, Windows doesn't allow such a file name.


* Protected pages on the website that allow simple functions: like uploading a picture that then becomes a new logo for the website. FTP details are completely different to the password used to log onto the protected pages on the website. As are database and cPanel credentials. I've ensured that people can't even view the folder and file structure of the site. There is literally no way I can think of to rename a .jpg, or .png extension to .php on this site if you don't have FTP details.

2
  • Comments are not for extended discussion; this conversation has been moved to chat.
    – Rory Alsop
    Jan 10, 2017 at 18:16
  • Short answer: client side validation can easily be bypassed by setting a proxy. They can just submit a.jpg first and pass all the checkings. The request will then intercepted at the proxy and changed to a.php. You need to do validation on server side.
    – user50312
    Dec 16, 2018 at 3:43

11 Answers 11

316

Client side validation

The validation code you have provided is in JavaScript. That suggests it is code that you use to do the validation on the client.

Rule number one of securing webapps is to never trust the client. The client is under the full control of the user - or in this case, the attacker. You can not be sure that any code you send to the client is used for anything, and no blocks you put in place on the client has any security value what so ever. Validation on the client is just for providing a smooth user experience, not to actually enforce any security relevant constraints.

An attacker could just change that piece of JavaScript in their browser, or turn scripts off completely, or just not send the file from a browser but instead craft their own POST request with a tool like curl.

You need to revalidate everything on the server. That means that your PHP must check that the files are of the right type, something your current code doesn't.

How to do that is a broad issue that I think is outside the scope of your question, but this question are good places to start reading. You might want to take a look at your .htaccess files as well.

Getting the extension

Not a security issue maybe, but this is a better way to do it:

$ext = pathinfo($filename, PATHINFO_EXTENSION);

Magic PHP functions

When I store data from edit boxes, I use all three of PHP's functions to clean it:

$cleanedName = strip_tags($_POST[name]); // Remove HTML tags
$cleanedName = htmlspecialchars($cleanedName); // Allow special chars, but store them safely. 
$cleanedName = mysqli_real_escape_string($connectionName, $cleanedName);

This is not a good strategy. The fact that you mention this in the context of validating file extensions makes me worried that you are just throwing a couple of security related functions at your input hoping it will solve the problem.

For instance, you should be using prepared statements and not escaping to protect against SQLi. Escaping of HTML tags and special characters to prevent XSS needs to be done after the data is retrieved from the database, and how to do it depends on where you insert that data. And so on, and so on.

Conclusion

I am not saying this to be mean, but you seem to be doing a lot of mistakes here. I would not be surprised if there are other vulnerabilities. If your app handles any sensitive information I would highly recommend that you let someone with security experience have a look at it before you take it into production.

13
  • 122
    To take a simple example: Open your website in Chrome. Press f12 to open the developer tools. Select the sources tab. You'll see all your JavaScripts, lined up and nicely editable. Find your validation function. Delete it, and replace with return true; Jan 4, 2017 at 11:12
  • 106
    /\ Not only in Chrome
    – Marc.2377
    Jan 4, 2017 at 11:36
  • 42
    One thing that important to emphasize is "revalidate everything" -- just because you are validating on the server doesn't mean you shouldn't in the browser. Just don't use it for security. This way your users can know things like "oh I put an A in my phone number, whoops" before sending the form across. Jan 5, 2017 at 15:57
  • 8
    And to make things even more interesting, there's always polyglots... blog.portswigger.net/2016/12/…
    – lorenzog
    Jan 6, 2017 at 11:16
  • 4
    @Cryptopat something something Node.js
    – noisypixy
    Jan 8, 2017 at 16:28
36

First off, if someone exploited a file upload function, ensure that you're verifying the file on both the client and server. Bypassing client-side JavaScript protection is trivial.

Second, ensure that you go through and implement these ideas from OWASP.

That should cover most of your problems. Also ensure that you don't have any other unpatched flaws on your server (for example, outdated plugins, exploitable servers, etc.)

4
  • 4
    Can you elaborate on the "OWASP" ideas, sending people away from here is not necessarily a good thing. Even bringing some of the content or ideas here would greatly improve this answer.
    – Möoz
    Jan 5, 2017 at 21:32
  • 1
    I agree with @Mooz, OWASP is also a wiki, i.e. many of those ideas can be edited later and a few of them may help the OP in this particular situation. If you focus on adding the ideas/implementations that may help the OP, I may upvote... Currently your answer is just a copy of previous answers/comments suggestions (even the simple suggestion to disable PHP on the upload directory is more useful than a link to a long list of ideas in another website).
    – CPHPython
    Jan 9, 2017 at 14:12
  • @CPHPython - My bad. I'll fix it. Thanks for the heads up.
    – thel3l
    Jan 10, 2017 at 15:27
  • validating the input at the client-side is not necessary, it's just if you want to give users the convenience of early error messages, but from a security-point-of-view, client-sided validation is worthless. Apr 22, 2019 at 15:51
26

It is super-important to note here that PHP was designed to be embedded in other content. Specifically, it was designed to be embedded within HTML pages, complementing a largely static page with minor bits of dynamically updated data.

And, quite simply, it doesn't care what it's embedded in. The PHP interpreter will scan through any arbitrary file type, and execute any PHP content that's properly marked with script tags.

The historic problem that this has caused is that, yes, image uploads can contain PHP. And if the web server is willing to filter those files through the PHP interpreter, then that code will execute. See the Wordpress "Tim Thumb" plugin fiasco.

The proper solution is to make sure that your web server never, ever treats untrusted content as something it should run through PHP. Attempting to filter the input is one way to go about it, but as you've found, limited and error prone.

Much better to verify that both file location and file extension are used to define what PHP will process, and to segregate uploads and other untrusted content into locations and extensions that won't be processed. (How you do so varies from server to server, and suffers from the "making things work usually means loosening security in ways you're not aware" problem.)

4
  • Curiously enough, this nature has been used (not saying it's a good or a bad thing) by the timthumb PHP image resizer library. Essentially, the string <?php die('Execution denied!'); // is prepended to any image that touches the library, effectively stealing the show from another would-be embedded script. - github.com/penibelst/timthumb/blob/master/timthumb.php Jan 6, 2017 at 4:54
  • 1
    @LukeA.Leber I would assume that's the coded fix for the older Tim Thumb problem which was that scripts embedded in images pwned Wordpress servers left and right... I'm not sure whether to consider that an elegant solution or a bletcherous hack :)
    – gowenfawr
    Jan 6, 2017 at 5:04
  • 4
    I won't judge the author, but when the Joomla 3.4.8 zero-day came out I got a MASSIVE scare when my in-house heuristics scanner was turning up an opening php tag in over 10,000 images across 3 servers. One of those moments that you can feel the blood drain from your face. Jan 6, 2017 at 5:10
  • The fact that you can upload scripts to be executed by the interpreter is not related to PHP allowing itself to be embedded, though. Just as with PHP, you could upload Python or Ruby scripts. The issue, however, is that PHP and Apache (by default) expose the server’s directory structure publicly and let you access any file you want on the server via HTTP.
    – caw
    Sep 27, 2018 at 7:13
23

You can disable PHP in your upload directory by .htaccess so your server won't execute any PHP code in the directory and in its subdirectories.

Create a .htaccess file inside your upload directory with this rule:

php_flag engine off

Please note that the .htaccess rule will work only if your PHP server is running in mod_php mode.

Edit: Of course, i forgot to mention that .htaccess works only in apache.

3
  • 17
    Note that .htaccess only applies to apache.
    – com2ghz
    Jan 6, 2017 at 13:33
  • This is unhelpful if somebody can overwrite the .htaccess file.
    – wizzwizz4
    Dec 31, 2020 at 16:32
  • 1
    @wizzwizz4 sudo chmod 444 .htaccess
    – svin83
    Sep 6, 2022 at 15:29
11

You are only checking the extension, this doesn't necessarily correlate to what the actual file type is. Server side you could use something like exif_imagetype to verify the file is an image. exif image type usage: http://php.net/manual/en/function.exif-imagetype.php

4
  • 9
    This might be a good idea, but remember that (a) it might be possible to create a file that is both valid as image and PHP, and (b) a PHP file with a .png extension doesn't do much harm on your server, since it will not be executed.
    – Anders
    Jan 4, 2017 at 14:41
  • 10
    @Anders but it is still malicious code, and all it takes for it to break things is someone/something telling it to run foo.png as a php file. While it's unlikely, I'd rather stay away from keeping pocket bombs in my pocket, even if I'm the only one with a detonator.
    – Delioth
    Jan 4, 2017 at 16:00
  • 3
    @Anders Unless your server is configured to interpret files based on their content and not the extension, of course. Security is hard...
    – Luaan
    Jan 4, 2017 at 16:01
  • @Delioth Agreed, I was a bit to categorical. I should have said is that it is not as dangerous, not that it is without risk.
    – Anders
    Jan 4, 2017 at 16:22
6

I totally agree with the accepted answer, but I would suggest to do a little bit more than just playing around with the filename. You should re-compress the original/uploaded file with PHP using GD or Imagick and use the new image. This way, you destroy any injected code (to be honest, 90% of the time, there are ways to make the code survive the compression, but it's a lot of work).

Also, you could use .htaccess files to prevent the upload directory running PHP code (I don't know about IIS and there is probably an equivalent .webconfig).

<FilesMatch \.php$>
    SetHandler application/x-httpd-php
</FilesMatch>

This way, the uploaded file will be executed only if the "final extension" is PHP, so image.php.jpg will not be executed.

Some resources:

2
  • "This way, the uploaded file will be executed only if the "final extension" is PHP, so image.php.jpg will not be executed." — presumably for the type of site the OP is describing though, you'd just disable PHP wholesale in the uploads directory & not worry about what file extension it has. If for whatever reason someone managed to upload a file ending in .php, you still wouldn't want it to execute Jan 10, 2017 at 19:09
  • 1
    @anotherdave true! The main part is to recompress the image... the second is just an extra which can be configured according to one's need
    – akman
    Jan 10, 2017 at 19:14
6

One possible part of a solution here is to keep the image, but also to make a resized copy with PHP such that you can ensure that you are getting an image, and reduce the size of the original image. This has several positive effects:

  • Never lets the original image be used for display so its more secure
  • Saves space on your server (which may or may not be an issue)
  • Better load time for site visitors as they would not be waiting for some 10 MB image to load. Instead they have a 300 KB image at a reasonable size loading

The main point is that you can't trust user data, ever. Validate everything, file uploads, inputs, cookies to ensure that the data can't cause problems.

3

I allow image uploads but assume every one is loaded with trojan code. I temporarily place the uploaded image above web root upon upload, then use ImageMagick to convert it to BMP, then over to JPG, then move it where the web app can use it. This effectively removes any embedded PHP code.

As @Sneftel points out in the comment below, performing this conversion on a JPG will result in some degradation of the image. For my purpose, this is an acceptable trade off.

3
  • 2
    Converting to a BMP and then to a JPG, particularly if the input was a JPG, will entail noticeable quality loss. Repacking the image isn't a bad idea, but you should do it in a way that doesn't degrade the image.
    – Sneftel
    Jan 10, 2017 at 10:42
  • For our purposes slight image degradation was not a problem.
    – a coder
    Jan 10, 2017 at 14:49
  • I'm using the package included with RHEL 7, which is currently ImageMagick 6.7
    – a coder
    Jan 11, 2017 at 18:01
2

I just have one small thing to add to this that hasn't been touched: using white-list strategy.

As others already have pointed out, relying solely on client side validation is certainly not a good thing. You have also employed a black-list strategy meaning that you are checking for things you know/think are bad and allowing for everything else. A much more robust strategy is to check for things you know are ok and rejecting everything else.

Sometimes this is trade-off against user feedback, like letting inexperienced users know what they need to do to successfully upload a picture.

In my experience, the two strategies are not necessarily mutually exclusive as long as they are used for different purposes: client-side black-list validation for user feedback and server-side white-list validation for security.

1

Verify a image EXIF tags or use a strpos function to find a PHP code in image content. Example:

$imageFile = file_get_contents($your_image_temp_path); // ATTENTION! Temporary path for images is really needed for security reasons!
$dangerousSyntax = ['<?', '<?php', '?>'];
$error = '';
foreach($dangerousSyntax as $value) {
  $find = strpos($value, $imageFile);
  if( $find !== true ) {
    unlink($your_image_temp_path);
    $error = 'We found a dangerous code in your image';
  }
}

if(!empty($error)) { 
  die($error);
}
1
  • 3
    What makes you think any given image file wouldn't have the characters '<?' somewhere in its data?
    – Sneftel
    Jan 10, 2017 at 10:26
0

In addition to the accepted answer, I would suggest using the PHP built-in getImageSize function to get the MIME type and make sure a PHP file is not hiding under an image file extension.

2
  • Can you detail better in your answer how you make sure there is no PHP tags by consulting the MIME type? Are you doing something similar to example 1 and resetting the Content-type header with any of the available types?
    – CPHPython
    Jan 9, 2017 at 14:35
  • @CPHPython That comment was intetned to be sued in addition to the accepted answer to make sure no PHP (or other incorrect) file types are uploaded by just changing the extension (for example, a PHP file, but named image.jpeg). It does not provide any security against having code in the filename.
    – Grey
    Apr 13, 2017 at 16:51

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .