4 ms·
Also, one of the examples is: 03/12/2016 19:33:11: $pdo->prepare("UPDATE users SET avatar=? WHERE id=22 ")->execute([$u]) ; Isn't that the correct way to
by oneeyedpigeon 10y ago
Also, one of the examples is:
03/12/2016 19:33:11: $pdo->prepare("UPDATE users SET avatar=? WHERE id=22 ")->execute([$u]) ;
Isn't that the correct way to do it?
- DCoder 10y agoThat's what happens when you use Regexes to find problems: https://github.com/laurent22/so-sql-injections/blob/7b9d99c06c5d4e751d48a0d9b6c99ec4fc11d009/src/AppBundle/InjectionFinder.php#L62 https://github.com/laurent22/so-sql-injections/blob/7b9d99c0...
- tyingq 10y agoFound the regex's being used: https://github.com/laurent22/so-sql-injections/blob/master/src/AppBundle/InjectionFinder.php#L62 https://github.com/laurent22/so-sql-injections/blob/master/s... It's pretty basic. For UPDATE, it's matching anything with UPDATE followed by SET, followed by a dollar sign+alphanumeric And, it's specifically going after dynamic table names. Hmm. '/UPDATE\s+.*?\$[a-zA-Z0-9].*?\sSET.*?/i', // UPDATE $table SET ... Edit: You can also escape the scans by using variable names that start with an underscore :) $_dontscanme
- DCoder 10y ago> You can also escape the scans by using variable names that start with an underscore So people who use "WHERE id=" . $_GET['id'] will not get detected?
- oneeyedpigeon 10y agoI just did the same :) It flags, for example, the following as an SQL injection: query("DELETE FROM foo WHERE id=1");$id=2; In other words, it's flagging any line that begins with "DELETE ... FROM" and then later has a variable. Now, that might be a fairly strong correlation, but it's definitely far from perfect.
- tyingq 10y agoI suspect you can fool it by splitting into different lines, which is actually common with longish queries. $query="SELECT whatever from foo WHERE". "id=$bar ". "AND " . "field=$value";
- laurent123456 10y agoYes there are false positives. It would be too time consuming to extract and parse the PHP code from the questions since there are millions of them to process, however I found that the regexes, though not perfect, provide a good estimate and are quick to execute.
- tyingq 10y agoYou could tweak the regexes a bit to fix some things... - it's currently skipping variable names with underscores, and it's matching variable names that start with numbers, which isn't a valid variable name - the false positives coming from ->execute($whatever) could be fixed by not searching after a > character. I don't think that would create any notable amount of false negatives. Like, for example, changing: '/UPDATE\s+.*?\sSET\s.*?\$[a-zA-Z0-9].*?/i' To '/UPDATE\s+.*?\sSET\s[^>]*?\$[a-zA-Z_]/i'
- oneeyedpigeon 10y ago> You can also escape the scans ... ... or by splitting the SQL string over multiple lines. Or building it up by concatenating strings. Let's stop this now, we've demonstrated that the code is very brittle :)
- tyingq 10y agoAnother comment mentions that it would skip $_GET['whatever'] and $_POST['whatever'], which is probably in the most egregiously vulnerable code. I think the underscore thing is worth fixing.
- deleted 10y ago[deleted]
- laurent123456 10y agoYes, that's a false positive. I guess the results could be tweaked to filter out lines that include `->prepare(...`.