Open p5pRT opened 7 years ago
p5pRT
commented
7 years ago Hello\,
I've attached the poc and the asan log. Tested on git version of perl.
Information about configuration:
Distributor ID: Ubuntu Description: Ubuntu 16.10 Release: 16.10 Codename: yakkety Arch: x86_64
Best Regards\, Marcin T.
p5pRT
commented
7 years ago p5pRT
commented
7 years ago ==16877==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62100006f1c8 at pc 0x0000009c22fe bp 0x7ffc46edd6d0 sp 0x7ffc46edd6c8 WRITE of size 8 at 0x62100006f1c8 thread T0
p5pRT
commented
7 years ago Hello\,
I've attached the poc and the asan log. Tested on git version of perl.
Information about configuration:
Distributor ID: Ubuntu Description: Ubuntu 16.10 Release: 16.10 Codename: yakkety Arch: x86_64
Best Regards\, Marcin T.
p5pRT
commented
7 years ago p5pRT
commented
7 years ago ==10957==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61d000015d38 at pc 0x000000a2676e bp 0x7ffcf7b25770 sp 0x7ffcf7b25768 WRITE of size 8 at 0x61d000015d38 thread T0 #0 0xa2676d in Perl_newSVpvn_flags /home/mtowalski/Fuzzing/Programs/perl-git/sv.c:9298:2 #1 0xa5e64a in Perl_sv_vcatpvfn_flags /home/mtowalski/Fuzzing/Programs/perl-git/sv.c:13014:20 #2 0xa4f22f in Perl_sv_catpvf /home/mtowalski/Fuzzing/Programs/perl-git/sv.c:10871:5 #3 0x86d553 in Perl_mess_sv /home/mtowalski/Fuzzing/Programs/perl-git/util.c:1436:17 #4 0x871bcf in Perl_vmess /home/mtowalski/Fuzzing/Programs/perl-git/util.c:1486:12 #5 0x871bcf in Perl_vwarn /home/mtowalski/Fuzzing/Programs/perl-git/util.c:1859 #6 0x8724fb in Perl_vwarner /home/mtowalski/Fuzzing/Programs/perl-git/util.c:1975:2 #7 0x872f64 in Perl_warner /home/mtowalski/Fuzzing/Programs/perl-git/util.c:1951:5 #8 0x7109e9 in S_pad_check_dup /home/mtowalski/Fuzzing/Programs/perl-git/pad.c:878:6 #9 0x7109e9 in Perl_pad_add_name_pvn /home/mtowalski/Fuzzing/Programs/perl-git/pad.c:601 #10 0x507ca0 in Perl_allocmy /home/mtowalski/Fuzzing/Programs/perl-git/op.c:676:11 #11 0x67210c in S_pending_ident /home/mtowalski/Fuzzing/Programs/perl-git/toke.c:8922:26 #12 0x67210c in Perl_yylex /home/mtowalski/Fuzzing/Programs/perl-git/toke.c:4811 #13 0x6fc45a in Perl_yyparse /home/mtowalski/Fuzzing/Programs/perl-git/perly.c:340:34 #14 0x5e61fd in S_parse_body /home/mtowalski/Fuzzing/Programs/perl-git/perl.c:2377:9 #15 0x5ddbae in perl_parse /home/mtowalski/Fuzzing/Programs/perl-git/perl.c:1692:2 #16 0x5031d1 in main /home/mtowalski/Fuzzing/Programs/perl-git/perlmain.c:121:18
p5pRT
commented
7 years ago Just updating subject to the function from the stacktrace.
p5pRT
commented
7 years ago On Wed\, 22 Feb 2017 13:46:26 -0800\, mtowalski@pentest.net.pl wrote:
Hello\,
I've attached the poc and the asan log. Tested on git version of perl.
Hi Marcin\, it'd be useful to know how you built perl: I can't reproduce based on my default ASAN build. Please could you supply the output of 'perl -V' for the perl in question?
Also\, it looks as if some of your examples may have been generated with a fuzzer. If so\, and that fuzzer provides a minimizer (such as afl-cmin in the case of afl)\, it's usually more helpful for us to have minimized test cases. I strongly recommend setting PERL_HASH_SEED when fuzzing or minimizing\, to avoid confusing the tools with random behaviour: see perlrun(1) for details.
Thanks\,
Hugo
p5pRT
commented
7 years ago The RT System itself - Status changed from 'new' to 'open'
p5pRT
commented
7 years ago On Wed\, 22 Feb 2017 12:36:31 -0800\, mtowalski@pentest.net.pl wrote:
Hello\,
I've attached the poc and the asan log. Tested on git version of perl.
Information about configuration:
Distributor ID: Ubuntu Description: Ubuntu 16.10 Release: 16.10 Codename: yakkety Arch: x86_64
Best Regards\, Marcin T.
This looks like an allocation size overflow\, at the point of the crash:
(gdb) p PL_tmps_ix $1 = 537 (gdb) p PL_tmps_max $2 = 1306846015273836496
How do we get that ridiculous value:
(gdb) watch PL_tmps_max
Hardware watchpoint 1: PL_tmps_max
(gdb) condition 1 PL_tmps_max > 10000
(gdb) r
Starting program: /home/tony/dev/perl/git/perl/perl -Ilib ../130839.pl
\<\<\<\
Old value = 537 New value = 1306846015273836496 Perl_tmps_grow_p (ix=1306846015273836495) at scope.c:190 190 Renew(PL_tmps_stack\, PL_tmps_max\, SV*); (gdb) p extend_to $1 = 1306846015273836495 (gdb) bt #0 Perl_tmps_grow_p (ix=1306846015273836495) at scope.c:190 #1 0x00000000009e12d1 in Perl_pp_flop () at pp_ctl.c:1228 #2 0x0000000000749474 in Perl_runops_debug () at dump.c:2450 #3 0x000000000044499f in S_gen_constant_list (o=0x61d000020648) at op.c:4673 #4 0x000000000042e687 in Perl_list (o=0x61d000020648) at op.c:2299 #5 0x00000000004941dc in Perl_ck_entersub_args_list ( entersubop=0x61d000020588) at op.c:11531 #6 0x0000000000499a12 in Perl_ck_subr (o=0x61d000020588) at op.c:12122 #7 0x0000000000447bed in Perl_newUNOP (type=184\, flags=64\, first=0x61d000020600) at op.c:5093 #8 0x000000000060d784 in Perl_yyparse (gramtype=258) at perly.y:1136 #9 0x00000000004bcc19 in S_parse_body (env=0x0\, xsinit=0x41fdf3 \<xs_init>) at perl.c:2377 #10 0x00000000004b89fa in perl_parse (my_perl=0x60200000eff0\, xsinit=0x41fdf3 \<xs_init>\, argc=3\, argv=0x7fffffffe848\, env=0x0) at perl.c:1692 #11 0x000000000041fc74 in main (argc=3\, argv=0x7fffffffe848\, env=0x7fffffffe868) at perlmain.c:121 (gdb) p PL_tmps_max * sizeof(SV*) $2 = -7991975951518859648 (gdb)
Which should fail.
Tony
p5pRT
commented
7 years ago The RT System itself - Status changed from 'new' to 'open'
p5pRT
commented
7 years ago On Wed\, 22 Feb 2017 13:46:26 -0800\, mtowalski@pentest.net.pl wrote:
Hello\,
I've attached the poc and the asan log. Tested on git version of perl.
Information about configuration:
Distributor ID: Ubuntu Description: Ubuntu 16.10 Release: 16.10 Codename: yakkety Arch: x86_64
Best Regards\, Marcin T.
This fails on:
(gdb) up 7 #7 0x00000000008f18ea in Perl_newSVpvn_flags ( s=0x60400000d732 "../130841.pl"\, len=12\, flags=524288) at sv.c:9298 9298 PUSH_EXTEND_MORTAL__SV_C(sv);
and given the code has the same:
-1306846015273835958 .. 5
as 130839\, I suspect it's the same base cause.
Tony
p5pRT
commented
7 years ago On Wed\, 22 Feb 2017 16:41:18 -0800\, tonyc wrote:
On Wed\, 22 Feb 2017 13:46:26 -0800\, mtowalski@pentest.net.pl wrote:
Hello\,
I've attached the poc and the asan log. Tested on git version of perl.
Information about configuration:
Distributor ID: Ubuntu Description: Ubuntu 16.10 Release: 16.10 Codename: yakkety Arch: x86_64
Best Regards\, Marcin T.
This fails on:
(gdb) up 7 #7 0x00000000008f18ea in Perl_newSVpvn_flags ( s=0x60400000d732 "../130841.pl"\, len=12\, flags=524288) at sv.c:9298 9298 PUSH_EXTEND_MORTAL__SV_C(sv);
and given the code has the same:
-1306846015273835958 .. 5
as 130839\, I suspect it's the same base cause.
I think it is somehow triggered by the number of warnings. I was eventually able to reproduce\, and can reduce it to this:
% perl -Mbigint -e '$big = "0" + 2 ** 63 - 256; print qq{xx(0 .. $big); } . (qq[my \$z = 1; ] x 24)' | ./perl -Ilib -w
With 23 instead of 24 repetitions I get the expected "Out of memory during array extend at - line 1."; the precise count needed seems to be sensitive to other factors though.
Hugo
p5pRT
commented
7 years ago Hello\,
Iv’e compiled it like this : ./Configure -des -Dusedevel -DDEBUGGING -Dcc=clang -Doptimize=-O2 -Accflags="-fsanitize=address -fsanitize-coverage=edge" -Aldflags="-fsanitize=address -fsanitize-coverage=edge" -Alddlflags=-shared
Next round of fuzzing I will do with Yours suggestions.
Best Regards\, Marcin T.
On 23 Feb 2017\, at 01:09\, Hugo van der Sanden via RT \perl5\-security\-report@​perl\.org wrote:
On Wed\, 22 Feb 2017 13:46:26 -0800\, mtowalski@pentest.net.pl wrote:
Hello\,
I've attached the poc and the asan log. Tested on git version of perl.
Hi Marcin\, it'd be useful to know how you built perl: I can't reproduce based on my default ASAN build. Please could you supply the output of 'perl -V' for the perl in question?
Also\, it looks as if some of your examples may have been generated with a fuzzer. If so\, and that fuzzer provides a minimizer (such as afl-cmin in the case of afl)\, it's usually more helpful for us to have minimized test cases. I strongly recommend setting PERL_HASH_SEED when fuzzing or minimizing\, to avoid confusing the tools with random behaviour: see perlrun(1) for details.
Thanks\,
Hugo
p5pRT
commented
7 years ago Ah\, with the reduced version I can run it with miniperl. -Dl output pointed to corruption of the savestack\, which turned out to be overrun from the tmps stack (as Tony found in [perl #130839])\, which turns out to be as follows:
In pp_flop() we carefully check that n\, the number of items we want to push onto the tmps stack\, has not overflowed to something invalid. If it's ok\, we then call EXTEND_MORTAL(n). However that macro wants to increase the size to (PL_tmps_ix + n)\, and the macro does not protect against overflow from that further addition.
I tried the simplistic:
--- a/pp.h
+++ b/pp.h
@@ -513\,6 +513\,10 @@ Does not use C\
.. but that didn't fix it. I do think that's the place for it\, though\, even if we also want an additional explicit check in pp_flop to be able to give a better error message.
I'll merge this with 130839 now.
Hugo
p5pRT
commented
7 years ago On Thu\, 23 Feb 2017 04:29:11 -0800\, hv wrote:
[...] which turns out to be as follows:
In pp_flop() we carefully check that n\, the number of items we want to push onto the tmps stack\, has not overflowed to something invalid. If it's ok\, we then call EXTEND_MORTAL(n). However that macro wants to increase the size to (PL_tmps_ix + n)\, and the macro does not protect against overflow from that further addition.
It's not that\, though that may also be a problem.
In this case we're not actually overflowing in that addition\, but proceeding to hit the wraparound check in Renew()\, spotting the overflow and calling croak_memory_wrap(). However by that point we've already updated PL_tmps_max as if the Renew() were going to succeed\, and hilarity ensues.
I think the below is the immediate fix\, not sure if we need to change EXTEND_MORTAL as well\, and I'm not sure if the two manipulations of extend_to visible in the diff context also need extra checks.
And I can't tell right now if this is exploitable.
Hugo --- a/scope.c +++ b/scope.c @@ -186\,8 +186\,8 @@ Perl_tmps_grow_p(pTHX_ SSize_t ix) if (ix - PL_tmps_max \< 128) extend_to += (PL_tmps_max \< 512) ? 128 : 512; #endif + Renew(PL_tmps_stack\, extend_to + 1\, SV*); PL_tmps_max = extend_to + 1; - Renew(PL_tmps_stack\, PL_tmps_max\, SV*); return ix; }
p5pRT
commented
7 years ago On Thu\, 23 Feb 2017 06:31:34 -0800\, hv wrote:
In this case we're [...] proceeding to hit the wraparound check in Renew()\, spotting the overflow and calling croak_memory_wrap(). However by that point we've already updated PL_tmps_max as if the Renew() were going to succeed\, and hilarity ensues.
I did a quick crude audit (below) for similar cases by grepping for Renew and then attempting to check for each case whether the new length passed to Renew came from something that might still be visible after a longjmp. I found quite a few cases.
My current inclination is that it would be hard to rule out exploitability for the pp_flop case - at least\, I'm confident that there will be perfectly reasonable-looking code out there using tainted range-ends. Ruling it out for the general case will likely be harder still.
It's possible that many of these cannot wraparound (either absolutely or practically)\, but I didn't attempt to think about that for this pass - I'm not sure if wraparound is the only failure mode that could cause us to hit a croak path that then attempt to continue doing stuff. If it is\, then making that path more immediately fatal might be the easiest next step.
In any case\, I'm going to stop there and wait for some feedback from others.
Hugo
Perl_my_cxt_init stores PL_my_cxt_size #ifndef PERL_GLOBAL_STRUCT_PRIVATE util.c:5357: Renew(PL_my_cxt_list\, PL_my_cxt_size\, void *); #else util.c:5420: Renew(PL_my_cxt_list\, PL_my_cxt_size\, void *); util.c:5421: Renew(PL_my_cxt_keys\, PL_my_cxt_size\, const char *); #endif
Perl_reentrant_retry stores many fields in PL_reentrant_buffer (comes from/also in regen/reentr.pl) reentr.c:330: Renew(PL_reentrant_buffer->_hostent_buffer\, reentr.c:363: Renew(PL_reentrant_buffer->_grent_buffer\, reentr.c:398: Renew(PL_reentrant_buffer->_netent_buffer\, reentr.c:430: Renew(PL_reentrant_buffer->_pwent_buffer\, reentr.c:466: Renew(PL_reentrant_buffer->_protoent_buffer\, reentr.c:496: Renew(PL_reentrant_buffer->_servent_buffer\,
pp_entersub stores AvMAX(av) pp_hot.c:4138: Renew(ary\, items\, SV*);
PerlIO_list_push stores list->len perlio.c:552: Renew(list->array\, list->len\, PerlIO_pair_t);
S_sortcv_stacked sets AvMAX(av) = 1 pp_sort.c:1835: Renew(ary\,2\,SV*);
yylex: I think these are never stored\, probably ok (but churny) toke.c:4801: Renew(PL_lex_brackstack\, PL_lex_brackets + 10\, char); toke.c:4877: Renew(PL_lex_casestack\, PL_lex_casemods + 2\, char); toke.c:5806: Renew(PL_lex_brackstack\, PL_lex_brackets + 10\, char); toke.c:6052: Renew(PL_lex_brackstack\, PL_lex_brackets + 10\, char); toke.c:11982: Renew(PL_lex_brackstack\, PL_lex_brackets + 10\, char);
Perl_cxinc stores cxstack_max scope.c:94: Renew(cxstack\, cxstack_max + 1\, PERL_CONTEXT); Perl_push_scope stores PL_scopestack_max scope.c:106: Renew(PL_scopestack\, PL_scopestack_max\, I32); scope.c:108: Renew(PL_scopestack_name\, PL_scopestack_max\, const char*); Perl_savestack_grow stores PL_savestack_max scope.c:150: Renew(PL_savestack\, PL_savestack_max + SS_MAXPUSH\, ANY); Perl_savestack_grow_cnt stores PL_savestack_max scope.c:159: Renew(PL_savestack\, PL_savestack_max + SS_MAXPUSH\, ANY); Perl_tmps_grow_p stores PL_tmps_max scope.c:189: Renew(PL_tmps_stack\, extend_to + 1\, SV*);
macro TRIE_LIST_PUSH stores TRIE_LIST_LEN( state ) regcomp.c:2359: Renew( trie->states[ state ].trans.list\, ging\, reg_trie_trans_le ); S_concat_pat stores pRExC_state->code_blocks->count regcomp.c:6420: Renew(pRExC_state->code_blocks->cb\,
Perl_hv_ename_add stores aux->xhv_name_count hv.c:2427: Renew(aux->xhv_name_u.xhvnameu_names\, count + 1\, HEK *);
macro DEFER_OP stores defer_stack_alloc\, but that's local (in multiple functions)\, probably ok op.c:184: Renew(defer_stack\, defer_stack_alloc\, OP *);
pthread_startit stores thread_join_count os2/os2.c:360: Renew(thread_join_data\, thread_join_count\, thread_join_t);
my_cxinc stores cxstack_max (dup of scope.c:Perl_cxinc) cpan/Scalar-List-Utils/ListUtil.xs:41: Renew(cxstack\, cxstack_max + 1\, struct context);
win32_readdir stores dirp->size win32/win32.c:972: Renew(dirp->start\, dirp->size\, char); win32/wince.c:769: Renew(dirp->start\, dirp->size\, char);
macro Renew_d_if_not_enough_to stores dlen\, but that's local (in multiple functions)\, probably ok dist/Unicode-Normalize/Normalize.xs:70: Renew(dstart\, dlen+1\, U8); \
p5pRT
commented
7 years ago On Fri\, Feb 24\, 2017 at 03:39:31AM -0800\, Hugo van der Sanden via RT wrote:
On Thu\, 23 Feb 2017 06:31:34 -0800\, hv wrote:
In this case we're [...] proceeding to hit the wraparound check in Renew()\, spotting the overflow and calling croak_memory_wrap(). However by that point we've already updated PL_tmps_max as if the Renew() were going to succeed\, and hilarity ensues.
I did a quick crude audit (below) for similar cases by grepping for Renew and then attempting to check for each case whether the new length passed to Renew came from something that might still be visible after a longjmp. I found quite a few cases.
My current inclination is that it would be hard to rule out exploitability for the pp_flop case - at least\, I'm confident that there will be perfectly reasonable-looking code out there using tainted range-ends. Ruling it out for the general case will likely be harder still.
For ones (such as pp_flop) that involve accidentally setting PL_tmps_max to a huge value\, I don't actually see it as much of a risk. Let me explain...
The sample code involves the constant-folding of (0..huge_const_value)\, which it does with an eval implicitly wrapped round it. Realistic code which allows the attacker to control the upper bound would have to be a run-time flop that happens to be wrapped in an eval or require\, e.g.
eval { my @range = 1..$attacker_controlled_huge_number; ... }
Note that code like this is already susceptible to a DoS attack\, since the attacker can make the perl process malloc as much memory as they want.
On return from the eval\, any attempt to push SVs on the tmps stack beyond what it's already been grown to will write SV pointers beyond the stack\, overwriting other stuff. There is a good chance however that the code has already reached the high-water mark in tmps stack allocation\, and never needs to grow further (crashing out from the eval/require block is likely to free up tmps stack slots).
Even if the code does write beyond the end of the tmps stack\, this is likely to just cause a crash (and remember such code is already vulnerable to a DoS attack anyway); an exploit can only overwrite with the addresses of recently allocated SVs rather than arbitrary code or data.
Overall this feels more like several other 32/64 or wraparound allocation issues we've had in the past\, where we've fixed it but haven't treated it as a security issue. The main exception to this was the (' ' x $large_int) issue\, which by tickling 32/64-bit issues present in both perl *and* glibc\, could have involved treating a random area of memory as the jump table of memset(). For that we released 5.14.4 (IIRC).
It's possible that many of these cannot wraparound (either absolutely or practically)\, but I didn't attempt to think about that for this pass - I'm not sure if wraparound is the only failure mode that could cause us to hit a croak path that then attempt to continue doing stuff. If it is\, then making that path more immediately fatal might be the easiest next step.
I haven't looked closely at your other Renew() examples yet.
In any case\, I'm going to stop there and wait for some feedback from others.
Regardless of what we end up deciding security-issue wise\, should I provide fixes or leave that to you? (I am happy to work on this). I think the fixes should go in 5.26.0.
-- Any [programming] language that doesn't occasionally surprise the novice will pay for it by continually surprising the expert. -- Larry Wall
p5pRT
commented
7 years ago On Tue\, 28 Feb 2017 06:34:57 -0800\, davem wrote:
In any case\, I'm going to stop there and wait for some feedback from others.
Regardless of what we end up deciding security-issue wise\, should I provide fixes or leave that to you? (I am happy to work on this). I think the fixes should go in 5.26.0.
I did start working a bit more on it today\, covering all the Renew() cases annotated in my previous message except for hv.c:Perl_hv_ename_add()\, which needs a bit more effort.
Attached is the first pass at those\, I need to go back through to make sure I haven't messed up (but it passes a basic 'make test').
I've made the assumption that retaining existing "catchable die" semantics is necessary; it's open for debate whether that's correct.
I think it's also worth discussing whether we should go further. Most of these pathways involve additional calculations that can wrap\, without taking appropriate steps. I'd have loved to provide some new macros that could have moved more of the logic into a separate place (eg a "Renew and update my size in the right order" macro)\, but sadly I found so little regularity there seemed little I could do there.
I'd love to at least standardize on types\, and provide for all those a minimum set of functions such as memsafe_add_size_t\, memsafe_mul_ssize_t\, etc. (I had a think about type-agnostic macros\, but I believe they'd need to know MAX_FOR_MY_TYPE and signedness\, and could see no way to provide those in the general case.)
I'll look more at your analysis; I already answered my first two objections while writing this. :)
Hugo
p5pRT
commented
7 years ago p5pRT
commented
7 years ago p5pRT
commented
7 years ago p5pRT
commented
7 years ago p5pRT
commented
7 years ago p5pRT
commented
7 years ago On Tue\, 28 Feb 2017 10:01:47 -0800\, hv wrote:
I did start working a bit more on it today\, covering all the Renew() cases annotated in my previous message except for hv.c:Perl_hv_ename_add()\, which needs a bit more effort.
Attached that additional piece\, as well as a fixup for porting/customized.
Hugo
p5pRT
commented
7 years ago p5pRT
commented
7 years ago p5pRT
commented
7 years ago Hello\,
I've attached the poc and the asan log. Tested on git version of perl.
Configure options:
“./Configure -des -Dusedevel -DDEBUGGING -Dcc=clang -Doptimize=-O2 -Accflags="-fsanitize=address -fsanitize-coverage=edge" -Aldflags="-fsanitize=address -fsanitize-coverage=edge" -Alddlflags=-shared"
Information about configuration:
Distributor ID: Ubuntu Description: Ubuntu 16.10 Release: 16.10 Codename: yakkety Arch: x86_64
Best Regards\, Marcin T.
p5pRT
commented
7 years ago p5pRT
commented
7 years ago ==11968==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x619000000980 at pc 0x0000008fff62 bp 0x7ffe7c6f9440 sp 0x7ffe7c6f9438 WRITE of size 8 at 0x619000000980 thread T0 #0 0x8fff61 in Perl_sv_2mortal /home/mtowalski/Fuzzing/Programs/perl-git/sv.c:9324:5 #1 0x6d4220 in S_missingterm /home/mtowalski/Fuzzing/Programs/perl-git/toke.c:596:10 #2 0x698655 in Perl_yylex /home/mtowalski/Fuzzing/Programs/perl-git/toke.c:6869:6 #3 0x6e834d in Perl_yyparse /home/mtowalski/Fuzzing/Programs/perl-git/perly.c:340:34 #4 0x5e3dc6 in S_parse_body /home/mtowalski/Fuzzing/Programs/perl-git/perl.c:2377:9 #5 0x5db300 in perl_parse /home/mtowalski/Fuzzing/Programs/perl-git/perl.c:1692:2 #6 0x5242ce in main /home/mtowalski/Fuzzing/Programs/perl-git/perlmain.c:121:18 #7 0x7fde8b6d53f0 in __libc_start_main /build/glibc-jxM2Ev/glibc-2.24/csu/../csu/libc-start.c:291 #8 0x4356f9 in _start (/home/mtowalski/Fuzzing/Programs/perl-git/perl+0x4356f9)
0x619000000980 is located 0 bytes to the right of 1024-byte region [0x619000000580\,0x619000000980) allocated by thread T0 here: #0 0x4eb0a8 in malloc (/home/mtowalski/Fuzzing/Programs/perl-git/perl+0x4eb0a8) #1 0x80087e in Perl_safesysmalloc /home/mtowalski/Fuzzing/Programs/perl-git/util.c:153:21 #2 0x7fde8b6d53f0 in __libc_start_main /build/glibc-jxM2Ev/glibc-2.24/csu/../csu/libc-start.c:291
SUMMARY: AddressSanitizer: heap-buffer-overflow /home/mtowalski/Fuzzing/Programs/perl-git/sv.c:9324:5 in Perl_sv_2mortal Shadow bytes around the buggy address: 0x0c327fff80e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c327fff80f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c327fff8100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c327fff8110: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c327fff8120: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x0c327fff8130:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c327fff8140: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c327fff8150: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c327fff8160: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c327fff8170: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c327fff8180: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==11968==ABORTING
p5pRT
commented
7 years ago On Wed\, Mar 01\, 2017 at 06:07:23AM -0800\, Hugo van der Sanden via RT wrote:
On Tue\, 28 Feb 2017 10:01:47 -0800\, hv wrote:
I did start working a bit more on it today\, covering all the Renew() cases annotated in my previous message except for hv.c:Perl_hv_ename_add()\, which needs a bit more effort.
Attached that additional piece\, as well as a fixup for porting/customized.
Attached is the first pass at those\, I need to go back through to make sure I ha ven't messed up (but it passes a basic 'make test').
My feeling about your patches is that they should all be applied now\, except for the following\, which should be left for 5.27.1 or whatever:
0001-OS2-update-size-after-Renew.patch
This is only dependent on the number of threads running\, which
will never even nearly approach wraparound. Also it affects a
platform we (presumably) where we can't compile or test.
0005-Scalar-List-Utils-update-size-after-Renew.patch
0001-Scalar-List-Utils-fixup.patch
This code path is only used in 5.6.x and earlier\, and would only
trigger a wrap croak if a sub had recursed so deeply that the
context stack had gradually grown to consume most of memory. But
all the local state for each sub (new lexical vars etc) would have
consumed all memory long before the context stack could wrap. So I
think this is just a case of providing it to the upstream
maintainer for completeness. This avoiding messign with a cpan/
module after code freeze.
0002-Perl_hv_ename_add-update-size-after-Renew.patch This is adding to a stash's list of effective names. I can't think that the list could ever get near wrapping point without having long since exhausted memory from allocating string buffers for the actual names themselves. Also\, the function itself is a bit tricksy\, using a negative count to indicate something or another\, which makes me slightly less than 100% certain the patch is correct (it almost certainly is\, but hey\, lets wait for 5.27.1?)
As for the others\, they all look good to me. Some parts of some of them could perhaps be argued as not being theoretically possible to exploit\, but I think they might as well all go in.
Can you provide any tests? Perhaps some fresh_perl_is()es that expect an eval to catch a 'wrap' croak\, followed by some code that allocates a lot of things that will overrun whatever stack has got a screwed up PL_foo_max var? I could have a go at this if you want.
I've made the assumption that retaining existing "catchable die" semantics is ne cessary; it's open for debate whether that's correct.
We generally seem to do a catchable croak() on memory wrap issues\, so my gut feeling is that we should continue to do so.
I think it's also worth discussing whether we should go further. Most of these p athways involve additional calculations that can wrap\, without taking appropriat e steps. I'd have loved to provide some new macros that could have moved more of the logic into a separate place (eg a "Renew and update my size in the right or der" macro)\, but sadly I found so little regularity there seemed little I could do there.
That sounds like something to look at further post 5.26.0. Ideally we could reach a situation where a bare Renew() is no longer directly used in the core\, and is deprecated for XS use.
I'll look more at your analysis; I already answered my first two objections whil e writing this. :)
My analysis may well have been wrong. (Stranger things have been known ;-).)
-- Justice is when you get what you deserve. Law is when you get what you pay for.
p5pRT
commented
7 years ago On Mon\, 06 Mar 2017 09:01:27 -0800\, davem wrote:
My feeling about your patches is that they should all be applied now\, except for the following\, which should be left for 5.27.1 or whatever:
0001-OS2-update-size-after-Renew.patch [...] 0005-Scalar-List-Utils-update-size-after-Renew.patch 0001-Scalar-List-Utils-fixup.patch [...] 0002-Perl_hv_ename_add-update-size-after-Renew.patch
I'm comfortable with that; I assume it needs some Pumpkin juice to ok at this point though. That'd be these 3: 0002-WIN32-update-size-after-Renew.patch 0003-reentr-update-size-after-Renew.patch 0004-update-size-after-Renew.patch
Did you also have a chance to think further about the other unchecked additions I noted\, in EXTEND_MORTAL (once) and Perl_tmps_grow_p (twice)? I need to go and look at those again myself as well\, and it's a class of bugs that similarly merits a full audit.
Can you provide any tests? Perhaps some fresh_perl_is()es that expect an eval to catch a 'wrap' croak\, followed by some code that allocates a lot of things that will overrun whatever stack has got a screwed up PL_foo_max var? I could have a go at this if you want.
Thanks\, I'd appreciate it if you could give that a go.
Hugo
p5pRT
commented
7 years ago On Mon\, Mar 06\, 2017 at 09:36:51AM -0800\, Hugo van der Sanden via RT wrote:
Did you also have a chance to think further about the other unchecked additions I noted\, in EXTEND_MORTAL (once) and Perl_tmps_grow_p (twice)? I need to go and look at those again myself as well\, and it's a class of bugs that similarly merits a full audit.
Do you mean where EXTEND_MORTAL does
eMiX = PL_tmps_ix + (n)
and Perl_tmps_grow_p does
extend_to += something; ... Renew(....\, extend_to + 1)
where both are in danger of wraparound?
I think they would be hard to fix for 5.26.0. Have a quick look at the various _MEM_WRAP_* macros in handy.h: these were mostly my work\, and took *many* iterations to
a) be secure; b) be efficient (especially making checks at compile-time where possible); c) not to endlessly emit compiler warnings like "comparison is always false due to limited range of data type" for every permutation of C and C++ compiler and 32\, 64 and 32/64 bit platforms.
So I think that should be left for 5.27.x. I suspect that it may require changing the interface of the (non-API) function Perl_tmps_grow_p()\, e.g. to pass n\, rather than (PL_tmps_ix+n) to it. Macros/funcs for growing other stacks probably also need some TLC. I've had on my todo list quite a while now an intention to generally overhaul perl's various allocation mechanisms\, especially over-allocation (where each layer of code adds its own "and a bit more for good measure" additional growth factor. This is especially bad for PVX buffers.
Can you provide any tests? Perhaps some fresh_perl_is()es that expect an eval to catch a 'wrap' croak\, followed by some code that allocates a lot of things that will overrun whatever stack has got a screwed up PL_foo_max var? I could have a go at this if you want.
Thanks\, I'd appreciate it if you could give that a go.
I'll do that now.
-- No matter how many dust sheets you use\, you will get paint on the carpet.
p5pRT
commented
7 years ago On Tue\, 07 Mar 2017 04:10:21 -0800\, davem wrote:
Do you mean where EXTEND_MORTAL does
eMiX = PL\_tmps\_ix \+ \(n\)and Perl_tmps_grow_p does
extend\_to \+= something; \.\.\. Renew\(\.\.\.\.\, extend\_to \+ 1\)where both are in danger of wraparound?
Yes.
I think they would be hard to fix for 5.26.0.
Ok\, that was pretty much the conclusion I was heading towards.
I'll [try to add some tests] now.
Thanks!
Hugo
p5pRT
commented
7 years ago This is the same issue as [perl #130841]\, presumably triggered by the (7 .. 2240639990569944149) range: applying the Perl_tmps_grow_p() fragment from that ticket is enough to avoid triggering the bad write.
I'll merge them.
Hugo
p5pRT
commented
7 years ago The RT System itself - Status changed from 'new' to 'open'
p5pRT
commented
7 years ago On Tue\, Mar 07\, 2017 at 05:18:23AM -0800\, Hugo van der Sanden via RT wrote:
I'll [try to add some tests] now.
I've written a test\, but it turns out that the only one which can be safely done in the test suite is the original 1..(~0 >> 1) which was the subject of this ticket: all the other ones require improbable conditions such as a win32 folder with billions of files in it\, which are not amenable to testing.
But when I applied your 000[234] patches to my system\, reentr.c failed to compile. I spotted one simple typo\, &PL_reentrant_buffer-_hostent_size missing the '>'\, and the RenewDouble macro was missing a 'STMT_END'\, but after fixing those\, it still failed to compile\, and I was left in macro expansion hell\, so I gave up. This was on a threaded build. Do you fancy fixing this?
-- Counsellor Troi states something other than the blindingly obvious. -- Things That Never Happen in "Star Trek" #16
p5pRT
commented
7 years ago On Tue\, 07 Mar 2017 08:04:14 -0800\, davem wrote:
But when I applied your 000[234] patches to my system\, reentr.c failed to compile.
Ugh\, it never occurred to me a non-threaded build wouldn't exercise that one.
It looks like the parens around (type) in RenewDouble were the problem. New version of that commit attached\, with all three fixes. This now at least passes minitest on a threaded build.
Hugo
p5pRT
commented
7 years ago p5pRT
commented
7 years ago On Tue\, Mar 07\, 2017 at 10:06:49AM -0800\, Hugo van der Sanden via RT wrote:
Ugh\, it never occurred to me a non-threaded build wouldn't exercise that one.
That's the perl core for you ;-)
This now at least passes minitest on a threaded build.
Thanks. Builds on my system now.
Attached is a test for this issue. I'd like to check it on my 32-bit system when I get home this evening\, before declaring it ok to apply. Then I suggest you apply your fixes to blead\, and you can apply my test too if you want.
-- Please note that ash-trays are provided for the use of smokers\, whereas the floor is provided for the use of all patrons. -- Bill Royston
p5pRT
commented
7 years ago p5pRT
commented
7 years ago On Wed\, 08 Mar 2017 00:41:11 -0800\, davem wrote:
That's the perl core for you ;-)
There's a win32/wince part as well\, is someone in a position to do at least a build test with those ("0002-WIN32-update-size-after-Renew.patch")? Alternatively if the timeline for release permits I could first push a smoke-me branch\, with the clean version of the 3 patches plus your test.
Attached is a test for this issue. I'd like to check it on my 32-bit system when I get home this evening\, before declaring it ok to apply. Then I suggest you apply your fixes to blead\, and you can apply my test too if you want.
Sawyer\, are you happy to ok this\, with or without a preceding smoke-me? We're talking about:
0001-add-range.t-test-for-RT-130841.patch 0002-WIN32-update-size-after-Renew.patch 0003b-reentr-update-size-after-Renew.patch 0004-update-size-after-Renew.patch
Hugo
p5pRT
commented
7 years ago On Wed\, Mar 08\, 2017 at 08:40:18AM +0000\, Dave Mitchell wrote:
Attached is a test for this issue. I'd like to check it on my 32-bit system when I get home this evening
Now passed on a 32-bit and 32-bit with -Duse64bitint system.
-- Justice is when you get what you deserve. Law is when you get what you pay for.
p5pRT
commented
7 years ago On Thu\, Mar 09\, 2017 at 10:16:34PM +0000\, Dave Mitchell wrote:
On Wed\, Mar 08\, 2017 at 08:40:18AM +0000\, Dave Mitchell wrote:
Attached is a test for this issue. I'd like to check it on my 32-bit system when I get home this evening
Now passed on a 32-bit and 32-bit with -Duse64bitint system.
I have now merged into blead the agreed subsut of Hugo's patch set\, plus my test.
-- "You may not work around any technical limitations in the software" -- Windows Vista license
p5pRT
commented
7 years ago On Wed\, 15 Mar 2017 02:28:46 -0700\, davem wrote:
On Thu\, Mar 09\, 2017 at 10:16:34PM +0000\, Dave Mitchell wrote:
On Wed\, Mar 08\, 2017 at 08:40:18AM +0000\, Dave Mitchell wrote:
Attached is a test for this issue. I'd like to check it on my 32-bit system when I get home this evening
Now passed on a 32-bit and 32-bit with -Duse64bitint system.
I have now merged into blead the agreed subsut of Hugo's patch set\, plus my test.
Do we consider any part of this as a security issue?
I'd tend to think the buffer write overflow was a security issue\, while large range limits below the overflow boundary would cause a DoS (either allocation failing\, or an eventual process kill on say Linux with overcommit enabled) as opposed to writing beyond the end of the allocated temps stack.
As Dave pointed out a mitigating factor is that the values written are addresses of SVs rather than arbritrary attacker supplied data.
Tony
p5pRT
commented
6 years ago On Wed\, 05 Jul 2017 19:07:09 -0700\, tonyc wrote:
On Wed\, 15 Mar 2017 02:28:46 -0700\, davem wrote:
On Thu\, Mar 09\, 2017 at 10:16:34PM +0000\, Dave Mitchell wrote:
On Wed\, Mar 08\, 2017 at 08:40:18AM +0000\, Dave Mitchell wrote:
Attached is a test for this issue. I'd like to check it on my 32- bit system when I get home this evening
Now passed on a 32-bit and 32-bit with -Duse64bitint system.
I have now merged into blead the agreed subsut of Hugo's patch set\, plus my test.
Do we consider any part of this as a security issue?
I'd tend to think the buffer write overflow was a security issue\, while large range limits below the overflow boundary would cause a DoS (either allocation failing\, or an eventual process kill on say Linux with overcommit enabled) as opposed to writing beyond the end of the allocated temps stack.
As Dave pointed out a mitigating factor is that the values written are addresses of SVs rather than arbritrary attacker supplied data.
Tony
So this is in blead\, and apparently not backported to other releases. What should be done?
-- Karl Williamson
p5pRT
commented
6 years ago On 6 February 2018 at 22:26\, Karl Williamson via RT \perl5\-security\-report\-followup@​perl\.org wrote:
On Wed\, 05 Jul 2017 19:07:09 -0700\, tonyc wrote:
On Wed\, 15 Mar 2017 02:28:46 -0700\, davem wrote:
On Thu\, Mar 09\, 2017 at 10:16:34PM +0000\, Dave Mitchell wrote:
On Wed\, Mar 08\, 2017 at 08:40:18AM +0000\, Dave Mitchell wrote:
Attached is a test for this issue. I'd like to check it on my 32- bit system when I get home this evening
Now passed on a 32-bit and 32-bit with -Duse64bitint system.
I have now merged into blead the agreed subsut of Hugo's patch set\, plus my test.
Do we consider any part of this as a security issue?
I'd tend to think the buffer write overflow was a security issue\, while large range limits below the overflow boundary would cause a DoS (either allocation failing\, or an eventual process kill on say Linux with overcommit enabled) as opposed to writing beyond the end of the allocated temps stack.
As Dave pointed out a mitigating factor is that the values written are addresses of SVs rather than arbritrary attacker supplied data.
Tony
So this is in blead\, and apparently not backported to other releases. What should be done?
I would much prefer we backport this.
p5pRT
commented
6 years ago On Wed\, Feb 07\, 2018 at 11:42:43AM +0200\, Sawyer X wrote:
On 6 February 2018 at 22:26\, Karl Williamson via RT \perl5\-security\-report\-followup@​perl\.org wrote:
On Wed\, 05 Jul 2017 19:07:09 -0700\, tonyc wrote:
On Wed\, 15 Mar 2017 02:28:46 -0700\, davem wrote:
On Thu\, Mar 09\, 2017 at 10:16:34PM +0000\, Dave Mitchell wrote:
On Wed\, Mar 08\, 2017 at 08:40:18AM +0000\, Dave Mitchell wrote:
Attached is a test for this issue. I'd like to check it on my 32- bit system when I get home this evening
Now passed on a 32-bit and 32-bit with -Duse64bitint system.
I have now merged into blead the agreed subsut of Hugo's patch set\, plus my test.
Do we consider any part of this as a security issue?
I'd tend to think the buffer write overflow was a security issue\, while large range limits below the overflow boundary would cause a DoS (either allocation failing\, or an eventual process kill on say Linux with overcommit enabled) as opposed to writing beyond the end of the allocated temps stack.
As Dave pointed out a mitigating factor is that the values written are addresses of SVs rather than arbritrary attacker supplied data.
Tony
So this is in blead\, and apparently not backported to other releases. What should be done?
I would much prefer we backport this.
Note that these fixes were applied as v5.25.10-74-g95d8e32b92\, so are already present in 5.26.x. The only issue whether to backport them to maint-5.24. We currently have perl-5.24.4-RC1 released\, with perl-5.24.4 ready to go now\, soon after which 5.24 slips out of maintenance.
Personally my gut felling is to leave maint.5.24 alone.
-- "There's something wrong with our bloody ships today\, Chatfield." -- Admiral Beatty at the Battle of Jutland\, 31st May 1916.
p5pRT
commented
6 years ago On 9 April 2018 at 19:40\, Dave Mitchell \davem@​iabyn\.com wrote:
On Wed\, Feb 07\, 2018 at 11:42:43AM +0200\, Sawyer X wrote:
On 6 February 2018 at 22:26\, Karl Williamson via RT \perl5\-security\-report\-followup@​perl\.org wrote:
On Wed\, 05 Jul 2017 19:07:09 -0700\, tonyc wrote:
On Wed\, 15 Mar 2017 02:28:46 -0700\, davem wrote:
On Thu\, Mar 09\, 2017 at 10:16:34PM +0000\, Dave Mitchell wrote:
On Wed\, Mar 08\, 2017 at 08:40:18AM +0000\, Dave Mitchell wrote:
Attached is a test for this issue. I'd like to check it on my 32- bit system when I get home this evening
Now passed on a 32-bit and 32-bit with -Duse64bitint system.
I have now merged into blead the agreed subsut of Hugo's patch set\, plus my test.
Do we consider any part of this as a security issue?
I'd tend to think the buffer write overflow was a security issue\, while large range limits below the overflow boundary would cause a DoS (either allocation failing\, or an eventual process kill on say Linux with overcommit enabled) as opposed to writing beyond the end of the allocated temps stack.
As Dave pointed out a mitigating factor is that the values written are addresses of SVs rather than arbritrary attacker supplied data.
Tony
So this is in blead\, and apparently not backported to other releases. What should be done?
I would much prefer we backport this.
Note that these fixes were applied as v5.25.10-74-g95d8e32b92\, so are already present in 5.26.x. The only issue whether to backport them to maint-5.24. We currently have perl-5.24.4-RC1 released\, with perl-5.24.4 ready to go now\, soon after which 5.24 slips out of maintenance.
Personally my gut felling is to leave maint.5.24 alone.
That makes sense.
p5pRT
commented
6 years ago So\, this is in the field in 5.26.3. Is there a reason not to close this ticket? -- Karl Williamson
Migrated from rt.perl.org#130841 (status was 'open')
Searchable as RT130841$