heap-buffer-overflow (READ of size 1) in S_scan_const (toke.c:3060)

[p5pRT]

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

Searchable as RT132245$

[p5pRT]

From @geeknik

Triggered in 1195d90. Not a security concern as per Hugo in #129342 which
was marked resolved for 5.26.0.

./perl -e 'y//\N{}-0/'

==3236==ERROR​: AddressSanitizer​: heap-buffer-overflow on address
0x602000000eef at pc 0x00000069952c bp 0x7ffdf060f1f0 sp 0x7ffdf060f1e8
READ of size 1 at 0x602000000eef thread T0
  #0 0x69952b in S_scan_const /root/perl/toke.c​:3060​:33
  #1 0x628a3c in Perl_yylex /root/perl/toke.c​:5042​:10
  #2 0x6c7943 in Perl_yyparse /root/perl/perly.c​:340​:34
  #3 0x5bb75b in S_parse_body /root/perl/perl.c​:2450​:9
  #4 0x5b355a in perl_parse /root/perl/perl.c​:1753​:2
  #5 0x505095 in main /root/perl/perlmain.c​:121​:18
  #6 0x7f176def882f in __libc_start_main
/build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c​:291
  #7 0x435fa8 in _start (/root/perl/perl+0x435fa8)

0x602000000eef is located 1 bytes to the left of 10-byte region
[0x602000000ef0,0x602000000efa)
allocated by thread T0 here​:
  #0 0x4d9013 in malloc (/root/perl/perl+0x4d9013)
  #1 0x7ee078 in Perl_safesysmalloc /root/perl/util.c​:153​:21
  #2 0x8e000b in Perl_sv_grow /root/perl/sv.c​:1603​:17
  #3 0x923227 in Perl_newSV /root/perl/sv.c​:5691​:2
  #4 0x68aa5c in S_scan_const /root/perl/toke.c​:2877​:14
  #5 0x628a3c in Perl_yylex /root/perl/toke.c​:5042​:10
  #6 0x6c7943 in Perl_yyparse /root/perl/perly.c​:340​:34
  #7 0x5bb75b in S_parse_body /root/perl/perl.c​:2450​:9
  #8 0x5b355a in perl_parse /root/perl/perl.c​:1753​:2
  #9 0x505095 in main /root/perl/perlmain.c​:121​:18
  #10 0x7f176def882f in __libc_start_main
/build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c​:291

SUMMARY​: AddressSanitizer​: heap-buffer-overflow /root/perl/toke.c​:3060​:33
in S_scan_const

The unminimized version of this testcase results in a slightly different
stack​:

./perl -e 'y/+4N{U0220}/\N{}-\N{U+400220}/\N{U+402<0}/c[rp'

==4003==ERROR​: AddressSanitizer​: heap-buffer-overflow on address
0x6030000006cf at pc 0x00000069914d bp 0x7ffc81e758b0 sp 0x7ffc81e758a8
READ of size 1 at 0x6030000006cf thread T0
  #0 0x69914c in Perl_utf8_hop /root/perl/./inline.h​:946​:13
  #1 0x69914c in S_scan_const /root/perl/toke.c​:3047
  #2 0x628a3c in Perl_yylex /root/perl/toke.c​:5042​:10
  #3 0x6c7943 in Perl_yyparse /root/perl/perly.c​:340​:34
  #4 0x5bb75b in S_parse_body /root/perl/perl.c​:2450​:9
  #5 0x5b355a in perl_parse /root/perl/perl.c​:1753​:2
  #6 0x505095 in main /root/perl/perlmain.c​:121​:18
  #7 0x7f5d033c782f in __libc_start_main
/build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c​:291
  #8 0x435fa8 in _start (/root/perl/perl+0x435fa8)

0x6030000006cf is located 1 bytes to the left of 19-byte region
[0x6030000006d0,0x6030000006e3)
allocated by thread T0 here​:
  #0 0x4d9013 in malloc (/root/perl/perl+0x4d9013)
  #1 0x7ee078 in Perl_safesysmalloc /root/perl/util.c​:153​:21
  #2 0x8e000b in Perl_sv_grow /root/perl/sv.c​:1603​:17
  #3 0x923227 in Perl_newSV /root/perl/sv.c​:5691​:2
  #4 0x68aa5c in S_scan_const /root/perl/toke.c​:2877​:14
  #5 0x628a3c in Perl_yylex /root/perl/toke.c​:5042​:10
  #6 0x6c7943 in Perl_yyparse /root/perl/perly.c​:340​:34
  #7 0x5bb75b in S_parse_body /root/perl/perl.c​:2450​:9
  #8 0x5b355a in perl_parse /root/perl/perl.c​:1753​:2
  #9 0x505095 in main /root/perl/perlmain.c​:121​:18
  #10 0x7f5d033c782f in __libc_start_main
/build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c​:291

