Eliminating the problem [LWN.net]

Eliminating the problem

Posted Jun 1, 2006 8:28 UTC (Thu) by ncm (subscriber, #165)
Parent article: SQL injection vulnerabilities in PostgreSQL

Most injection holes are a result of trying to scrub user input by trying to escape characters from a list of troublemakers (or, worse, not bothering). If, instead, programs would discard (or, if necessary, escape) all characters *except* those in a known-good list, most of the subtleties would vanish. It's much better to eliminate a problem than to patch around it.

(Log in to post comments)

Eliminating the problem

Posted Jun 1, 2006 9:24 UTC (Thu) by smitty_one_each (subscriber, #28989) [Link]

Certainly the problem exists for multiple applications across arbitrary platforms. Could something like the LSB champion an unbork_string( some_string ) function that Does The Right Thing, and then just gently ridicule everyone to get on board?
The only winners in a piecemeal approach are the bad guys and the security consultants.

Eliminating the problem

Posted Jun 1, 2006 12:39 UTC (Thu) by im14u2c (subscriber, #5246) [Link]

We can't even get people to agree on strlcpy.... I can't imagine a more complex function getting traction easily.

Eliminating the problem

Posted Jun 1, 2006 13:27 UTC (Thu) by mrshiny (subscriber, #4266) [Link]

The thing is, SQL injection is trivial to prevent; simply use prepared statements with placeholders that are bound using an API. This improves the performance of the system AND increases security... Frankly I'm confused as to why you WOULDN'T use prepared queries.

Consider this example:

String sql = "select * from user_acct where username = '" + username + "' and password = '" + password + "'";
Statement stmt = connection.createStatement(sql);
stmt.executeQuery();

In this case the database will get a new statement every time a new user tries to log in to the system. The database usually caches compiled statements and execution plans to increase performance, but here each statement will look different. Also, it's vulnerable to the SQL injection where the password is ' or 1 = 1--.

Now consider the following example:

String sql = "select * from user_acct where username = ? and password = ?"; PreparedStatement stmt = connection.prepareStatement(sql); stmt.setString(1, username); stmt.setString(2, password); stme.executeQuery();

In this case, there is no chance for SQL injection, and furthermore the database can cache the SQL and execution plan on the server to increase performance, because for each user it is the same. Clearly anyone who DOESN'T use prepared statements is in need of some job training or a new career path.

Eliminating the problem

Posted Jun 1, 2006 14:31 UTC (Thu) by iabervon (subscriber, #722) [Link]

The thing I find even stranger is that it's pretty simple to write an equivalent for StringBuffer that has different methods for appending SQL text and constants, and automatically handles formatting for PreparedStatements. So:

SQLBuffer buffer = new SQLBuffer();
buffer.append("select * from user_acct where username = ").
       add(username).append(" and password = ").
       add(password);
PreparedStatement stmt = connection.prepareStatement(buffer.getSQL());
buffer.fill(stmt);
stmt.executeQuery();

That way, you don't have to worry about getting the variables mixed up, or dealing with the fact that you can't really trust the database driver to handle a java.util.Date.

The example you gave isn't as compact in this form, but that difference goes away if you've got queries where some clause is optional.

Eliminating the problem

Posted Jun 1, 2006 14:50 UTC (Thu) by mrshiny (subscriber, #4266) [Link]

I'd still feel better using a prepared statement, even if there are optional clauses in the statement. In such cases I normally do something like this:

List args = new ArrayList();
String sql = "select * from daily_revenue where transaction type = ? ";
args.add(transType);
if (sinceLastLogin) {
  sql += " and trans_date >= ? "; // for lots of optional clauses, use StringBuffer
  args.add(lastLoginDate);
}

Then later on you just bind all the variables in the order they appeared in the list. Also you can use a var-args function (in Java 1.5 and other languages that support var-args) to automate things like date converstion; if the type of one of the objects in the args list is Date or Calendar or some other non-SQL-friendly type, you can convert it automatically.

Eliminating the problem

Posted Jun 1, 2006 15:12 UTC (Thu) by iabervon (subscriber, #722) [Link]

My version is using a prepared statement. My SQLBuffer contains a StringBuffer and a List, and SQLBuffer.add() appends a "?" to the buffer, and adds the argument to its list, which it goes through in fill() using the loop that you omitted from the end of your example. My version is really identical to yours, except that my SQLBuffer methods abstract the pattern that you're open-coding (and, therefore, it's harder to screw up).

Eliminating the problem

Posted Jun 2, 2006 12:05 UTC (Fri) by smitty_one_each (subscriber, #28989) [Link]

>Frankly I'm confused as to why you WOULDN'T use prepared queries.

Oh, the motives might break down along the traditional compiled/dynamic lines.
I like to have a single function that can transform a the Request.Form into an arbitrary array of SQL statements, particularly for INSERT/UPDATE situations.
For generic text fields, I just replace ' with `, and I'm on my merry way. O`Neal never noticed, though I admit this could simply be "moving the problem".

Eliminating the problem

Posted Jun 2, 2006 19:02 UTC (Fri) by mrshiny (subscriber, #4266) [Link]

The only advantage, that I can think of, is that you can generate complete SQL statements ahead of time in one place, and later on execute them. However, if that is the pattern you wish to accomplish, it's trivial to wrap the generated string and the arguments to bind together in one object. Otherwise, I still don't see the problem... you can generate dynamic sql statements for prepared queries, and bind the parameters afterwards. Where I work we do this all the time; also another poster in this thread has even gone to the lengths of creating an SQL statement abstraction that generates the SQL and stores the parameters to bind in one step. It's easy and foolproof.

Eliminating the problem

Posted Jun 1, 2006 14:42 UTC (Thu) by jschrod (subscriber, #1646) [Link]

But this approach immediately leads to problems in an international context -- because most often it leads to the ban of all non-ASCII characters in names or addresses, as we have experienced so often in the past. But I live in Rödermark, and not in Rodermark or Roedermark, and I want to input that properly. The same holds surely for folks from China or Japan.

Nah, IMNSHO prepared queries with parameters are the only proper way to go.

Cheers, Joachim

Eliminating the problem

Posted Jun 2, 2006 2:45 UTC (Fri) by xoddam (subscriber, #2322) [Link]

> The same holds surely for folks from China or Japan.

Not to mention Iceland :-)

Eliminating the problem

Posted Jun 9, 2006 9:25 UTC (Fri) by aquasync (guest, #26654) [Link]

This shouldn't matter, the ö will be replaced with \ö, and then when evaluated as an part of an sql string, it should be turned back into ö (even if it was actually made up of multiple bytes).
That is provided that the escape policy is to replace \[\a-z]|(0-9){3} or whatever with the relevant unescaped thing, and otherwise to just copy the character verbatim into the output string.

Eliminating the problem

Posted Jun 9, 2006 9:03 UTC (Fri) by aquasync (guest, #26654) [Link]

Exactly, this seems a lot safer to me.
Perl's quotemeta function works in this way, (``all characters not matching "/[A-Za-z_0-9]/" will be preceded by a backslash...''), and provided SQL's string escapes work in a similar way, there should be no problems.