heap-use-after-free S_mro_gather_and_rename (mro_core.c:930)

p5pRT commented 8 years ago

Migrated from rt.perl.org#129861 (status was 'open')

Searchable as RT129861$

p5pRT commented 8 years ago

From @geeknik

Triggered in Perl v5.25.6 (v5.25.5-76-g91dca83) with AFL+ASAN.

==982==ERROR: AddressSanitizer: heap-use-after-free on address 0x60600000e420 at pc 0x00000085cff0 bp 0x7ffded1cb150 sp 0x7ffded1cb148 READ of size 8 at 0x60600000e420 thread T0 #0 0x85cfef in S_mro_gather_and_rename /root/perl-blead/mro_core.c:930:2 #1 0x85b4d4 in S_mro_gather_and_rename /root/perl-blead/mro_core.c:1186:4 #2 0x856bec in Perl_mro_package_moved /root/perl-blead/mro_core.c:851:5 #3 0x93196d in S_glob_assign_glob /root/perl-blead/sv.c:3981:6 #4 0x921fe2 in Perl_sv_setsv_flags /root/perl-blead/sv.c:4462:7 #5 0x8a219e in Perl_pp_sassign /root/perl-blead/pp_hot.c:226:5 #6 0x7f4493 in Perl_runops_debug /root/perl-blead/dump.c:2246:23 #7 0x5a1226 in S_run_body /root/perl-blead/perl.c:2526:2 #8 0x5a1226 in perl_run /root/perl-blead/perl.c:2449 #9 0x4de5cd in main /root/perl-blead/perlmain.c:123:9 #10 0x7ff37132bb44 in __libc_start_main /build/glibc-daoqzt/glibc-2.19/csu/libc-start.c:287 #11 0x4de23c in _start (/root/perl/perl+0x4de23c)

0x60600000e420 is located 0 bytes inside of 64-byte region [0x60600000e420\,0x60600000e460) freed by thread T0 here: #0 0x4c0eae in realloc (/root/perl/perl+0x4c0eae) #1 0x7f89c6 in Perl_safesysrealloc /root/perl-blead/util.c:274:18

previously allocated by thread T0 here: #0 0x4c0d10 in calloc (/root/perl/perl+0x4c0d10) #1 0x7f9311 in Perl_safesyscalloc /root/perl-blead/util.c:442:18 #2 0x875a35 in Perl_hv_common_key_len /root/perl-blead/hv.c:333:12 #3 0x856bec in Perl_mro_package_moved /root/perl-blead/mro_core.c:851:5

SUMMARY: AddressSanitizer: heap-use-after-free /root/perl-blead/mro_core.c:930 S_mro_gather_and_rename Shadow bytes around the buggy address: 0x0c0c7fff9c30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c0c7fff9c40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c0c7fff9c50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c0c7fff9c60: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00 0x0c0c7fff9c70: 00 00 00 00 fa fa fa fa 00 00 00 00 00 00 00 00 =>0x0c0c7fff9c80: fa fa fa fa[fd]fd fd fd fd fd fd fd fa fa fa fa 0x0c0c7fff9c90: fd fd fd fd fd fd fd fd fa fa fa fa fd fd fd fd 0x0c0c7fff9ca0: fd fd fd fd fa fa fa fa 00 00 00 00 00 00 00 00 0x0c0c7fff9cb0: fa fa fa fa fd fd fd fd fd fd fd fd fa fa fa fa 0x0c0c7fff9cc0: 00 00 00 00 00 00 00 06 fa fa fa fa 00 00 00 00 0x0c0c7fff9cd0: 00 00 06 fa fa fa fa fa 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 Heap right redzone: fb Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc ASan internal: fe ==982==ABORTING

