so-sql-injections icon indicating copy to clipboard operation
so-sql-injections copied to clipboard

Rename to "Non-prepared statements"

Open ss23 opened this issue 7 years ago • 2 comments

As per #3 it's acknowledged this script is, for performance reasons, actually finding whether questions are using prepared statements, not whether there is SQL injection.

It seems prudent to change relevant references to indicate as such, rather than mislead people who haven't viewed the code.

ss23 avatar Dec 07 '16 10:12 ss23

No the regexes find bad SQL queries, and that can include poorly written prepared statements (sadly a good number of prepared statements in Stack Overflow also have SQL injection vulnerabilities). For instance here are the last two prepared statements I could find:

$stmt = $mysqli->prepare("SELECT AccountType, Password, StaffName FROM Staff WHERE Username='$username1'");

$query=$conn->prepare("INSERT INTO favourites VALUES ('$uid','$value') ON DUPLICATE KEY UPDATE value = VALUES(value + 1");

So it's not just "non-prepared statements". That being said, I agree that a FAQ or something to better explain the data would be useful.

laurent22 avatar Dec 07 '16 22:12 laurent22

How about adding some regexes that filter out content that probably has been correctly escaped? For example, use of mysql_real_escape_string() or mysqli::escape_string() pretty much guarantees that even though the code skips prepared statements (for backwards compatibility, performance or any other reason) the code probably does not contain a vulnerability. I agree that any code that contains a variable in SQL string without clearly visible encoding probably contains a vulnerability. I'm not sure if use of assert() qualifies as a trustworthy marker for "no SQL injection".

mikkorantalainen avatar Dec 08 '16 11:12 mikkorantalainen