Skip Menu |
Report information
Id: 131606
Status: open
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: tadinhsung [at] gmail.com
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: (no value)
Type: (no value)
Perl Version: (no value)
Fixed In: (no value)



To: perl5-security-report [...] perl.org
Subject: Unknow address reference in Perl_hv_common
Date: Tue, 20 Jun 2017 14:40:06 +0700
From: Ta Sung <tadinhsung [...] gmail.com>
Download (untitled) / with headers
text/plain 2.7k
Attached file below triggered crash because reference to unknow address in funtion Perl_hv_common.
Effect on perl version 5.27.1 and 5.26.0

Some info about this bug on gdb-peda:

[----------------------------------registers-----------------------------------]
RAX: 0x7fd7a8 --> 0x0 
RBX: 0xc0b948 --> 0x7fd7a8 --> 0x0 
RCX: 0x810540 --> 0x0 
RDX: 0x6e ('n')
RSI: 0x5 
RDI: 0x800c10 --> 0x737365666e ('nfess')
RBP: 0x0 
RSP: 0x7fffffffe120 --> 0xc78b50 ("#", ' ' <repeats 19 times>, "'Can't locate object method \"c\" via package \"Carp\" at (eval 7) line 3.\n# '\n#     doesn't match '(?^s:(?:(?^:__ANON__::)|SubHelper::).*(?:(?^:__ANON__::)|Sub::))'\n")
RIP: 0x4ae68e (<Perl_hv_common+494>:    mov    r13,QWORD PTR [r8+rcx*8])
R8 : 0x7fd7a8 --> 0x0 
R9 : 0x20 (' ')
R10: 0x20 (' ')
R11: 0x7ffff7240390 --> 0xfffda380fffda0af 
R12: 0x15f31d6c 
R13: 0x0 
R14: 0x0 
R15: 0x800c15 --> 0x211000000
EFLAGS: 0x10202 (carry parity adjust zero sign trap INTERRUPT direction overflow)
[-------------------------------------code-------------------------------------]
   0x4ae684 <Perl_hv_common+484>:       mov    ecx,r12d
   0x4ae687 <Perl_hv_common+487>:       mov    r8,QWORD PTR [rbx+0x10]
   0x4ae68b <Perl_hv_common+491>:       and    ecx,DWORD PTR [rax+0x18]
=> 0x4ae68e <Perl_hv_common+494>:       mov    r13,QWORD PTR [r8+rcx*8]
   0x4ae692 <Perl_hv_common+498>:       test   r13,r13
   0x4ae695 <Perl_hv_common+501>:       jne    0x4aedf0 <Perl_hv_common+2384>
   0x4ae69b <Perl_hv_common+507>:       mov    esi,DWORD PTR [rbx+0xc]
   0x4ae69e <Perl_hv_common+510>:       test   esi,0x8010000
[------------------------------------stack-------------------------------------]
0000| 0x7fffffffe120 --> 0xc78b50 ("#", ' ' <repeats 19 times>, "'Can't locate object method \"c\" via package \"Carp\" at (eval 7) line 3.\n# '\n#     doesn't match '(?^s:(?:(?^:__ANON__::)|SubHelper::).*(?:(?^:__ANON__::)|Sub::))'\n")
0008| 0x7fffffffe128 --> 0x20 (' ')
0016| 0x7fffffffe130 --> 0x800c10 --> 0x737365666e ('nfess')
0024| 0x7fffffffe138 --> 0x0 
0032| 0x7fffffffe140 --> 0x7fd7a8 --> 0x0 
0040| 0x7fffffffe148 --> 0x4900aa7e00000020 
0048| 0x7fffffffe150 --> 0x5 
0056| 0x7fffffffe158 --> 0x4969aa7e56186400 
[------------------------------------------------------------------------------] blue
Legend: code, data, rodata, value
Stopped reason: SIGSEGV
0x00000000004ae68e in Perl_hv_common ()
gdb-peda$ where
#0  0x00000000004ae68e in Perl_hv_common ()
#1  0x00000000004afd74 in Perl_hv_common_key_len ()
#2  0x0000000000445e1a in Perl_gv_fetchpvn_flags ()
#3  0x00000000004be163 in Perl_pp_method_redir ()
#4  0x00000000004b3373 in Perl_runops_standard ()
#5  0x00000000004436f1 in perl_run ()
#6  0x000000000041fe95 in main ()
#7  0x00007ffff70cc830 in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6
#8  0x000000000041fed9 in _start ()
gdb-peda$ q


--
Ta Dinh Sung,

 
Download poc
application/octet-stream 585b

Message body not shown because it is not plain text.

