apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Darroch <chr...@pearsoncmg.com>
Subject Re: current dbd initiatives
Date Thu, 08 Jun 2006 16:02:22 GMT
Bojan Smojver wrote:

>> 4) I've been wondering if we should deprecate and then remove the
>> apr_dbd_escape() function.
>>
>>    Because we encourage the use of placeholders, and because we
>> introduce a new special character (%) which we really ought to be
>> escaping as well as the usual ones
> 
> Not clear on the escaping of % here. What would be a typical use for  
> escaping of prepared statements?

   There isn't one, that I know of, anyway.  Personally, I have no
use for the apr_dbd_escape() function, because we use prepared
statements with placeholders for all data.

   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.

   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.

   That's what I was thinking about % escaping.  Then there are
the usual SQL injection issues if someone tries the above code,
minus the escape function; on the other hand, even with the
escape function, if there are problems with DB's escaping function
(e.g., these recent alerts about charsets and backslashes and MySQL
and PostgreSQL), the APR DBD layer will just pass them along.

   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.

   I guess it just seems to me that offering the apr_dbd_escape()
function as-is implies that it'll do the job if you want to safely
construct a SQL query without using placeholders.  So I'd think that
either all the drivers need to do %-escaping plus whatever else is
required to ensure full safety, or, be deprecated and removed.
My own gut feeling is just that the former might turn out to be a
sinkhole: you keep trying to get it right and problems keep popping
up in one driver or another.  For example, can you pass non-ASCII
strings to it?  Clearly we expect the SQL queries themselves to
be ASCII or a superset, since we go parsing for % and so forth.
What if your data isn't ASCII-based, though, and you try the escape
function?  Hence my suggestion just to axe the thing and save some
possible headaches down the road.

Chris.

-- 
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263  E4DE C8E3 FA36 366A 375B

Mime
View raw message