Hacker News Reader

Comment by maratd | Hacker News Reader

> Many inputs to query construction can't be parameterized.

I do not allow the client direct access to the database. They access the database through objects, which have methods that only permit parameterized input.

The client cannot specify the database, table, or columns. That is hard-coded. They can only specify values. Values that are always entered through a prepared statement.

In that scenario, isn't injection impossible?

View on HN

No. Of course not. In all likelihood your SQL RDBMS will only allow you to bind data. Do you paginate? What about "LIMIT" and "OFFSET"? Do you sort tables? Then you have to handle column names. Do you sort in both directions? Then you have to handle "ASC" and "DESC". And so on.

Obviously, you can write simple code to do each of these safely. But then, you can write simple code to handle query parameters too.

By all means, use parameterized queries. Just don't assume they're a magic totem against SQLI.

Could you give some actual examples of prepared statements that are injectable? I do not understand how it would work.

For example the following example from http://de.php.net/manual/en/sqlite3.prepare.php :

$stmt = $db->prepare('SELECT bar FROM foo WHERE id=:id');

$stmt->bindValue(':id', 1, SQLITE3_INTEGER);

You're missing his point somewhat. I don't know if this applies universally to SQL, but consider MySQL.

    $stmt = $db->prepare('SELECT bar FROM foo 
                            LIMIT :offset, :count');
    ...

You can't do that. You can only bind data to fields.

    $stmt = $db->prepare('SELECT bar FROM foo
                            ORDER BY :column :direction');
    $stmt->bindValue(':column', 'foo');
    $stmt->bindValue(':direction', 'DESC');

You can't do that either, because again, it's not data.

So, you end up having to do something similar to this (for the love of god, don't actually do this):

    $stmt->prepare("SELECT bar FROM foo WHERE id = :id
                    ORDER BY {$column} {$direction}
                    LIMIT {$offset}, {$count}");

Bound parameters won't save you, and the potential attack vector is there if you're not really careful. That example shows the most stupid thing you could ever do, so don't do it.

Other than probably historical reasons and interface definitions, is there any reason why this is the case? I can't immediately think of any sensible argument against allowing the binding of a value into a limit/order field. In fact, it should be type-checking/converting it as well, to prevent exactly mistakes like this.

Even if the DB interface doesn't directly support this, it seems like it's something all client wrappers should handle, preferably at the low level in addition to having to pull in a whole ORM + assorted bacon for the purpose.

Easy: bound parameters are a performance feature, not a security feature.

Thanks!

That would not have been a "real" prepared statement in my mindset so I get why I was confused.

I don't feel super smart, but wouldn't I simply make sure that $column is a valid column (ok, that one might need extra attention) and $direction would be either ASC or DESC and that $offset and $count are integers?

You can and should bind LIMIT and OFFSET with parameters on many if not most drivers (some can't handle it but most that I work with these days do, though I'm in Python, perhaps JDBC or other drivers are a different story). ASC and DESC are SQL keywords so assuming you're not a terrible programmer (which I know is what you're addressing) you wouldn't have those rendered from user input any more than the WHERE or ORDER BY keywords.

The lexical structure of the statement and the parameters themselves are two different things. You are correct that "parameterization can't save you from SQL injection", if you're such a bad programmer that you're feeding user input directly to produce lexical tokens. One example is a recent blog post talking about how to escape table names for apps that feed things like "customer_name" to serve as the table name (totally insane). Your example of feeding in "sort=xyz" to produce the column name in the ORDER BY is also a pretty awful practice - I haven't seen that one in like a decade, but sure.

So of course parameterization doesn't magically protect a system against all forms of attack - the programmer could be feeding user input directly into shell commands too. The recommendation for parameterization is addressing the bulk of the issue at least among the code that I regularly work with, maybe you deal with crappier programmers than I do on a regular basis.

> Your example of feeding in "sort=xyz" to produce the column name in the ORDER BY is also a pretty awful practice - I haven't seen that one in like a decade, but sure.

This still happens every day. Google how to do dynamic sorting and you'll find a hundred examples of it in PHP, and as unfortunate as it is, that's how a significant portion of code is authored.

Yes it is impossible. For example the following query "SELECT person_name FROM persons WHERE person_id = ?" Will be parsed and put in the query cache. Then when it is run (multiple times) it will bind the value in the correct place, and if there is anything else than a numeric value when it is trying to bind, the query will fail to execute.

Yeah, that's kinda the whole point. You can put an incorrect value in there and it'll fail, but you can't change the query itself. (i.e. Bobby tables would be an incorrect value, not a dropped students table.)

Rephrasing the question, pessimistically (ideally as kindly as possible, but my intent could easily be lost):

I've implemented a [complex?] white-listing solution to a problem in a domain where there is a long history of developer mistakes, passing user input to a 3rd-party technology most developers only know the minimum required for their job. Isn't injection impossible?

Edit: Sorry for any confusion; this was my way of saying 'this is only bulletproof if you got every edge case 100% right; few security experts make guarantees of impossibility of failure'.

How about a black listing solution? commit block on anything containing the full word "Statement" (or equivalent in in your language of choice).

Also code review, of which there will be need for plenty, when bare minimum developers is involved.

For the record, it's my understanding that white-listing is normally better than black-listing. I didn't mean that white-listing wasn't the best option, just that it sounded like it was being used.

Reply on news.ycombinator.com