false detection

[divinity76]

seems this script thought that https://stackoverflow.com/questions/40964119/showing-query-mysqli-using-select-tag-html-input was vulnerable to SQL injections..

it isn't. was quoted as:

04/12/2016 22:52:02: $stmt = $con->prepare('SELECT titlu, linknews, autorID, data, count FROM stiinta WHERE autorID = ? ORDER BY ? DESC');//atentie la ordine din SELECT , prin aceasta functie se scapa de $variabla=$row['COLUMN'] diferenta dintre get_result VS bind_result(avatanj nu necesita mysqlnd, dezavantaj nu merge SELECT *) ALTA DIFERENTA get_result necesita FETCH_ASSOS(), bind_result necesita doar FETCH

[malarzm]

As noble as the goal is, the data that drives charts seems to be wretched. For instance select count(*) from jobs where corp_id = '.$this->user->corp_id.' and DATE(created_at) = DATE_ADD(current_date(), INTERVAL -0 DAY) and job_status = 0 limit 1 (reported from https://stackoverflow.com/questions/40970904/need-to-query-in-mysql-based-on-dates) is most probably safe as corp_id seems to come from DB already and won't lead to any kind of SQL injection.

[laurent22]

Yes there are false positives (and negatives). For performance reason, the script uses regular expressions, which provide a good enough approximation, but indeed aren't perfect. Also the script can't determine if a potential vulnerability can be exploited or not.

Regarding this particular line of code, the problem is that over time, as more and more developers modify the code, it can become vulnerable to SQL injections.

Today dev 1 wrote this:

select count(*) from jobs where corp_id = '.$this->user->corp_id

Six months later, dev2 refactor to this:

$corpId = $this->user->corp_id;
// ...
// ... more code that deals with $corpId
// ...
select count(*) from jobs where corp_id = '.$corpId.'....';

A year later, dev3 comes in and writes this:

$corpId = $_POST['coporation_id']; // Get corporation ID from a form
// ...
// ... more code that deals with $corpId
// ...
select count(*) from jobs where corp_id = '.$corpId.'....';

And now the code is open to SQL injections via the post variable. Had the original developer used prepared statements from day one, the code would have remained safe and maintainable (no need to check how and where a variable is used) over the years.

Also we don't know where $this->user->corp_id comes from. Presumably it's an integer, but maybe via a hack it can be set to an arbitrary value, which again would make the app vulnerable to SQL injections.

So there's no reason today to use concatenated strings like this, prepared statements are just as easy to write and way safer.

[malarzm]

Also the script can't determine if a potential vulnerability can be exploited or not.

Yet this is what the site claims it does by stating even in the chart in the top "PHP questions that contains SQL injections, per month" - should it read somewhere that this is potentional vulnerability I wouldn't say a word.

In no way I'm trying to justify not using prepared statements, the point I'm trying to make is that it just shows PHP developers in really bad light for no apparent reason: according to the site ~50% of questions posted on StackOverflow that relate to PHP and MySQL ARE vulnerable to SQL injections while they may not.

[laurent22]

I'm not sure there's a huge difference between a "potential one" and "real one" actually. Something like delete from students where email = " . $email is a vulnerability, not just a potential one (even if the $email variable has been filtered) because it makes the code more vulnerable to current or future attacks

However I agree that we can't know if it can be exploited or not. So I guess those are vulnerabilities that can be "potentially exploited". Maybe I could add some FAQ to the article to clarify what the data is showing (or if someone wants to make a pull request - the file to edit is index_template.html).