heap-use-after-free in Perl_op_free op.c:861

p5pRT commented 5 years ago

Migrated from rt.perl.org#133879 (status was 'resolved')

Searchable as RT133879$

p5pRT commented 5 years ago

From @dur-randir

Created by @dur-randir

While fuzzing perl v5.29.8-21-gde59f38ed9 built with afl and run under libdislocator\, I found the following program

BEGIN{$^H*=$^H};//;qr/\(?{/

to perform an access outside of an allocated memory slot. ASAN diagnostics are:

==21764==ERROR: AddressSanitizer: heap-use-after-free on address 0x61500000f708 at pc 0x00000050f72d bp 0x7ffffba1e960 sp 0x7ffffba1e958 READ of size 2 at 0x61500000f708 thread T0 #0 0x50f72c in Perl_op_free /home/afl/afl-asan/op.c:861:27 #1 0x5d63e5 in perl_destruct /home/afl/afl-asan/perl.c:885:2 #2 0x50b7d5 in main /home/afl/afl-asan/perlmain.c:138:18 #3 0x7f225f29909a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a) #4 0x43bdc9 in _start (/home/afl/afl-asan/perl+0x43bdc9)

0x61500000f708 is located 392 bytes inside of 512-byte region [0x61500000f580\,0x61500000f780) freed by thread T0 here: #0 0x4da200 in __interceptor_cfree.localalias.0 (/home/afl/afl-asan/perl+0x4da200) #1 0x50dbc1 in Perl_opslab_free /home/afl/afl-asan/op.c:512:2 #2 0x50e812 in Perl_opslab_force_free /home/afl/afl-asan/op.c:555:5 #3 0x715bc2 in Perl_cv_undef_flags /home/afl/afl-asan/pad.c:341:17 #4 0xa1e2aa in Perl_sv_clear /home/afl/afl-asan/sv.c:6631:6 #5 0xa269f7 in Perl_sv_free2 /home/afl/afl-asan/sv.c:7092:9 #6 0x70e31d in S_SvREFCNT_dec /home/afl/afl-asan/./inline.h:216:6 #7 0x70e31d in Perl_yyparse /home/afl/afl-asan/perly.c:439 #8 0x5eba6a in S_parse_body /home/afl/afl-asan/perl.c:2507:9 #9 0x5e1e23 in perl_parse /home/afl/afl-asan/perl.c:1798:2 #10 0x50b5de in main /home/afl/afl-asan/perlmain.c:126:10 #11 0x7f225f29909a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)

previously allocated by thread T0 here: #0 0x4da570 in calloc (/home/afl/afl-asan/perl+0x4da570) #1 0x50bc79 in S_new_slab /home/afl/afl-asan/op.c:264:30 #2 0x50bc79 in Perl_Slab_Alloc /home/afl/afl-asan/op.c:310 #3 0x533145 in Perl_newSVOP /home/afl/afl-asan/op.c:7312:5 #4 0x6cc1a0 in S_scan_const /home/afl/afl-asan/toke.c:4122:27 #5 0x67185f in Perl_yylex /home/afl/afl-asan/toke.c:5062:10 #6 0x705070 in Perl_yyparse /home/afl/afl-asan/perly.c:340:34 #7 0x5eba6a in S_parse_body /home/afl/afl-asan/perl.c:2507:9 #8 0x5e1e23 in perl_parse /home/afl/afl-asan/perl.c:1798:2 #9 0x50b5de in main /home/afl/afl-asan/perlmain.c:126:10 #10 0x7f225f29909a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)

This is a regression between 5.24 and 5.26\, bisect points to

commit 7c449856260e14be9e73a4060cb86a5e2f680a65 Author: Karl Williamson \khw@cpan\.org Date: Tue Feb 14 13:22:58 2017 -0700

Improve handling pattern compilation errors