RT-Send-CC: rt-deliver-to-perl5-security-report [...] rt.perl.org
Download (untitled) / with headers
text/plain 3.4k
On Tue, 20 Jun 2017 00:41:01 -0700, tadinhsung@gmail.com wrote: Show quoted text
> Attached file below triggered crash because reference to unknow > address in > funtion Perl_hv_common. > Effect on perl version 5.27.1 and 5.26.0
Simplifies to: my $unknown_pat = qr/__ANON__::/; my $sub = sub { SubHelper::x(); }; delete ${'::'}{'SubHelper::'}; $@ =~ $unknown_pat; eval { $sub->() }; package SubHelper; sub x { Carp::c nfess("blah"); } On a debugging build: miniperl: hv.c:353: Perl_hv_common: Assertion `((svtype)((hv)->sv_flags & 0xff)) == SVt_PVHV' failed. Aborted The stash passed to hv_fetch is a regexp: miniperl: hv.c:360: Perl_hv_common: Assertion `((svtype)((hv)->sv_flags & 0xff)) == SVt_PVHV' failed. Program received signal SIGABRT, Aborted. 0x00007ffff6cf2067 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 56 ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory. (gdb) bt #0 0x00007ffff6cf2067 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x00007ffff6cf3448 in __GI_abort () at abort.c:89 #2 0x00007ffff6ceb266 in __assert_fail_base ( fmt=0x7ffff6e23f18 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7d8540 "((svtype)((hv)->sv_flags & 0xff)) == SVt_PVHV", file=file@entry=0x7d852b "hv.c", line=line@entry=360, function=function@entry=0x7da9b7 <__PRETTY_FUNCTION__.15100> "Perl_hv_common") at assert.c:92 #3 0x00007ffff6ceb312 in __GI___assert_fail ( assertion=0x7d8540 "((svtype)((hv)->sv_flags & 0xff)) == SVt_PVHV", file=0x7d852b "hv.c", line=360, function=0x7da9b7 <__PRETTY_FUNCTION__.15100> "Perl_hv_common") at assert.c:101 #4 0x000000000057f5f1 in Perl_hv_common (hv=0xaa1dd0, keysv=0x0, key=0xab1180 "nfess", klen=5, flags=0, action=32, val=0x0, hash=0) at hv.c:360 #5 0x000000000057f525 in Perl_hv_common_key_len (hv=0xaa1dd0, key=0xab1180 "nfess", klen_i32=5, action=32, val=0x0, hash=0) at hv.c:337 #6 0x00000000004684ee in Perl_gv_fetchpvn_flags (nambeg=0xab1180 "nfess", full_len=5, flags=0, sv_type=SVt_PVIO) at gv.c:2357 #7 0x00000000005adfd9 in S_opmethod_stash (meth=0xaafde8) at pp_hot.c:4444 #8 0x00000000005af400 in Perl_pp_method_redir () at pp_hot.c:4563 #9 0x0000000000547d82 in Perl_runops_debug () at dump.c:2451 #10 0x0000000000448d4a in S_run_body (oldscope=1) at perl.c:2548 #11 0x000000000044832d in perl_run (my_perl=0xa84010) at perl.c:2471 #12 0x00000000007365f9 in main (argc=2, argv=0x7fffffffe868, env=0x7fffffffe880) at miniperlmain.c:129 (gdb) up 6 #6 0x00000000004684ee in Perl_gv_fetchpvn_flags (nambeg=0xab1180 "nfess", full_len=5, flags=0, sv_type=SVt_PVIO) at gv.c:2357 2357 gvp = (GV**)hv_fetch(stash,name,is_utf8 ? -(I32)len : (I32)len,add); (gdb) call Perl_sv_dump(stash) SV = PV(0xa86d70) at 0xaa1dd0 REFCNT = 1 FLAGS = (POK,IsCOW,pPOK) PV = 0xaa2d40 "Regexp" CUR = 6 LEN = 0 Bisects to: commit bee7c5743fabd49c584c5edbb637c66c5c25ca9a Author: Father Chrysostomos <sprout@cpan.org> Date: Fri May 18 17:02:39 2012 -0700 sv.c: Don’t fiddle with AMAGIC in sv_bless Since overloading itself now checks whether caches are up to date, and since changes to the stash (@ISA, methods) turn the flag on and over- loading itself turns the flag off when it can, sv_bless no longer needs to deal with it at all. but I suspect this just a case of a freed SV being overwritten. I don't think this is a security issue. Tony
Date: Wed, 21 Jun 2017 13:06:05 +0700
From: Ta Sung <tadinhsung [...] gmail.com>
Subject: Re: [perl #131606] Unknow address reference in Perl_hv_common
To: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 6.1k
Sorry, i'm not really good on perl :( . But when i debug perl(v5.27.0-324-g3aa3d69) on gdb-peda with my poc ...
RAX: 0x6210002149c8 --> 0x0 
RBX: 0x5 
RCX: 0x20 (' ')
RDX: 0x6e ('n')
RSI: 0x5 
RDI: 0x42b40 
RBP: 0x62100010e838 --> 0x6210002149c8 --> 0x0 
RSP: 0x7fffffffe100 --> 0x62500017e900 ("#", ' ' <repeats 19 times>, "'Can't locate object method \"c\" via package \"Carp\" at (eval 7) line 3.\n# '\n#     doesn't match '(?^s:(?:(?^:__ANON__::)|SubHelper::).*(?:(?^:__ANON__::)|Sub::))'\n")
RIP: 0x4b03c7 (<Perl_hv_common+807>:    mov    rbx,QWORD PTR [r9+rdi*8])
R8 : 0x0 
R9 : 0x6210002149c8 --> 0x0 
R10: 0x6d376bd5 
R11: 0x7ffff6288790 --> 0xfffca500fffc9d40 
R12: 0x0 
R13: 0x0 
R14: 0x20 (' ')
R15: 0x0
EFLAGS: 0x10202 (carry parity adjust zero sign trap INTERRUPT direction overflow)
[-------------------------------------code-------------------------------------]
   0x4b03bd <Perl_hv_common+797>:       mov    edi,r10d
   0x4b03c0 <Perl_hv_common+800>:       mov    r9,QWORD PTR [rbp+0x10]
   0x4b03c4 <Perl_hv_common+804>:       and    edi,DWORD PTR [rax+0x18]
=> 0x4b03c7 <Perl_hv_common+807>:       mov    rbx,QWORD PTR [r9+rdi*8]
   0x4b03cb <Perl_hv_common+811>:       test   rbx,rbx
   0x4b03ce <Perl_hv_common+814>:       jne    0x4b0ba0 <Perl_hv_common+2816>
   0x4b03d4 <Perl_hv_common+820>:       mov    esi,DWORD PTR [rbp+0xc]
   0x4b03d7 <Perl_hv_common+823>:       test   esi,0x8010000
 
Obviously, at address 0x4b03c4 register edi was loaded from address [rax+0x18](mmap section) but this memory was overwrite.
If attacker can init value for section X(0x0000621000000000-0x0000621000220000) before,that mean we can control rbx because at this time register rax and r9 point to 0x6210002149c8  in section X, and we jump in 0x4b0ba0.
gdb-peda$ pdisass 0x4b0ba0
Dump of assembler code from 0x4b0ba0 to 0x4b0bc0::      Dump of assembler code from 0x4b0ba0 to 0x4b0bc0:
   0x00000000004b0ba0 <Perl_hv_common+2816>:    movzx  eax,r13b
   0x00000000004b0ba4 <Perl_hv_common+2820>:    mov    ecx,r10d
   0x00000000004b0ba7 <Perl_hv_common+2823>:    mov    DWORD PTR [rsp+0x28],eax
   0x00000000004b0bab <Perl_hv_common+2827>:    mov    rax,QWORD PTR [rsp+0x58]
   0x00000000004b0bb0 <Perl_hv_common+2832>:    mov    QWORD PTR [rsp+0x38],rbp
   0x00000000004b0bb5 <Perl_hv_common+2837>:    mov    rbp,rbx
   0x00000000004b0bb8 <Perl_hv_common+2840>:    mov    DWORD PTR [rsp+0x40],r13d

at address 0x4b0ba7 variable [rsp+0x28](orig_entry) can control, https://github.com/Perl/perl5/blob/blead/hv.c#L660.
 

2017-06-21 9:02 GMT+07:00 Tony Cook via RT <perl5-security-report@perl.org>:
Show quoted text
On Tue, 20 Jun 2017 00:41:01 -0700, tadinhsung@gmail.com wrote:
> Attached file below triggered crash because reference to unknow
> address in
> funtion Perl_hv_common.
> Effect on perl version 5.27.1 and 5.26.0

Simplifies to:

my $unknown_pat = qr/__ANON__::/;
my $sub = sub {
    SubHelper::x();
};
delete ${'::'}{'SubHelper::'};
$@ =~ $unknown_pat;
eval { $sub->() };

package SubHelper;
sub x {
    Carp::c     nfess("blah");
}

On a debugging build:

miniperl: hv.c:353: Perl_hv_common: Assertion `((svtype)((hv)->sv_flags & 0xff)) == SVt_PVHV' failed.
Aborted

The stash passed to hv_fetch is a regexp:

miniperl: hv.c:360: Perl_hv_common: Assertion `((svtype)((hv)->sv_flags & 0xff)) == SVt_PVHV' failed.

Program received signal SIGABRT, Aborted.
0x00007ffff6cf2067 in __GI_raise (sig=sig@entry=6)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56      ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x00007ffff6cf2067 in __GI_raise (sig=sig@entry=6)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007ffff6cf3448 in __GI_abort () at abort.c:89
#2  0x00007ffff6ceb266 in __assert_fail_base (
    fmt=0x7ffff6e23f18 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
    assertion=assertion@entry=0x7d8540 "((svtype)((hv)->sv_flags & 0xff)) == SVt_PVHV", file=file@entry=0x7d852b "hv.c", line=line@entry=360,
    function=function@entry=0x7da9b7 <__PRETTY_FUNCTION__.15100> "Perl_hv_common") at assert.c:92
