Fuzzer-detected use-after-free in Perl_yylex #15549
Comments
From @dcollinsnFirst, the two testcases: Detected using AFL with libdislocator, but reproducible using valgrind. The first one: ==56912== Invalid read of size 1 The second is just a write instead of a read: ==60617== Invalid write of size 1 The buffer in question is allocated by Perl_lex_start (in the else case of this conditional: if (line) { It is then reallocated in a grow here: if (len && (len != 4 || strNE(PL_tokenbuf+1, "CORE")) In the first case, we then hit this code: retry: In the second testcase, the error happens here: else { When stepping through with libdislocator, which forces the realloc to return a new memory segment, I see the following change in Perl_sv_grow: (gdb) (that is, ~cff6 is the old address, and ~1fe8 is the new address) Stepping out, I get: (gdb) n 1338 buf = SvPVX(linestr); Perl_lex_read_space (flags=2) at toke.c:1530 S_skipspace_flags (s=0x7ffff63acfff "", flags=0) at toke.c:1834 (gdb) info locals More ideas to follow, perhaps. -- |
From @geeknikReads like a dupe of #129021. On Wed, Aug 24, 2016 at 12:48 PM, Dan Collins <perlbug-followup@perl.org>
|
|
The RT System itself - Status changed from 'new' to 'open' |
From @dcollinsnSome diagnosis: Perl_yylex maintains up to two pointers, `s` and `d`, into the parser buffer at PL_bufptr. It can call skipspace(), which can potentially grow (and realloc) its argument. This can leave the second pointer pointing at the old buffer. Under most cases it isn't visible, because the old buffer isn't reused or zeroed. However, under Valgrind or libdislocator, this memory management error becomes visible. Ideally, these would both just be offsets relative to PL_bufptr rather than pointers, but I understand the desire have them be pointers for performance reasons. This would involve refactoring Perl_yylex as well as changing how skipspace is called (argument and retval would be an offset against PL_bufptr instead of a pointer into PL_bufptr). However, even just looking at skipspace, I don't understand this code well enough to do anything like that. The simpler fix is to patch the holes by ensuring that the second pointer is always updated when we call skipspace, as in the attached. That fixes all of my testcases, not sure if Brian has any similar ones. This also `make test`s clean. -- |
From @dcollinsn0001-RT-129069-Perl_yylex-Fix-two-use-after-free-bugs.patchFrom ffdb9c00f4f0fd5a3cfc469ae23b3fe4026b559a Mon Sep 17 00:00:00 2001
From: Dan Collins <dcollinsn@gmail.com>
Date: Wed, 24 Aug 2016 14:19:09 -0400
Subject: [PATCH] [RT #129069] Perl_yylex: Fix two use-after-free bugs
Perl_yylex maintains up to two pointers, `s` and `d`, into the parser
buffer at PL_bufptr. It can call skipspace(), which can potentially
grow (and realloc) its argument. This can leave the second pointer
pointing at the old buffer. Under most cases it isn't visible, because
the old buffer isn't reused or zeroed. However, under Valgrind or
libdislocator, this memory management error becomes visible.
This patch saves the location of the second pointer in two locations,
and restores it after the call to skipspace.
---
toke.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/toke.c b/toke.c
index 2da8366..dd616fc 100644
--- a/toke.c
+++ b/toke.c
@@ -7509,7 +7509,9 @@ Perl_yylex(pTHX)
1, &len);
if (len && (len != 4 || strNE(PL_tokenbuf+1, "CORE"))
&& !keyword(PL_tokenbuf + 1, len, 0)) {
+ SSize_t off = s-PL_bufptr;
d = skipspace(d);
+ s = PL_bufptr+off;
if (*d == '(') {
force_ident_maybe_lex('&');
s = d;
@@ -8285,8 +8287,9 @@ Perl_yylex(pTHX)
const int key = tmp;
SV *format_name = NULL;
- d = s;
+ SSize_t off = s-PL_bufptr;
s = skipspace(s);
+ d = PL_bufptr+off;
if (isIDFIRST_lazy_if(s,UTF)
|| *s == '\''
--
2.8.1
|
From [Unknown Contact. See original ticket]Some diagnosis: Perl_yylex maintains up to two pointers, `s` and `d`, into the parser buffer at PL_bufptr. It can call skipspace(), which can potentially grow (and realloc) its argument. This can leave the second pointer pointing at the old buffer. Under most cases it isn't visible, because the old buffer isn't reused or zeroed. However, under Valgrind or libdislocator, this memory management error becomes visible. Ideally, these would both just be offsets relative to PL_bufptr rather than pointers, but I understand the desire have them be pointers for performance reasons. This would involve refactoring Perl_yylex as well as changing how skipspace is called (argument and retval would be an offset against PL_bufptr instead of a pointer into PL_bufptr). However, even just looking at skipspace, I don't understand this code well enough to do anything like that. The simpler fix is to patch the holes by ensuring that the second pointer is always updated when we call skipspace, as in the attached. That fixes all of my testcases, not sure if Brian has any similar ones. This also `make test`s clean. -- |
From @cpansproutOn Wed Aug 24 10:54:01 2016, brian.carpenter@gmail.com wrote:
RT Error Which ticket did you mean? -- Father Chrysostomos |
From @cpansproutOn Wed Aug 24 12:13:58 2016, dcollinsn@gmail.com wrote:
Both hunks look correct to me. (I have a guilty feeling I may have written one of those bits of code.) Could you add tests too, so that ‘make test.valgrind’ (or whatever it’s called) will catch any regressions? -- Father Chrysostomos |
From @geeknikOn Wed, Aug 24, 2016 at 5:17 PM, Father Chrysostomos via RT <
#129021 is a bug that I reported to the security list on 20 August 2016. |
From @dcollinsnI tried to add tests by using fresh_perl_is() and adding the tests to Come to think of it, running the testsuite under libdislocator as well as On Wed, Aug 24, 2016 at 6:22 PM, Brian Carpenter via RT <
|
From @tonycozOn Wed Aug 24 17:44:41 2016, dcollinsn@gmail.com wrote:
My patch on the security ticket (I don't think this is a security issue +{ For valgrind to detect it you need to get runperl() to use valgrind, so PERL_RUNPERL_DEBUG='valgrind -q' before running the test, for my testing I was doing: PERL_RUNPERL_DEBUG='valgrind -q' ./perl op/do.t I'm not sure basing the position off PL_bufptr is correct, skipspace() if (PL_linestart > PL_bufptr) but FatherC would know better. Tony |
From @dcollinsnYeah, I think you're right Tony. My patch also doesn't fix your testcase, (gdb) p *PL_parser Emphasis on bufptr > bufend. It happens here: Hardware watchpoint 2: PL_parser->bufptr Old value = 0xac2509 ";" I /think/ that skipspace is skipping past the end of the line, so rather |
From @dcollinsn0001-RT-129069-Perl_yylex-Fix-two-use-after-free-bugs.patchFrom 1e347a3324c7bc7b68be50fa02c2d05688ffcad5 Mon Sep 17 00:00:00 2001
From: Dan Collins <dcollinsn@gmail.com>
Date: Wed, 24 Aug 2016 14:19:09 -0400
Subject: [PATCH] [RT #129069] Perl_yylex: Fix two use-after-free bugs
Perl_yylex maintains up to two pointers, `s` and `d`, into the parser
buffer at PL_bufptr. It can call skipspace(), which can potentially
grow (and realloc) its argument. This can leave the second pointer
pointing at the old buffer. Under most cases it isn't visible, because
the old buffer isn't reused or zeroed. However, under Valgrind or
libdislocator, this memory management error becomes visible.
This patch saves the location of the second pointer in two locations,
and restores it after the call to skipspace.
---
t/op/lex.t | 16 +++++++++++++++-
toke.c | 5 ++++-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/t/op/lex.t b/t/op/lex.t
index e68fab4..a667183 100644
--- a/t/op/lex.t
+++ b/t/op/lex.t
@@ -7,7 +7,7 @@ use warnings;
BEGIN { chdir 't' if -d 't'; require './test.pl'; }
-plan(tests => 28);
+plan(tests => 30);
{
no warnings 'deprecated';
@@ -227,3 +227,17 @@ fresh_perl_is(
like runperl(prog => 'sub ub(){0} ub ub', stderr=>1), qr/Bareword found/,
'[perl #126482] Assert failure when mentioning a constant twice in a row';
+
+fresh_perl_is(
+ "do\0"."000000",
+ "",
+ {},
+ '[perl #129069] - no output and valgrind clean'
+);
+
+fresh_perl_is(
+ "00my sub\0",
+ "Missing name in \"my sub\" at - line 1.\n",
+ {},
+ '[perl #129069] - "Missing name" warning and valgrind clean'
+);
diff --git a/toke.c b/toke.c
index 2da8366..dbeecd1 100644
--- a/toke.c
+++ b/toke.c
@@ -7509,7 +7509,9 @@ Perl_yylex(pTHX)
1, &len);
if (len && (len != 4 || strNE(PL_tokenbuf+1, "CORE"))
&& !keyword(PL_tokenbuf + 1, len, 0)) {
+ SSize_t off = s-SvPVX(PL_linestr);
d = skipspace(d);
+ s = SvPVX(PL_linestr)+off;
if (*d == '(') {
force_ident_maybe_lex('&');
s = d;
@@ -8285,8 +8287,9 @@ Perl_yylex(pTHX)
const int key = tmp;
SV *format_name = NULL;
- d = s;
+ SSize_t off = s-SvPVX(PL_linestr);
s = skipspace(s);
+ d = SvPVX(PL_linestr)+off;
if (isIDFIRST_lazy_if(s,UTF)
|| *s == '\''
--
2.8.1
|
From @cpansproutOn Wed Aug 24 18:13:52 2016, tonyc wrote:
Duh. Yes. Thank you for pointing out my thinko (not noticing this). Dan Collins is right that SvPVX(PL_linestr) is the right thing to use. -- Father Chrysostomos |
From @tonycozOn Wed Aug 24 19:51:20 2016, dcollinsn@gmail.com wrote:
Thanks, applied as 3781748. Tony |
|
@tonycoz - Status changed from 'open' to 'pending release' |
From @geeknikOn Sun Aug 28 23:33:26 2016, tonyc wrote:
I think we should re-open this bug. v5.25.5 (v5.25.4-104-g49fc490) od -tx1 test00 ./perl test00==8619==ERROR: AddressSanitizer: heap-use-after-free on address 0x60200000e278 at pc 0x0000006595de bp 0x7ffcba0d6490 sp 0x7ffcba0d6488 0x60200000e278 is located 8 bytes inside of 10-byte region [0x60200000e270,0x60200000e27a) previously allocated by thread T0 here: SUMMARY: AddressSanitizer: heap-use-after-free /root/perl/toke.c:4880 Perl_yylex |
From @tonycozOn Thu Sep 08 17:16:43 2016, brian.carpenter@gmail.com wrote:
This is a different issue, which is duplicated by #129190 (in the security Tony |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release today of Perl 5.26.0, this and 210 other issues have been Perl 5.26.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#129069 (status was 'resolved')
Searchable as RT129069$
The text was updated successfully, but these errors were encountered: