using mysql_real_escape_string in blind code

Question

I have a client which has very large amount of code on its production server, now we are securing this server. They have SQL vulnerability is there code, so we are adding these commands in the beginning of the code. Will this solve the issue even if temporarily.

$_POST = array_map('mysql_real_escape_string', $_POST);
$_GET = array_map('mysql_real_escape_string', $_GET);
$_REQUEST = array_map('mysql_real_escape_string', $_REQUEST);

It might, but it’ll eff up any other use of these parameters, for example when they are displayed on the page and contain characters that get escaped; and if they would be used to display are pre-filled form again after an error, you’d get the escaping characters “multiplied” … and have them eventually end up that messed up in the database. So this could only be an absolute “hot fix”, and might have unforeseen consequences on other parts of the application. So is it recommendable? No, I’d say. — C3roe, Commented Mar 2, 2014 at 18:36
What about other attacks then SQL injection? if even SQL injection protection wasn't done.... — CodeBird, Commented Mar 2, 2014 at 18:45

Bill Karwin · Accepted Answer · 2014-03-02 18:46:50Z

4

There's no way for us to tell if that solves the issue, because we don't see how you the application code is using the $_GET, $_POST and $_REQUEST variables. It might actually break other uses of the request variables to force them all to have escaping.

You might use them as unquoted numeric values in SQL, instead of as quoted string or date literals. Escaping doesn't help unless you're quoting the values in SQL.
You might use them as column names, SQL keywords, etc. Escaping is only for strings and dates, it doesn't escape the back-tick.
You might not use them in SQL at all. You might use them for some other function argument, or in a case statement or as an associative array key, etc.

Basically, you're trying to do the same thing that magic quotes used to do, but magic quotes was deprecated because it's a bad idea to have a one-size-fits-all solution for security. It's just based on too many assumptions that turn out not to be true.

The proper solution (if you must stick with ext/mysql for now) is to apply filtering or escaping to request variables as you use them in SQL.

Of course it's best to upgrade to ext/mysqli or PDO, and use query parameters. But I realize this is a lot more work to adapt a large codebase to a new API, and you're looking for what you can do short-term.

You might also look at mod_security, which can help you filter out suspicious patterns of request variables without changing any PHP code at all. But it's also a lot of work to design mod_security rules that will work for your app without blocking legitimate requests.

edited Mar 2, 2014 at 18:46

answered Mar 2, 2014 at 18:38

Bill Karwin

564k87 gold badges702 silver badges861 bronze badges

Currently they have something like this INSERT INTO abc(asd) VALUES('".$_REQUEST['variable']."');
– Nik
Commented Mar 2, 2014 at 18:43
Yes I understand. That's clearly not safe. But what other uses of $_REQUEST or $_POST or $_GET do they have in the application?
– Bill Karwin
Commented Mar 2, 2014 at 18:47
Its just for quickix at initial phase,after this It will be audited properly and fixed all.
– Nik
Commented Mar 3, 2014 at 8:31
I think I should get escaped $_REQUEST for db queries and $_GET and $_POST for other internal functions, this way they will not crash.
– Nik
Commented Mar 3, 2014 at 8:32
No, that's the wrong way to use $_REQUEST. Use $_GET for read queries and $_POST for queries that change data. Otherwise when web crawlers visit all the links in your site, they can visit the "delete user" link.
– Bill Karwin
Commented Mar 3, 2014 at 15:53

| Show 2 more comments

Collectives™ on Stack Overflow

using mysql_real_escape_string in blind code

1 Answer 1

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

Related