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

heap-buffer-overflow S_scan_heredoc (toke.c:9587) #15546

Closed
p5pRT opened this issue Aug 24, 2016 · 10 comments
Closed

heap-buffer-overflow S_scan_heredoc (toke.c:9587) #15546

p5pRT opened this issue Aug 24, 2016 · 10 comments

Comments

@p5pRT
Copy link

p5pRT commented Aug 24, 2016

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

Searchable as RT129064$

@p5pRT
Copy link
Author

p5pRT commented Aug 24, 2016

From @geeknik

AFL, ASAN and libdislocator trigger a heap-buffer-overflow in Perl
v5.25.4-5-g92d73bf.

od -tx1 test38
0000000 3c 3c 60 5c
0000004

==4872==ERROR​: AddressSanitizer​: heap-buffer-overflow on address
0x60200000e29a at pc 0x0000004aa445 bp 0x7fffd1c76110 sp 0x7fffd1c758d0
READ of size 5 at 0x60200000e29a thread T0
  #0 0x4aa444 in __interceptor_index (/root/perl/perl+0x4aa444)
  #1 0x6468eb in S_scan_heredoc /root/perl/toke.c​:9587​:9
  #2 0x6468eb in Perl_yylex /root/perl/toke.c​:6216
  #3 0x6ac9f5 in Perl_yyparse /root/perl/perly.c​:334​:19
  #4 0x59c4f1 in S_parse_body /root/perl/perl.c​:2373​:9
  #5 0x59288c in perl_parse /root/perl/perl.c​:1689​:2
  #6 0x4de835 in main /root/perl/perlmain.c​:121​:18
  #7 0x7f5626732b44 in __libc_start_main
/build/glibc-uPj9cH/glibc-2.19/csu/libc-start.c​:287
  #8 0x4de4cc in _start (/root/perl/perl+0x4de4cc)

0x60200000e29a is located 0 bytes to the right of 10-byte region
[0x60200000e290,0x60200000e29a)
allocated by thread T0 here​:
  #0 0x4c0e4b in malloc (/root/perl/perl+0x4c0e4b)
  #1 0x7f5bf7 in Perl_safesysmalloc /root/perl/util.c​:153​:21

SUMMARY​: AddressSanitizer​: heap-buffer-overflow ??​:0 __interceptor_index
Shadow bytes around the buggy address​:
  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 fa fa fa fa fa fa
  0x0c047fff9c20​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9c30​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9c40​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa 00 02
=>0x0c047fff9c50​: fa fa 00[02]fa fa 00 04 fa fa 02 fa fa fa 00 02
  0x0c047fff9c60​: fa fa 00 07 fa fa 00 fa fa fa 00 02 fa fa 05 fa
  0x0c047fff9c70​: fa fa 00 02 fa fa 06 fa fa fa 00 02 fa fa 05 fa
  0x0c047fff9c80​: fa fa 00 05 fa fa 04 fa fa fa 05 fa fa fa 05 fa
  0x0c047fff9c90​: fa fa 00 00 fa fa 00 02 fa fa 05 fa fa fa 00 02
  0x0c047fff9ca0​: fa fa 00 fa fa fa 00 04 fa fa 07 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
  ASan internal​: fe
==4872==ABORTING