Perl tries to continue parsing in the face of errors for the convenience of the person running the script\, so as to batch up as many errors as possible\, and cut down the number of runs. Some errors will\, however\, have a cascading effect\, resulting in the parser getting confused as to the intent. Perl currently aborts parsing if 10 errors accumulate.

Perl Info

``` Flags: category=core severity=medium Site configuration information for perl 5.29.9: Configured by dur-randir at Wed Feb 27 14:51:01 MSK 2019. Summary of my perl5 (revision 5 version 29 subversion 9) configuration: Commit id: c1e47bad34ce1d9c84ed57c9b8978bcbd5a02e98 Platform: osname=darwin osvers=13.4.0 archname=darwin-thread-multi-2level uname='darwin isengard.local 13.4.0 darwin kernel version 13.4.0: mon jan 11 18:17:34 pst 2016; root:xnu-2422.115.15~1release_x86_64 x86_64 ' config_args='-de -Dusedevel -DDEBUGGING -Dusethreads' hint=recommended useposix=true d_sigaction=define useithreads=define usemultiplicity=define use64bitint=define use64bitall=define uselongdouble=undef usemymalloc=n default_inc_excludes_dot=define bincompat5005=undef Compiler: cc='cc' ccflags ='-fno-common -DPERL_DARWIN -mmacosx-version-min=10.9 -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -DPERL_USE_SAFE_PUTENV' optimize='-O3 -g' cppflags='-fno-common -DPERL_DARWIN -mmacosx-version-min=10.9 -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion='' gccversion='4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.56)' gccosandvers='' intsize=4 longsize=8 ptrsize=8 doublesize=8 byteorder=12345678 doublekind=3 d_longlong=define longlongsize=8 d_longdbl=define longdblsize=16 longdblkind=3 ivtype='long' ivsize=8 nvtype='double' nvsize=8 Off_t='off_t' lseeksize=8 alignbytes=8 prototype=define Linker and Libraries: ld='cc' ldflags =' -mmacosx-version-min=10.9 -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/clang/6.0/lib /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib /usr/lib libs=-lpthread -lgdbm -ldbm -ldl -lm -lutil -lc perllibs=-lpthread -ldl -lm -lutil -lc libc= so=dylib useshrplib=false libperl=libperl.a gnulibc_version='' Dynamic Linking: dlsrc=dl_dlopen.xs dlext=bundle d_dlsymun=undef ccdlflags=' ' cccdlflags=' ' lddlflags=' -mmacosx-version-min=10.9 -bundle -undefined dynamic_lookup -L/usr/local/lib -fstack-protector' @INC for perl 5.29.9: lib /usr/local/lib/perl5/site_perl/5.29.9/darwin-thread-multi-2level /usr/local/lib/perl5/site_perl/5.29.9 /usr/local/lib/perl5/5.29.9/darwin-thread-multi-2level /usr/local/lib/perl5/5.29.9 Environment for perl 5.29.9: DYLD_LIBRARY_PATH (unset) HOME=/Users/dur-randir LANG=en_US.UTF-8 LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/Users/dur-randir/perlbrew/bin:/Users/dur-randir/perlbrew/perls/perl-5.22.1/bin:/usr/local/bin:/usr/local/sbin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/texbin PERLBREW_HOME=/Users/dur-randir/.perlbrew PERLBREW_MANPATH=/Users/dur-randir/perlbrew/perls/perl-5.22.1/man PERLBREW_PATH=/Users/dur-randir/perlbrew/bin:/Users/dur-randir/perlbrew/perls/perl-5.22.1/bin PERLBREW_PERL=perl-5.22.1 PERLBREW_ROOT=/Users/dur-randir/perlbrew PERLBREW_SHELLRC_VERSION=0.84 PERLBREW_VERSION=0.84 PERL_BADLANG (unset) SHELL=/usr/local/bin/zsh ```

p5pRT commented 5 years ago

From @hvds

On Sun\, 03 Mar 2019 05:30:16 -0800\, randir wrote:

While fuzzing perl v5.29.8-21-gde59f38ed9 built with afl and run under libdislocator\, I found the following program

BEGIN{$^H*=$^H};//;qr/\(?{/

to perform an access outside of an allocated memory slot.

I tried reproducing this with clang sanitize=undefined\, but was unable to.

When you run this under libdislocator\, do you get other diagnostics emitted? My guess is that your $^H contains 0x100 (HINT_BLOCK_SCOPE) initially\, and the BEGIN block is changing that to 0x10000 (HINT_NEW_RE) thus sending it into the path in which S_scan_const calls S_new_constant for "\\(?{"\, which will then report 'Constant (qq) unknown'.

After reporting the error\, that does then return the original sv (containing the raw string)\, so it makes sense to me that this could interact with Karl's change in 7c449856. But I don't think I can get much further on this myself.

Hugo

p5pRT commented 5 years ago

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

p5pRT commented 5 years ago

From @dur-randir

пн\, 4 мар. 2019 г. в 16:11\, Hugo van der Sanden via RT \perl5\-security\-report\-followup@perl\.org:

I tried reproducing this with clang sanitize=undefined\, but was unable to. Have you tried -fsanitize=address? AFL adds '-U_FORTIFY_SOURCE -fsanitize=address' in AFL_USE_ASAN mode.

When you run this under libdislocator\, do you get other diagnostics emitted? My guess is that your $^H contains 0x100 (HINT_BLOCK_SCOPE) initially\, and the BEGIN block is changing that to 0x10000 (HINT_NEW_RE) thus sending it into the path in which S_scan_const calls S_new_constant for "\\(?{"\, which will then report 'Constant (qq) unknown'.

Output indeed starts with ' Constant(qq) unknown at -e line 1\, within pattern Execution of -e aborted due to compilation errors. ' and then I get heap-use-after-free error. And printing hints after assigment with 'BEGIN{$^H*=$^H; warn $^H}' yelds 65536\, confirming your assumptions.

Best regards\, Sergey Aleynikov

p5pRT commented 5 years ago

From @hvds

On Mon\, 04 Mar 2019 08:56:02 -0800\, randir wrote:

пн\, 4 мар. 2019 г. в 16:11\, Hugo van der Sanden via RT \perl5\-security\-report\-followup@perl\.org:

I tried reproducing this with clang sanitize=undefined\, but was unable to. Have you tried -fsanitize=address? AFL adds '-U_FORTIFY_SOURCE -fsanitize=address' in AFL_USE_ASAN mode.

You're quite right: I had thought sanitize=undefined was always stricter\, but apparently not.

I'll try to dig deeper.

Hugo

p5pRT commented 5 years ago

From @hvds

On Tue\, 05 Mar 2019 10:22:07 -0800\, hv wrote:

On Mon\, 04 Mar 2019 08:56:02 -0800\, randir wrote:

пн\, 4 мар. 2019 г. в 16:11\, Hugo van der Sanden via RT \perl5\-security\-report\-followup@perl\.org:

I tried reproducing this with clang sanitize=undefined\, but was unable to. Have you tried -fsanitize=address? AFL adds '-U_FORTIFY_SOURCE -fsanitize=address' in AFL_USE_ASAN mode.

You're quite right: I had thought sanitize=undefined was always stricter\, but apparently not.

I'll try to dig deeper.

It seems to me that this is working as intended\, but that the intent is dodgy (and there are implementation warts).

In op_free() when we are walking the kids\, we try to allow for the fact that something may have got there before us: if we see a NULL op\, or one of type OP_FREED\, we skip it. However before we check for that we already read the next kid in the chain from the pointer.

Given that\, a) the NULL check is redundant - reading OpSIBLING would have crashed before we check it; b) in the OP_FREED case it also seems dodgy\, since we're reading the pointer out of an already freed op.

Here's the chunk in question; I hope Dave or Tony may have more insight into this.

for (kid = cUNOPo->op_first; kid; kid = nextkid) { nextkid = OpSIBLING(kid); /* Get before next freeing kid */ if (!kid || kid->op_type == OP_FREED) /* During the forced freeing of ops after compilation failure\, kidops may be freed before their parents. */ continue; if (!(kid->op_flags & OPf_KIDS)) /* If it has no kids\, just free it now */ op_free(kid); else DEFER_OP(kid); }

I would have assumed the check should happen before reading OpSIBLING\, and that it should break rather than continue. But I'm confused that we've survived 4+ years with this code as is.

Hugo

p5pRT commented 5 years ago

From @iabyn

On Wed\, Mar 13\, 2019 at 08:37:24AM -0700\, Hugo van der Sanden via RT wrote:

It seems to me that this is working as intended\, but that the intent is dodgy (and there are implementation warts).

In op_free() when we are walking the kids\, we try to allow for the fact that something may have got there before us: if we see a NULL op\, or one of type OP_FREED\, we skip it. However before we check for that we already read the next kid in the chain from the pointer.

Given that\, a) the NULL check is redundant - reading OpSIBLING would have crashed before we check it; b) in the OP_FREED case it also seems dodgy\, since we're reading the pointer out of an already freed op.

Here's the chunk in question; I hope Dave or Tony may have more insight into this.

The specific bug triggered by the code in this ticket is due to PL_compcv not getting restored at the correct time. I've fixed that with the first commit shown below. The bug requires compile-time code to have an error\, so is exceedingly unlikely to appear in real code. So I'm classifying this as not a security bug.

The issue you raise about op_free: I agree that the !kid null test is pointless: I've removed it and replaced it with an assert.

The OP_FREED test is in fact safe. Some work by FC means that ops are now allocated from a per-CV chain of slabs. Normally when a CV is freed\, its ops are processed by op_free() in tree order\, then the slabs are free()ed. Note that op_free() doesn't actually free() an op any more - it just deallocates any resources associated with the op (such as op_sv for an OP_CONST) then leaves the husk of the op in the slab with OP_FREED set. Note that this husk still contains valid structural pointers like op_first\, and also that it can't be re-allocated.

However\, on error\, the CV freeing code does a linear scan of the slab chain\, calling op_free() on each one. This ensures that ops which had been allocated but not yet attached to the optree have any resources released. this means that

eval 'some some of error' while 1;

no longer leaks. So op_free() skipping OP_FREED ops is safe.

commit 170c919fc4986a85062e9292e4cfed24771d2224 Author: David Mitchell \davem@iabyn\.com AuthorDate: Tue Mar 19 10:58:46 2019 +0000

