Perhaps, this discussion should be happening on theticket
<https://issues.apache.org/bugzilla/show_bug.cgi?id=53666> itself? Oh, well :)
Comments inline:
On 22.08.2012 10:47, Nick Kew wrote:
I think I've pointed a few people at the ODBC driver as an alternative.
Do you have a strong reason to use FreeTDS rather than ODBC?
Yes, ODBC is yet another layer of abstraction. I'd understand, if apr_dbd.c used
purely ODBC, leaving the ODBC implementation to deal with the backends' native
drivers. But if apr_dbd offers "native" drivers of its own, better to use those,
if possible. That's my "strong" reason :-)
One question here may be, does anyone have the platforms to test-drive
your patch?
I would not know... But, in my opinion, even without getting tested by anyone
else, my patch is still a vast improvement over the current situation...
It seems to do rather more than just fix a simple bug:
That's because the "bug" was not simple -- the driver remained broken ever since
the apr_dbd.c was changed to do the parsing of query-templates centrally instead
of delegating the job to each driver. Other drivers got updated back then, but
not the FreeTDS one :-( It still tried to do its own parsing, and was failing...
you've removed the untainting code which was part of protecting against
injection attacks.
I don't think, the untainting was ever properly implemented (you can't do it
right without prepared statements) and I don't believe, it is the driver's job
to do it anyway. None of the other drivers do it either -- though they rely on
the respective client-libraries. Sybase/FreeTDS client does not offer such
checks -- so be it... The untainting was costly (done for each call) and not
guaranteed -- better to leave it to the caller, IMHO.
Have you implemented prepared statements properly for all backends?
To the best of my knowledge, neither the Sybase's db-client library nor the
FreeTDS reimplementation of it offer prepared statements
<http://lists.ibiblio.org/pipermail/freetds/2010q1/025493.html>, unfortunately.
The current implementation certainly does not use them. Sybase offers a newer
client interface (ct-lib), that does have prepared statements (see ct_dynamic()
<http://infocenter.sybase.com/help/index.jsp?topic=/com.sybase.help.sdk_12.5.1.ctref/html/ctref/CHDIGFHG.htm>),
but FreeTDS does not provide it, so, for the driver to be compatible with both,
it has to use the old db-interface... To me this seems like an acceptable
caveat, as long as it is known and documented.
My patch does not touch any of the other backends.
Perhaps, some day I (or someone else) will implement apr_dbd_sybase.c -- using
the ct-interface, which would give prepared statements and other improvements.
But that would have to be maintained outside of Apache, because, foolishly,
Sybase would not open-source the client... It would also be Sybase-only (no MS
SQL Server).
For the time being, my patch offers a major improvement over the status quo --
the driver becomes usable. I am, in fact, preparing to use it with RewriteMaps
to do SEO for a giant site, that still uses an old Sybase-backed CMS. The
RewriteRules will ensure, no tainted keys are passed to the queries and the
DB-user's credentials will be limited to only SELECTs and only for a particular
table to mitigate the risk of any SQL-injection attack.
Yours,
-mi
