Bug 53666 - The Sybase/FreeTDS driver is broken -- misparses the queries
Summary: The Sybase/FreeTDS driver is broken -- misparses the queries
Status: NEW
Alias: None
Product: APR
Classification: Unclassified
Component: APR-util (show other bugs)
Version: 1.4.1
Hardware: All All
: P2 critical (vote)
Target Milestone: ---
Assignee: Apache Portable Runtime bugs mailinglist
URL:
Keywords: PatchAvailable
: 53676 (view as bug list)
Depends on: 53687
Blocks:
  Show dependency tree
 
Reported: 2012-08-06 20:23 UTC by Mikhail T.
Modified: 2013-02-26 02:59 UTC (History)
0 users



Attachments
Fix/improve the FreeTDS-driver (15.03 KB, patch)
2012-08-09 21:16 UTC, Mikhail T.
Details | Diff
Fix/improve the FreeTDS-driver (15.35 KB, patch)
2013-01-02 19:25 UTC, Mikhail T.
Details | Diff
The same patch but against APR Util 1.5.1 (15.20 KB, patch)
2013-02-26 02:59 UTC, Mikhail T.
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail T. 2012-08-06 20:23:51 UTC
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 :-(
Comment 1 Mikhail T. 2012-08-06 20:57:25 UTC
FWIW, the same problem persists in APR/APU's current trunk...
Comment 2 Bojan Smojver 2012-08-08 01:26:28 UTC
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,
Comment 3 Mikhail T. 2012-08-08 14:31:17 UTC
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...
Comment 4 Mikhail T. 2012-08-09 21:16:52 UTC
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)
Comment 5 Mikhail T. 2012-08-10 16:02:38 UTC
*** Bug 53676 has been marked as a duplicate of this bug. ***
Comment 6 Mikhail T. 2013-01-02 19:25:17 UTC
Created attachment 29807 [details]
Fix/improve the FreeTDS-driver

Additional improvements/fixes for items discovered during ongoing use of the driver.
Comment 7 Nick Kew 2013-01-02 21:35:07 UTC
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?
Comment 8 Mikhail T. 2013-01-02 21:49:18 UTC
(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...
Comment 9 Mikhail T. 2013-01-22 18:55:40 UTC
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.
Comment 10 Mikhail T. 2013-02-26 02:59:46 UTC
Created attachment 29992 [details]
The same patch but against APR Util 1.5.1