heap-use-after-free in S_cleanup_regmatch_info_aux

[p5pRT]

Migrated from rt.perl.org#134208 (status was 'pending release')

Searchable as RT134208$

[p5pRT]

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

eval q!s,(?{stat//}),,r!

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

==31811==ERROR​: AddressSanitizer​: heap-use-after-free on address
0x604000009768 at pc 0x000000ca6f2d bp 0x7ffec1c9b1a0 sp
0x7ffec1c9b198
WRITE of size 8 at 0x604000009768 thread T0
  #0 0xca6f2c in S_cleanup_regmatch_info_aux
/home/afl/afl-asan/regexec.c​:10074​:43
  #1 0xb1c83a in Perl_leave_scope /home/afl/afl-asan/scope.c​:1269​:6
  #2 0xb580ad in S_pop_eval_context_maybe_croak
/home/afl/afl-asan/pp_ctl.c​:1632​:5
  #3 0xb576cd in Perl_die_unwind /home/afl/afl-asan/pp_ctl.c​:1784​:13
  #4 0x895c94 in Perl_vcroak /home/afl/afl-asan/util.c​:1716​:5
  #5 0x895c94 in Perl_croak /home/afl/afl-asan/util.c​:1761
  #6 0x985dc7 in Perl_pp_match /home/afl/afl-asan/pp_hot.c​:2960​:21
  #7 0x88f834 in Perl_runops_debug /home/afl/afl-asan/dump.c​:2537​:23
  #8 0xcdee7d in S_regmatch /home/afl/afl-asan/regexec.c​:7321​:3
  #9 0xca76bd in S_regtry /home/afl/afl-asan/regexec.c​:3933​:14
  #10 0xc6d2b3 in Perl_regexec_flags /home/afl/afl-asan/regexec.c​:3790​:7
  #11 0x98316c in Perl_pp_match /home/afl/afl-asan/pp_hot.c​:3018​:10
  #12 0x88f834 in Perl_runops_debug /home/afl/afl-asan/dump.c​:2537​:23
  #13 0xcdee7d in S_regmatch /home/afl/afl-asan/regexec.c​:7321​:3
  #14 0xca76bd in S_regtry /home/afl/afl-asan/regexec.c​:3933​:14
  #15 0xc6d2b3 in Perl_regexec_flags /home/afl/afl-asan/regexec.c​:3790​:7
  #16 0x992608 in Perl_pp_subst /home/afl/afl-asan/pp_hot.c​:4231​:10
  #17 0x88f834 in Perl_runops_debug /home/afl/afl-asan/dump.c​:2537​:23
  #18 0x5f1015 in S_run_body /home/afl/afl-asan/perl.c​:2692​:2
  #19 0x5f1015 in perl_run /home/afl/afl-asan/perl.c​:2615
  #20 0x50b60a in main /home/afl/afl-asan/perlmain.c​:127​:9
  #21 0x7f0b773ff09a in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
  #22 0x43bdc9 in _start (/home/afl/afl-asan/perl+0x43bdc9)

0x604000009768 is located 24 bytes inside of 48-byte region
[0x604000009750,0x604000009780)
freed by thread T0 here​:
  #0 0x4da200 in __interceptor_cfree.localalias.0
(/home/afl/afl-asan/perl+0x4da200)
  #1 0x89696a in Perl_safesysfree /home/afl/afl-asan/util.c​:385​:2
  #2 0x8c4396 in S_mg_free_struct /home/afl/afl-asan/mg.c​:567​:5
  #3 0x8c4396 in Perl_mg_free /home/afl/afl-asan/mg.c​:588
  #4 0xa1e210 in Perl_sv_clear /home/afl/afl-asan/sv.c​:6598​:3
  #5 0xa269f7 in Perl_sv_free2 /home/afl/afl-asan/sv.c​:7092​:9
  #6 0xb1ef16 in S_SvREFCNT_dec /home/afl/afl-asan/./inline.h​:216​:6
  #7 0xb1ef16 in Perl_leave_scope /home/afl/afl-asan/scope.c​:973
  #8 0xb580ad in S_pop_eval_context_maybe_croak
/home/afl/afl-asan/pp_ctl.c​:1632​:5
  #9 0xb576cd in Perl_die_unwind /home/afl/afl-asan/pp_ctl.c​:1784​:13
  #10 0x895c94 in Perl_vcroak /home/afl/afl-asan/util.c​:1716​:5
  #11 0x895c94 in Perl_croak /home/afl/afl-asan/util.c​:1761
  #12 0x985dc7 in Perl_pp_match /home/afl/afl-asan/pp_hot.c​:2960​:21
  #13 0x88f834 in Perl_runops_debug /home/afl/afl-asan/dump.c​:2537​:23
  #14 0xcdee7d in S_regmatch /home/afl/afl-asan/regexec.c​:7321​:3
  #15 0xca76bd in S_regtry /home/afl/afl-asan/regexec.c​:3933​:14
  #16 0xc6d2b3 in Perl_regexec_flags /home/afl/afl-asan/regexec.c​:3790​:7
  #17 0x98316c in Perl_pp_match /home/afl/afl-asan/pp_hot.c​:3018​:10
  #18 0x88f834 in Perl_runops_debug /home/afl/afl-asan/dump.c​:2537​:23
  #19 0xcdee7d in S_regmatch /home/afl/afl-asan/regexec.c​:7321​:3
  #20 0xca76bd in S_regtry /home/afl/afl-asan/regexec.c​:3933​:14
  #21 0xc6d2b3 in Perl_regexec_flags /home/afl/afl-asan/regexec.c​:3790​:7
  #22 0x992608 in Perl_pp_subst /home/afl/afl-asan/pp_hot.c​:4231​:10
  #23 0x88f834 in Perl_runops_debug /home/afl/afl-asan/dump.c​:2537​:23
  #24 0x5f1015 in S_run_body /home/afl/afl-asan/perl.c​:2692​:2
  #25 0x5f1015 in perl_run /home/afl/afl-asan/perl.c​:2615
  #26 0x50b60a in main /home/afl/afl-asan/perlmain.c​:127​:9
  #27 0x7f0b773ff09a 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 0x896bbe in Perl_safesyscalloc /home/afl/afl-asan/util.c​:439​:18
  #2 0xa115a6 in Perl_sv_magicext /home/afl/afl-asan/sv.c​:5684​:5
  #3 0xc65cc1 in S_setup_eval_state /home/afl/afl-asan/regexec.c​:9993​:18
  #4 0xc65cc1 in Perl_regexec_flags /home/afl/afl-asan/regexec.c​:3411
  #5 0x992608 in Perl_pp_subst /home/afl/afl-asan/pp_hot.c​:4231​:10
  #6 0x88f834 in Perl_runops_debug /home/afl/afl-asan/dump.c​:2537​:23
  #7 0x5f1015 in S_run_body /home/afl/afl-asan/perl.c​:2692​:2
  #8 0x5f1015 in perl_run /home/afl/afl-asan/perl.c​:2615
  #9 0x50b60a in main /home/afl/afl-asan/perlmain.c​:127​:9
  #10 0x7f0b773ff09a in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x2409a)

This is a regression between 5.26 and 5.28, bisect points to

commit b66d79a
Author​: David Mitchell <davem@​iabyn.com>
Date​: Mon Aug 22 09​:50​:43 2016 +0100

  FREETMPS when leaving eval, even when void/dying

  [ This commit was originally added as v5.25.2-77-g214949f then reverted
  by v5.25.2-89-gcc040a9, since it broke Variable​::Magic. That distribution
  has since been fixed, so this fix can be re-applied to blead ]

  When a scope is exited normally (e.g. pp_leavetry, pp_leavesub),
  we do a FREETMPS only in scalar or list context; in void context
  we don't bother for efficiency reasons. Similarly, when there's an
  exception and we unwind to (and then pop) an EVAL context, we haven't
  been bothering to FREETMPS.

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]

From @hvds

On Tue, 05 Mar 2019 09​:27​:56 -0800, randir wrote​:

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

eval q!s,(?{stat//}),,r!

to perform an access outside of an allocated memory slot.
[...]
This is a regression between 5.26 and 5.28, bisect points to

commit b66d79a
Author​: David Mitchell <davem@​iabyn.com>
Date​: Mon Aug 22 09​:50​:43 2016 +0100

FREETMPS when leaving eval, even when void/dying

Dave, can you take a look at this? I'm not sure where to begin with diagnosis - clearly the FREETMPS change has somehow violated this requirement carefully documented in regexec.c​:
  /* SAVEFREESV, not sv_mortalcopy, as this SV must last until after
  S_cleanup_regmatch_info_aux has executed (registered by
  SAVEDESTRUCTOR_X below). S_cleanup_regmatch_info_aux modifies
  magic belonging to this SV.

My initial assumption is that there's no security issue here (needs dying code), but it's just possible it's a symptom of a deeper problem with the FREETMPS change.

Hugo

[p5pRT]

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

[p5pRT]

From imdb95@gmail.com

Hi,
I found this bug when fuzzing perlembed with afl-fuzz.

**********Compilation**********
Version of perl​: the dev version (https://perl5.git.perl.org/perl.git)

root@​peter-VirtualBox​:~/perl_crash_asan# ../perl/perl -v

This is perl 5, version 31, subversion 1 (v5.31.1) built for x86_64-linux

(with 1 registered patch, see perl -V for more detail)

OS​: Ubuntu 16.04 LTS 64bit
Compilation of perl, with pre-built clang+llvm (
http​://releases.llvm.org/4.0.0/clang+llvm-4.0.0-x86_64-linux-gnu-ubuntu-16.04.tar.xz)
and afl-2.52b​:
AFL_USE_ASAN=1 ./Configure -des -Dusedevel -DDEBUGGING -Dcc=afl-clang-fast
-Doptimize=-g
Compilation of file perl_crash.cpp​:

AFL_USE_ASAN=1 afl-clang-fast++ perl_crash.cpp -o perl_crash `./perl/perl
-MExtUtils​::Embed -e ccopts -e ldopts` -std=c++11

**********Reproduce**********

root@​peter-VirtualBox​:~/perl_crash_asan# ./perl_crash

==10049==ERROR​: AddressSanitizer​: heap-use-after-free on address
0x604000004668 at pc 0x000000bc0308 bp 0x7ffc5799b8a0 sp
0x7ffc5799b898
WRITE of size 8 at 0x604000004668 thread T0
  #0 0xbc0307 in S_cleanup_regmatch_info_aux /root/perl/regexec.c​:10282​:43
  #1 0xa39ee0 in Perl_leave_scope /root/perl/scope.c
  #2 0xa74846 in S_pop_eval_context_maybe_croak /root/perl/pp_ctl.c​:1633​:5
  #3 0xa73e78 in Perl_die_unwind /root/perl/pp_ctl.c​:1785​:13
  #4 0x7d29cf in Perl_vcroak /root/perl/util.c​:1711​:5
  #5 0x7c97da in Perl_croak /root/perl/util.c​:1756​:5
  #6 0x8d747d in S_opmethod_stash /root/perl/pp_hot.c
  #7 0x8d7c1e in Perl_pp_method_named /root/perl/pp_hot.c​:5540​:23
  #8 0x7c362c in Perl_runops_debug /root/perl/dump.c​:2537​:23
  #9 0xbf6cf6 in S_regmatch /root/perl/regexec.c​:7423​:3
  #10 0xbc0abc in S_regtry /root/perl/regexec.c​:3976​:14
  #11 0xb84ce9 in Perl_regexec_flags /root/perl/regexec.c​:3839​:7
  #12 0x8af8c4 in Perl_pp_match /root/perl/pp_hot.c​:3014​:10
  #13 0x7c362c in Perl_runops_debug /root/perl/dump.c​:2537​:23
  #14 0x533b83 in Perl_eval_sv /root/perl/perl.c​:3161​:2
  #15 0x535340 in Perl_eval_pv /root/perl/perl.c​:3221​:5
  #16 0x50d904 in main /root/perl_crash_asan/perl_crash.cpp​:27​:2
  #17 0x7f9e97cbf82f in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
  #18 0x436e28 in _start (/root/perl_crash_asan/perl_crash+0x436e28)

0x604000004668 is located 24 bytes inside of 48-byte region
[0x604000004650,0x604000004680)
freed by thread T0 here​:
  #0 0x4dd7db in free
/scratch/llvm/clang-4/xenial/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc​:47​:3
  #1 0x7ca327 in Perl_safesysfree /root/perl/util.c​:399​:2

previously allocated by thread T0 here​:
  #0 0x4ddd13 in calloc
/scratch/llvm/clang-4/xenial/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc​:74​:3
  #1 0x7ca52e in Perl_safesyscalloc /root/perl/util.c​:453​:18
  #2 0x8af8c4 in Perl_pp_match /root/perl/pp_hot.c​:3014​:10
  #3 0x7c362c in Perl_runops_debug /root/perl/dump.c​:2537​:23

SUMMARY​: AddressSanitizer​: heap-use-after-free
/root/perl/regexec.c​:10282​:43 in S_cleanup_regmatch_info_aux
Shadow bytes around the buggy address​:
  0x0c087fff8870​: fa fa 00 00 00 00 00 fa fa fa fd fd fd fd fd fa
  0x0c087fff8880​: fa fa 00 00 00 00 03 fa fa fa 00 00 00 00 00 02
  0x0c087fff8890​: fa fa 00 00 00 00 03 fa fa fa 00 00 00 00 00 00
  0x0c087fff88a0​: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fd
  0x0c087fff88b0​: fa fa fd fd fd fd fd fa fa fa 00 00 00 00 03 fa
=>0x0c087fff88c0​: fa fa 00 00 00 00 04 fa fa fa fd fd fd[fd]fd fd
  0x0c087fff88d0​: fa fa fd fd fd fd fd fd fa fa fa fa fa fa fa fa
  0x0c087fff88e0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff88f0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff8900​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff8910​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
==10049==ABORTING

*************************

Please confirm.

Thanks

[p5pRT]

From imdb95@gmail.com

#include <fstream>
#include <string>
#include <iostream>
#include <sstream>
#include <ctime>
#include <cstdlib>
#include <EXTERN.h>
#include <perl.h>

typedef const char* CONSTCSTR;

int main(int argc, char** argv)
{
  CONSTCSTR* embedding = new CONSTCSTR [4];
  embedding[0] = "";
  embedding[1] = "-e";
  embedding[2] = "0";
  embedding[3] = 0;

  PerlInterpreter *my_perl = perl_alloc();
  perl_construct(my_perl);
  perl_parse(my_perl, NULL, 3, (char**)embedding, NULL);
  PL_exit_flags |= PERL_EXIT_DESTRUCT_END;
  perl_run(my_perl);

  eval_pv("\"@​a $1\" =~ /(?{push @​a,b $1})/", true);
  perl_destruct(my_perl);
  perl_free(my_perl);

  return 0;
}

[p5pRT]

From @iabyn

On Tue, Jun 18, 2019 at 06​:21​:03AM -0700, Nguyen Duc Manh wrote​:

I found this bug when fuzzing perlembed with afl-fuzz.

It can be reproduced with a plain perl script run under valgrind; no
embedding required.

  eval '"@​a $1" =~ /(?{push @​a,b $1})/';

I'm looking into it.

--
Indomitable in retreat, invincible in advance, insufferable in victory
  -- Churchill on Montgomery

[p5pRT]

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

[p5pRT]

From imdb95@gmail.com

Thanks!

On Jun 18, 2019 10​:16 PM, "Dave Mitchell via RT" <
perl5-security-report-followup@​perl.org> wrote​:

On Tue, Jun 18, 2019 at 06​:21​:03AM -0700, Nguyen Duc Manh wrote​:

I found this bug when fuzzing perlembed with afl-fuzz.

It can be reproduced with a plain perl script run under valgrind; no
embedding required.

eval  '"@​a $1" =~ /\(?\{push @​a\,b $1\}\)/';

I'm looking into it.

--
Indomitable in retreat, invincible in advance, insufferable in victory
-- Churchill on Montgomery

[p5pRT]

From @iabyn

On Tue, Jun 18, 2019 at 04​:16​:48PM +0100, Dave Mitchell wrote​:

It can be reproduced with a plain perl script run under valgrind; no
embedding required.

eval  '"@​a $1" =~ /\(?\{push @​a\,b $1\}\)/';

It can be further reduced to these two examples​:

  eval { "$a $1" =~ /(?{ die })/ };
  eval { sub { " " }->() =~ /(?{ die })/ };

The use-after-free will only occur if
1) perl croaks during the execution of a /(?{...})/ code block; and
2) the string being matched against is an SvTEMP. That condition is
satisfied in the two examples above because function calls return SVTEMPs
and string concatenation using pp_multiconcat uses an SvTEMP when one of
the strings being concatted is magic ($1).

When these conditions are met, the SV being matched is freed, followed
immediately by updating of its magic. Thus, although it's a
use-after-free, it is exceedingly unlikely that the freed mg buffer has
been reallocated in the meantime.

So it's almost certainly harmless, and would be hard for an attacker to
trigger - they would have to find some exiting (?{...}) code that croaks
while matching against a string produced in relatively unusual ways.

I have a fairly trivial fix for it below. I propose that this just gets
committed to blead.

commit 7aacf2906f2cf73782a902008845ed287040dd59
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Wed Jun 19 13​:03​:22 2019 +0100
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Wed Jun 19 13​:03​:22 2019 +0100

  avoid use-after free in /(?{...})/
 
  RT #134208
 
  In something like
 
  eval { sub { " " }->() =~ /(?{ die })/ }
 
  When the match string gets aliased to $_, the SAVE_DEFSV is done after the
  SAVEDESTRUCTOR_X(S_cleanup_regmatch_info_aux). So if croaking, the SV
  gets SvREFCNT_dec()ed by the SAVE_DEFSV, then S_cleanup_regmatch_info_aux()
  manipulates the SV's magic.
 
  This doesn't cause a problem unless the match string is temporary, in
  which case the only other reference keeping it alive will be removed
  by the FREETMPs during the croak.
 
  The fix is to make sure an extra ref to the sv is held.

Affected files ...
  M regexec.c
  M regexp.h
  M t/re/pat_re_eval.t

Differences ...

Inline Patch

diff --git a/regexec.c b/regexec.c
index eaaef94881..bbb73c20ff 100644
--- a/regexec.c
+++ b/regexec.c
@@ -10188,6 +10188,7 @@ S_setup_eval_state(pTHX_ regmatch_info *const reginfo)
     regmatch_info_aux_eval *eval_state = reginfo->info_aux_eval;
 
     eval_state->rex = rex;
+    eval_state->sv  = reginfo->sv;
 
     if (reginfo->sv) {
         /* Make $_ available to executed code. */
@@ -10195,6 +10196,8 @@ S_setup_eval_state(pTHX_ regmatch_info *const reginfo)
             SAVE_DEFSV;
             DEFSV_set(reginfo->sv);
         }
+        /* will be dec'd by S_cleanup_regmatch_info_aux */
+        SvREFCNT_inc_NN(reginfo->sv);
 
         if (!(mg = mg_find_mglob(reginfo->sv))) {
             /* prepare for quick setting of pos */
@@ -10286,6 +10289,7 @@ S_cleanup_regmatch_info_aux(pTHX_ void *arg)
         }
 
         PL_curpm = eval_state->curpm;
+        SvREFCNT_dec(eval_state->sv);
     }
 
     PL_regmatch_state = aux->old_regmatch_state;
diff --git a/regexp.h b/regexp.h
index 0f35205e1a..ccbc64a009 100644
--- a/regexp.h
+++ b/regexp.h
@@ -658,6 +658,7 @@ typedef struct {
     STRLEN  sublen;     /* saved sublen     field from rex */
     STRLEN  suboffset;  /* saved suboffset  field from rex */
     STRLEN  subcoffset; /* saved subcoffset field from rex */
+    SV      *sv;        /* $_  during (?{}) */
     MAGIC   *pos_magic; /* pos() magic attached to $_ */
     SSize_t pos;        /* the original value of pos() in pos_magic */
     U8      pos_flags;  /* flags to be restored; currently only MGf_BYTES*/
diff --git a/t/re/pat_re_eval.t b/t/re/pat_re_eval.t
index 8325451377..7b2bffc33e 100644
--- a/t/re/pat_re_eval.t
+++ b/t/re/pat_re_eval.t
@@ -23,7 +23,7 @@ BEGIN {
 
 our @global;
 
-plan tests => 504;  # Update this when adding/deleting tests.
+plan tests => 506;  # Update this when adding/deleting tests.
 
 run_tests() unless caller;
 
@@ -1317,6 +1317,20 @@ sub run_tests {
         ok "ABC" =~ /^ $runtime_re (?(?{ 0; })xy|BC) $/x, 'RT #133687 yes|no';
     }
 
+    # RT #134208
+    # when string being matched was an SvTEMP and the re eval died,
+    # the SV's magic was being restored after it was freed.
+    # Give ASan something to play with
+
+    {
+        my $a;
+        no warnings 'uninitialized';
+        eval { "$a $1" =~ /(?{ die })/ };
+        pass("SvTEMP 1");
+        eval { sub { " " }->() =~ /(?{ die })/ };
+        pass("SvTEMP 2");
+    }
+




-- 

"But Sidley Park is already a picture, and a most amiable picture too.
The slopes are green and gentle. The trees are companionably grouped at
intervals that show them to advantage. The rill is a serpentine ribbon
unwound from the lake peaceably contained by meadows on which the right
amount of sheep are tastefully arranged." -- Lady Croom, "Arcadia"

[p5pRT]

From @hvds

On Wed, 19 Jun 2019 06​:35​:03 -0700, davem wrote​:

I have a fairly trivial fix for it below. I propose that this just
gets committed to blead.

I think that works for me. Do we have info on how old the problem is?

+++ b/regexp.h
+ SV *sv; /* $_ during (?{}) */

I think that's the sv holding the target string rather than literally $_. It'd be worth refining the comment if so.

Hugo

[p5pRT]

From @iabyn

On Wed, Jun 19, 2019 at 07​:52​:09AM -0700, Hugo van der Sanden via RT wrote​:

On Wed, 19 Jun 2019 06​:35​:03 -0700, davem wrote​:

I have a fairly trivial fix for it below. I propose that this just
gets committed to blead.

I think that works for me. Do we have info on how old the problem is?

Appears to be 5.28.0 onwards for my test cases, although its possible
the flaw is older.

+++ b/regexp.h
+ SV *sv; /* $_ during (?{}) */

I think that's the sv holding the target string rather than literally $_. It'd be worth refining the comment if so.

During execution of /(?{..})/ code blocks, $_ is aliased to the string
being matched against.

--
More than any other time in history, mankind faces a crossroads. One path
leads to despair and utter hopelessness. The other, to total extinction.
Let us pray we have the wisdom to choose correctly.
  -- Woody Allen

[p5pRT]

From @hvds

On Thu, 20 Jun 2019 00​:46​:47 -0700, davem wrote​:

On Wed, Jun 19, 2019 at 07​:52​:09AM -0700, Hugo van der Sanden via RT
wrote​:

On Wed, 19 Jun 2019 06​:35​:03 -0700, davem wrote​:

I have a fairly trivial fix for it below. I propose that this just
gets committed to blead.

I think that works for me. Do we have info on how old the problem is?

Appears to be 5.28.0 onwards for my test cases, although its possible
the flaw is older.

Ok, I'd be inclined to go for it, with the aim of pushing to maint releases too.

+++ b/regexp.h
+ SV *sv; /* $_ during (?{}) */

I think that's the sv holding the target string rather than literally
$_. It'd be worth refining the comment if so.

During execution of /(?{..})/ code blocks, $_ is aliased to the string
being matched against.

Ah ok.

Hugo

[p5pRT]

From @iabyn

On Thu, Mar 21, 2019 at 09​:29​:52AM -0700, Hugo van der Sanden via RT wrote​:

On Tue, 05 Mar 2019 09​:27​:56 -0800, randir wrote​:

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

eval q!s,(?{stat//}),,r!

to perform an access outside of an allocated memory slot.
[...]
This is a regression between 5.26 and 5.28, bisect points to

commit b66d79a
Author​: David Mitchell <davem@​iabyn.com>
Date​: Mon Aug 22 09​:50​:43 2016 +0100

FREETMPS when leaving eval, even when void/dying

Dave, can you take a look at this? I'm not sure where to begin with diagnosis - clearly the FREETMPS change has somehow violated this requirement carefully documented in regexec.c​:
/* SAVEFREESV, not sv_mortalcopy, as this SV must last until after
S_cleanup_regmatch_info_aux has executed (registered by
SAVEDESTRUCTOR_X below). S_cleanup_regmatch_info_aux modifies
magic belonging to this SV.

My initial assumption is that there's no security issue here (needs dying code), but it's just possible it's a symptom of a deeper problem with the FREETMPS change.

This appears to be be the same issue as #134208, which I provided a
proposed fix for yesterday.

--
Wesley Crusher gets beaten up by his classmates for being a smarmy git,
and consequently has a go at making some friends of his own age for a
change.
  -- Things That Never Happen in "Star Trek" #18

[p5pRT]

From imdb95@gmail.com

Hello Dave Mitchell,
I am wondering if this bug is trivial or not.
You said​:
"When these conditions are met, the SV being matched is freed, followed
immediately by updating of its magic. Thus, although it's a
use-after-free, it is exceedingly unlikely that the freed mg buffer has
been reallocated in the meantime.
So it's almost certainly harmless".
After updating of its magic and being able not to crash, is there any other
operation that read/write to the freed memory? If there is, there will be
another use after free. And will the freed memory be freed again? It would
become a double free.
Could you confirm these cases?
Thanks,
Manh Nguyen

On Jun 19, 2019 9​:17 PM, "Peter Nguyen" <imdb95@​gmail.com> wrote​:

Hello

On Jun 19, 2019 9​:13 PM, "Peter Nguyen" <imdb95@​gmail.com> wrote​:

I think that

On Jun 19, 2019 8​:35 PM, "Dave Mitchell via RT" <
perl5-security-report-followup@​perl.org> wrote​:

On Tue, Jun 18, 2019 at 04​:16​:48PM +0100, Dave Mitchell wrote​:

It can be reproduced with a plain perl script run under valgrind; no
embedding required.

eval  '"@​a $1" =~ /\(?\{push @​a\,b $1\}\)/';

It can be further reduced to these two examples​:

eval \{ "$a $1" =~ /\(?\{ die \}\)/ \};
eval \{ sub \{ " " \}\->\(\) =~ /\(?\{ die \}\)/ \};

The use-after-free will only occur if
1) perl croaks during the execution of a /(?{...})/ code block; and
2) the string being matched against is an SvTEMP. That condition is
satisfied in the two examples above because function calls return SVTEMPs
and string concatenation using pp_multiconcat uses an SvTEMP when one of
the strings being concatted is magic ($1).

When these conditions are met, the SV being matched is freed, followed
immediately by updating of its magic. Thus, although it's a
use-after-free, it is exceedingly unlikely that the freed mg buffer has
been reallocated in the meantime.

So it's almost certainly harmless, and would be hard for an attacker to
trigger - they would have to find some exiting (?{...}) code that croaks
while matching against a string produced in relatively unusual ways.

I have a fairly trivial fix for it below. I propose that this just gets
committed to blead.

commit 7aacf2906f2cf73782a902008845ed287040dd59
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Wed Jun 19 13​:03​:22 2019 +0100
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Wed Jun 19 13​:03​:22 2019 +0100

avoid use\-after free in /\(?\{\.\.\.\}\)/

RT \#134208

In something like

    eval \{ sub \{ " " \}\->\(\) =~ /\(?\{ die \}\)/ \}

When the match string gets aliased to $\_\, the SAVE\_DEFSV is done

after the
SAVEDESTRUCTOR_X(S_cleanup_regmatch_info_aux). So if croaking, the
SV
gets SvREFCNT_dec()ed by the SAVE_DEFSV, then
S_cleanup_regmatch_info_aux()
manipulates the SV's magic.

This doesn't cause a problem unless the match string is temporary\, in
which case the only other reference keeping it alive will be removed
by the FREETMPs during the croak\.

The fix is to make sure an extra ref to the sv is held\.

Affected files ...
M regexec.c
M regexp.h
M t/re/pat_re_eval.t

Differences ...

diff --git a/regexec.c b/regexec.c
index eaaef94881..bbb73c20ff 100644
--- a/regexec.c
+++ b/regexec.c
@​@​ -10188,6 +10188,7 @​@​ S_setup_eval_state(pTHX_ regmatch_info *const
reginfo)
regmatch_info_aux_eval *eval_state = reginfo->info_aux_eval;

 eval\_state\->rex = rex;

+ eval_state->sv = reginfo->sv;

 if \(reginfo\->sv\) \{
     /\* Make $\_ available to executed code\. \*/

@​@​ -10195,6 +10196,8 @​@​ S_setup_eval_state(pTHX_ regmatch_info *const
reginfo)
SAVE_DEFSV;
DEFSV_set(reginfo->sv);
}
+ /* will be dec'd by S_cleanup_regmatch_info_aux */
+ SvREFCNT_inc_NN(reginfo->sv);

     if \(\!\(mg = mg\_find\_mglob\(reginfo\->sv\)\)\) \{
         /\* prepare for quick setting of pos \*/

@​@​ -10286,6 +10289,7 @​@​ S_cleanup_regmatch_info_aux(pTHX_ void *arg)
}

     PL\_curpm = eval\_state\->curpm;

+ SvREFCNT_dec(eval_state->sv);
}

 PL\_regmatch\_state = aux\->old\_regmatch\_state;

diff --git a/regexp.h b/regexp.h
index 0f35205e1a..ccbc64a009 100644
--- a/regexp.h
+++ b/regexp.h
@​@​ -658,6 +658,7 @​@​ typedef struct {
STRLEN sublen; /* saved sublen field from rex */
STRLEN suboffset; /* saved suboffset field from rex */
STRLEN subcoffset; /* saved subcoffset field from rex */
+ SV *sv; /* $_ during (?{}) */
MAGIC *pos_magic; /* pos() magic attached to $_ */
SSize_t pos; /* the original value of pos() in pos_magic */
U8 pos_flags; /* flags to be restored; currently only
MGf_BYTES*/
diff --git a/t/re/pat_re_eval.t b/t/re/pat_re_eval.t
index 8325451377..7b2bffc33e 100644
--- a/t/re/pat_re_eval.t
+++ b/t/re/pat_re_eval.t
@​@​ -23,7 +23,7 @​@​ BEGIN {

our @​global;

-plan tests => 504; # Update this when adding/deleting tests.
+plan tests => 506; # Update this when adding/deleting tests.

run_tests() unless caller;

@​@​ -1317,6 +1317,20 @​@​ sub run_tests {
ok "ABC" =~ /^ $runtime_re (?(?{ 0; })xy|BC) $/x, 'RT #133687
yes|no';
}

+ # RT #134208
+ # when string being matched was an SvTEMP and the re eval died,
+ # the SV's magic was being restored after it was freed.
+ # Give ASan something to play with
+
+ {
+ my $a;
+ no warnings 'uninitialized';
+ eval { "$a $1" =~ /(?{ die })/ };
+ pass("SvTEMP 1");
+ eval { sub { " " }->() =~ /(?{ die })/ };
+ pass("SvTEMP 2");
+ }
+

--
"But Sidley Park is already a picture, and a most amiable picture too.
The slopes are green and gentle. The trees are companionably grouped at
intervals that show them to advantage. The rill is a serpentine ribbon
unwound from the lake peaceably contained by meadows on which the right
amount of sheep are tastefully arranged." -- Lady Croom, "Arcadia"

[p5pRT]

From @iabyn

On Wed, Jun 19, 2019 at 02​:34​:54PM +0100, Dave Mitchell wrote​:

I have a fairly trivial fix for it below. I propose that this just gets
committed to blead.

Now pushed to blead as v5.31.2-63-g1d48e83dd8.

I propose that this ticket is moved to the public queue and closed.

--
Little fly, thy summer's play my thoughtless hand
has terminated with extreme prejudice.
  (with apologies to William Blake)