3

I've been assigned to one of my company's legacy webapps, and after a day or two of poking around the source, I've found an SQL injection vector similar to the following:

mysql_query("SELECT * FROM foo WHERE bar='" . $_GET['baz'] . "'");

I've tried to perform an SQL injection test against this, but it fails, due to PHP's magic_quotes_gpc module being switched on.

I know magic_quotes_gpc is dirty, but we have hundreds - if not thousands - of lines of code similar to the one above. We simply can't afford to switch magic_quotes_gpc off, as this would leave code like this wide open to attack.

I'd like to know how 'exploitable' the code above is, and whether we should fix it immediately, or include the task of fixing it in with our other refactoring tasks.

Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
James K.
  • 31
  • 1
  • 2

3 Answers3

7

The usual way to transition sites away from magic_quotes_gpc is to add a wrapper function:

function m($s) {
    if (get_magic_quotes_gpc())
        $s= stripslashes($s);
    return mysql_real_escape_string($s);
}

mysql_query("SELECT * FROM foo WHERE bar='".m($_GET['baz'])."'");

This will fix the problem of addslashes not being character-set-aware that can cause it to be vulnerable in some cases, and will generally make the code continue to ‘work’ as before.

However in the long term relying on input-escaping is unsustainable, as it will multiply slashes into input strings you are not inserting into the database, and fail to escape strings you're inserting into the database from other sources. This is the real reason magic_quotes_gpc is the wrong thing: it's applying an output-stage encoding to the input stage.

So add the wrapper function and then slowly go through updating all the SQL interpolations to use it. When you've got them all you can turn the magic quotes off.

bobince
  • 528,062
  • 107
  • 651
  • 834
  • And why it is so bad having all values escaped in input-stage, of course, if you are aware of the fact? In my opinion magic_quotes_gpc is bad only because you are not exactly aware of it and only because of this. Original questioner mentioned thousands of code lines he will have to patch, I guess he wouldn't ask if he could afford the change you've proposed. Why not to filter all input arguments ($_GET, $_POST, $_REQUEST, etc) at the entrance to the main code? – jayarjo Nov 14 '10 at 14:18
  • 1
    Because at the input stage you do not necessarily know whether one particular input is going to go into an SQL string literal, be output directly to HTML, dropped into a sent e-mail, compared against another string fetched from the database, used in a LIKE query, or many of these things or others. To make it work correctly you would have to remember what strings have SQL escapes in them and add an *unescape* for each operation that isn't putting each property into an SQL string literal. This is way, way messier than just escaping when you do an SQL query, which at least stays in one place. – bobince Nov 15 '10 at 11:47
  • 1
    `magic_quotes_gpc` is also bad because `addslashes` is totally not the right function for SQL-escaping: in MySQL there is the multibyte encoding issue, and in other databases that use ANSI standard string literals instead of backslashes it'll do nothing. – bobince Nov 15 '10 at 11:48
0

As long as magic quotes are on, and you don't use some special character encoding which could slip things through it, you should be fine -- so to speak. Problem is, when for whatever reason magic quotes are not active (server change, configuration change) you'll have a lot of holes to fix.

Matteo Riva
  • 24,728
  • 12
  • 72
  • 104
-1

I would add a line at the beginning that makes sure magic_quotes are enabled, also if they are disabled in the server configuration. Then you'll be more or less safe.

eflorico
  • 3,589
  • 2
  • 30
  • 41