#3  0x00007ffff6ceb312 in __GI___assert_fail (
    assertion=0x7d8540 "((svtype)((hv)->sv_flags & 0xff)) == SVt_PVHV",
    file=0x7d852b "hv.c", line=360,
    function=0x7da9b7 <__PRETTY_FUNCTION__.15100> "Perl_hv_common")
    at assert.c:101
#4  0x000000000057f5f1 in Perl_hv_common (hv=0xaa1dd0, keysv=0x0,
    key=0xab1180 "nfess", klen=5, flags=0, action=32, val=0x0, hash=0)
    at hv.c:360
#5  0x000000000057f525 in Perl_hv_common_key_len (hv=0xaa1dd0,
    key=0xab1180 "nfess", klen_i32=5, action=32, val=0x0, hash=0) at hv.c:337
#6  0x00000000004684ee in Perl_gv_fetchpvn_flags (nambeg=0xab1180 "nfess",
    full_len=5, flags=0, sv_type=SVt_PVIO) at gv.c:2357
#7  0x00000000005adfd9 in S_opmethod_stash (meth=0xaafde8) at pp_hot.c:4444
#8  0x00000000005af400 in Perl_pp_method_redir () at pp_hot.c:4563
#9  0x0000000000547d82 in Perl_runops_debug () at dump.c:2451
#10 0x0000000000448d4a in S_run_body (oldscope=1) at perl.c:2548
#11 0x000000000044832d in perl_run (my_perl=0xa84010) at perl.c:2471
#12 0x00000000007365f9 in main (argc=2, argv=0x7fffffffe868,
    env=0x7fffffffe880) at miniperlmain.c:129
(gdb) up 6
#6  0x00000000004684ee in Perl_gv_fetchpvn_flags (nambeg=0xab1180 "nfess",
    full_len=5, flags=0, sv_type=SVt_PVIO) at gv.c:2357
2357        gvp = (GV**)hv_fetch(stash,name,is_utf8 ? -(I32)len : (I32)len,add);
(gdb) call Perl_sv_dump(stash)
SV = PV(0xa86d70) at 0xaa1dd0
  REFCNT = 1
  FLAGS = (POK,IsCOW,pPOK)
  PV = 0xaa2d40 "Regexp"
  CUR = 6
  LEN = 0

Bisects to:

commit bee7c5743fabd49c584c5edbb637c66c5c25ca9a
Author: Father Chrysostomos <sprout@cpan.org>
Date:   Fri May 18 17:02:39 2012 -0700

    sv.c: Don’t fiddle with AMAGIC in sv_bless

    Since overloading itself now checks whether caches are up to date, and
    since changes to the stash (@ISA, methods) turn the flag on and over-
    loading itself turns the flag off when it can, sv_bless no longer
    needs to deal with it at all.

but I suspect this just a case of a freed SV being overwritten.

I don't think this is a security issue.

Tony



--
Ta Dinh Sung,

 
Date: Wed, 21 Jun 2017 15:57:00 +0100
From: Dave Mitchell <davem [...] iabyn.com>
To: perl5-security-report [...] perl.org
Subject: Re: [perl #131606] Unknow address reference in Perl_hv_common
Download (untitled) / with headers
text/plain 1.1k
On Tue, Jun 20, 2017 at 12:41:02AM -0700, sung wrote: Show quoted text
> Attached file below triggered crash because reference to unknow address in > funtion Perl_hv_common.
The code can be reduced to package F { sub x { blah->c(); } } my $sub = sub { F::x(); } ; delete ${'::'}{'F::'}; my $pat = qr/X/; "" =~ /$pat/; eval { $sub->() }; the call to the class method 'blah->c' involves a lookup of the stash 'blah', which isn't found, so S_find_default_stash() falls back using CopSTASH(PL_curcop). The current stash is F::, but that's been deleted, and it appears that CopSTASH() (via PL_stashpad[n] on threaded builds, and maybe an issue on unthreaded too?) doesn't hold a reference count on the stash. So the delete ${'::'}{'F::'}; frees the stash and leaves the cops pointing at a freed or reallocated SV which may not be a HV any more, hence the assert failure. This doesn't seem like a security issue, since malicious code that has the ability to delete stashes can probably do lots of harm anyway. If no-one objects, I'll move this to the public queue soon. -- In economics, the exam questions are the same every year. They just change the answers.
Subject: Re: [perl #131606] Unknow address reference in Perl_hv_common
To: Dave Mitchell <davem [...] iabyn.com>
From: demerphq <demerphq [...] gmail.com>
Date: Wed, 21 Jun 2017 17:14:22 +0200
CC: "perl5-security-report [...] perl.org" <perl5-security-report [...] perl.org>
Download (untitled) / with headers
text/plain 3.3k
On 21 June 2017 at 16:57, Dave Mitchell <davem@iabyn.com> wrote: Show quoted text
> On Tue, Jun 20, 2017 at 12:41:02AM -0700, sung wrote:
>> Attached file below triggered crash because reference to unknow address in >> funtion Perl_hv_common.
> > The code can be reduced to > > package F { > sub x { blah->c(); } > } > > my $sub = sub { F::x(); } ; > > delete ${'::'}{'F::'}; > > my $pat = qr/X/; > "" =~ /$pat/; > > eval { $sub->() }; > > the call to the class method 'blah->c' involves a lookup of the stash > 'blah', which isn't found, so S_find_default_stash() falls back > using CopSTASH(PL_curcop). The current stash is F::, but that's been > deleted, and it appears that CopSTASH() (via PL_stashpad[n] on threaded > builds, and maybe an issue on unthreaded too?) doesn't hold a reference > count on the stash. So the > > delete ${'::'}{'F::'}; > > frees the stash and leaves the cops pointing at a freed or reallocated SV > which may not be a HV any more, hence the assert failure. > > This doesn't seem like a security issue, since malicious code that has the > ability to delete stashes can probably do lots of harm anyway. > > If no-one objects, I'll move this to the public queue soon.
No objection, but i have a question. I chased this down a bit as well, and didnt get as far as you did I think, but on the other hand, I did try reverting the patch from the bisect, and it does reliably fix this problem. Also, this code is a bit "interesting", as the bless call is coming from Perl itself, internally, when it is constructing the qr//. So it is not actually clear to me that this is a refcount bug. Here is what I see when I trace the code, we end up calling S_find_default_stash() from gv.c line 2355: if (!stash && !find_default_stash(&stash, name, len, is_utf8, add, sv_type)) { return NULL; } Which looks like this in the gdb trace: S_find_default_stash (my_perl=0xaee010, stash=0x7fffffffd770, name=0xb32d68 "nfess", len=5, is_utf8=0, add=0, sv_type=SVt_PVIO) at gv.c:1765 1765 if (IN_PERL_COMPILETIME) { (gdb) 1804 *stash = CopSTASH(PL_curcop); (gdb) 1808 if (!*stash) { (gdb) 1839 if (!SvREFCNT(*stash)) /* symbol table under destruction */ (gdb) 1842 return TRUE; (gdb) 1843 } (gdb) Perl_gv_fetchpvn_flags (my_perl=0xaee010, nambeg=0xb32d68 "nfess", full_len=5, flags=0, sv_type=SVt_PVIO) at gv.c:2357 2357 gvp = (GV**)hv_fetch(stash,name,is_utf8 ? -(I32)len : (I32)len,add); When I dump the stash object its REFCNT is 1, which doesnt seem to line up with your analysis: (gdb) call Perl_sv_dump(my_perl,stash) SV = PV(0xaf1b18) at 0xb273a0 REFCNT = 1 FLAGS = (POK,IsCOW,pPOK) PV = 0xb1c4a8 "Regexp" CUR = 6 LEN = 0 And, if I revert FC's patch the bug goes away. FC's patch amounts to this: if (Gv_AMG(stash)) SvAMAGIC_on(sv); else (void)SvAMAGIC_off(sv); Which BTW, results in the else branch being called, however, it appears to me that the important part is not the else but the conditional itself, if I replace that code with simply doing: Gv_AMG(stash); then the segfault also goes away. This combined with the fact that this is the special Regexp class being created by the internals makes me think that we are somehow not correctly setting up the Regexp stash properly. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
Subject: Re: [perl #131606] Unknow address reference in Perl_hv_common
To: demerphq <demerphq [...] gmail.com>
CC: "perl5-security-report [...] perl.org" <perl5-security-report [...] perl.org>
From: Dave Mitchell <davem [...] iabyn.com>
Date: Wed, 21 Jun 2017 16:29:45 +0100
Download (untitled) / with headers
text/plain 1.5k
On Wed, Jun 21, 2017 at 05:14:22PM +0200, demerphq wrote: Show quoted text
> Here is what I see when I trace the code, we end up calling > S_find_default_stash() from > gv.c line 2355: > > if (!stash && !find_default_stash(&stash, name, len, is_utf8, add, > sv_type)) { > return NULL; > } > > Which looks like this in the gdb trace: > > S_find_default_stash (my_perl=0xaee010, stash=0x7fffffffd770, > name=0xb32d68 "nfess", len=5, > is_utf8=0, add=0, sv_type=SVt_PVIO) at gv.c:1765 > 1765 if (IN_PERL_COMPILETIME) { > (gdb) > 1804 *stash = CopSTASH(PL_curcop); > (gdb) > 1808 if (!*stash) { > (gdb) > 1839 if (!SvREFCNT(*stash)) /* symbol table under destruction */ > (gdb) > 1842 return TRUE; > (gdb) > 1843 } > (gdb) > Perl_gv_fetchpvn_flags (my_perl=0xaee010, nambeg=0xb32d68 "nfess", > full_len=5, flags=0, > sv_type=SVt_PVIO) at gv.c:2357 > 2357 gvp = (GV**)hv_fetch(stash,name,is_utf8 ? -(I32)len : (I32)len,add); > > When I dump the stash object its REFCNT is 1, which doesnt seem to > line up with your analysis: > (gdb) call Perl_sv_dump(my_perl,stash) > SV = PV(0xaf1b18) at 0xb273a0 > REFCNT = 1 > FLAGS = (POK,IsCOW,pPOK) > PV = 0xb1c4a8 "Regexp" > CUR = 6 > LEN = 0
What you're seeing is an SV that has been freed, and then later reallocated by other code (as a PV in your case - I saw an AV). It's supposed to be a HV. -- The crew of the Enterprise encounter an alien life form which is surprisingly neither humanoid nor made from pure energy. -- Things That Never Happen in "Star Trek" #22
CC: "perl5-security-report [...] perl.org" <perl5-security-report [...] perl.org>
From: demerphq <demerphq [...] gmail.com>
Date: Wed, 21 Jun 2017 17:29:57 +0200
Subject: Re: [perl #131606] Unknow address reference in Perl_hv_common
To: Dave Mitchell <davem [...] iabyn.com>
Download (untitled) / with headers
text/plain 6.1k
On 21 June 2017 at 17:14, demerphq <demerphq@gmail.com> wrote: Show quoted text
> On 21 June 2017 at 16:57, Dave Mitchell <davem@iabyn.com> wrote:
>> On Tue, Jun 20, 2017 at 12:41:02AM -0700, sung wrote:
>>> Attached file below triggered crash because reference to unknow address in >>> funtion Perl_hv_common.
>> >> The code can be reduced to >> >> package F { >> sub x { blah->c(); } >> } >> >> my $sub = sub { F::x(); } ; >> >> delete ${'::'}{'F::'}; >> >> my $pat = qr/X/; >> "" =~ /$pat/; >> >> eval { $sub->() }; >> >> the call to the class method 'blah->c' involves a lookup of the stash >> 'blah', which isn't found, so S_find_default_stash() falls back >> using CopSTASH(PL_curcop). The current stash is F::, but that's been >> deleted, and it appears that CopSTASH() (via PL_stashpad[n] on threaded >> builds, and maybe an issue on unthreaded too?) doesn't hold a reference >> count on the stash. So the >> >> delete ${'::'}{'F::'}; >> >> frees the stash and leaves the cops pointing at a freed or reallocated SV >> which may not be a HV any more, hence the assert failure. >> >> This doesn't seem like a security issue, since malicious code that has the >> ability to delete stashes can probably do lots of harm anyway. >> >> If no-one objects, I'll move this to the public queue soon.
> > No objection, but i have a question. I chased this down a bit as well, > and didnt get as far as you did I think, but on the other hand, I did > try reverting the patch from the bisect, and it does reliably fix this > problem. Also, this code is a bit "interesting", as the bless call is > coming from Perl itself, internally, when it is constructing the qr//. > So it is not actually clear to me that this is a refcount bug. > > Here is what I see when I trace the code, we end up calling > S_find_default_stash() from > gv.c line 2355: > > if (!stash && !find_default_stash(&stash, name, len, is_utf8, add, > sv_type)) { > return NULL; > } > > Which looks like this in the gdb trace: > > S_find_default_stash (my_perl=0xaee010, stash=0x7fffffffd770, > name=0xb32d68 "nfess", len=5, > is_utf8=0, add=0, sv_type=SVt_PVIO) at gv.c:1765 > 1765 if (IN_PERL_COMPILETIME) { > (gdb) > 1804 *stash = CopSTASH(PL_curcop); > (gdb) > 1808 if (!*stash) { > (gdb) > 1839 if (!SvREFCNT(*stash)) /* symbol table under destruction */ > (gdb) > 1842 return TRUE; > (gdb) > 1843 } > (gdb) > Perl_gv_fetchpvn_flags (my_perl=0xaee010, nambeg=0xb32d68 "nfess", > full_len=5, flags=0, > sv_type=SVt_PVIO) at gv.c:2357 > 2357 gvp = (GV**)hv_fetch(stash,name,is_utf8 ? -(I32)len : (I32)len,add); > > When I dump the stash object its REFCNT is 1, which doesnt seem to > line up with your analysis: > (gdb) call Perl_sv_dump(my_perl,stash) > SV = PV(0xaf1b18) at 0xb273a0 > REFCNT = 1 > FLAGS = (POK,IsCOW,pPOK) > PV = 0xb1c4a8 "Regexp" > CUR = 6 > LEN = 0 > > And, if I revert FC's patch the bug goes away. FC's patch amounts to this: > > if (Gv_AMG(stash)) > SvAMAGIC_on(sv); > else > (void)SvAMAGIC_off(sv); > > Which BTW, results in the else branch being called, however, it > appears to me that the important part is not the else but the > conditional itself, if I replace that code with simply doing: > > Gv_AMG(stash); > > then the segfault also goes away. This combined with the fact that > this is the special Regexp class being created by the internals makes > me think that we are somehow not correctly setting up the Regexp stash > properly.
And If I dump the Regexp stash in sv.c with and without the Gv_AMG(stash) call I see significant differences between the two. Gv_AMG(stash) has significant side-effects, the code that FC removed does much much more than it looks like it does. With: SV = PVHV(0xaf99f8) at 0xb1a2b8 REFCNT = 2 FLAGS = (RMG,OOK,SHAREKEYS) MAGIC = 0xb2e858 MG_VIRTUAL = &PL_vtbl_ovrld MG_TYPE = PERL_MAGIC_overload_table(c) MG_LEN = 8 MG_PTR = 0xb131e8 "\0\2\0\0\10\0\0\0" AUX_FLAGS = 0 ARRAY = 0xb1c3e8 (0:7, 1:1) hash quality = 100.0% KEYS = 1 FILL = 1 MAX = 7 RITER = -1 EITER = 0x0 RAND = 0x6bee8911 NAME = "Regexp" ENAME = "Regexp" BACKREFS = 0xb1a2d0 SV = PVGV(0xb10d98) at 0xb1a2d0 REFCNT = 1 FLAGS = (RMG,MULTI) MAGIC = 0xb1c608 MG_VIRTUAL = &PL_vtbl_backref MG_TYPE = PERL_MAGIC_backref(<) MG_OBJ = 0xb1a2e8 NAME = "DESTROY" NAMELEN = 7 GvSTASH = 0xb1a2b8 "Regexp" FLAGS = 0x2 GP = 0xb1c528 SV = 0x0 REFCNT = 1 IO = 0x0 FORM = 0x0 AV = 0x0 HV = 0x0 CV = 0xb1a2e8 CVGEN = 0x0 GPFLAGS = 0x0 () LINE = 2147483647 FILE = "t.pl" EGV = 0xb1a2d0 "DESTROY" MRO_WHICH = "dfs" (0x837b40) CACHE_GEN = 0x1 PKG_GEN = 0x2 MRO_LINEAR_CURRENT = 0xaf0f48 SV = PVAV(0xaf2b70) at 0xaf0f48 REFCNT = 2 FLAGS = (READONLY,PROTECT) ARRAY = 0xb1db08 FILL = 0 MAX = 3 FLAGS = (REAL) ISA = 0xb1a540 SV = PVHV(0xaf9a98) at 0xb1a540 REFCNT = 2 FLAGS = (READONLY,PROTECT,SHAREKEYS) ARRAY = 0xb1ddc8 (0:6, 1:2) hash quality = 125.0% KEYS = 2 FILL = 2 MAX = 7 Without: SV = PVHV(0xaf99f8) at 0xb1a2b8 REFCNT = 2 FLAGS = (OOK,SHAREKEYS,OVERLOAD) AUX_FLAGS = 0 ARRAY = 0xb1c3b8 (0:7, 1:1) hash quality = 100.0% KEYS = 1 FILL = 1 MAX = 7 RITER = -1 EITER = 0x0 RAND = 0xe94c400a NAME = "Regexp" ENAME = "Regexp" BACKREFS = 0xb1a2d0 SV = PVGV(0xb10d98) at 0xb1a2d0 REFCNT = 1 FLAGS = (RMG,MULTI) MAGIC = 0xb1c5d8 MG_VIRTUAL = &PL_vtbl_backref MG_TYPE = PERL_MAGIC_backref(<) MG_OBJ = 0xb1a2e8 NAME = "DESTROY" NAMELEN = 7 GvSTASH = 0xb1a2b8 "Regexp" FLAGS = 0x2 GP = 0xb1c4f8 SV = 0x0 REFCNT = 1 IO = 0x0 FORM = 0x0 AV = 0x0 HV = 0x0 CV = 0xb1a2e8 CVGEN = 0x0 GPFLAGS = 0x0 () LINE = 2147483647 FILE = "t.pl" EGV = 0xb1a2d0 "DESTROY" MRO_WHICH = "dfs" (0x8379e0) CACHE_GEN = 0x1 PKG_GEN = 0x2 -- perl -Mre=debug -e "/just|another|perl|hacker/"
Subject: Re: [perl #131606] Unknow address reference in Perl_hv_common
To: Dave Mitchell <davem [...] iabyn.com>
CC: "perl5-security-report [...] perl.org" <perl5-security-report [...] perl.org>
Date: Wed, 21 Jun 2017 17:32:38 +0200
From: demerphq <demerphq [...] gmail.com>
Download (untitled) / with headers
text/plain 734b
On 21 June 2017 at 17:29, Dave Mitchell <davem@iabyn.com> wrote: Show quoted text
> On Wed, Jun 21, 2017 at 05:14:22PM +0200, demerphq wrote:
>> When I dump the stash object its REFCNT is 1, which doesnt seem to >> line up with your analysis: >> (gdb) call Perl_sv_dump(my_perl,stash) >> SV = PV(0xaf1b18) at 0xb273a0 >> REFCNT = 1 >> FLAGS = (POK,IsCOW,pPOK) >> PV = 0xb1c4a8 "Regexp" >> CUR = 6 >> LEN = 0
> > What you're seeing is an SV that has been freed, and then later > reallocated by other code (as a PV in your case - I saw an AV). It's > supposed to be a HV.
Yes, but my point is that somehow the Gv_AMG() changes the original var enough that the free does not happen. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
From: demerphq <demerphq [...] gmail.com>
Date: Wed, 21 Jun 2017 17:41:00 +0200
CC: "perl5-security-report [...] perl.org" <perl5-security-report [...] perl.org>
Subject: Re: [perl #131606] Unknow address reference in Perl_hv_common
To: Dave Mitchell <davem [...] iabyn.com>
Download (untitled) / with headers
text/plain 946b
On 21 June 2017 at 17:32, demerphq <demerphq@gmail.com> wrote: Show quoted text
> On 21 June 2017 at 17:29, Dave Mitchell <davem@iabyn.com> wrote:
>> On Wed, Jun 21, 2017 at 05:14:22PM +0200, demerphq wrote:
>>> When I dump the stash object its REFCNT is 1, which doesnt seem to >>> line up with your analysis: >>> (gdb) call Perl_sv_dump(my_perl,stash) >>> SV = PV(0xaf1b18) at 0xb273a0 >>> REFCNT = 1 >>> FLAGS = (POK,IsCOW,pPOK) >>> PV = 0xb1c4a8 "Regexp" >>> CUR = 6 >>> LEN = 0
>> >> What you're seeing is an SV that has been freed, and then later >> reallocated by other code (as a PV in your case - I saw an AV). It's >> supposed to be a HV.
> > Yes, but my point is that somehow the Gv_AMG() changes the original > var enough that the free does not happen.
With the Gv_AMG() call, PL_curcop ends up as NULL not as a dangling pointer. I think there is something wrong with FC's patch. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
To: Dave Mitchell <davem [...] iabyn.com>
Subject: Re: [perl #131606] Unknow address reference in Perl_hv_common
CC: "perl5-security-report [...] perl.org" <perl5-security-report [...] perl.org>
From: demerphq <demerphq [...] gmail.com>
Date: Wed, 21 Jun 2017 17:49:36 +0200
Download (untitled) / with headers
text/plain 1.5k
On 21 June 2017 at 17:41, demerphq <demerphq@gmail.com> wrote: Show quoted text
> On 21 June 2017 at 17:32, demerphq <demerphq@gmail.com> wrote:
>> On 21 June 2017 at 17:29, Dave Mitchell <davem@iabyn.com> wrote:
>>> On Wed, Jun 21, 2017 at 05:14:22PM +0200, demerphq wrote:
>>>> When I dump the stash object its REFCNT is 1, which doesnt seem to >>>> line up with your analysis: >>>> (gdb) call Perl_sv_dump(my_perl,stash) >>>> SV = PV(0xaf1b18) at 0xb273a0 >>>> REFCNT = 1 >>>> FLAGS = (POK,IsCOW,pPOK) >>>> PV = 0xb1c4a8 "Regexp" >>>> CUR = 6 >>>> LEN = 0
>>> >>> What you're seeing is an SV that has been freed, and then later >>> reallocated by other code (as a PV in your case - I saw an AV). It's >>> supposed to be a HV.
>> >> Yes, but my point is that somehow the Gv_AMG() changes the original >> var enough that the free does not happen.
> > With the Gv_AMG() call, PL_curcop ends up as NULL not as a dangling > pointer. I think there is something wrong with FC's patch.
So for instance, if I take the code that FC removed from sv_bless() and put it into pp_qr() in pp_hot.c the segfault reliably goes away: $ git diff diff --git a/pp_hot.c b/pp_hot.c index 7c98c90..2394464 100644 --- a/pp_hot.c +++ b/pp_hot.c @@ -1923,6 +1923,10 @@ PP(pp_qr) HV *const stash = gv_stashsv(pkg, GV_ADD); SvREFCNT_dec_NN(pkg); (void)sv_bless(rv, stash); + if (Gv_AMG(stash)) + SvAMAGIC_on(rv); + else + (void)SvAMAGIC_off(rv); } if (UNLIKELY(RX_ISTAINTED(rx))) { -- perl -Mre=debug -e "/just|another|perl|hacker/"
CC: "perl5-security-report [...] perl.org" <perl5-security-report [...] perl.org>
Date: Thu, 22 Jun 2017 11:54:49 +0100
From: Dave Mitchell <davem [...] iabyn.com>
To: demerphq <demerphq [...] gmail.com>
Subject: Re: [perl #131606] Unknow address reference in Perl_hv_common
Download (untitled) / with headers
text/plain 1.4k
On Wed, Jun 21, 2017 at 05:49:36PM +0200, demerphq wrote: Show quoted text
> Yes, but my point is that somehow the Gv_AMG() changes the original > var enough that the free does not happen.
I'm afraid you are mistaken. With all 6 combinations of 1) blead 2) FC's patch reverted 3) the "FC blob" of code added to pp_qr instead against A) OP's original poc file B) my stripped down variant I see an assertion failure, and using a watchpoint under gdb shows the stash being prematurely freed by the delete ${'::'}{'SubHelper::'} (or more precisely in the freetmps in the scope exit following the delete), and then that freed SV being reallocated when the qr// is compiled. I think everything else you see is just a side affect of 1) the regex code (legitimately) treating an SV as a regex object; 2) some other code (illegitimately) treating the SV as a stash. I think FC's patch is probably correct. I don't remember the full details, but the way he changed things was that stashes get kind of speculatively marked as maybe having cached overload methods, then the first time an overload is tried, it properly regenerates the cache, realises there isn't that particular type of overloading and returns. Then subsequent overload checks fail quickly. -- Music lesson: a symbiotic relationship whereby a pupil's embellishments concerning the amount of practice performed since the last lesson are rewarded with embellishments from the teacher concerning the pupil's progress over the corresponding period.
RT-Send-CC: rt-deliver-to-perl5-security-report [...] rt.perl.org
On Thu, 22 Jun 2017 03:55:01 -0700, davem wrote: Show quoted text
> On Wed, Jun 21, 2017 at 05:49:36PM +0200, demerphq wrote:
> > Yes, but my point is that somehow the Gv_AMG() changes the original > > var enough that the free does not happen.
> > I'm afraid you are mistaken. With all 6 combinations of > 1) blead > 2) FC's patch reverted > 3) the "FC blob" of code added to pp_qr instead > against > A) OP's original poc file > B) my stripped down variant > > I see an assertion failure, and using a watchpoint under gdb shows the > stash being prematurely freed by the delete ${'::'}{'SubHelper::'} > (or more precisely in the freetmps in the scope exit following the delete), > and then that freed SV being reallocated when the qr// is compiled. > > I think everything else you see is just a side affect of > 1) the regex code (legitimately) treating an SV as a regex object; > 2) some other code (illegitimately) treating the SV as a stash.
This variant fails an assertion all the way back to 5.002: { package F ; sub x { blah->c(); } } delete $::{"F::"}; my $thing = [(0) x 5000]; F::x(); The $thing assignment (with a ridiculous repeat count for good measure) makes sure that the freed stash gets reused as an IV. How do we go about fixing this kind of bug? For GVs being freed when there are still CVs, we give the CV a strong pointer, instead of a weak one. I.e., we switch the direction of the weak pointer. That complicates the code, so I wonder whether it is the right thing to do in the case of freed stashes, which are much rarer than freed globs. Would the right thing to do be simply to plug the code paths that can fail an assertion when CopSTASH(&PL_curcop) is something weird (as I did for pp_caller)? What about those cases when a freed stash is reused as another stash (in which case you get subtle bugs hard to track down)? I thought about perhaps nulling out the pointer in PL_stashpad when the stash is freed (and plugging the crashing code paths), but that does not help us with non-threaded builds, which have their stash pointers directly in the COP. (And I don’t like to expand the complexity of PL_stashpad to unthreaded builds, just for the sake of this.) BTW, the test case crashes because it is looking to see whether the class method call could actually be a filehandle method call, which takes precedence over classes. Show quoted text
> I think FC's patch is probably correct. I don't remember the full details, > but the way he changed things was that stashes get kind of speculatively > marked as maybe having cached overload methods, then the first time > an overload is tried, it properly regenerates the cache, realises there > isn't that particular type of overloading and returns. Then subsequent > overload checks fail quickly.
Yes, that is how it works. That means that for most classes you get exactly one calculation of the overload table, not one for each blessing. And you can change your overload methods and have them take effect immediately on existing objects, instead of waiting for the next blessing to come along. -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 283b
On Wed, 21 Jun 2017 07:57:14 -0700, davem wrote: Show quoted text
> This doesn't seem like a security issue, since malicious code that has the > ability to delete stashes can probably do lots of harm anyway. > > If no-one objects, I'll move this to the public queue soon.
I agree, now public. Tony


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org