Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use-After-Free in Parser #15009

Closed
p5pRT opened this issue Oct 24, 2015 · 15 comments
Closed

Use-After-Free in Parser #15009

p5pRT opened this issue Oct 24, 2015 · 15 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Oct 24, 2015

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

Searchable as RT126443$

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 24, 2015

From hectohertz@gmail.com

Hi All,

I've been doing some fuzzing of perl with afl, and I've found a use-after
free in the parsing code. Attached are three minimized test cases that
trigger it, all only a few bytes.

All fuzzing was done against a recent
commit​: c37b157

Perl was built with the following​:
./Configure -de -Dusedevel -DEBUGGING -Dcc=clang-3.6
-Accflags="-fsanitize=address -fno-omit-frame-pointer"
-Aldflags="-fsanitize=address -fno-omit-frame-pointer"
-Alddlflags="-fsanitize=address -fno-omit-frame-pointer"

These test-cases produce UAFs in toke.c, here's a trace using address
sanitizer​:

root@​vagrant-ubuntu-trusty-64​:~/perl-debug/perl#
ASAN_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer-3.6 PERL5LIB=./perl/lib
./perl /vagrant/crash1min

=================================================================

==7139==ERROR​: AddressSanitizer​: heap-use-after-free on address
0x60200000e170 at pc 0x0000006182e3 bp 0x7ffd042c3c50 sp 0x7ffd042c3c48

READ of size 1 at 0x60200000e170 thread T0

  #0 0x6182e2 in Perl_yyerror_pvn
/home/vagrant/perl-debug/perl/toke.c​:10966​:9

  #1 0x60cdd9 in Perl_yyerror_pv
/home/vagrant/perl-debug/perl/toke.c​:10936​:12

  #2 0x60cdd9 in S_yywarn /home/vagrant/perl-debug/perl/toke.c​:10921

  #3 0x60cdd9 in S_no_op /home/vagrant/perl-debug/perl/toke.c​:523

  #4 0x5cc1e3 in Perl_yylex /home/vagrant/perl-debug/perl/toke.c​:6472​:6

  #5 0x6203a8 in Perl_yyparse /home/vagrant/perl-debug/perl/perly.c​:322​:19

  #6 0x56ca07 in S_parse_body /home/vagrant/perl-debug/perl/perl.c​:2307​:9

  #7 0x567c97 in perl_parse /home/vagrant/perl-debug/perl/perl.c​:1634​:2

  #8 0x4f8b88 in main /home/vagrant/perl-debug/perl/perlmain.c​:114​:18

  #9 0x7f8ed4161ec4 in __libc_start_main
/build/buildd/eglibc-2.19/csu/libc-start.c​:287

  #10 0x452626 in _start (/home/vagrant/perl-debug/perl/perl+0x452626)

0x60200000e170 is located 0 bytes inside of 10-byte region
[0x60200000e170,0x60200000e17a)

freed by thread T0 here​:

  #0 0x4d9302 in __interceptor_free
(/home/vagrant/perl-debug/perl/perl+0x4d9302)

  #1 0x7c01f3 in Perl_sv_clear /home/vagrant/perl-debug/perl/sv.c​:6610​:7

  #2 0x7c3806 in Perl_sv_free2 /home/vagrant/perl-debug/perl/sv.c​:6885​:9

  #3 0x5dc849 in S_SvREFCNT_dec
/home/vagrant/perl-debug/perl/./inline.h​:166​:6

  #4 0x5dc849 in S_scan_heredoc /home/vagrant/perl-debug/perl/toke.c​:9672

  #5 0x5dc849 in Perl_yylex /home/vagrant/perl-debug/perl/toke.c​:6123

  #6 0x6203a8 in Perl_yyparse /home/vagrant/perl-debug/perl/perly.c​:322​:19

  #7 0x56ca07 in S_parse_body /home/vagrant/perl-debug/perl/perl.c​:2307​:9

  #8 0x567c97 in perl_parse /home/vagrant/perl-debug/perl/perl.c​:1634​:2

  #9 0x4f8b88 in main /home/vagrant/perl-debug/perl/perlmain.c​:114​:18

  #10 0x7f8ed4161ec4 in __libc_start_main
