Somebody's trying to hack one of my sites - what to do?

[bunner_bob]

One of the sites I manage has been hit by a bunch of requests where an id is replaced by a URL, pointing to a file on a remote site. I d/l'ed the file and it's full of exec() and other hacky-looking commands.

The attempts aren't getting through - the ids need to be numeric, and since they aren't it's just generating SQL errors (NOT echoed to the browser, as I have display errors disabled.

The requests look like:

http://www.mysite.com/store.php?CategoryID=http://bad_url_herePHP]

What should I do about this? Is there a way to block this machine from making these attempts?

- Bob

[NogDog]

Make sure you "sanitize" all external inputs before using them. In this case, since CategoryID must be an integer, before using it in a query you can cast it to an integer and avoid SQL errors:

$query = "SELECT blah...blah...blah WHERE CategoryID = " . (int) $_GET['CategoryID'];

String values should be run through mysql_real_escape_string() before using them in a query (assuming you are using MySQL).

[bunner_bob]

Hey - that's brilliant. Will do.

Yes, I do need to pump up my sanitization. I do mysql_real_escape_string on all my inserts and updates, but haven't been on selects or deletes.

Do you think I should just run some sort of function on the $_GET array in general, to real-escape everything in it?

(for $_POST I generally compare each field against a specified type for that db field - if it doesn't match it gets rejected)

[MarkR]

There is no way to really block these attempts; mostly if you install a URL filtering system (such as mod_security), its rules will block legitimate stuff too.

False positives are a very real risk with such things, and a forum or anything else where users can post arbitrary data will definitely hit them.

If your site is secure it shouldn't matter.

Your error log may be getting clogged however, with spurious error reports created by these exploit attempts.

Personally I have a few things in my applications which block certain types of automated attack early on in processing (before you start doing anything expensive like opening db connections), and do minimal logging on it. This reduces the amount of noise in my logs.

If you do have an include() which blocks these requests, always make sure you send the right http error status. In most cases this is 400 (bad request) if the client has sent a request that you're blocking by a rule.

Mark

[NogDog]

You could do something like this:

function sanitize($value)
{
   if(!is_numeric($value))
   {
      if(get_magic_quotes_gpc())  // handle magic quotes if turned on
      {
         $value = stripslashes($value);
      }
      if(($result = @mysql_real_escape_string($value)) === FALSE)
      {
         // use mysql_escape_string if not connected to MySQL
         $value = mysql_escape_string($value);
      }
      else
      {
         $value = $result;
      }
   }
   return($value);
}
// if any $_GET data, sanitize it:
if(count($_GET))
{
   array_walk($_GET, 'sanitize');
}

[bunner_bob]

That looks useful.

Probably best if I deliberately sanitize each query though, eh? That way I can sanitize values coming from POST or SESSION too, as needed. Anyway, I did that, last night. Took a while but I can sleep easier now!

• Bob

[bradgrafelman]

bunner bob wrote:

Probably best if I deliberately sanitize each query though, eh?

Precisely, and for a good reason: sanitizing the data for DB insertion should only happen at the DB-interaction layer, not on a global scope in terms of the PHP script.

If you wanted to echo out a person's username that was passed via URL, you wouldn't want to see: Hello, Mr. O\'Sullivan! would you?

And note that "sanitizing" doesn't just mean using a function such as [man]mysql_real_escape_string/man as pointed out in this thread; sometimes you don't even use this at all:

$id = intval($_GET['id']);

That's it - $id can now be considered sanitized, because not only is it guarenteed not to allow SQL injection attacks, but it also makes sure that you're getting the type of data you're expecting - an integer.

[bunner_bob]

I'm using (int)$_GET['id'], per NogDog - hope that's reasonably equivalent.

It's interesting - I find that the vast majority of my select queries are via one or two integer IDs. It's relatively rare for me to query on a string, and usually then it's something explicit in the query, and not passed as a variable:

SELECT * FROM table WHERE id=$id AND type='explicit_type';

I would like to establish some kind of standard for where in the process I'm sanitizing things. In some cases I'm doing it at the beginning of a procedural bit that calls a bunch of classes, since I can sanitize once and pass a clean variable to each class/instance. In other cases I'm doing it when building my where string, before passing it to my data access layer.

I don't suppose it hurts anything to go a little overboard with (int)$variable, does it? Not a big processor hog or anything? I know there are quite a few cases where I'm typecasting a variable twice, just because my code is a little spaghetti-ish and I figured better safe than sorry.

• Bob

[laserlight]

I'm using (int)$_GET['id'], per NogDog - hope that's reasonably equivalent.

It is, except that intval() seems very slightly slower but has the option of specifying the base for converting numeric strings to integers.

[bradgrafelman]

bunner bob wrote:

I would like to establish some kind of standard for where in the process I'm sanitizing things.

Then you might want to look into using the [man]PDO[/man] extension if you have PHP5; it does all of the escaping for you automagically.