Valgrind + Perl v5.25.6 (v5.25.5-76-g91dca83): Operator or semicolon missing before *sreEdV at test84 line 1. Ambiguous use of * resolved as operator * at test84 line 1. ==12501== Invalid read of size 8 ==12501== at 0x4EE82C: S_mro_gather_and_rename (mro_core.c:930) ==12501== by 0x4EF08C: S_mro_gather_and_rename (mro_core.c:1186) ==12501== by 0x4F0B7D: Perl_mro_package_moved (mro_core.c:851) ==12501== by 0x52EDB1: S_glob_assign_glob (sv.c:3981) ==12501== by 0x521F77: Perl_sv_setsv_flags (sv.c:4462) ==12501== by 0x5001B5: Perl_pp_sassign (pp_hot.c:226) ==12501== by 0x4D7131: Perl_runops_debug (dump.c:2246) ==12501== by 0x453146: S_run_body (perl.c:2526) ==12501== by 0x453146: perl_run (perl.c:2449) ==12501== by 0x421944: main (perlmain.c:123) ==12501== Address 0x5f854b0 is 32 bytes before a block of size 48 in arena "client" ==12501== ==12501== Invalid read of size 8 ==12501== at 0x4EE844: S_mro_gather_and_rename (mro_core.c:932) ==12501== by 0x4EF08C: S_mro_gather_and_rename (mro_core.c:1186) ==12501== by 0x4F0B7D: Perl_mro_package_moved (mro_core.c:851) ==12501== by 0x52EDB1: S_glob_assign_glob (sv.c:3981) ==12501== by 0x521F77: Perl_sv_setsv_flags (sv.c:4462) ==12501== by 0x5001B5: Perl_pp_sassign (pp_hot.c:226) ==12501== by 0x4D7131: Perl_runops_debug (dump.c:2246) ==12501== by 0x453146: S_run_body (perl.c:2526) ==12501== by 0x453146: perl_run (perl.c:2449) ==12501== by 0x421944: main (perlmain.c:123) ==12501== Address 0x98 is not stack'd\, malloc'd or (recently) free'd ==12501== ==12501== ==12501== Process terminating with default action of signal 11 (SIGSEGV) ==12501== Access not within mapped region at address 0x98 ==12501== at 0x4EE844: S_mro_gather_and_rename (mro_core.c:932) ==12501== by 0x4EF08C: S_mro_gather_and_rename (mro_core.c:1186) ==12501== by 0x4F0B7D: Perl_mro_package_moved (mro_core.c:851) ==12501== by 0x52EDB1: S_glob_assign_glob (sv.c:3981) ==12501== by 0x521F77: Perl_sv_setsv_flags (sv.c:4462) ==12501== by 0x5001B5: Perl_pp_sassign (pp_hot.c:226) ==12501== by 0x4D7131: Perl_runops_debug (dump.c:2246) ==12501== by 0x453146: S_run_body (perl.c:2526) ==12501== by 0x453146: perl_run (perl.c:2449) ==12501== by 0x421944: main (perlmain.c:123) ==12501== If you believe this happened as a result of a stack ==12501== overflow in your program's main thread (unlikely but ==12501== possible)\, you can try to increase the size of the ==12501== main thread stack using the --main-stacksize= flag. ==12501== The main thread stack size used in this run was 8388608. Segmentation fault

