Hacker News Reader

Comment by tyingq | Hacker News Reader

Found the regex's being used: 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

View on HN

> 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 :)

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

> 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?

That doesn't make any sense...

How so?

Sorry, I'm agreeing with you. I think the stats are bogus on this one for a few reasons

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

I 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";

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

You 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'

  '/UPDATE\s+.*?\sSET\s[^>]*?\$[a-zA-Z_]/i'

Reply on news.ycombinator.com