SUMMARY​: AddressSanitizer​: heap-buffer-overflow
/root/perl/./inline.h​:946​:13 in Perl_utf8_hop

[p5pRT]

From @tonycoz

On Sun, 08 Oct 2017 03​:41​:21 -0700, brian.carpenter@​gmail.com wrote​:

Triggered in 1195d90. Not a security concern as per Hugo in #129342 which
was marked resolved for 5.26.0.

./perl -e 'y//\N{}-0/'

The first attached patch fixes this for me.

The second fixes a SV leak in the same area of code.

Tony

[p5pRT]

From @tonycoz

0001-perl-132245-don-t-try-to-process-a-char-range-with-n.patch

From 92ecd10d32742cd35f6ac53c77a6c51b50db0e81 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 19 Oct 2017 10:46:04 +1100
Subject: (perl #132245) don't try to process a char range with no preceding
 char

A range like \N{}-0 eventually results in compilation failing, but
before that, get_and_check_backslash_N_name() attempts to treat
the memory before the empty output of \N{} as a character.
---
 t/lib/croak/toke | 6 ++++++
 toke.c           | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/t/lib/croak/toke b/t/lib/croak/toke
index 87d9580..1a7468f 100644
--- a/t/lib/croak/toke
+++ b/t/lib/croak/toke
@@ -413,3 +413,9 @@ EXPECT
 Illegal operator following parameter in a subroutine signature at - line 3, near "($a += 1"
 syntax error at - line 3, near "($a += 1"
 Execution of - aborted due to compilation errors.
+########
+# NAME tr/// range with empty \N{} at the start
+tr//\N{}-0/;
+EXPECT
+Unknown charname '' at - line 1, within string
+Execution of - aborted due to compilation errors.
diff --git a/toke.c b/toke.c
index 46dba4d..79be40c 100644
--- a/toke.c
+++ b/toke.c
@@ -2969,9 +2969,9 @@ S_scan_const(pTHX_ char *start)
 
                 /* Here, we don't think we're in a range.  If the new character
                  * is not a hyphen; or if it is a hyphen, but it's too close to
-                 * either edge to indicate a range, then it's a regular
-                 * character. */
-                if (*s != '-' || s >= send - 1 || s == start) {
+                 * either edge to indicate a range, or if we haven't output any
+                 * characters yet then it's a regular character. */
+                if (*s != '-' || s >= send - 1 || s == start || d == SvPVX(sv)) {
 
                     /* A regular character.  Process like any other, but first
                      * clear any flags */
-- 
2.1.4

[p5pRT]

From @tonycoz

0002-perl-132245-don-t-leak-on-N.patch

From a24241f5473549aee40d36bedbd6a714a81a110f Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 19 Oct 2017 10:47:22 +1100
Subject: (perl #132245) don't leak on \N{}

get_and_check_backslash_N_name() failed to free its working SV if
the name was empty.
---
 t/op/svleak.t | 7 ++++++-
 toke.c        | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/op/svleak.t b/t/op/svleak.t
index e4e881d..7226dd8 100644
--- a/t/op/svleak.t
+++ b/t/op/svleak.t
@@ -15,7 +15,7 @@ BEGIN {
 
 use Config;
 
-plan tests => 141;
+plan tests => 142;
 
 # run some code N times. If the number of SVs at the end of loop N is
 # greater than (N-1)*delta at the end of loop 1, we've got a leak
@@ -593,3 +593,8 @@ EOF
     }
     ::leak(2, 0, \&named, "Perl_reg_named_buff_fetch() on no-name RE");
 }
+
+{
+    sub N_leak { eval 'tr//\N{}-0/' }
+    ::leak(2, 0, \&N_leak, "a bad \\N{} in a range leaks");
+}
diff --git a/toke.c b/toke.c
index 79be40c..58a651b 100644
--- a/toke.c
+++ b/toke.c
@@ -2595,6 +2595,7 @@ S_get_and_check_backslash_N_name(pTHX_ const char* s, const char* const e)
     PERL_ARGS_ASSERT_GET_AND_CHECK_BACKSLASH_N_NAME;
 
     if (!SvCUR(res)) {
+        SvREFCNT_dec_NN(res);
         /* diag_listed_as: Unknown charname '%s' */
         yyerror("Unknown charname ''");
         return NULL;
-- 
2.1.4

[p5pRT]

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

[p5pRT]

From @tonycoz

On Wed, 18 Oct 2017 16​:55​:37 -0700, tonyc wrote​:

On Sun, 08 Oct 2017 03​:41​:21 -0700, brian.carpenter@​gmail.com wrote​:

Triggered in 1195d90. Not a security concern as per Hugo in #129342 which
was marked resolved for 5.26.0.

./perl -e 'y//\N{}-0/'

The first attached patch fixes this for me.

The second fixes a SV leak in the same area of code.

Applied as e8d55f2 and ebcc725.

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'