heap-use-after-free in S_cleanup_regmatch_info_aux #17051
Comments
From @dur-randirCreated by @dur-randirWhile fuzzing perl v5.29.8-21-gde59f38ed9 built with afl and run 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 is located 24 bytes inside of 48-byte region previously allocated by thread T0 here: This is a regression between 5.26 and 5.28, bisect points to commit b66d79a FREETMPS when leaving eval, even when void/dying [ This commit was originally added as v5.25.2-77-g214949f then reverted When a scope is exited normally (e.g. pp_leavetry, pp_leavesub), Perl Info |
From @hvdsOn Tue, 05 Mar 2019 09:27:56 -0800, randir wrote:
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: 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 |
|
The RT System itself - Status changed from 'new' to 'open' |
From imdb95@gmail.comHi, **********Compilation********** 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 AFL_USE_ASAN=1 afl-clang-fast++ perl_crash.cpp -o perl_crash `./perl/perl **********Reproduce********** root@peter-VirtualBox:~/perl_crash_asan# ./perl_crash==10049==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000004668 is located 24 bytes inside of 48-byte region previously allocated by thread T0 here: SUMMARY: AddressSanitizer: heap-use-after-free ************************* Please confirm. Thanks |
From imdb95@gmail.com#include <fstream> typedef const char* CONSTCSTR; int main(int argc, char** argv) PerlInterpreter *my_perl = perl_alloc(); eval_pv("\"@a $1\" =~ /(?{push @a,b $1})/", true); return 0; |
From @iabynOn Tue, Jun 18, 2019 at 06:21:03AM -0700, Nguyen Duc Manh wrote:
It can be reproduced with a plain perl script run under valgrind; no eval '"@a $1" =~ /(?{push @a,b $1})/'; I'm looking into it. -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From imdb95@gmail.comThanks! On Jun 18, 2019 10:16 PM, "Dave Mitchell via RT" <
|
From @iabynOn Tue, Jun 18, 2019 at 04:16:48PM +0100, Dave Mitchell wrote:
It can be further reduced to these two examples: eval { "$a $1" =~ /(?{ die })/ }; The use-after-free will only occur if When these conditions are met, the SV being matched is freed, followed So it's almost certainly harmless, and would be hard for an attacker to I have a fairly trivial fix for it below. I propose that this just gets commit 7aacf2906f2cf73782a902008845ed287040dd59 avoid use-after free in /(?{...})/ Affected files ... Differences ... Inline Patchdiff --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. |
From @hvdsOn Wed, 19 Jun 2019 06:35:03 -0700, davem wrote:
I think that works for me. Do we have info on how old the problem is?
I think that's the sv holding the target string rather than literally $_. It'd be worth refining the comment if so. Hugo |
From @iabynOn Wed, Jun 19, 2019 at 07:52:09AM -0700, Hugo van der Sanden via RT wrote:
Appears to be 5.28.0 onwards for my test cases, although its possible
During execution of /(?{..})/ code blocks, $_ is aliased to the string -- |
From @hvdsOn Thu, 20 Jun 2019 00:46:47 -0700, davem wrote:
Ok, I'd be inclined to go for it, with the aim of pushing to maint releases too.
Ah ok. Hugo |
From @iabynOn Thu, Mar 21, 2019 at 09:29:52AM -0700, Hugo van der Sanden via RT wrote:
This appears to be be the same issue as #134208, which I provided a -- |
From imdb95@gmail.comHello Dave Mitchell, On Jun 19, 2019 9:17 PM, "Peter Nguyen" <imdb95@gmail.com> wrote:
|
From @iabynOn Wed, Jun 19, 2019 at 02:34:54PM +0100, Dave Mitchell wrote:
Now pushed to blead as v5.31.2-63-g1d48e83dd8. I propose that this ticket is moved to the public queue and closed. -- |
From @iabynOn Thu, Jun 20, 2019 at 10:47:31AM +0100, Dave Mitchell wrote:
That fix has now been pushed, and I've just merged this ticket into -- |
|
@iabyn - Status changed from 'open' to 'pending release' |
|
Also searchable as RT133900$ |
Migrated from rt.perl.org#134208 (status was 'pending release')
Searchable as RT134208$
The text was updated successfully, but these errors were encountered: