Bug #75614 for DBI: dbih_imp_sv use after free in global destruction

Wed Mar 07 17:54:46 2012 RURBAN [...] cpan.org - Ticket created

Subject:

dbih_imp_sv use after free in global destruction

asan still reports this (see below) with 1.616 I fixed it with: --- ./DBI.xs~ 2010-12-21 16:59:27.000000000 -0600 +++ ./DBI.xs 2011-12-08 14:16:45.282057663 -0600 @@ -1331,7 +1331,6 @@ /* Use DBI magic on inner handle to carry handle attributes */ sv_magic(SvRV(h), dbih_imp_sv, DBI_MAGIC, Nullch, 0); - SvREFCNT_dec(dbih_imp_sv); /* since sv_magic() incremented it */ SvRMAGICAL_on(SvRV(h)); /* so DBI magic gets sv_clear'd ok */ DBI_SET_LAST_HANDLE(h); But this not work anymore. I suggest to check for the status of ${^GLOBAL_PHASE} in global destruction. gdb --args /usr/local/bin/perl5.15.8d-nt-asan -Mblib t/11fetch.t ================================================================= ==28536== ERROR: AddressSanitizer heap-use-after-free on address 0x7f22abf87084 at pc 0x7f22aaa169a4 bp 0x7fff4808af70 sp 0x7fff4808af68 READ of size 4 at 0x7f22abf87084 thread T0 #2 0x00007ffff29119a4 in XS_DBI_dispatch (cv=<value optimized Show quoted text

out>) at DBI.xs:3387

#3 0x00007ffff708dc52 in Perl_pp_entersub () at pp_hot.c:2778 #4 0x00007ffff67ae38d in Perl_call_sv (sv=0x7ffff4d47ec0, flags=45) at perl.c:2699 #5 0x00007ffff72354f0 in S_curse (sv=<value optimized out>, check_refcnt=Unhandled dwarf expression opcode 0x0) at sv.c:6377 #6 0x00007ffff7222b9f in Perl_sv_clear (orig_sv=<value optimized Show quoted text

out>) at sv.c:6042

#7 0x00007ffff70b24c5 in Perl_sv_free2 (sv=<value optimized out>) at sv.c:6509 #8 0x00007ffff70a7e49 in do_clean_objs (ref=<value optimized out>) at sv.c:478 #9 0x00007ffff70a57b3 in S_visit (f=<value optimized out>, flags= <value optimized out>, mask=<value optimized out>) at sv.c:420 #10 0x00007ffff70a6044 in Perl_sv_clean_objs () at sv.c:577 #11 0x00007ffff677facd in perl_destruct (my_perl=<value optimized Show quoted text

out>) at perl.c:776

#12 0x000000000040483a in main (argc=<value optimized out>, argv= <value optimized out>, env=<value optimized out>) at perlmain.c:131 0x7f22abf87084 is located 4 bytes inside of 200-byte region [0x7f22abf87080,0x7f22abf87148) freed by thread T0 here: previously allocated by thread T0 here: ==28536== ABORTING Stats: 9M malloced (31M for red zones) by 140521 calls Stats: 0M realloced by 13194 calls Stats: 4M freed by 81460 calls Stats: 0M really freed by 0 calls Stats: 76M (19463 full pages) mmaped in 19 calls mmaps by size class: 8:147447; 9:8191; 10:4095; 11:2047; 12:1024; 13:512; 14:256; 15:128; 16:64; 17:32; 18:16; mallocs by size class: 8:138839; 9:642; 10:282; 11:81; 12:123; 13:456; 14:81; 15:3; 16:8; 17:5; 18:1; frees by size class: 8:80811; 9:307; 10:136; 11:30; 12:18; 13:70; 14:76; 15:3; 16:7; 17:2; rfrees by size class: Stats: malloc large: 6 small slow: 329 Shadow byte and word: 0x1fe4557f0e10: fd 0x1fe4557f0e10: fd fd fd fd fd fd fd fd More shadow bytes: 0x1fe4557f0df0: fa fa fa fa fa fa fa fa 0x1fe4557f0df8: fa fa fa fa fa fa fa fa 0x1fe4557f0e00: fa fa fa fa fa fa fa fa 0x1fe4557f0e08: fa fa fa fa fa fa fa fa =>0x1fe4557f0e10: fd fd fd fd fd fd fd fd 0x1fe4557f0e18: fd fd fd fd fd fd fd fd 0x1fe4557f0e20: fd fd fd fd fd fd fd fd 0x1fe4557f0e28: fd fd fd fd fd fd fd fd 0x1fe4557f0e30: fa fa fa fa fa fa fa fa how to repro: http://blogs.perl.org/users/rurban/2012/03/address-sanitizer-round- 2.html -- Reini Urban

Wed Mar 07 17:57:40 2012 RURBAN [...] cpan.org - Correspondence added

FYI: Test Summary Report ------------------- t/11fetch.t (Wstat: 256 Tests: 24 Failed: 0) Non-zero exit status: 1 t/12quote.t (Wstat: 256 Tests: 10 Failed: 0) Non-zero exit status: 1 t/14utf8.t (Wstat: 256 Tests: 16 Failed: 0) Non-zero exit status: 1 t/zvg_11fetch.t (Wstat: 256 Tests: 24 Failed: 0) Non-zero exit status: 1 t/zvg_12quote.t (Wstat: 256 Tests: 10 Failed: 0) Non-zero exit status: 1 t/zvg_14utf8.t (Wstat: 256 Tests: 16 Failed: 0) Non-zero exit status: 1 -- Reini Urban

Wed Mar 07 17:57:41 2012 RURBAN [...] cpan.org - Status changed from 'new' to 'open'

Mon Apr 23 10:54:02 2012 RURBAN [...] cpan.org - Correspondence added

DBI-1.618_902 improved the situation a bit (tested with 5.15.9 non- threaded). Remaining failures: t/12quote.t, t/zvg_11fetch.t, t/zvg_12quote.t, t/zvg_14utf8.t, Failed 4/182 test programs. 0/9414 subtests failed. t/zvg_11fetch.t e.g. at test 25 #2 0x00007ffff288976f in XS_DBI_dispatch (cv) at DBI.xs:3426 3426 is_nested_call = ( call_depth > 1 || (DBIc_PARENT_COM(imp_xxh) && (DBIc_CALL_DEPTH(DBIc_PARENT_COM(imp_xxh)) Show quoted text

>= 1)) );

On Wed Mar 07 17:57:40 2012, RURBAN wrote: Show quoted text

> FYI: > Test Summary Report > ------------------- > t/11fetch.t (Wstat: 256 Tests: 24 Failed: 0) > Non-zero exit status: 1 > t/12quote.t (Wstat: 256 Tests: 10 Failed: 0) > Non-zero exit status: 1 > t/14utf8.t (Wstat: 256 Tests: 16 Failed: 0) > Non-zero exit status: 1 > t/zvg_11fetch.t (Wstat: 256 Tests: 24 Failed: 0) > Non-zero exit status: 1 > t/zvg_12quote.t (Wstat: 256 Tests: 10 Failed: 0) > Non-zero exit status: 1 > t/zvg_14utf8.t (Wstat: 256 Tests: 16 Failed: 0) > Non-zero exit status: 1 >

-- Reini Urban

Thu May 24 06:00:28 2012 TIMB [...] cpan.org - Severity Normal changed to Unimportant

Wed Jun 06 14:32:36 2012 RURBAN [...] cpan.org - Correspondence added

Still fails with 1.622 Also repro with MALLOC_PERTURB_ See http://www.fortran- 2000.com/ArnaudRecipes/CompilerTricks.html#Glibc_RTC

Mon Jul 09 14:15:47 2012 RURBAN [...] cpan.org - Correspondence added 120 min

Attached patch fixes the use-after-free during global destruction. Note that I consider the issue security relevant. The imp_xxh handle is totally unprotected. It can be anything, it is not asserted for consistency for the two usecases, and under memory pressure you can abuse it. -- Reini Urban

Subject:

rt75614-use-after-free-global-destruction.patch

Index: t/12quote.t =================================================================== --- t/12quote.t (revision 15342) +++ t/12quote.t (working copy) @@ -13,6 +13,7 @@ $| = 1; my $dbh = DBI->connect('dbi:ExampleP:', '', ''); +# $dbh->debug(4); sub check_quote { # checking quote Index: dbixs_rev.h =================================================================== --- dbixs_rev.h (revision 15342) +++ dbixs_rev.h (working copy) @@ -1,4 +1,3 @@ -/* Wed Apr 18 12:37:44 2012 */ -/* Mixed revision working copy (15267M:15268) */ +/* Mon Jul 9 12:45:07 2012 */ /* Code modified since last checkin */ -#define DBIXS_REVISION 15267 +#define DBIXS_REVISION 15342 Index: DBI.xs =================================================================== --- DBI.xs (revision 15342) +++ DBI.xs (working copy) @@ -1497,7 +1497,8 @@ /* also store a direct pointer to imp, aka PVX(dbih_imp_sv), */ /* in mg_ptr (with mg_len set to null, so it wont be freed) */ sv_magic(SvRV(h), dbih_imp_sv, DBI_MAGIC, (char*)imp, 0); - SvREFCNT_dec(dbih_imp_sv); /* since sv_magic() incremented it */ + if (SvREFCNT(dbih_imp_sv) > 1) + SvREFCNT_dec(dbih_imp_sv); /* if sv_magic() incremented it */ SvRMAGICAL_on(SvRV(h)); /* so DBI magic gets sv_clear'd ok */ { @@ -3336,7 +3337,8 @@ } if (ima_flags & IMA_KEEP_ERR) keep_error = TRUE; - if (ima_flags & IMA_KEEP_ERR_SUB + if ((ima_flags & IMA_KEEP_ERR_SUB) + && !PL_dirty && DBIc_PARENT_COM(imp_xxh) && DBIc_CALL_DEPTH(DBIc_PARENT_COM(imp_xxh)) > 0) keep_error = TRUE; if (ima_flags & IMA_CLEAR_STMT) { @@ -3410,6 +3412,7 @@ DBIc_ACTIVE_off(imp_xxh); } call_depth = 0; + is_nested_call = 0; } else { DBI_SET_LAST_HANDLE(h); @@ -3422,11 +3425,13 @@ /* XXX sv_copy() if Profiling? */ (void)hv_store((HV*)SvRV(parent), "Statement", 9, SvREFCNT_inc(tmp_sv), 0); } + is_nested_call = + (call_depth > 1 || + (!PL_dirty && /* not in global destruction [CPAN #75614] */ + DBIc_PARENT_COM(imp_xxh) && + DBIc_CALL_DEPTH(DBIc_PARENT_COM(imp_xxh))) >= 1); } - is_nested_call = ( call_depth > 1 || (DBIc_PARENT_COM(imp_xxh) && (DBIc_CALL_DEPTH(DBIc_PARENT_COM(imp_xxh)) >= 1)) ); - - /* --- dispatch --- */ if (!keep_error && meth_type != methtype_set_err) {

Thu Jul 12 12:21:21 2012 Tim.Bunce [...] pobox.com - Correspondence added

Subject:	Re: [rt.cpan.org #75614] dbih_imp_sv use after free in global destruction
Date:	Thu, 12 Jul 2012 17:20:52 +0100
To:	Reini Urban via RT <bug-DBI [...] rt.cpan.org>
From:	Tim Bunce <Tim.Bunce [...] pobox.com>

Thanks Reini. Can you tell me more about this hunk: + if (SvREFCNT(dbih_imp_sv) > 1) + SvREFCNT_dec(dbih_imp_sv); /* if sv_magic() incremented it */ Tim.

Thu Jul 12 12:36:32 2012 rurban [...] x-ray.at - Correspondence added

CC:	TIMB [...] cpan.org
Subject:	Re: [rt.cpan.org #75614] dbih_imp_sv use after free in global destruction
Date:	Thu, 12 Jul 2012 11:36:20 -0500
To:	bug-DBI [...] rt.cpan.org
From:	Reini Urban <rurban [...] x-ray.at>

On Thu, Jul 12, 2012 at 11:21 AM, Tim Bunce via RT <bug-DBI@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=75614 > > > Thanks Reini. > > Can you tell me more about this hunk: > > + if (SvREFCNT(dbih_imp_sv) > 1) > + SvREFCNT_dec(dbih_imp_sv); /* if sv_magic() incremented it */

I hoped you could explain it to me :) Somehow the refcounts went beyond or magic did not increment it properly, so I just protected it. I checked with leak tools and it seems to work. But if course it is lame. -- Reini Urban http://cpanel.net/ http://www.perl-compiler.org/

Tue Aug 14 08:26:10 2012 TIMB [...] cpan.org - Correspondence added

For the record: ---snip--- Show quoted text

> Take a typical perl webserver with a database. > Let DBI fetch a result and do a parallel request with a crafted > payload from user data. It can be big, up to max post size. It can be > stored in any string or HEK. > When dbih_imp_sv with the imp in PV is freed, and the second request > with the payload is allocated in the buffer which was just freed, the > next DBI request will use parts of the crafted payload, which can be > anything but not the imp you think it is. This might result in > crashes, if we are lucky, but might call to anything.

In https://rt.cpan.org/Ticket/Display.html?id=75614#txn-1070827 the line highlighted is: is_nested_call = ( call_depth > 1 || (DBIc_PARENT_COM(imp_xxh) && (DBIc_CALL_DEPTH(DBIc_PARENT_COM(imp_xxh)) >= 1)) ) I know that if the parent handle has already been freed then DBIc_PARENT_COM(imp_xxh) will point at memory that might have been reallocated. All the code above does is read an integer from that memory and check if it's >= 1. I don't see a security risk either in the reading or the effect on the value of is_nested_call. So I don't see how it "might call to anything". I'd be grateful if you could explain it to me. [I do plan to work on making the DBI destroy any child handles of a handle that's being DESTROY'd, so reading of possibly-reallocated memory during global destruction can be avoided.] Show quoted text

> I reported that long ago and could fix it for myself, until you > changed DBI in subsequent updates and I could not fix it anymore.

I doubt the removal of SvREFCNT_dec(dbih_imp_sv); you proposed in https://rt.cpan.org/Ticket/Display.html?id=75614#txn-1047494 was ever a correct fix. If I build 1.616 and apply that change I get many test failures (tested with perl 5.12.2). ---snip---

Tue Aug 14 08:26:12 2012 TIMB [...] cpan.org - Status changed from 'open' to 'stalled'

Mon Oct 01 22:16:33 2012 RURBAN [...] cpan.org - Correspondence added

dbih_imp_sv = newSVsv(imp_templ); imp = (imp_xxh_t*)(void*)SvPVX(dbih_imp_sv); dbih_imp_sv is subject to use-after-free at run-time, not only global destruction. It is triggered a few times in the testsuite, in the simpliest usecases. I showed that to you in December 2011 and it is still unfixed and marked as unimportant. I fail to see your simple integer argument. It is rather an opaque pointer to some DBH db handle or statement handle. Furthermore the type bit check for the handle in the switch in #70052 is not protected at all, so you can use any value for imp, not just the two cases checked.

Mon Oct 01 22:16:35 2012 The RT System itself - Status changed from 'stalled' to 'open'

Mon Nov 19 18:31:16 2012 TIMB [...] cpan.org - Correspondence added

I've uploaded DBI-1.622_921 to PAUSE. It includes a change to force destruction of children before parents. I'd be grateful if you could test it with asan. Thanks.

Mon Nov 19 18:31:17 2012 TIMB [...] cpan.org - Status changed from 'open' to 'patched'

Tue Nov 20 10:49:33 2012 rurban [...] x-ray.at - Correspondence added

CC:	RURBAN [...] cpan.org
Subject:	Re: [rt.cpan.org #75614] dbih_imp_sv use after free in global destruction
Date:	Tue, 20 Nov 2012 09:49:15 -0600
To:	bug-DBI [...] rt.cpan.org
From:	Reini Urban <rurban [...] x-ray.at>

Message body is not shown because it is too large.

Message body is not shown because sender requested not to inline it.

Tue Nov 20 10:49:34 2012 The RT System itself - Status changed from 'patched' to 'open'

Wed Jan 02 10:20:50 2013 RURBAN [...] cpan.org - Correspondence added

Message body is not shown because it is too large.

Wed Jan 02 17:17:26 2013 TIMB [...] cpan.org - Correspondence added

Not applied but not forgotten. I want to have a better understanding of what's actually happening and didn't have the time. I hope to get to it soon.

Thu Mar 28 08:33:26 2013 TIMB [...] cpan.org - Correspondence added

Patched in r15591. Basically the same as the original proposed patch except without the addition of "if (SvREFCNT(dbih_imp_sv) > 1)" before the SvREFCNT_dec since I couldn't see a good reason for it and it didn't seem to be needed.

Thu Mar 28 08:33:27 2013 TIMB [...] cpan.org - Status changed from 'open' to 'patched'

Thu Mar 28 09:25:07 2013 TIMB [...] cpan.org - Correspondence added

Actually there were a couple of followup changes, including one to give you credit in the Changes file. Thanks Reini.

Thu Mar 28 09:25:09 2013 The RT System itself - Status changed from 'patched' to 'open'

Thu Mar 28 14:51:45 2013 rurban [...] x-ray.at - Correspondence added

CC:	TIMB [...] cpan.org
Subject:	Re: [rt.cpan.org #75614] dbih_imp_sv use after free in global destruction
Date:	Thu, 28 Mar 2013 13:51:30 -0500
To:	bug-DBI [...] rt.cpan.org
From:	Reini Urban <rurban [...] x-ray.at>

On Thu, Mar 28, 2013 at 8:25 AM, Tim_Bunce via RT <bug-DBI@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=75614 > > > Actually there were a couple of followup changes, including one to give you credit in the Changes file. Thanks Reini.

I verified the fixes with svn r15593, thanks! Parts of my previous two patches are now obsolete. But I'd still like to have the folllowing change for RT #79952 in: Index: DBI.xs =================================================================== --- DBI.xs (revision 15593) +++ DBI.xs (working copy) @@ -1369,6 +1369,7 @@ imp_xxh_t *imp; imp_xxh_t *parent_imp; int trace_level; + int htype; h = dbih_inner(aTHX_ orv, "dbih_setup_handle"); parent = dbih_inner(aTHX_ parent, NULL); /* check parent valid (& inner) */ @@ -1478,7 +1479,8 @@ DBIc_LongReadLen(imp) = DBIc_LongReadLen_init; } - switch (DBIc_TYPE(imp)) { + htype = DBIc_TYPE(imp); + switch (htype) { case DBIt_DB: /* cache _inner_ handle, but also see quick_FETCH */ (void)hv_store((HV*)SvRV(h), "Driver", 6, newRV_inc(SvRV(parent)), 0); @@ -1492,10 +1494,15 @@ tmp_svp = hv_fetch((HV*)SvRV(h), "Statement", 9, 1); (void)hv_store((HV*)SvRV(parent), "Statement", 9, SvREFCNT_inc(*tmp_svp), 0); break; + case DBIt_DR: + case DBIt_FD: + break; + default: + die("Wrong DBIc_TYPE %d=%s", htype, dbih_htype_name(htype)); } } else - die("panic: invalid DBIc_TYPE"); + die("panic: invalid DBIc_TYPE %d", DBIc_TYPE(imp)); /* Use DBI magic on inner handle to carry handle attributes */ /* Note that we store the imp_sv in mg_obj, but as a shortcut, */ -- Reini Urban http://cpanel.net/ http://www.perl-compiler.org/

Thu Mar 28 18:13:30 2013 TIMB [...] cpan.org - Correspondence added

I don't see any value in that. Perhaps I'm missing something. Per my previous comment https://rt.cpan.org/Ticket/Display.html?id=79952#txn-1144455 DBIc_TYPE(imp) is an unsigned value. The code you're patching is within an "if (DBIc_TYPE(imp) <= DBIt_ST)" block. So the only other possible value within the switch() is DBIt_DB. I'm resolving this #75614 now as I've just released DBI-1.625.tar.gz Please don't reply, but reopen #79952 if you think there's a real need - but you'll have to make a good case for it.

Thu Mar 28 18:13:31 2013 TIMB [...] cpan.org - Status changed from 'open' to 'resolved'