From mtowalski@pentest.net.pl

Hello,

I've attached the poc and the asan log.
Tested on git version of perl.

Configure options​:

“./Configure -des -Dusedevel -DDEBUGGING -Dcc=clang -Doptimize=-O2 -Accflags="-fsanitize=address -fsanitize-coverage=edge" -Aldflags="-fsanitize=address -fsanitize-coverage=edge" -Alddlflags=-shared"

Information about configuration​:

Distributor ID​: Ubuntu
Description​: Ubuntu 16.10
Release​: 16.10
Codename​: yakkety
Arch​: x86_64

ps. last crash from this round of fuzzing

Best Regards,
Marcin T.

From mtowalski@pentest.net.pl

SEGV-b40-108-c44

From mtowalski@pentest.net.pl

/usr/bin/llvm-symbolizer
perl​: warning​: Setting locale failed.
perl​: warning​: Please check that your locale settings​:
  LANGUAGE = (unset),
  LC_ALL = (unset),
  LC_CTYPE = "UTF-8",
  LANG = "en_US.UTF-8"
  are supported and installed on your system.
perl​: warning​: Falling back to a fallback locale ("en_US.UTF-8").
Use of "goto" to jump into a construct is deprecated at ./SEGV-b40-108-c44 line 2.
ASAN​:DEADLYSIGNAL

==11490==ERROR​: AddressSanitizer​: SEGV on unknown address 0x618df5f5f678 (pc 0x0000008d9b41 bp 0x7ffc51df4390 sp 0x7ffc51df42c0 T0)
==11490==The signal is caused by a READ memory access.
  #0 0x8d9b40 in S_opmethod_stash /home/mtowalski/Fuzzing/Programs/perl-git/pp_hot.c​:4408​:5
  #1 0x8db108 in Perl_pp_method_named /home/mtowalski/Fuzzing/Programs/perl-git/pp_hot.c​:4523​:23
  #2 0x7fbc44 in Perl_runops_debug /home/mtowalski/Fuzzing/Programs/perl-git/dump.c​:2451​:23
  #3 0x5e7bb3 in perl_run /home/mtowalski/Fuzzing/Programs/perl-git/perl.c
  #4 0x524302 in main /home/mtowalski/Fuzzing/Programs/perl-git/perlmain.c​:123​:9
  #5 0x7f33d13be3f0 in __libc_start_main /build/glibc-jxM2Ev/glibc-2.24/csu/../csu/libc-start.c​:291
  #6 0x4356f9 in _start (/home/mtowalski/Fuzzing/Programs/perl-git/perl+0x4356f9)

AddressSanitizer can not provide additional info.
SUMMARY​: AddressSanitizer​: SEGV /home/mtowalski/Fuzzing/Programs/perl-git/pp_hot.c​:4408​:5 in S_opmethod_stash
==11490==ABORTING

From @arc

via RT <perl5-security-report@​perl.org> wrote​:

I've attached the poc and the asan log.

Reduction​:

$ ./miniperl -e 'goto X; meth {X​:}'
Use of "goto" to jump into a construct is deprecated at -e line 1.
ASAN​:SIGSEGV

==73698==ERROR​: AddressSanitizer​: SEGV on unknown address
0x618df5f6af78 (pc 0x000102b549ff bp 0x7fff5d49fe90 sp 0x7fff5d49fda0
T0)
  #0 0x102b549fe in S_opmethod_stash (miniperl+0x1003f59fe)
  #1 0x102b55fca in Perl_pp_method_named (miniperl+0x1003f6fca)
  #2 0x102b0f822 in Perl_runops_standard (miniperl+0x1003b0822)
  #3 0x10283eb69 in perl_run (miniperl+0x1000dfb69)
  #4 0x102f46c7f in main (miniperl+0x1007e7c7f)
  #5 0x7fff92f4a5fc in start (libdyld.dylib+0x35fc)
  #6 0x2 (<unknown module>)

AddressSanitizer can not provide additional info.
SUMMARY​: AddressSanitizer​: SEGV (miniperl+0x1003f59fe) in S_opmethod_stash
==73698==ABORTING
Abort trap​: 6

Patch attached. I think this isn't a security vulnerability, as it
doesn't involve attacker-controlled data. I therefore propose it for
application to blead before 5.26.0; Sawyer?

I believe that other uses of TOPMARK may have similar bugs. For example​:

$ ./miniperl -e 'goto X; map { X​: } ()'
Use of "goto" to jump into a construct is deprecated at -e line 1.

