Why is it unsafe to pass a value to a PHP function to then escape it and return it back

Question

I don't know if this is a good question or not but it is something that got me into a lot of doubt today. So I was using a PHP function to basically save time and escape values like we used to do with mysql_real_escape() before but I was told that doing this is actually dangerous, this is what I was doing:

function mysql_string_safe($stringtoclean)
{
    $safestring = mysqli_real_escape_string($GLOBALS['confDBLink'],$stringtoclean);
    return $safestring;
}

I now have the doubt.. How doing any of this can actually be unsafe? Since all I'm doing is sending a value to the function to then escape it in mysqli_real_escape_string? (and then returned of course) So when I actually want to escape a value I would be doing the following: mysql_string_safe($valuetoescape); I really wanted to know, not only because of the doubt but to also know, because if this is really dangerous as I was told, then I'll remove this from my applications as well. Thank you very much for your time.

Out of pure curiosity: what's the advantage of calling a function that just calls another function? less typing? — Damien Pirsy, Oct 30 '12 at 22:37
It's dangerous that you have to escape your strings at all, when you should be using parametrized queries. — Waleed Khan, Oct 30 '12 at 22:37
This doesn't look unsafe to me. Who told you that and what else did they say? — Pekka, Oct 30 '12 at 22:37
It's not dangerous. It's just dated and cumbersome. PDO and prepared statements make this redundant. — mario, Oct 30 '12 at 22:38
@Pekka http://stackoverflow.com/questions/13147555/php-sql-injection-prevention-with-string-operations/13147638 That's where I was told that it was unsafe, so I had enough doubt to ask. — Ignacio Belhot Colistro, Oct 30 '12 at 22:39
I would argue that it's a bad program structure (pointless function wrapper, globals, not prepared statements), but it's not inherently insecure when used properly. — deceze, Oct 30 '12 at 22:41
He didn't say it's unsafe, just that it's confusing. Which is kinda true. It's not a terrible problem, but it makes your code harder to read. — Pekka, Oct 30 '12 at 22:43
Thank you for your answers, I now have this pretty much clear. — Ignacio Belhot Colistro, Oct 30 '12 at 22:46

score 4 · Accepted Answer · answered Oct 30 '12 at 22:40

4

There's nothing unsafe per se, but using prepared statements/binding values is preferred over escaping inputs explicitly. The latter is potentially unsafe for a couple reasons:

You may forget to escape an input
You may accidentally escape the same input twice

answered Oct 30 '12 at 22:40

Explosion Pills

188,624
52
326
405

Also `mysql_real_escape_string()` has been found to have exploitable bugs in the past (e.g. with multibyte character sets); relying upon string escaping is unsound as other such bugs could always exist in the future. As you say, parameterised statements are safer because the RDBMS does not parse one's variables for SQL so even specially crafted values cannot result in SQL injection. – eggyal Oct 30 '12 at 23:04
1

@eggyal he's using `mysqli`, not `mysql` – Explosion Pills Oct 30 '12 at 23:07

score 1 · Answer 2 · edited May 25 '19 at 14:45

1

I'm willing to be that whoever told you this wasn't referring to the fact that you had a function that called mysql_real_escape_string() but rather to the fact that you were using the mysql_* functions at all. You may have seen a variation of this message around which explains why you shouldn't use those functions:

Please, don't use mysql_* functions in new code. They are no longer maintained and the deprecation process has begun on it. See the red box? Learn about prepared statements instead, and use PDO, or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.

edited May 25 '19 at 14:45

Zoe

27,060
21
118
148

answered Oct 30 '12 at 22:41

SomeKittens

38,868
19
114
143

Yes, that was what I was trying to explain before. mysql_ functions are deprecated and people should now use PDO or mysqli. – Ignacio Belhot Colistro Oct 30 '12 at 22:42

score 0 · Answer 3 · answered Oct 30 '12 at 22:49

Depending on how much of your system is already using mysqli, I would make the switch to PDO:

$stmt = $pdo->prepare('SELECT * FROM users WHERE username = :username');
$stmt->bindValue(':username', $_POST['username']);
$stmt->execute();

$row = $stmt->fetch();

By binding the values as shown, PDO handles the escaping of characters for you. This is generally considered the improved way of interacting with a database with PHP, you can read about it here: http://php.net/manual/en/book.pdo.php

I know this doesn't strictly answer your question, I think this is a case where a different solution altogether might be useful.

Why is it unsafe to pass a value to a PHP function to then escape it and return it back

3 Answers3