heap-buffer-overflow (WRITE of size 8) in Perl_pp_reverse

[p5pRT]

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

Searchable as RT132544$

[p5pRT]

From @geeknik

This bug is triggered in Perl v5.27.6-156-g5d4548b73b compiled with Clang
6.0.0-trunk and -fsanitize=address. I thought this was fixed back in June
with #131555 (which is still marked private), so maybe this is a
regression?

./perl -e 'for$0(0..1000){()=(0..$0,scalar reverse)}'

==16254==ERROR​: AddressSanitizer​: heap-buffer-overflow on address
0x619000000480 at pc 0x000000995f56 bp 0x7ffcd64a7470 sp 0x7ffcd64a7468
WRITE of size 8 at 0x619000000480 thread T0
  #0 0x995f55 in Perl_pp_reverse /root/perl/pp.c​:5663​:2
  #1 0x7b4868 in Perl_runops_debug /root/perl/dump.c​:2495​:23
  #2 0x5a68b1 in S_run_body /root/perl/perl.c
  #3 0x5a5efb in perl_run /root/perl/perl.c​:2517​:2
  #4 0x5035b7 in main /root/perl/perlmain.c​:123​:9
  #5 0x7fcd0f7233f0 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x203f0)
  #6 0x436109 in _start (/root/perl/perl+0x436109)

0x619000000480 is located 0 bytes to the right of 1024-byte region
[0x619000000080,0x619000000480)
freed by thread T0 here​:
  #0 0x4d7242 in realloc
/b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cc​:107​:3
  #1 0x7b9ab4 in Perl_safesysrealloc /root/perl/util.c​:271​:18
  #2 0x847b36 in Perl_av_extend_guts /root/perl/av.c​:163​:3
  #3 0x9a9ef2 in Perl_stack_grow /root/perl/scope.c​:57​:5
  #4 0x995c29 in Perl_pp_reverse /root/perl/pp.c​:5624​:13
  #5 0x7b4868 in Perl_runops_debug /root/perl/dump.c​:2495​:23
  #6 0x5a68b1 in S_run_body /root/perl/perl.c
  #7 0x5a5efb in perl_run /root/perl/perl.c​:2517​:2
  #8 0x5035b7 in main /root/perl/perlmain.c​:123​:9
  #9 0x7fcd0f7233f0 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x203f0)

previously allocated by thread T0 here​:
  #0 0x4d6e43 in malloc
/b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cc​:88​:3
  #1 0x7b9538 in Perl_safesysmalloc /root/perl/util.c​:153​:21
  #2 0x8479ee in Perl_av_extend_guts /root/perl/av.c​:186​:3
  #3 0x9aa084 in Perl_new_stackinfo /root/perl/scope.c​:78​:5
  #4 0x59275f in Perl_init_stacks /root/perl/perl.c​:4205​:23
  #5 0x5909ca in perl_construct /root/perl/perl.c​:271​:5
  #6 0x50340e in main /root/perl/perlmain.c​:117​:2
  #7 0x7fcd0f7233f0 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x203f0)

SUMMARY​: AddressSanitizer​: heap-buffer-overflow /root/perl/pp.c​:5663​:2 in
Perl_pp_reverse

[p5pRT]

From zefram@fysh.org

Brian Carpenter wrote​:

./perl -e 'for$0(0..1000){()=(0..$0,scalar reverse)}'

Thanks. The stack reallocation logic in pp_reverse is faulty.
Attached patch fixes.

I'm not sure about whether this should be a security ticket. I wouldn't
have thought to class it so if I'd discovered the bug myself, but there
is some sense in so classifying it. I'm holding off from pushing the
fix to blead, pending instructions here.

-zefram

[p5pRT]

From zefram@fysh.org

0001-don-t-lose-mark-when-pp_reverse-extends-stack.patch

From c9bfd95aa536fc56adcac57d5982ff324dd56979 Mon Sep 17 00:00:00 2001
From: Zefram <zefram@fysh.org>
Date: Fri, 8 Dec 2017 19:23:29 +0000
Subject: [PATCH] don't lose mark when pp_reverse extends stack

Nullary reverse needs to extend the stack to push its result scalar.
It was actually extending the stack, but doing so invalidated MARK,
which it relied upon to place the stack pointer afterwards.  Upon stack
reallocation it was therefore leaving the stack pointer pointing to the
freed stack memory.  Reformulate stack manipulation to not rely on MARK
after extending.  Fixes [perl #132544].
---
 pp.c           | 13 +++++++------
 t/op/reverse.t |  7 ++++++-
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/pp.c b/pp.c
index 130019f..3bf04d1 100644
--- a/pp.c
+++ b/pp.c
@@ -5615,13 +5615,16 @@ PP(pp_reverse)
 	STRLEN len;
 
 	SvUTF8_off(TARG);				/* decontaminate */
-	if (SP - MARK > 1)
+	if (SP - MARK > 1) {
 	    do_join(TARG, &PL_sv_no, MARK, SP);
-	else if (SP > MARK)
+	    SP = MARK + 1;
+	    SETs(TARG);
+	} else if (SP > MARK) {
 	    sv_setsv(TARG, *SP);
-        else {
+	    SETs(TARG);
+        } else {
 	    sv_setsv(TARG, DEFSV);
-            EXTEND(SP, 1);
+	    XPUSHs(TARG);
 	}
 
 	up = SvPV_force(TARG, len);
@@ -5659,8 +5662,6 @@ PP(pp_reverse)
 	    }
 	    (void)SvPOK_only_UTF8(TARG);
 	}
-	SP = MARK + 1;
-	SETTARG;
     }
     RETURN;
 }
diff --git a/t/op/reverse.t b/t/op/reverse.t
index fd06560..a7d3178 100644
--- a/t/op/reverse.t
+++ b/t/op/reverse.t
@@ -6,7 +6,7 @@ BEGIN {
     set_up_inc('../lib');
 }
 
-plan tests => 25;
+plan tests => 26;
 
 is(reverse("abc"), "cba", 'simple reverse');
 
@@ -105,3 +105,8 @@ SKIP: {
     ok defined $a[-1] && ${$a[-1]} eq '1', "in-place reverse strengthens weak reference";
     ok defined $a[2] && ${$a[2]} eq '3', "in-place reverse strengthens weak reference in the middle";
 }
+
+# [perl #132544] stack pointer used to go wild when nullary reverse
+# required extending the stack
+for(0..1000){()=(0..$_,scalar reverse )}
+pass "extending the stack without crashing";
-- 
2.1.4

[p5pRT]

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

[p5pRT]

From zefram@fysh.org

I wrote​:

I'm not sure about whether this should be a security ticket. I wouldn't
have thought to class it so if I'd discovered the bug myself, but there
is some sense in so classifying it. I'm holding off from pushing the
fix to blead, pending instructions here.

Prod. Anyone got an opinion?

-zefram

[p5pRT]

From @tonycoz

On Wed, 13 Dec 2017 12​:17​:26 -0800, zefram@​fysh.org wrote​:

I wrote​:

I'm not sure about whether this should be a security ticket. I wouldn't
have thought to class it so if I'd discovered the bug myself, but there
is some sense in so classifying it. I'm holding off from pushing the
fix to blead, pending instructions here.

Prod. Anyone got an opinion?

We don't really have a fixed policy for this stuff.

As a heap buffer overflow it could corrupt the heap, possibly leading to denial of service attacks on some platform, iff an attacker can cause reverse() to execute under right conditions. An attacker has little to no control over the value written to the buffer (an SV pointer).

In general I don't think we've treated such overflows as security issues, see #131555 for example.

Based on past practice we shouldn't treat this as a security issue.

Tony

[p5pRT]

From @tonycoz

On Thu, 07 Dec 2017 20​:21​:58 -0800, brian.carpenter@​gmail.com wrote​:

This bug is triggered in Perl v5.27.6-156-g5d4548b73b compiled with
Clang
6.0.0-trunk and -fsanitize=address. I thought this was fixed back in
June
with #131555 (which is still marked private), so maybe this is a
regression?

#131555 is now public.

The difference is #131555 it was a simple buffer overflow.

In this case we're writing past the end of a buffer that has already been freed - the bug was introduced by the fix for #131555.

I had a quick look over the other #131555 fixes, but didn't see any similar problems.

Tony

[p5pRT]

From @iabyn

On Wed, Dec 13, 2017 at 02​:44​:45PM -0800, Tony Cook via RT wrote​:

On Thu, 07 Dec 2017 20​:21​:58 -0800, brian.carpenter@​gmail.com wrote​:

This bug is triggered in Perl v5.27.6-156-g5d4548b73b compiled with
Clang
6.0.0-trunk and -fsanitize=address. I thought this was fixed back in
June
with #131555 (which is still marked private), so maybe this is a
regression?

#131555 is now public.

The difference is #131555 it was a simple buffer overflow.

In this case we're writing past the end of a buffer that has already been freed - the bug was introduced by the fix for #131555.

In which case the new bug hasn't appeared in a production release, so it
should be safe to just push the fix and make the ticket public.

--
But Pity stayed his hand. "It's a pity I've run out of bullets",
he thought. -- "Bored of the Rings"

[p5pRT]

From zefram@fysh.org

Dave Mitchell wrote​:

In which case the new bug hasn't appeared in a production release, so it
should be safe to just push the fix and make the ticket public.

OK. Fix applied to blead as commit
47836a1.

-zefram

[p5pRT]

From @tonycoz

On Thu, 14 Dec 2017 11​:43​:57 -0800, zefram@​fysh.org wrote​:

Dave Mitchell wrote​:

In which case the new bug hasn't appeared in a production release, so it
should be safe to just push the fix and make the ticket public.

True, I've moved it to the public queue.

OK. Fix applied to blead as commit
47836a1.

And closed it.

Tony

[p5pRT]

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

[p5pRT]

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release yesterday of Perl 5.28.0, this and 185 other issues have been
resolved.

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

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

[p5pRT]

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