Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename to "Non-prepared statements" #5

Open
ss23 opened this issue Dec 7, 2016 · 2 comments
Open

Rename to "Non-prepared statements" #5

ss23 opened this issue Dec 7, 2016 · 2 comments

Comments

@ss23
Copy link

ss23 commented Dec 7, 2016

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.

@laurent22
Copy link
Owner

laurent22 commented Dec 7, 2016

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.

@mikkorantalainen
Copy link

mikkorantalainen commented Dec 8, 2016

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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants