While trying to configure some Sybase DB-backed redirections, I noticed, that ALL of my queries were failing, even when the reported keys were present in the queried table. Digging deep into the dbd/dbd_freetds, I noticed, that the specified query never makes it to the freetds-driver's code. For example, the driver's dbd_freetds_prepare() invoked by apr_dbd_prepare() sees the line, that was already "munched" by the caller. In the below output from gdb, notice the difference between query in frame #1 and #2: #1 0x00002aaab11a866d in dbd_freetds_prepare (pool=0x2aaab32c3178, sql=0x2aaab32c1260, query=0x2aaab32c32e0 "SELECT seo_url FROM CT_SEO WHERE vgn_url = ''", label=0x2aaaaae082f0 "map_v2s", nargs=1, nvals=1, types=0x2aaab32c3320, statement=0x7fffffffe0f0) at dbd/apr_dbd_freetds.c:493 stmt = 0x2aaab32c3328 #2 0x00002aaaab19a523 in apr_dbd_prepare (driver=0x2aaab13a97e0, pool=0x2aaab32c3178, handle=0x2aaab32c1260, query=0x2aaaaae08304 "SELECT seo_url FROM CT_SEO WHERE vgn_url = '%s'", label=0x2aaaaae082f0 "map_v2s", statement=0x7fffffffe0f0) at dbd/apr_dbd.c:476 qlen = 62 i = 1 nargs = 1 nvals = 1 p = 0x2aaab32c330d "" pq = 0x2aaab32c32e0 "SELECT seo_url FROM CT_SEO WHERE vgn_url = ''" q = 0x2aaaaae08333 "" t = 0x2aaab32c3320 Perhaps, the driver should not be counting the args (nargs) on its own, relying on the apr_dbd_prepare to provide the count?.. Whatever it is, the current latest RELEASE of apr-util's dbd is broken in this regard :-(
FWIW, the same problem persists in APR/APU's current trunk...
Possible fix (completely untested): Index: apr_dbd_freetds.c =================================================================== --- apr_dbd_freetds.c (revision 1370619) +++ apr_dbd_freetds.c (working copy) @@ -794,7 +794,7 @@ dbd_freetds_get_name, dbd_freetds_transaction_mode_get, dbd_freetds_transaction_mode_set, - "", + "%%s", dbd_freetds_pvbquery, dbd_freetds_pvbselect, dbd_freetds_pbquery,
No, that is not sufficient. Even running the dbd/test freetds .... crashes on "prepared select": ======== prepared select ======== Program received signal SIGSEGV, Segmentation fault. 0x0000003501079b80 in strlen () from /lib64/libc.so.6 (gdb) where #0 0x0000003501079b80 in strlen () from /lib64/libc.so.6 #1 0x00002aaaab143a89 in dbd_statement (pool=0x608128, stmt=0x608780, nargs=1, args=0x6087f0) at dbd/apr_dbd_freetds.c:220 #2 0x00002aaaab143c2f in dbd_freetds_pselect (pool=0x608128, sql=0x608200, results=0x7fffffffe500, statement=0x608780, seek=0, values=0x6087f0) at dbd/apr_dbd_freetds.c:248 #3 0x00002aaaab143d68 in dbd_freetds_pvselect (pool=0x608128, sql=0x608200, results=0x7fffffffe500, statement=0x608780, seek=0, args=0x7fffffffe3d0) at dbd/apr_dbd_freetds.c:270 #4 0x00002aaaaaac27c8 in apr_dbd_pvselect (driver=0x2aaaab345760, pool=0x608128, handle=0x608200, res=0x7fffffffe500, statement=0x608780, random=0) at dbd/apr_dbd.c:519 #5 0x0000000000401a97 in test_pselect (pool=0x608128, handle=0x608200, driver=0x2aaaab345760) at dbd.c:287 #6 0x000000000040232e in main (argc=3, argv=0x7fffffffe688) at dbd.c:395 (gdb) up #1 0x00002aaaab143a89 in dbd_statement (pool=0x608128, stmt=0x608780, nargs=1, args=0x6087f0) at dbd/apr_dbd_freetds.c:220 220 len = strlen(stmt->fmt) +1; (gdb) p stmt->fmt $1 = 0x73000000006087a0 <Address 0x73000000006087a0 out of bounds> The fmt is corrupted by earlier attempts at regexp-matching and string-moving (actual culprit is the memmove call in recurse_args()) -- neither are necessary at all...
Created attachment 29204 [details] Fix/improve the FreeTDS-driver The patch contains the following fix/improvements: * Compiles against both FreeTDS and Sybase's own OpenClient headers * Fixes access of uninitialized memory, when parsing the driver's parameters (The above two resolve the Bug 53676) * Removes remnants of the driver's own attempts to parse the statement relying instead on the caller (apr_dbd_prepare()) to insert magical '\1' characters into the placeholders for us * Speeds-up creating actual statements from the pre-processed templates * Fixes extraction cells to work with both Sybase and FreeTDS libraries (use dbdatlen() instead of dbcollen()) * Records and makes available errors/messages * Removes the attempts to "untaint" the arguments -- this should, probably, be done by the callers and -- when the input data is untrusted -- mitigated by the database-permissions Potential problems: * New code may be mixing tabs and spaces not in accordance with APU's current coding style * The new error-handling code may be too cavalier in allocating strings from the pool(s). This should be infrequent, but, if the caller keeps submitting erroneous statements using the same handle, the handle's pool may grow too large -- help wanted. * On a few (rare) occasions the new code will use fprintf(stderr) -- in the belief, that outputting such messages somewhere is still better, than ignoring them completely (and leaving the user wonder, why a login does not work, for example) * Parsing of the statement template relies on the unprintable character '\1' to be inserted into the placeholders for us by the apr_dbd_prepare(). I would much rather reimplement this using the proposal in Bug 53689 instead Comfortable reassurance: * With these changes the module (tested against both FreeTDS and Sybase) passes most of the test/dbd.c (which needs to be modified as per Bug 53687) -- while remaining clean under valgrind: % valgrind --show-reachable=yes --leak-check=full --track-origins=yes test/dbd freetds server=v6_icstest_nj2x,host=s605202nj2sl267.uswhwk6.savvis.net,username=testV6icsuser,password=tV6icsuser,port=6200,charset=iso15,lang=us_english ==27683== Memcheck, a memory error detector ==27683== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al. ==27683== Using Valgrind-3.5.0 and LibVEX; rerun with -h for copyright info ==27683== Command: test/dbd freetds server=v6_icstest_nj2x,host=s605202nj2sl267,username=XXXX,password=XXX,port=6200,charset=iso15,lang=us_english ==27683== Loaded freetds driver OK. Name too long for LOGINREC field v6_test_nj2x: Changed database context to 'v6content'. Opened freetds[server=v6_icstest_nj2x,host=s605202nj2sl267,username=XXXX,password=XXX,port=6200,charset=iso15,lang=us_english] OK ======== create table ======== create table test successful ======== insert rows ======== insert rows test successful ======== invalid op ======== invalid op returned 1 (should be nonzero). Error msg follows 'v6_test_nj2x: apr_dbd_test1 not found. Specify owner.objectname or use sp_help to check whether the object exists (sp_help may produce lots of output). Line 1 208: General SQL Server error: Check messages from the SQL Server' valid op returned 0 (should be zero; error shouldn't affect subsequent ops) invalid op test successful ======== select random ======== get_row failed: NO_MORE_ROWS (DBBUFFER option must be on for dbgetrow() to work) Error in select random: rc=-1 ======== select sequential ======== ROW 1: asdfgh bar 1 ROW 2: bar foo ROW 3: foo ROW 4: qwerty foo 0 ROW 5: wibble nothing 5 ROW 6: wibble other 5 select sequential test successful ======== transactions ======== Transaction 1 1 rows updated Valid insert returned 0. Should be nonzero (fail) because transaction is bad Transaction ended (should be rollback) - viewing table A column of "failed" indicates transaction failed (no rollback) ROW 1: asdfgh failed 1 ROW 2: bar failed ROW 3: foo failed ROW 4: qwerty failed 0 ROW 5: wibble failed 5 ROW 6: wibble failed 5 ROW 7: zzz aaa 3 Transaction 2 1 rows updated Valid insert returned 0. Should be zero (OK) Transaction ended (should be commit) - viewing table ROW 1: aaa zzz 3 ROW 2: asdfgh success 1 ROW 3: bar success ROW 4: foo success ROW 5: qwerty success 0 ROW 6: wibble success 5 ROW 7: wibble success 5 ROW 8: zzz success 3 transactions test successful ======== prepared select ======== Selecting rows where col3 <= 3 and bar row where it's unset. Should show four rows. ROW 1: qwerty success 0 ROW 2: asdfgh success 1 ROW 3: bar success ROW 4: zzz success 3 ROW 5: aaa zzz 3 prepared select test successful ======== prepared query ======== Showing table (should now contain row "prepared insert 2") ROW 1: aaa zzz 3 ROW 2: asdfgh success 1 ROW 3: bar success ROW 4: foo success ROW 5: prepared insert 2 ROW 6: qwerty success 0 ROW 7: wibble success 5 ROW 8: wibble success 5 ROW 9: zzz success 3 prepared query test successful ======== drop table ======== drop table test successful ==27683== ==27683== HEAP SUMMARY: ==27683== in use at exit: 320 bytes in 2 blocks ==27683== total heap usage: 514 allocs, 512 frees, 345,428 bytes allocated ==27683== ==27683== 32 bytes in 1 blocks are still reachable in loss record 1 of 2 ==27683== at 0x4A05430: calloc (vg_replace_malloc.c:418) ==27683== by 0x350180156A: _dlerror_run (in /lib64/libdl-2.5.so) ==27683== by 0x3501800F10: dlopen@@GLIBC_2.2.5 (in /lib64/libdl-2.5.so) ==27683== by 0x4E65339: apr_dso_load (dso.c:139) ==27683== by 0x4C2C442: apu_dso_load (apu_dso.c:164) ==27683== by 0x4C1C97B: apr_dbd_get_driver (apr_dbd.c:195) ==27683== by 0x401E57: main (dbd.c:359) ==27683== ==27683== 288 bytes in 1 blocks are still reachable in loss record 2 of 2 ==27683== at 0x4A0610C: malloc (vg_replace_malloc.c:195) ==27683== by 0x3500C10C4B: add_to_global (in /lib64/ld-2.5.so) ==27683== by 0x3500C110C8: dl_open_worker (in /lib64/ld-2.5.so) ==27683== by 0x3500C0D075: _dl_catch_error (in /lib64/ld-2.5.so) ==27683== by 0x3500C107EB: _dl_open (in /lib64/ld-2.5.so) ==27683== by 0x3501800F99: dlopen_doit (in /lib64/libdl-2.5.so) ==27683== by 0x3500C0D075: _dl_catch_error (in /lib64/ld-2.5.so) ==27683== by 0x350180150C: _dlerror_run (in /lib64/libdl-2.5.so) ==27683== by 0x3501800F10: dlopen@@GLIBC_2.2.5 (in /lib64/libdl-2.5.so) ==27683== by 0x4E65339: apr_dso_load (dso.c:139) ==27683== by 0x4C2C442: apu_dso_load (apu_dso.c:164) ==27683== by 0x4C1C97B: apr_dbd_get_driver (apr_dbd.c:195) ==27683== ==27683== LEAK SUMMARY: ==27683== definitely lost: 0 bytes in 0 blocks ==27683== indirectly lost: 0 bytes in 0 blocks ==27683== possibly lost: 0 bytes in 0 blocks ==27683== still reachable: 320 bytes in 2 blocks ==27683== suppressed: 0 bytes in 0 blocks ==27683== ==27683== For counts of detected and suppressed errors, rerun with: -v ==27683== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 35 from 7)
*** Bug 53676 has been marked as a duplicate of this bug. ***
Created attachment 29807 [details] Fix/improve the FreeTDS-driver Additional improvements/fixes for items discovered during ongoing use of the driver.
I can believe that it's broken, especially in the parts marked as unimplemented. But why does your patch remove all the untainting code? Can you explain, for example, how a user of mod_authn_dbd executes the standard user lookup query without opening the server to all kinds of SQL injection attack?
(In reply to comment #7) > I can believe that it's broken, especially in the parts marked as > unimplemented. It is broken beyond belief. The global/driver API has changed since this driver was written and it was never brought up to date. The changes made to it allowed it to compile, but not work. Try it, if you dare :-) > But why does your patch remove all the untainting code? No other driver is doing it, is one reason. It is also expensive (applying a regexp for each query) -- and not necessary, see below. But most importantly, because it may reject valid (and useful code): depending on application -- apr_dbd stuff can be used for purposes other than a read-only lookup from inside httpd. > Can you explain, for example, how a user of mod_authn_dbd executes the > standard user lookup query without opening the server to all kinds of > SQL injection attack? By setting up -- and using -- a special database account whose access-permissions only allow it to SELECT from certain tables or, better yet, to only EXEC certain stored procedures. This is the only method guaranteed to work anyway...
FYI: The patch is now part of the FreeBSD port devel/apr1 My own apr-dbd using application now works on FreeBSD in addition to Linux.
Created attachment 29992 [details] The same patch but against APR Util 1.5.1