mysqli: error on invalid escaping

[staabm]

wrong (sql injection vulnerable):

$query = sprintf('SELECT CountryCode FROM City WHERE name="%s"', // double quotes in query
    mysqli_real_escape_string($mysqli, $city));

$query = sprintf('SELECT CountryCode FROM City WHERE name="%s"', // double quotes in query
    $mysqli->real_escape_string($city));

$query = sprintf('SELECT CountryCode FROM City WHERE name=%s', // no quotes in query
    mysqli_real_escape_string($mysqli, $city));

$query = sprintf('SELECT CountryCode FROM City WHERE name=%s', // no quotes in query
    $mysqli->real_escape_string($city));

correct:

$query = sprintf("SELECT CountryCode FROM City WHERE name='%s'",
    $mysqli->real_escape_string($city));

refs

• https://www.php.net/manual/en/security.database.sql-injection.php
• https://www.php.net/manual/en/mysqli.real-escape-string.php
• https://www.php.net/manual/en/function.pg-escape-string.php
• https://www.php.net/manual/en/pdo.quote.php
• https://www.php.net/manual-lookup.php?pattern=escape-string

might take inspiration from https://github.com/complex-gmbh/php-clx-symplify/pull/257 (closed source)

[staabm]

@craigfrancis do you agree that these are cases we should report errors for, as they hide errors in wrong escaping?

do other php apis have the same problem?
I checked PDO, but PDO::quote already wrapps the returned value in single-quotes, so at least the mentioned examples shouldn't be a problem for PDO.

[staabm]

I think I have seen similar code like this in my life somewhere:

wrong (sql injection vulnerable):

$query = sprintf('SELECT CountryCode FROM City WHERE name="%s"', 
    addslashes($mysqli, $city)); // invalid escaping method

$query = sprintf('SELECT CountryCode FROM City WHERE name="%s"', 
    addcslashes($city));  // invalid escaping method

[craigfrancis]

Yep, missing quotes is a common problem... a code base with many thousands of values being escaped, and all it takes is 1 mistake.

The mysqli_real_escape_string() function is dangerous, developers learn about escaping, and they use it, and think they are now safe... it's one of the main reasons why I keep pushing developers to use database abstractions, or parameterised queries (both still need to use the literal-string type to check mistakes haven't been made).

It's even more fun when developers try escaping field names, e.g. $sql .= ' ORDER BY `' . $db->real_escape_string($field) . '`' (and yes, I've seen it without the backtick quotes as well).

As to similar API's...

• PDO, yes, you're correct, PDO::quote is much safer because it adds the quotes itself, but I will note the manual says "you are strongly recommended to use PDO::prepare() to prepare SQL statements with bound parameters".
• PostgreSQL, the pg_escape_string() has the missing quotes issue, and you also need to use single quotes as well (double quotes don't get escaped, source), that's why you should at least use pg_escape_literal()... or, please, just use parameterised queries :-)
• MSSQL, the mssql_* functions were removed in PHP 7, but I seem to remember it didn't have a string escaping function, I believe they were trying to get everyone to use parameterised queries. Unfortunately I've seen developers make their own, from str_replace('"', '""', str_replace("'", "''", $string)) to using a hex bytestring.
• SQLSRV, the replacement for MSSQL, that also pushes developers to use prepared queries.
• HTML encoding has a similar issue "<img src=" . htmlentities($url) . " alt='' />", where $url could be set to '/ onerror=alert(1)' (and yes, it does not consider javascript: urls in <a href="">).
• Fortunately escapeshellarg() does add single quotes.
• And when I find addslashes() being used incorrectly, I should put a part of those invoice payments into a holiday fund, and go on a lot more holidays in it's (dis)honor.

And when you say you have seen similar code before, we kinda discussed this last year, ref issue 316.

In your examples, a name=%s is most likely going to be picked up fairly quickly (usually an SQL parse error), I tend to find this mistake takes the form of id=%s, because a number is normally being passed to it, and that works ™.

Extra bonus point for parameterised queries... the developer defined SQL is sent to the database first, the query execution plan is created from it (sometimes that can be cached)... then the user values are accepted/used; this means the user values cannot have any affect on the query execution plan, so even if there was an implementation issue, it's far too late to change it... in contrast, (escaped) user values are mixed in with the SQL, and separating them again is tricky.

[staabm]

thanks. I agree that prepared statements is a must have. still there is a lot of vulnerable code out there, where I want to point out security issues.

I will add a "phpstan-tip" to the rule that escaping should be prevented. you had a website somewhere, where all the problems and solutions are described in detail, right?

[craigfrancis]

That works.

As to the website, yep, although it's more of a quick summary (so people are more likely to read): https://eiv.dev/

It does link to pages on my GitHub repo with some typical examples:

https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification/escaping.php
https://github.com/craigfrancis/php-is-literal-rfc/blob/main/justification/mistakes.php

And yes, there is a lot of vulnerable code out there, and why I'm so glad you're tackling it from another angle... hopefully, with enough people working on this problem, we can stop boring injection vulnerabilities. and I can move on to more interesting problems :-)