I ran afl-tmin and it came up with ./perl -e '%::::::=*::::::=::;%0=*0=*::::=::" which doesn't crash but results in perl: sv.c:4569: void Perl_sv_setsv_flags(SV *\, SV *\, const I32): Assertion `((svtype)((_gvgp)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((_gvgp)->sv_flags & 0xff)) == SVt_PVLV' failed. Aborted

p5pRT commented 8 years ago

From @geeknik

test84.gz

p5pRT commented 8 years ago

From @tonycoz

On Wed Oct 12 12:37:14 2016\, brian.carpenter@gmail.com wrote:

I ran afl-tmin and it came up with ./perl -e '%::::::=*::::::=::;%0=*0=*::::=::" which doesn't crash but results in perl: sv.c:4569: void Perl_sv_setsv_flags(SV *\, SV *\, const I32): Assertion `((svtype)((_gvgp)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((_gvgp)->sv_flags & 0xff)) == SVt_PVLV' failed. Aborted

This seems to be the simplified reproducer:

./perl -e '*::::::=*::xx; %::::::=*::; *::::=*::;'

At the point of failure SVf_OOK isn't set on oldstash.

Tony

p5pRT commented 8 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 7 years ago

From @iabyn

On Wed\, Oct 12\, 2016 at 09:14:18PM -0700\, Tony Cook via RT wrote:

On Wed Oct 12 12:37:14 2016\, brian.carpenter@gmail.com wrote:

I ran afl-tmin and it came up with ./perl -e '%::::::=*::::::=::;%0=*0=*::::=::" which doesn't crash but results in perl: sv.c:4569: void Perl_sv_setsv_flags(SV *\, SV *\, const I32): Assertion `((svtype)((_gvgp)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((_gvgp)->sv_flags & 0xff)) == SVt_PVLV' failed. Aborted

This seems to be the simplified reproducer:

./perl -e '*::::::=*::xx; %::::::=*::; *::::=*::;'

At the point of failure SVf_OOK isn't set on oldstash.

It can be simplified (or at least deobfuscated) further to this: (amongst other things renaming all the zero-length package names between the pairs of colons to 'p'\, 'q' etc):

%x = qw(foo bar); $q::p::{'p::'} = *x; *q::p:: = *q::;

(Or it may be a separate but related case - the reduction process took a lot of twisty paths).

The two basic issues here are that

a) line 2 in the code above sticks an ordinary glob (*x) containing an ordinary hash (%x) into the 'p::' slot of the q::p:: symbol table.

b) during the glob assignment in line 3\, the recursive 'fix up stashes and stuff' done by Perl_mro_package_moved() expects that any entries in stashes that have a name ending in '::' point to another stash.

Its not clear to me whether the fix is to

1) ensure that anything added to a stash which could be treated as a stash should be upgraded to a stash whenever it's inserted;

2) that Perl_mro_package_moved()/S_mro_gather_and_rename() should be fixed to cope with non-stashes masquerading as stashes (by simply skipping them).

I suspect that (1) is impossible\, so (2) would be the way to go\, but I'd be interested to hear any further opinions.

-- Hofstadter's Law: It always takes longer than you expect\, even when you take into account Hofstadter's Law.

p5pRT commented 7 years ago

From @iabyn

On Wed\, Feb 08\, 2017 at 04:48:17PM +0000\, Dave Mitchell wrote:

It can be simplified (or at least deobfuscated) further to this: (amongst other things renaming all the zero-length package names between the pairs of colons to 'p'\, 'q' etc):
%x = qw$foo bar$;
$q&#8203;::p&#8203;::\{'p&#8203;::'\} = \*x;
\*q&#8203;::p&#8203;::  = \*q&#8203;::;
(Or it may be a separate but related case - the reduction process took a lot of twisty paths).

The two basic issues here are that

a) line 2 in the code above sticks an ordinary glob (*x) containing an ordinary hash (%x) into the 'p::' slot of the q::p:: symbol table.

b) during the glob assignment in line 3\, the recursive 'fix up stashes and stuff' done by Perl_mro_package_moved() expects that any entries in stashes that have a name ending in '::' point to another stash.

Its not clear to me whether the fix is to

1) ensure that anything added to a stash which could be treated as a stash should be upgraded to a stash whenever it's inserted;

2) that Perl_mro_package_moved()/S_mro_gather_and_rename() should be fixed to cope with non-stashes masquerading as stashes (by simply skipping them).

I suspect that (1) is impossible\, so (2) would be the way to go\, but I'd be interested to hear any further opinions.

I also don't think its a security issue. If an attacker can already force perl to execute weird code that allows attacker-specified stash and typeglob aliasing\, then you've got bigger problems already (just think of any sub or package var suddenly moving to another package of your choice).

The bug is unlikely to be found in existing code\, as that would have crashed the first time it was run.

-- "I do not resent criticism\, even when\, for the sake of emphasis\, it parts for the time with reality". -- Winston Churchill\, House of Commons\, 22nd Jan 1941.

p5pRT commented 7 years ago

From @iabyn

On Fri\, Feb 10\, 2017 at 09:38:28AM +0000\, Dave Mitchell wrote:

On Wed\, Feb 08\, 2017 at 04:48:17PM +0000\, Dave Mitchell wrote:
It can be simplified (or at least deobfuscated) further to this: (amongst other things renaming all the zero-length package names between the pairs of colons to 'p'\, 'q' etc):
%x = qw$foo bar$;
$q&#8203;::p&#8203;::\{'p&#8203;::'\} = \*x;
\*q&#8203;::p&#8203;::  = \*q&#8203;::;
(Or it may be a separate but related case - the reduction process took a lot of twisty paths).

The two basic issues here are that

a) line 2 in the code above sticks an ordinary glob (*x) containing an ordinary hash (%x) into the 'p::' slot of the q::p:: symbol table.

b) during the glob assignment in line 3\, the recursive 'fix up stashes and stuff' done by Perl_mro_package_moved() expects that any entries in stashes that have a name ending in '::' point to another stash.

Its not clear to me whether the fix is to

1) ensure that anything added to a stash which could be treated as a stash should be upgraded to a stash whenever it's inserted;

2) that Perl_mro_package_moved()/S_mro_gather_and_rename() should be fixed to cope with non-stashes masquerading as stashes (by simply skipping them).

I suspect that (1) is impossible\, so (2) would be the way to go\, but I'd be interested to hear any further opinions.
I also don't think its a security issue. If an attacker can already force perl to execute weird code that allows attacker-specified stash and typeglob aliasing\, then you've got bigger problems already (just think of any sub or package var suddenly moving to another package of your choice).

The bug is unlikely to be found in existing code\, as that would have crashed the first time it was run.

I've move this ticket to the public queue in a few days time if no-one objects.

-- "There's something wrong with our bloody ships today\, Chatfield." -- Admiral Beatty at the Battle of Jutland\, 31st May 1916.

p5pRT commented 7 years ago

From @iabyn

On Tue\, Feb 21\, 2017 at 11:02:59AM +0000\, Dave Mitchell wrote:

On Fri\, Feb 10\, 2017 at 09:38:28AM +0000\, Dave Mitchell wrote:

On Wed\, Feb 08\, 2017 at 04:48:17PM +0000\, Dave Mitchell wrote: I also don't think its a security issue. If an attacker can already force perl to execute weird code that allows attacker-specified stash and typeglob aliasing\, then you've got bigger problems already (just think of any sub or package var suddenly moving to another package of your choice).

The bug is unlikely to be found in existing code\, as that would have crashed the first time it was run.

I've move this ticket to the public queue in a few days time if no-one objects.

which I am now doing.

-- The Enterprise is captured by a vastly superior alien intelligence which does not put them on trial. -- Things That Never Happen in "Star Trek" #10

Perl / perl5