The state of PHP security [LWN.net]

The state of PHP security

Posted Dec 22, 2006 2:48 UTC (Fri) by denials (subscriber, #3413) [Link]

I still think the solution to SQL access is to remove support for using strings as SQL statements, and instead have a "SQL statement" type, with functions to append statement text (with it being an error to include any single quotes in this) and to append constants.

Sigh.

Every programming language uses strings for SQL statements. You can concatenate those strings and make mistakes in every language. The root of this particular problem, IMHO, goes back to MySQL's inability (until recently) to support placeholders. MySQL, you will recall, was the database most often paired up with PHP to enable beginning programmers to create dynamic Web applications. MySQL's lack of placeholders necessitated the interpolation of variables directly into SQL statements, or the concatenation of variables with SQL statement strings, in an unsafe manner. Top that off with the "user-friendly" but lax treatment of things like allowing you to quote-delimit integers (not legal in strict SQL) and you have a recipe for SQL injection attacks.

As an aside: single quotes are actually legal in SQL (usually escaped as ''), so making it an error would be even more foolish than creating an "SQL statement" type. SQL statements, like it or not, are made of strings.

The state of PHP security

Posted Dec 22, 2006 10:03 UTC (Fri) by kov (subscriber, #7423) [Link]

Every programming language uses strings for SQL statements. You can concatenate those strings and make mistakes in every language.

True. But many languages provide APIs that do _not_ use strings for SQL statements, and that are usually the recommended way of doing SQL.

The state of PHP security

Posted Dec 22, 2006 17:00 UTC (Fri) by iabervon (subscriber, #722) [Link]

Every programming language (with SQL bindings) does seem to use strings for SQL statements, and it's a design error in every one of them.

SQL string constants can have single quotes in them escaped as a sequence of two single quotes, but SQL statements can only have single quotes using weird other methods (for purposes like having a column named "don't", of course; none of the standard functions or keywords contain single quotes). And SQL statements can (and must) include single quotes around string constants, but I'm prohibiting writing string constants as statement text.

E.g., you'd have to write something like:

new Statement().cmd("SELECT uid FROM passwd WHERE password=").
                lit(password).cmd(" AND username=").lit(username)

And this would ensure that the two string literals are properly quoted and escaped. And the user can't do something like cmd("WHERE password='" + password + "'") because the single quotes in the command text are prohibited. And obviously cmd("WHERE password=").cmd(password) would never work, because the client-supplied password wouldn't get even the outer single quotes. So the correct way is the only way to get it to work at all, and you don't have any SQL insertion holes possible unless the language bindings get screwed up.

Of course, it's not necessary for the database to support placeholders in order for the language bindings to support substituted constants. The language bindings can substitute properly-handled constants, which would imply that any security flaws must be in the language bindings, which are exclusively written by people who ought to know better than to mess it up.

The state of PHP security

Posted Dec 25, 2006 4:39 UTC (Mon) by erich (subscriber, #7127) [Link]

Sorry, but that's just a hack to make users switch to a secure syntax.
And it especially prevents programmers who know about the security implications to make their code readable... e.g. by constructing queries in strings.

I used to have hardcoded statements such as 'WHERE email NOT LIKE "%@%"' and I'd sure prefer to keep them this way. Also note that with LIKE, you might need a different escaping (which eventually needs to escape %, too).
Having to use 'WHERE email NOT LIKE ?' and passing "%@%" as first parameter is fine with me, but don't force me to use that ugly pseduo-OOP syntax you suggested, with two different appends for the string buffer. Ugly!

P.S. sometimes you need quotes to be able to access certain tables or colum names. E.g. have you had a column named "like"? or "where"?

The state of PHP security

Posted Dec 25, 2006 6:08 UTC (Mon) by iabervon (subscriber, #722) [Link]

The company I was working for eventually ripped out all of the statements constructed in strings because they were too unreadable. It's fine if the query doesn't vary at all, but once you have any variability at all, either structural or with constants, it's more readable to have a smart buffer. Of course, the syntax should fit the language you're writing in (mine was Java, hence the StringBuffer method chain); maybe you'd rather

buffer = SQL("SELECT uid FROM passwd WHERE username=") + username +
  SQL(" AND password=") + password;

Incidentally, you're using entirely the wrong quotes. String constants have to be in single quotes (unless you're using old MySQL syntax), and column names can only be in double quotes (or, if you're using old MySQL, back tics). If you're using the same quotes for both string constants and column names that match keywords, you've got bigger problems than the library interfering (is "password" a constant or a column name?)

The state of PHP security

Posted Dec 26, 2006 12:49 UTC (Tue) by IkeTo (subscriber, #2122) [Link]

> I still think the solution to SQL access is to remove support for using
> strings as SQL statements, and instead have a "SQL statement" type, with
> functions to append statement text (with it being an error to include any
> single quotes in this) and to append constants.

I don't think so. There is no intrinsic problem of using strings as SQL statements. It is the same whether you write

handle.do_select("SELECT id, name FROM customers WHERE country='" + country + "'")

handle.do_select(new SQLStatement("SELECT id, name FROM customers WHERE country='" + country + "'")).

If your user don't know what's wrong with it, you can't expect them to do the right thing for it. If the syntax for doing things the secure way is harder than the syntax for doing things the insecure way, you will see lazy people doing the wrong things saying that they are only for the prototype and never correct it when the software is released.

The only answer is a combination of education and making correct things easier. Make the first example in the official database tutorial that needs a user-provided input to read handle.do_select("SELECT id, name FROM customers WHERE country=?", country), and tell learners that this is safe because even if country contains a single quote or semicolon it does the right thing to get them escaped. Build the library so that a single placeholder syntax is used for all different SQL implementations. As a bonus, teach them how to subvert programs that simply appends the user input to the SQL. Then doing it the secure way is easier than doing it the insecure way, and people actually feel insecure when they write insecurely. Then there will be no more problem of SQL injection.

The state of PHP security

Posted Dec 26, 2006 18:57 UTC (Tue) by iabervon (subscriber, #722) [Link]

My point is that doing

handle.do_select(new SQLStatement("SELECT id, name FROM customers WHERE country='" + country + "'"))

should always give a runtime error "Single quote in SQL statement". The only way to do this query should be something like

handle.do_select(new SQLStatement("SELECT id, name FROM customers WHERE country=") + country).

You'll note that this is easier than the insecure way (you don't have to know how to put a string in a SQL statement, or remember to close your single quotes or anything like that, and it's shorter anyway, unless your language happens to have operator overloading for strings and nothing else), and, additionally, the system prevents the insecure way from ever being executed, regardless of whether its input is malicious in a particular case or not. The slightly inconvenient case is where you'd be able to use a hard-coded constant embedded in your SQL if that were permitted; but hard-coded constants are a pain for further development anyway; some day your DBA will make you change them, and you'll be sad if they aren't split out into a single location with a logical name. (The only exception being patterns for LIKE, where you have to suffer through making your pattern a string constant instead of having it embedded in the SQL.)

The only sure way to educate people not to write insecure code is to make insecure code not work at all. The lazy people who do things the insecure way for the prototype will find that the prototype doesn't even run, so they'll go back and do it the secure way.