On Wed, Aug 22, 2012 at 12:26 PM, Mikhail T. <[email protected]> wrote: > Perhaps, this discussion should be happening on the ticket itself? Oh, well > :) Comments inline:
and/or [email protected] > > 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, 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()), 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 -- Born in Roswell... married an alien... http://emptyhammock.com/