==7103== Conditional jump or move depends on uninitialised value(s)
==7103== at 0x4C2BC5A​: __GI_strchr (vg_replace_strmem.c​:229)
==7103== by 0x477292​: S_scan_heredoc (toke.c​:9587)
==7103== by 0x477292​: Perl_yylex (toke.c​:6216)
==7103== by 0x48A222​: Perl_yyparse (perly.c​:334)
==7103== by 0x450CC7​: S_parse_body (perl.c​:2373)
==7103== by 0x45285C​: perl_parse (perl.c​:1689)
==7103== by 0x4217AF​: main (perlmain.c​:121)
==7103==
==7103== Conditional jump or move depends on uninitialised value(s)
==7103== at 0x4C2BC60​: __GI_strchr (vg_replace_strmem.c​:229)
==7103== by 0x477292​: S_scan_heredoc (toke.c​:9587)
==7103== by 0x477292​: Perl_yylex (toke.c​:6216)
==7103== by 0x48A222​: Perl_yyparse (perly.c​:334)
==7103== by 0x450CC7​: S_parse_body (perl.c​:2373)
==7103== by 0x45285C​: perl_parse (perl.c​:1689)
==7103== by 0x4217AF​: main (perlmain.c​:121)
==7103==
Can't find string terminator "\" anywhere before EOF at test38 line 1.

@p5pRT
Copy link
Author

p5pRT commented Aug 24, 2016

From @geeknik

test38.gz

@p5pRT
Copy link
Author

p5pRT commented Aug 26, 2016

From @iabyn

On Wed, Aug 24, 2016 at 02​:30​:08AM -0700, Brian Carpenter wrote​:

od -tx1 test38
0000000 3c 3c 60 5c
0000004

==4872==ERROR​: AddressSanitizer​: heap-buffer-overflow on address
0x60200000e29a at pc 0x0000004aa445 bp 0x7fffd1c76110 sp 0x7fffd1c758d0
READ of size 5 at 0x60200000e29a thread T0
#0 0x4aa444 in __interceptor_index (/root/perl/perl+0x4aa444)
#1 0x6468eb in S_scan_heredoc /root/perl/toke.c​:9587​:9
#2 0x6468eb in Perl_yylex /root/perl/toke.c​:6216
#3 0x6ac9f5 in Perl_yyparse /root/perl/perly.c​:334​:19
#4 0x59c4f1 in S_parse_body /root/perl/perl.c​:2373​:9
#5 0x59288c in perl_parse /root/perl/perl.c​:1689​:2
#6 0x4de835 in main /root/perl/perlmain.c​:121​:18
#7 0x7f5626732b44 in __libc_start_main
/build/glibc-uPj9cH/glibc-2.19/csu/libc-start.c​:287
#8 0x4de4cc in _start (/root/perl/perl+0x4de4cc)

0x60200000e29a is located 0 bytes to the right of 10-byte region
[0x60200000e290,0x60200000e29a)

The attached patch (intended to be applied over Tony's pending heredoc
fix) fixes this.

It was a bug in Perl_delimcpy(). This function copies a string
delineated by a specified char, while handling \-delimiter. If the last
char in the string was \ it could overrun by a couple of bytes.