==93438==ERROR​: AddressSanitizer​: heap-buffer-overflow on address
0x60c00000bf7c at pc 0x0001101addf8 bp 0x7fff4ffa8e40 sp
0x7fff4ffa8e38
READ of size 4 at 0x60c00000bf7c thread T0
  #0 0x1101addf7 in Perl_pp_mapwhile (miniperl+0x100557df7)
  #1 0x110006782 in Perl_runops_standard (miniperl+0x1003b0782)
  #2 0x10fd35ac9 in perl_run (miniperl+0x1000dfac9)
  #3 0x11043dc7f in main (miniperl+0x1007e7c7f)
  #4 0x7fff92f4a5fc in start (libdyld.dylib+0x35fc)
  #5 0x2 (<unknown module>)

0x60c00000bf7c is located 4 bytes to the left of 128-byte region
[0x60c00000bf80,0x60c00000c000)
allocated by thread T0 here​:
  #0 0x110626770 in wrap_malloc (libclang_rt.asan_osx_dynamic.dylib+0x46770)
  #1 0x10ff45d41 in Perl_safesysmalloc (miniperl+0x1002efd41)
  #2 0x10fd1b299 in perl_construct (miniperl+0x1000c5299)
  #3 0x11043dbcc in main (miniperl+0x1007e7bcc)
  #4 0x7fff92f4a5fc in start (libdyld.dylib+0x35fc)
  #5 0x2 (<unknown module>)

I'm not sure on how to fix that.

Or entersub, the prospect of hacking on which scares me even more​:

$ ./miniperl -e 'sub f {} goto X; f(do { X​: })'
Use of "goto" to jump into a construct is deprecated at -e line 1.
ASAN​:SIGSEGV

==93645==ERROR​: AddressSanitizer​: SEGV on unknown address
0x618df5f6af78 (pc 0x000101dd7ce7 bp 0x7fff5e216ef0 sp 0x7fff5e216d80
T0)
  #0 0x101dd7ce6 in Perl_pp_entersub (miniperl+0x1003efce6)
  #1 0x101d98782 in Perl_runops_standard (miniperl+0x1003b0782)
  #2 0x101ac7ac9 in perl_run (miniperl+0x1000dfac9)
  #3 0x1021cfc7f in main (miniperl+0x1007e7c7f)
  #4 0x7fff92f4a5fc in start (libdyld.dylib+0x35fc)
  #5 0x2 (<unknown module>)

AddressSanitizer can not provide additional info.
SUMMARY​: AddressSanitizer​: SEGV (miniperl+0x1003efce6) in Perl_pp_entersub
==93645==ABORTING
Abort trap​: 6

At the p5h we decided to remove goto-into-a-construct in the 5.27.x
series, but it got reprieved after objections on the mailing list.
Perhaps we should reconsider its reprieval.

--
Aaron Crane ** http​://aaroncrane.co.uk/

From @arc

From d3b18bad523a65512801ee0e9a8d1a78d5bb4755 Mon Sep 17 00:00:00 2001
From: Aaron Crane <arc@cpan.org>
Date: Mon, 6 Mar 2017 18:10:34 +0000
Subject: [PATCH] RT#130936: segfault on goto into empty invocant block

---
 pp_hot.c      | 10 +++++++---
 t/op/method.t |  9 ++++++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/pp_hot.c b/pp_hot.c
index 58bbe2f1e9..ae31281ca9 100644
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -4398,15 +4398,19 @@ S_opmethod_stash(pTHX_ SV* meth)
 {
     SV* ob;
     HV* stash;
+    SV* sv;
+
+    PERL_ARGS_ASSERT_OPMETHOD_STASH;
 
-    SV* const sv = PL_stack_base + TOPMARK == PL_stack_sp
+    if (PL_markstack_ptr <= PL_markstack)
+        goto undefined; /* No mark on stack */
+
+    sv = PL_stack_base + TOPMARK == PL_stack_sp
 	? (Perl_croak(aTHX_ "Can't call method \"%" SVf "\" without a "
 			    "package or object reference", SVfARG(meth)),
 	   (SV *)NULL)
 	: *(PL_stack_base + TOPMARK + 1);
 
-    PERL_ARGS_ASSERT_OPMETHOD_STASH;
-
     if (UNLIKELY(!sv))
        undefined:
 	Perl_croak(aTHX_ "Can't call method \"%" SVf "\" on an undefined value",
diff --git a/t/op/method.t b/t/op/method.t
index ef181c4ce0..e3d4428b20 100644
--- a/t/op/method.t
+++ b/t/op/method.t
@@ -13,7 +13,7 @@ BEGIN {
 use strict;
 no warnings 'once';
 
-plan(tests => 151);
+plan(tests => 152);
 
 @A::ISA = 'B';
 @B::ISA = 'C';
@@ -711,6 +711,13 @@ fresh_perl_is('eval { {}->$x }; print $@;',
               {},
               "no crash with undef method name on unblessed ref");
 
+# RT#130936: segfault on goto into invocant block with no mark on stack
+fresh_perl_is('goto X; meth {X: }',
+              qq/Use of "goto" to jump into a construct is deprecated at - line 1.\n/.
+              qq/Can't call method "meth" on an undefined value at - line 1./,
+              {},
+              "no segfault on goto into invocant block");
+
 __END__
 #FF9900
 #F78C08
-- 
2.11.0

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

From @iabyn

On Mon, Mar 06, 2017 at 06​:20​:48PM +0000, Aaron Crane wrote​:

via RT <perl5-security-report@​perl.org> wrote​:

I've attached the poc and the asan log.

Reduction​:

$ ./miniperl -e 'goto X; meth {X​:}'
Use of "goto" to jump into a construct is deprecated at -e line 1.

Patch attached.

Unless I'm misreading that, I don't think the patch really fixes the
underlying issue​: it checks whether there is a missing mark by seeing if
the mark stack is empty. But there could be other marks on the stack, so
you can't tell if just one happens to be missing.

I think this isn't a security vulnerability, as it
doesn't involve attacker-controlled data.

Agreed.

I therefore propose it for
application to blead before 5.26.0; Sawyer?

Given my reservations above, lets not!

I believe that other uses of TOPMARK may have similar bugs. For example​:

$ ./miniperl -e 'goto X; map { X​: } ()'
Use of "goto" to jump into a construct is deprecated at -e line 1.

IIUC, what really needs to happen is for goto, whenever it notices that's
about to leap into a contruct (i.e. where it would issue the deprecation
warning), either needs to forbid the goto, or needs to completely set up
the context, mark, save, scope stacks etc to accurately fake up as if the
contstruct had been successfully entered previously.
I think this latter would be very hard in practice. Therefore...

At the p5h we decided to remove goto-into-a-construct in the 5.27.x
series, but it got reprieved after objections on the mailing list.

(In http​://nntp.perl.org/group/perl.perl5.porters/242200)

Perhaps we should reconsider its reprieval.

+1

I propose we move this ticket to the public queue for any further
discussion.

--
My get-up-and-go just got up and went.

From @arc

Dave Mitchell <davem@​iabyn.com> wrote​:

On Mon, Mar 06, 2017 at 06​:20​:48PM +0000, Aaron Crane wrote​:

$ ./miniperl -e 'goto X; meth {X​:}'
Use of "goto" to jump into a construct is deprecated at -e line 1.

Patch attached.

Unless I'm misreading that, I don't think the patch really fixes the
underlying issue​: it checks whether there is a missing mark by seeing if
the mark stack is empty. But there could be other marks on the stack, so
you can't tell if just one happens to be missing.

Ah. This is presumably where it becomes apparent that I don't fully
understand the way this stuff works, and I'm just faking it :-)

I therefore propose it for
application to blead before 5.26.0; Sawyer?

Given my reservations above, lets not!

OK. In the light of your concerns about the approach my patch takes, I
suspect that any complete fix would be sufficiently invasive that it
wouldn't be a candidate for a 5.26.x maint release.

At the p5h we decided to remove goto-into-a-construct in the 5.27.x
series, but it got reprieved after objections on the mailing list.

(In http​://nntp.perl.org/group/perl.perl5.porters/242200)

Perhaps we should reconsider its reprieval.

+1

I propose we move this ticket to the public queue for any further
discussion.

Seconded.

--
Aaron Crane ** http​://aaroncrane.co.uk/

From @iabyn

On Fri, Mar 17, 2017 at 05​:07​:35PM +0000, Aaron Crane wrote​:

Dave Mitchell <davem@​iabyn.com> wrote​:

On Mon, Mar 06, 2017 at 06​:20​:48PM +0000, Aaron Crane wrote​:

$ ./miniperl -e 'goto X; meth {X​:}'
Use of "goto" to jump into a construct is deprecated at -e line 1.

Patch attached.

Unless I'm misreading that, I don't think the patch really fixes the
underlying issue​: it checks whether there is a missing mark by seeing if
the mark stack is empty. But there could be other marks on the stack, so
you can't tell if just one happens to be missing.

Ah. This is presumably where it becomes apparent that I don't fully
understand the way this stuff works, and I'm just faking it :-)

I therefore propose it for
application to blead before 5.26.0; Sawyer?

Given my reservations above, lets not!

OK. In the light of your concerns about the approach my patch takes, I
suspect that any complete fix would be sufficiently invasive that it
wouldn't be a candidate for a 5.26.x maint release.

At the p5h we decided to remove goto-into-a-construct in the 5.27.x
series, but it got reprieved after objections on the mailing list.

(In http​://nntp.perl.org/group/perl.perl5.porters/242200)

Perhaps we should reconsider its reprieval.

+1

I propose we move this ticket to the public queue for any further
discussion.

Seconded.

Now moved.

--
My get-up-and-go just got up and went.

From @cpansprout

Fixed in 6d90e98. The cases that would crash are now forbidden.

--

Father Chrysostomos

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

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.

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