Quoting Chris Darroch <[EMAIL PROTECTED]>:
But, suppose someone thinks they can do this safely:sql = apr_pstrdup(p, "SELECT foo FROM bar WHERE baz = '", apr_dbd_escape(user_input), "'"); apr_dbd_prepare(driver, p, handle, sql, NULL, &stmt); If user_input happens to contain a %s then subsequent p[v]select() calls will expect an extra parameter to be supplied, because our drivers parse for % in the queries but don't recognize when it occurs within single-quotes. Now, usually this would lead to a crash, I'd think, in p[v]select(). Not a security hole, but certainly unexpected behaviour.
Actually, given that the end result would be quoted, this would eventually (after APU DBD substitution) be interpreted as literal ?, $1, $2 etc. inside user input, depending on a database backend (i.e. it would be no parameter at all). Most likely, the user would get records where baz is exactly ?, $1, $2 etc. inside other text. Very confusing indeed.
I'm not sure why would someone want to do such a thing with prepared statements - it kind of defeats the purpose of prepared statements a bit. Usually, escaping is done for straight statements only - that's why I asked the question in the first place.
Anyhow, I get the point - we have the call, therefore, people may be tempted to use it.
However, in unusual cases where the coder accidentally provided an extra argument to p[v]select() that didn't correspond to a %s in the query string, and working with particular drivers, I suppose it might be possible for someone to make the application behave improperly by feeding input with %s in it. (Maybe the %s specifiers and the number of arguments were in sync originally, and then a coder removed the last specifier without thinking to remove the last p[v]select() argument.) Again, I'm not sure if it leads to a security issue per se, but it's not ideal.
I don't know for sure, but to be on the safe side, I think it's better to assume that it is possible.
So, my thought was: since APR DBD really encourages the use of placeholders and prepared statements, maybe it should discourage the use SQL escaping entirely. Obviously you can't prevent people from building SQL queries with completely unescaped user input. People can always shoot themselves in the foot. But it could be made as clear as possible that the API expects ASCII-based SQL queries with no embedded data, and all data should go into arguments.
You have me convinced. Simple stuff can be done with straight, constant strings and with query/select calls. More compilicated stuff should be done with p[v]query/select calls. We should not be in business of helping users along when they have a desire to shoot themselves in the foot.
At present, the only database backend that doesn't support prepared statements is sqlite2. So, we are relatively well covered there.
Now, the real question is: can we make apr_dbd_escape() return NULL in 1.3 and still claim binary compatibility with 1.2?
-- Bojan