It's not a security issue. Here are the details of the callers of
Perl_delimcpy​:

  S_scan_heredoc​:

  This can only be triggered if a source file (but not eval nor perl
  -e) literally ends with the chars [<<X...something...\], where X is
  one of ["'`] and there is no newline following the backslash -
  i.e. the backslash is the last byte in the file.

  S_scan_inputsymbol

  On something like
  $x = <\
  delimcpy() returns a pointer beyond the end of string, which
  immediately triggers this croak​:

  if (s >= end)
  Perl_croak(aTHX_ "Unterminated <> operator");

  Perl_magic_setenv​:

  Under taint, in $ENV{PATH} = "ABC​:XYZ", it splits the path using
  delimcpy() then examines each component to see whether it's
  non-absolute or world-writeable.

  About the worst that could happen is in

  $ENV{PATH} = "/foo\\"

  where it would check the directory "/foo\\\0" for
  world-writeability, rather than checking "/foo\\". But since
  system calls use \0-terminated strings anyway, it makes no
  difference.

--
I thought I was wrong once, but I was mistaken.

@p5pRT
Copy link
Author

p5pRT commented Aug 26, 2016

From @iabyn

0001-Perl_delimcpy-handle-backslash-as-last-char.patch
From 1d409039e301d0d1d8200252d91cd2ad165995ab Mon Sep 17 00:00:00 2001
From: David Mitchell <davem@iabyn.com>
Date: Thu, 25 Aug 2016 17:48:34 +0100
Subject: [PATCH] Perl_delimcpy(): handle backslash as last char

[perl #129064] heap-buffer-overflow S_scan_heredoc

Perl_delimcpy() is supposed to copy a delimited string to another buffer;
it handles \-<delimiter> escapes, but if the backslash is the last
character in the src buffer, it could overrun the end of the buffer
slightly.

Also document slightly better what this function is supposed to do.
---
 t/op/heredoc.t | 12 +++++++++++-
 util.c         |  9 +++++++--
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/t/op/heredoc.t b/t/op/heredoc.t
index f47f7ce..13d1074 100644
--- a/t/op/heredoc.t
+++ b/t/op/heredoc.t
@@ -7,7 +7,7 @@ BEGIN {
 }
 
 use strict;
-plan(tests => 42);
+plan(tests => 43);
 
 
 # heredoc without newline (#65838)
@@ -115,4 +115,14 @@ HEREDOC
 	{},
 	"Don't assert parsing a here-doc if we hit EOF early"
     );
+
+    # [perl #129064] heap-buffer-overflow S_scan_heredoc
+    fresh_perl_like(
+        qq(<<`\\),
+        # valgrind and asan reports an error between these two lines
+        qr/^Unterminated delimiter for here document/,
+        {},
+        "delimcpy(): handle last char being backslash properly"
+    );
+
 }
diff --git a/util.c b/util.c
index 7748c6c..4579d1e 100644
--- a/util.c
+++ b/util.c
@@ -522,7 +522,12 @@ Free_t   Perl_mfree (Malloc_t where)
 
 #endif
 
-/* copy a string up to some (non-backslashed) delimiter, if any */
+/* copy a string up to some (non-backslashed) delimiter, if any.
+ * Converts \<delimiter> to <delimiter>, while leaves \<non-delimiter>
+ * as-is.
+ * Returns the position in the src string of the closing delimiter, if
+ * any, or returns fromend otherwise
+ * */
 
 char *
 Perl_delimcpy(char *to, const char *toend, const char *from, const char *fromend, int delim, I32 *retlen)
@@ -532,7 +537,7 @@ Perl_delimcpy(char *to, const char *toend, const char *from, const char *fromend
     PERL_ARGS_ASSERT_DELIMCPY;
 
     for (tolen = 0; from < fromend; from++, tolen++) {
-	if (*from == '\\') {
+	if (*from == '\\' && from + 1 < fromend) {
 	    if (from[1] != delim) {
 		if (to < toend)
 		    *to++ = *from;
-- 
2.4.11

@p5pRT
Copy link
Author

p5pRT commented Aug 26, 2016

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

@p5pRT
Copy link
Author

p5pRT commented Sep 7, 2016

From @tonycoz

On Fri Aug 26 04​:19​:56 2016, davem wrote​:

The attached patch (intended to be applied over Tony's pending heredoc
fix) fixes this.

Which pending fix?

Tony

@p5pRT
Copy link
Author

p5pRT commented Sep 7, 2016

From @iabyn

On Tue, Sep 06, 2016 at 09​:28​:52PM -0700, Tony Cook via RT wrote​:

On Fri Aug 26 04​:19​:56 2016, davem wrote​:

The attached patch (intended to be applied over Tony's pending heredoc
fix) fixes this.

Which pending fix?

382450a, which you've now applied.

I've now pushed my fix as 19e1655.

--
I before E. Except when it isn't.

@p5pRT
Copy link
Author

p5pRT commented Sep 8, 2016

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

@p5pRT
Copy link
Author

p5pRT commented May 30, 2017

From @khwilliamson

Thank 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
resolved.

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

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

@p5pRT
Copy link
Author

p5pRT commented May 30, 2017

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

@p5pRT p5pRT closed this as completed May 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant