1

I have a built a script around class.upload from http://www.verot.net/php_class_upload.htm

Basically what it is that all my images are stored on the server in a directory called /images/

The script I built basically takes some parameters from my website such as /xyzDir/tomnjerry.jpg?w=100&h=100&fill=1&color=fff

Then I have mod_rewrite which reads the file from /xyzDir/ into a php script which then translates the width and height and returns the image.

Lately I have noticed some idiots from Turkey trying to input weird characters into the parameters w= and h=

On my script I do check to make sure only integer is allowed in width and heigh and fill can be either 1 or 2 and color can only be certain values which i check via array.

I just want to see if there is anything else I should be doing in order to avoid getting hacked.

Thanks

6 Answers 6

9

Always remember, Filter In, Escape Out for all user supplied (or untrusted) input.

When reading user supplied data, filter it to known values. DO NOT BLACKLIST! Always always always always whitelist what you are expecting to get. If you're expecting a hex number, validate it with a regex like: ^[a-f0-9]+$. Figure out what you expect, and filter towards that. Do none of your filenames have anything but alpha, numeric and .? Then filter to ^[a-z0-9.]+$. But don't start thinking blacklisting against things. It won't work.

When using user-data, escape it properly for the use at hand. If it's going in a database, either bind it as a parameterized query, or escape it with the database's escape function. If you're calling a shell command, escape it with escapeshellarg(). If you're using it in a regex pattern, escape it with preg_quote(). There are more than that, but you get the idea.

When outputting user data, escape it properly for the format you're outputting it as. If you're outputting it to HTML or XML, use htmlspecialchars(). If you're outputting to raw headers for some reason, escape any linebreaks (str_replace(array("\r", "\n"), array('\r', '\n'), $string)). Etc, etc, etc.

But always filter using a white-list, and always escape using the correct method for the context. Otherwise there's a significant chance you'll miss something...

4
  • 2
    hex is 0-9 a-f ... so ^[a-z0-9]+$ should be ^[a-f0-9]+$
    – Spechal
    Jan 21, 2011 at 20:30
  • So basically i for the width and height, i should only use intval and that's it?
    – Adil
    Jan 22, 2011 at 2:28
  • 1
    @Eddy: If it's an int, either explicitly fast to an int (either (int) $foo or intval($foo), or enforce numeric digits (/^\d+$/') since (int)` can take some string values and convert to a number. It really depends on your exact usecase.
    – ircmaxell
    Jan 22, 2011 at 4:38
  • 1
    may I add their are a few PHP security classes out their that help you filter inputs (I use PHPIDS but their are a number of others)
    – user70568
    Jan 22, 2011 at 11:17
2

create a validation class to validate your post params like so.

class MyValidation
{
    public function is_interger($val)
    {
       return is_int($val);
    }

    public function within_range($val,$min,$max)
    {
        if($this->is_interger($val))
        {
            return ($val < $max && $val > $min);
        }
        return false;
    }

    public function is_hex($val)
    {
        return preg_match("/^([a-f0-9]{3}){1,2}$/",$val);
    }
}

And use to validate your values.

Example:

$Validator = new MyValidation();

if($Validator->is_hex($_POST['color']))
{
    //Sweet.
}
6
  • Ummm, why the strlen checks? why not just ^([a-f0-9]{3}){1,2}$
    – ircmaxell
    Jan 21, 2011 at 19:43
  • yea could do, I put strlen to save resource but then I realised that preg_match would have to be called without writing too much code.
    – RobertPitt
    Jan 21, 2011 at 19:59
  • So basically for the width and height, i should only use is_integer and is_numeric and that's it?
    – Adil
    Jan 22, 2011 at 2:29
  • 1
    No, thats the idea of creating a method, so you can perform multiple checks within a single check, im not writing all the code for the OP, just basic outline.
    – RobertPitt
    Jan 22, 2011 at 12:30
  • you can use the within range method to validate your width etc, like so 'if($validator->within_range($_POST['width'],20,80)){}'.
    – RobertPitt
    Jan 23, 2011 at 21:10
1

Make sure the image name does not contain string like "../". Depending on your script, that could be a way to step out images directory and make the script deliver other files.

1

You should use intval() for ensuring that the width and height are integers

$width = intval($_GET['w']);
$height = intval($_GET['h']);

You can do

$fill = $fill == 1 ? 1 : 2;

Which is a ternary operator, so if it's anything apart from 1 it's going to be set to 2.

As for validation of hex, the rules of hex dictate that it must be in range of 0-9/A-F.

$color = preg_replace('/[^0-9a-f]/i', "", $_GET['color']);

Hope that helps. (It should be noted that my suggested code will perform the manipulation required to make it suitable for your page, rather than confirming that is is valid before hand)

1
  • So basically i for the width and height, i should only use intval and that's it?
    – Adil
    Jan 22, 2011 at 2:28
1

No one's mentioned the filter extension here which provides great filtering natively implemented in the PHP engine. IMHO this is a great extension and should always be used before rolling your own filtering code. For example, checking for an integer is as simple as:

<?php
if (filter_var($value, FILTER_VALIDATE_INT)) {
    //Valid Integer.
}
?>

Validating a hex number can be done with:

<?php
if (filter_var($value, FILTER_VALIDATE_INT, FILTER_FLAG_ALLOW_HEX)) {
    //Valid Hex number
}
?>
1

Then I have mod_rewrite which reads the file from /xyzDir/ into a php script which then translates the width and height and returns the image.

If you include the file, image or other type, it will execute any PHP code buried within it. So if you didn't shake off any possible code appended to a user uploaded image by reformatting it through imagemagick or gd into a completely new file, that is one way your server can be compromised.

So for example if you serve the user uploaded image like this...

<?php

header('Content-type: image/jpeg');
header('Content-Disposition: attachment; filename="tomnjerry.jpg"');

include('xyzDir/tomnjerry.jpg');

?>

...and if the user opened the jpg in a raw text editor and appended <?php phpinfo(); ?> to the very end before uploading it to your server, then they can browse to and download it and extract all phpinfo details of your PHP installation from it.

But since you mentioned resizing the image first, you're probably not even serving the image this way. So you should be safe from this attack.

Your Answer

Reminder: Answers generated by Artificial Intelligence tools are not allowed on Stack Overflow. Learn more

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.