handle scope error in qr/\(?{/
RT #133879
In this code:
BEGIN {$^H = 0x10000 }; # HINT_NEW_RE qr/\(?{/
When the toker sees the 'qr'\, it looks ahead and thinks that the pattern *might* contain code blocks\, so creates a new anon sub to wrap compilation of the pattern in (so that any code blocks get compiled as part of the anon sub rather than the main body of the code).
Normally at the end of parsing the qr construct\, the parser notes that no code blocks were found\, and throws the unneeded CV away and restores the old PL_compcv (via a LEAVE_SCOPE). This false positive is normal and is expected in the relevant code paths.
However\, setting the HINT_NEW_RE (which indicates that overload::constant is present for qr// but with no overloaded function actually present) causes an error to be raised. The parser does error recovery and continues.
However\, v5.25.9-148-g7c44985626 added a test to not bother compiling a pattern if the parser is in an errored state\, which again is fine\, except it turns out that if this branch is taken\, it skips the 'restore the old PL_compcv' code\, leading to the wrong value for PL_compcv when ops are freed.
The fix is simple: move the "skip if errored" test to after PL_compcv has been restored.

M op.c M t/re/reg_eval_scope.t

commit 44b0aff01ba282b14dc62a1137996136282bc17a Author: David Mitchell \davem@iabyn\.com AuthorDate: Tue Mar 19 11:15:21 2019 +0000

op_free() remove redundant !kid test
and replace with an assert.
If an op has the OPf_KIDS flag\, then cUNOPo->op_first must be non-null. So testing for !kid doesn't do much\, especially as on the previous line we dereference it anyway.

-- You never really learn to swear until you learn to drive.

p5pRT commented 5 years ago

From @hvds

On Tue\, 19 Mar 2019 04:46:11 -0700\, davem wrote:

The specific bug triggered by the code in this ticket is due to PL_compcv not getting restored at the correct time. I've fixed that with the first commit shown below.

Cool.

The OP_FREED test is in fact safe. Some work by FC means that ops are now allocated from a per-CV chain of slabs. Normally when a CV is freed\, its ops are processed by op_free() in tree order\, then the slabs are free()ed. Note that op_free() doesn't actually free() an op any more - it just deallocates any resources associated with the op (such as op_sv for an OP_CONST) then leaves the husk of the op in the slab with OP_FREED set. Note that this husk still contains valid structural pointers like op_first\, and also that it can't be re-allocated.

I infer from this comment in op.h that it _can_ be reallocated\, am I missing something?

* Freed ops are marked as freed and attached to the freed chain * via op_next pointers. * [...] used when allo- * cating an op if there are no freed ops available or big enough.

Hugo

p5pRT commented 5 years ago

From @iabyn

On Thu\, Mar 21\, 2019 at 04:10:43AM -0700\, Hugo van der Sanden via RT wrote:

The OP_FREED test is in fact safe. Some work by FC means that ops are now allocated from a per-CV chain of slabs. Normally when a CV is freed\, its ops are processed by op_free() in tree order\, then the slabs are free()ed. Note that op_free() doesn't actually free() an op any more - it just deallocates any resources associated with the op (such as op_sv for an OP_CONST) then leaves the husk of the op in the slab with OP_FREED set. Note that this husk still contains valid structural pointers like op_first\, and also that it can't be re-allocated.

I infer from this comment in op.h that it _can_ be reallocated\, am I missing something?

* Freed ops are marked as freed and attached to the freed chain * via op_next pointers. * [...] used when allo- * cating an op if there are no freed ops available or big enough.

I was discussing the phase where the parser has errored out and all ops associated with the current compiling CV (whether or not they've been attached to the optree yet) are being freed by a scan through the slabs associated with that slab. During that scan:

a) ops may be encountered that have been marked freed (as part of freeing a subtree associated with freeing an op earlier in the slab)\, but

b) no new ops will get allocated for that CV as compilation has been been abandoned.

During normal compilation ops may get freed and reused\, but we don't scan the slabs in that scenario.

-- 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 5 years ago

From @hvds

On Thu\, 21 Mar 2019 06:38:15 -0700\, davem wrote:

I was discussing the phase where the parser has errored out and all ops associated with the current compiling CV (whether or not they've been attached to the optree yet) are being freed by a scan through the slabs associated with that slab. During that scan:

a) ops may be encountered that have been marked freed (as part of freeing a subtree associated with freeing an op earlier in the slab)\, but b) no new ops will get allocated for that CV as compilation has been abandoned.

Ah ok\, thanks for the clarification.

Hugo

p5pRT commented 5 years ago

@iabyn - Status changed from 'open' to 'pending release'

p5pRT commented 5 years ago

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release today of Perl 5.30.0\, this and 160 other issues have been resolved.

Perl 5.30.0 may be downloaded via: https://metacpan.org/release/XSAWYERX/perl-5.30.0

If you find that the problem persists\, feel free to reopen this ticket.

p5pRT commented 5 years ago

@khwilliamson - Status changed from 'pending release' to 'resolved'

Perl / perl5