/build/buildd/eglibc-2.19/csu/libc-start.c​:287

previously allocated by thread T0 here​:

  #0 0x4d95e2 in malloc (/home/vagrant/perl-debug/perl/perl+0x4d95e2)

  #1 0x6efdd7 in Perl_safesysmalloc
/home/vagrant/perl-debug/perl/util.c​:153​:21

  #2 0x7898eb in Perl_sv_grow /home/vagrant/perl-debug/perl/sv.c​:1628​:17

  #3 0x79a084 in Perl_sv_setpvn /home/vagrant/perl-debug/perl/sv.c​:4853​:12

  #4 0x7c8083 in Perl_newSVpvn /home/vagrant/perl-debug/perl/sv.c​:9175​:5

  #5 0x5da50f in S_scan_heredoc
/home/vagrant/perl-debug/perl/toke.c​:9626​:20

  #6 0x5da50f in Perl_yylex /home/vagrant/perl-debug/perl/toke.c​:6123

  #7 0x6203a8 in Perl_yyparse /home/vagrant/perl-debug/perl/perly.c​:322​:19

  #8 0x56ca07 in S_parse_body /home/vagrant/perl-debug/perl/perl.c​:2307​:9

  #9 0x567c97 in perl_parse /home/vagrant/perl-debug/perl/perl.c​:1634​:2

  #10 0x4f8b88 in main /home/vagrant/perl-debug/perl/perlmain.c​:114​:18

  #11 0x7f8ed4161ec4 in __libc_start_main
/build/buildd/eglibc-2.19/csu/libc-start.c​:287

SUMMARY​: AddressSanitizer​: heap-use-after-free
/home/vagrant/perl-debug/perl/toke.c​:10966 Perl_yyerror_pvn

Shadow bytes around the buggy address​:

  0x0c047fff9bd0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa

  0x0c047fff9be0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa

  0x0c047fff9bf0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa

  0x0c047fff9c00​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa

  0x0c047fff9c10​: fa fa fa fa fa fa fa fa fa fa 00 02 fa fa fd fd

=>0x0c047fff9c20​: fa fa 00 02 fa fa fd fd fa fa fd fa fa fa[fd]fd

  0x0c047fff9c30​: fa fa 00 00 fa fa 00 02 fa fa fd fd fa fa 00 04

  0x0c047fff9c40​: fa fa 02 fa fa fa 00 02 fa fa 00 02 fa fa 00 01

  0x0c047fff9c50​: fa fa 00 02 fa fa 00 01 fa fa 00 01 fa fa 00 05

  0x0c047fff9c60​: fa fa 00 fa fa fa 00 02 fa fa 05 fa fa fa 00 07

  0x0c047fff9c70​: fa fa 00 05 fa fa 00 03 fa fa 06 fa fa fa 00 02

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

  Heap right redzone​: fb

  Freed heap region​: fd

  Stack left redzone​: f1

  Stack mid redzone​: f2

  Stack right redzone​: f3

  Stack partial redzone​: f4

  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

==7139==ABORTING

I realize similar bugs have been reported in perl before, and this team may
not consider this a security vulnerability. However, other languages have
considered memory corruption in their parsing logic to be a security issue,
so I figured better safe than sorry.

Best,

-jh

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 24, 2015

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 24, 2015

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 24, 2015

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 27, 2015

From @tonycoz

On Fri Oct 23 23​:51​:28 2015, hectohertz@​gmail.com wrote​:

Hi All,

I've been doing some fuzzing of perl with afl, and I've found a use-after
free in the parsing code. Attached are three minimized test cases that
trigger it, all only a few bytes.

The attached seems to fix all three test cases​:

tony@​mars​:.../git/perl$ valgrind -q ./perl ../126443a.pl
Number found where operator expected at ../126443a.pl line 1, near "<<""0"
  (Missing operator before 0?)
syntax error at ../126443a.pl line 1, near "<<""0"
Execution of ../126443a.pl aborted due to compilation errors.
tony@​mars​:.../git/perl$ valgrind -q ./perl ../126443b.pl
Use of bare << to mean <<"" is deprecated at ../126443b.pl line 1.
Scalar found where operator expected at ../126443b.pl line 1, near "<<$
;"
  (Missing operator before ;?)
syntax error at ../126443b.pl line 1, near "<<$
;"
Execution of ../126443b.pl aborted due to compilation errors.
tony@​mars​:.../git/perl$ valgrind -q ./perl ../126443c.pl
Use of bare << to mean <<"" is deprecated at ../126443c.pl line 1.
Scalar found where operator expected at ../126443c.pl line 1, near "<<$
;"
  (Missing operator before ;?)
syntax error at ../126443c.pl line 1, near "<<$
;"
Execution of ../126443c.pl aborted due to compilation errors.
tony@​mars​:.../git/perl$

I realize similar bugs have been reported in perl before, and this team may
not consider this a security vulnerability. However, other languages have
considered memory corruption in their parsing logic to be a security issue,
so I figured better safe than sorry.

I think in general we consider such issues as not a security concern, after all, if an attacker can provide code to the parser, you're already vulnerable.

The failure here is reading freed memory, and possibly sending its contents to stderr, while that will end up reading non-text occasionally, it doesn't *write* to that memory, so it won't corrupt that memory.

So I think this isn't a security issue.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 27, 2015

From @tonycoz

0001-perl-126443-make-sure-PL_oldbufptr-is-preserved-in-s.patch
From 36cd3a65d9abec33cbac02c1bc11dbeccec56b80 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 27 Oct 2015 11:22:19 +1100
Subject: [perl #126443] make sure PL_oldbufptr is preserved in scan_heredoc()

This is later used to update PL_oldoldbufptr, if the token following
the <<FOO is unexpected this caused S_no_op() to access an invalid
*PL_oldoldbufptr.

WIP, needs tests, I suspect it should save/restore more of the parser
buffer pointers.
---
 toke.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/toke.c b/toke.c
index b1bdfad..4ba7c0e 100644
--- a/toke.c
+++ b/toke.c
@@ -9617,12 +9617,14 @@ S_scan_heredoc(pTHX_ char *s)
     else
     {
       SV *linestr_save;
+      char *oldbufptr_save;
      streaming:
       sv_setpvs(tmpstr,"");   /* avoid "uninitialized" warning */
       term = PL_tokenbuf[1];
       len--;
       linestr_save = PL_linestr; /* must restore this afterwards */
       d = s;			 /* and this */
+      oldbufptr_save = PL_oldbufptr;
       PL_linestr = newSVpvs("");
       PL_bufend = SvPVX(PL_linestr);
       while (1) {
@@ -9639,6 +9641,7 @@ S_scan_heredoc(pTHX_ char *s)
 	       restore PL_linestr. */
 	    SvREFCNT_dec_NN(PL_linestr);
 	    PL_linestr = linestr_save;
+            PL_oldbufptr = oldbufptr_save;
 	    goto interminable;
 	}
 	CopLINE_set(PL_curcop, origline);
@@ -9673,6 +9676,7 @@ S_scan_heredoc(pTHX_ char *s)
 	    PL_linestr = linestr_save;
 	    PL_linestart = SvPVX(linestr_save);
 	    PL_bufend = SvPVX(PL_linestr) + SvCUR(PL_linestr);
+            PL_oldbufptr = oldbufptr_save;
 	    s = d;
 	    break;
 	}
-- 
2.1.4

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 27, 2015

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 31, 2015

From hectohertz@gmail.com

I figured as much, well regardless, glad to help catch a bug. If I find
similar bugs in the parser (even if they are out-of-bounds writes), should
I report them to the general bug tracker, or to the security team?

On Mon, Oct 26, 2015 at 8​:30 PM, Tony Cook via RT <
perl5-security-report@​perl.org> wrote​:

On Fri Oct 23 23​:51​:28 2015, hectohertz@​gmail.com wrote​:

Hi All,

I've been doing some fuzzing of perl with afl, and I've found a use-after
free in the parsing code. Attached are three minimized test cases that
trigger it, all only a few bytes.

The attached seems to fix all three test cases​:

tony@​mars​:.../git/perl$ valgrind -q ./perl ../126443a.pl
Number found where operator expected at ../126443a.pl line 1, near "<<""0"
(Missing operator before 0?)
syntax error at ../126443a.pl line 1, near "<<""0"
Execution of ../126443a.pl aborted due to compilation errors.
tony@​mars​:.../git/perl$ valgrind -q ./perl ../126443b.pl
Use of bare << to mean <<"" is deprecated at ../126443b.pl line 1.
Scalar found where operator expected at ../126443b.pl line 1, near "<<$
;"
(Missing operator before ;?)
syntax error at ../126443b.pl line 1, near "<<$
;"
Execution of ../126443b.pl aborted due to compilation errors.
tony@​mars​:.../git/perl$ valgrind -q ./perl ../126443c.pl
Use of bare << to mean <<"" is deprecated at ../126443c.pl line 1.
Scalar found where operator expected at ../126443c.pl line 1, near "<<$
;"
(Missing operator before ;?)
syntax error at ../126443c.pl line 1, near "<<$
;"
Execution of ../126443c.pl aborted due to compilation errors.
tony@​mars​:.../git/perl$

I realize similar bugs have been reported in perl before, and this team
may
not consider this a security vulnerability. However, other languages have
considered memory corruption in their parsing logic to be a security
issue,
so I figured better safe than sorry.

I think in general we consider such issues as not a security concern,
after all, if an attacker can provide code to the parser, you're already
vulnerable.

The failure here is reading freed memory, and possibly sending its
contents to stderr, while that will end up reading non-text occasionally,
it doesn't *write* to that memory, so it won't corrupt that memory.

So I think this isn't a security issue.

Tony

From 36cd3a65d9abec33cbac02c1bc11dbeccec56b80 Mon Sep 17 00​:00​:00 2001
From​: Tony Cook <tony@​develop-help.com>
Date​: Tue, 27 Oct 2015 11​:22​:19 +1100
Subject​: [perl #126443] make sure PL_oldbufptr is preserved in
scan_heredoc()

This is later used to update PL_oldoldbufptr, if the token following
the <<FOO is unexpected this caused S_no_op() to access an invalid
*PL_oldoldbufptr.

WIP, needs tests, I suspect it should save/restore more of the parser
buffer pointers.
---
toke.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/toke.c b/toke.c
index b1bdfad..4ba7c0e 100644
--- a/toke.c
+++ b/toke.c
@​@​ -9617,12 +9617,14 @​@​ S_scan_heredoc(pTHX_ char *s)
else
{
SV *linestr_save;
+ char *oldbufptr_save;
streaming​:
sv_setpvs(tmpstr,""); /* avoid "uninitialized" warning */
term = PL_tokenbuf[1];
len--;
linestr_save = PL_linestr; /* must restore this afterwards */
d = s; /* and this */
+ oldbufptr_save = PL_oldbufptr;
PL_linestr = newSVpvs("");
PL_bufend = SvPVX(PL_linestr);
while (1) {
@​@​ -9639,6 +9641,7 @​@​ S_scan_heredoc(pTHX_ char *s)
restore PL_linestr. */
SvREFCNT_dec_NN(PL_linestr);
PL_linestr = linestr_save;
+ PL_oldbufptr = oldbufptr_save;
goto interminable;
}
CopLINE_set(PL_curcop, origline);
@​@​ -9673,6 +9676,7 @​@​ S_scan_heredoc(pTHX_ char *s)
PL_linestr = linestr_save;
PL_linestart = SvPVX(linestr_save);
PL_bufend = SvPVX(PL_linestr) + SvCUR(PL_linestr);
+ PL_oldbufptr = oldbufptr_save;
s = d;
break;
}
--
2.1.4

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2015

From @tonycoz

On Sat Oct 31 09​:04​:58 2015, hectohertz@​gmail.com wrote​:

I figured as much, well regardless, glad to help catch a bug. If I
find
similar bugs in the parser (even if they are out-of-bounds writes),
should
I report them to the general bug tracker, or to the security team?

To the general bug tracker I think.

I'll move this ticket to the general bug tracker in a couple of days, unless someone objects.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 19, 2015

From @tonycoz

On Mon Oct 26 17​:30​:38 2015, tonyc wrote​:

The attached seems to fix all three test cases​:

Now with a test.

The test may only fail under ASAN/valgrind.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 19, 2015

From @tonycoz

0001-perl-126443-make-sure-PL_oldbufptr-is-preserved-in-s.patch
From c8fa4f7fcdb0936482dc3b0196ef55918993a5cc Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 19 Nov 2015 11:44:48 +1100
Subject: [perl #126443] make sure PL_oldbufptr is preserved in scan_heredoc()

This is later used to update PL_oldoldbufptr, if the token following
the <<FOO is unexpected this caused S_no_op() to access an invalid
*PL_oldoldbufptr.
---
 t/op/heredoc.t | 11 ++++++++++-
 toke.c         |  4 ++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/t/op/heredoc.t b/t/op/heredoc.t
index a239e92..dadf105 100644
--- a/t/op/heredoc.t
+++ b/t/op/heredoc.t
@@ -7,7 +7,7 @@ BEGIN {
 }
 
 use strict;
-plan(tests => 39);
+plan(tests => 40);
 
 
 # heredoc without newline (#65838)
@@ -89,4 +89,13 @@ HEREDOC
         {},
         "long terminator fails correctly"
     );
+
+    # this would read freed memory
+    fresh_perl_like(
+        qq(0<<<<""0\n\n),
+        # valgrind and asan reports an error between these two lines
+        qr/^Number found where operator expected at - line 1, near "<<""0"\s+\(Missing operator/,
+        {},
+        "don't use an invalid oldoldbufptr"
+    );
 }
diff --git a/toke.c b/toke.c
index 2c0a3c9..37fe35d 100644
--- a/toke.c
+++ b/toke.c
@@ -9617,12 +9617,14 @@ S_scan_heredoc(pTHX_ char *s)
     else
     {
       SV *linestr_save;
+      char *oldbufptr_save;
      streaming:
       sv_setpvs(tmpstr,"");   /* avoid "uninitialized" warning */
       term = PL_tokenbuf[1];
       len--;
       linestr_save = PL_linestr; /* must restore this afterwards */
       d = s;			 /* and this */
+      oldbufptr_save = PL_oldbufptr;
       PL_linestr = newSVpvs("");
       PL_bufend = SvPVX(PL_linestr);
       while (1) {
@@ -9639,6 +9641,7 @@ S_scan_heredoc(pTHX_ char *s)
 	       restore PL_linestr. */
 	    SvREFCNT_dec_NN(PL_linestr);
 	    PL_linestr = linestr_save;
+            PL_oldbufptr = oldbufptr_save;
 	    goto interminable;
 	}
 	CopLINE_set(PL_curcop, origline);
@@ -9673,6 +9676,7 @@ S_scan_heredoc(pTHX_ char *s)
 	    PL_linestr = linestr_save;
 	    PL_linestart = SvPVX(linestr_save);
 	    PL_bufend = SvPVX(PL_linestr) + SvCUR(PL_linestr);
+            PL_oldbufptr = oldbufptr_save;
 	    s = d;
 	    break;
 	}
-- 
2.1.4

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 24, 2015

From @tonycoz

On Wed Nov 18 16​:46​:23 2015, tonyc wrote​:

On Mon Oct 26 17​:30​:38 2015, tonyc wrote​:

The attached seems to fix all three test cases​:

Now with a test.

The test may only fail under ASAN/valgrind.

Applied as d3b9036 with a minor change to the description.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 24, 2015

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 13, 2016

From @khwilliamson

Thank you for submitting this report. You have helped make Perl better.
 
With the release of Perl 5.24.0 on May 9, 2016, this and 149 other issues have been resolved.

Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 13, 2016

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

@p5pRT p5pRT closed this May 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.