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_regmatch (regexec.c:6057) #15537

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

heap-buffer-overflow S_regmatch (regexec.c:6057) #15537

p5pRT opened this issue Aug 20, 2016 · 24 comments

Comments

@p5pRT
Copy link

p5pRT commented Aug 20, 2016

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

Searchable as RT129024$

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2016

From @geeknik

I'm attaching 2 test cases to this bug report because the orig556 test case
causes the heap-buffer-overflow, but when minimized to min556, it only
triggers an assertion failure (perl​: pp_hot.c​:3203​: OP *Perl_pp_subst()​:
Assertion `strend >= s' failed).

This was found with AFL, ASAN and libdislocator.so affects v5.25.4
(v5.25.3-305-g8c6b0c7). Perl 5.20.2 complains about a bareword with orig556
and throws a panic​:memory wrap with min556.

==14086==ERROR​: AddressSanitizer​: heap-buffer-overflow on address
0x61900000a4aa at pc 0x000000b9988f bp 0x7fff325d9630 sp 0x7fff325d9628
READ of size 1 at 0x61900000a4aa thread T0
  #0 0xb9988e in S_regmatch /root/perl/regexec.c​:6057​:24
  #1 0xb691ec in S_regtry /root/perl/regexec.c​:3619​:14
  #2 0xb5323c in S_find_byclass /root/perl/regexec.c​:2357​:9
  #3 0xb41439 in Perl_regexec_flags /root/perl/regexec.c​:3368​:13
  #4 0xa36982 in Perl_pp_substcont /root/perl/pp_ctl.c​:225​:18
  #5 0x7f1b53 in Perl_runops_debug /root/perl/dump.c​:2234​:23
  #6 0x5a0ff6 in S_run_body /root/perl/perl.c​:2524​:2
  #7 0x5a0ff6 in perl_run /root/perl/perl.c​:2447
  #8 0x4de68d in main /root/perl/perlmain.c​:123​:9
  #9 0x7fdb3ea25b44 in __libc_start_main
/build/glibc-uPj9cH/glibc-2.19/csu/libc-start.c​:287
  #10 0x4de2fc in _start (/root/perl/perl+0x4de2fc)

0x61900000a4aa is located 2 bytes to the right of 1064-byte region
[0x61900000a080,0x61900000a4a8)
allocated by thread T0 here​:
  #0 0x4c0c7b in malloc (/root/perl/perl+0x4c0c7b)
  #1 0x7f5997 in Perl_safesysmalloc /root/perl/util.c​:153​:21

SUMMARY​: AddressSanitizer​: heap-buffer-overflow /root/perl/regexec.c​:6057
S_regmatch
Shadow bytes around the buggy address​:
  0x0c327fff9440​: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c327fff9450​: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c327fff9460​: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c327fff9470​: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c327fff9480​: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c327fff9490​: 00 00 00 00 00[fa]fa fa fa fa fa fa fa fa fa fa
  0x0c327fff94a0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c327fff94b0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c327fff94c0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c327fff94d0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c327fff94e0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
==14086==ABORTING

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2016

From @geeknik

orig556.gz

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2016

From @geeknik

min556.gz

@p5pRT
Copy link
Author

p5pRT commented Aug 24, 2016

From @tonycoz

On Sat Aug 20 16​:24​:41 2016, brian.carpenter@​gmail.com wrote​:

I'm attaching 2 test cases to this bug report because the orig556 test case
causes the heap-buffer-overflow, but when minimized to min556, it only
triggers an assertion failure (perl​: pp_hot.c​:3203​: OP *Perl_pp_subst()​:
Assertion `strend >= s' failed).

This was found with AFL, ASAN and libdislocator.so affects v5.25.4
(v5.25.3-305-g8c6b0c7). Perl 5.20.2 complains about a bareword with orig556
and throws a panic​:memory wrap with min556.

Also detected by valgrind​:

tony@​mars​:.../git/perl$ valgrind -q ./perl ../129024ab.pl
==17417== Invalid read of size 1
==17417== at 0x6DCB0D​: S_regmatch (regexec.c​:6058)
==17417== by 0x6D3946​: S_regtry (regexec.c​:3619)
==17417== by 0x6CCB9D​: S_find_byclass (regexec.c​:2357)
==17417== by 0x6D2D1F​: Perl_regexec_flags (regexec.c​:3368)
==17417== by 0x65925F​: Perl_pp_substcont (pp_ctl.c​:225)
==17417== by 0x55906B​: Perl_runops_debug (dump.c​:2234)
==17417== by 0x462A94​: S_run_body (perl.c​:2525)
==17417== by 0x4620BF​: perl_run (perl.c​:2448)
==17417== by 0x41EFDD​: main (perlmain.c​:123)
...
perl​: regcomp.c​:8066​: Perl_reg_numbered_buff_fetch​: Assertion `(STRLEN)rx->sublen >= (STRLEN)((s - rx->subbeg) + i)' failed.
Aborted

I've reduced the original test case significantly (129024ab.pl).

00000000 24 5f 3d 0a 20 71 71 0a 2e 47 ff ff 80 00 0a 47 |$_=. qq..G.....G|
0000000 $ _ = \n q q \n . G � � 200 \0 \n G
00000010 2d 00 80 ff ff 2d 2d 2d 54 0a 20 20 20 20 54 80 |-....---T. T.|
0000010 - \0 200 � � - - - T \n T 200
00000020 ff 0a 2e 3b 0a 73 3b 28 5c 77 29 2e 2a 28 5c 77 |...;.s;(\w).*(\w|
0000020 � \n . ; \n s ; ( \ w ) . * ( \ w
00000030 29 2e 2a 5c 42 3b 24 5f 80 7b 20 24 2d 20 2b 2b |).*\B;$_.{ $- ++|
0000030 ) . * \ B ; $ _ 200 { $ - + +
00000040 20 3f 24 32 3a 24 31 7d 3b 67 6c 3b 0a | ?$2​:$1};gl;.|
0000040 ? $ 2 : $ 1 } ; g l ; \n
000004d

Further minimization prevented errors from valgrind, and changed the assertion thrown​:

tony@​mars​:.../git/perl$ valgrind -q ./perl ../129024ac.pl
perl​: pp_ctl.c​:231​: Perl_pp_substcont​: Assertion `cx->cx_u.cx_subst.sbu_strend >= s' failed.
Aborted

00000000 24 5f 3d 0a 20 71 71 0a 2e 47 ff ff 80 00 0a 47 |$_=. qq..G.....G|
0000000 $ _ = \n q q \n . G � � 200 \0 \n G
00000010 2d 00 80 ff ff 2d 2d 2d 54 0a 20 20 20 20 54 80 |-....---T. T.|
0000010 - \0 200 � � - - - T \n T 200
00000020 ff 0a 2e 3b 0a 73 3b 28 5c 77 29 2e 2a 28 5c 77 |...;.s;(\w).*(\w|
0000020 � \n . ; \n s ; ( \ w ) . * ( \ w
00000030 29 3b 24 5f 7b 20 24 2d 20 2b 2b 20 7d 3b 67 6c |);$_{ $- ++ };gl|
0000030 ) ; $ _ { $ - + + } ; g l
00000040 3b 0a |;.|
0000040 ; \n
0000042

which is a similar assetion to (but not the same as) the one your min556
throws.

Tony

@p5pRT
Copy link
Author

p5pRT commented Aug 24, 2016

From @tonycoz

129024ac.pl.gz

@p5pRT
Copy link
Author

p5pRT commented Aug 24, 2016

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

@p5pRT
Copy link
Author

p5pRT commented Aug 30, 2016

From @geeknik

khw asked me on irc if i can check my test cases against blead. neither
test case crashes/asserts with 5.25.4-25-g109ac34*.

On Tue, Aug 23, 2016 at 11​:30 PM, Tony Cook via RT <
perl5-security-report@​perl.org> wrote​:

On Sat Aug 20 16​:24​:41 2016, brian.carpenter@​gmail.com wrote​:

I'm attaching 2 test cases to this bug report because the orig556 test
case
causes the heap-buffer-overflow, but when minimized to min556, it only
triggers an assertion failure (perl​: pp_hot.c​:3203​: OP *Perl_pp_subst()​:
Assertion `strend >= s' failed).

This was found with AFL, ASAN and libdislocator.so affects v5.25.4
(v5.25.3-305-g8c6b0c7). Perl 5.20.2 complains about a bareword with
orig556
and throws a panic​:memory wrap with min556.

Also detected by valgrind​:

tony@​mars​:.../git/perl$ valgrind -q ./perl ../129024ab.pl
==17417== Invalid read of size 1
==17417== at 0x6DCB0D​: S_regmatch (regexec.c​:6058)
==17417== by 0x6D3946​: S_regtry (regexec.c​:3619)
==17417== by 0x6CCB9D​: S_find_byclass (regexec.c​:2357)
==17417== by 0x6D2D1F​: Perl_regexec_flags (regexec.c​:3368)
==17417== by 0x65925F​: Perl_pp_substcont (pp_ctl.c​:225)
==17417== by 0x55906B​: Perl_runops_debug (dump.c​:2234)
==17417== by 0x462A94​: S_run_body (perl.c​:2525)
==17417== by 0x4620BF​: perl_run (perl.c​:2448)
==17417== by 0x41EFDD​: main (perlmain.c​:123)
...
perl​: regcomp.c​:8066​: Perl_reg_numbered_buff_fetch​: Assertion
`(STRLEN)rx->sublen >= (STRLEN)((s - rx->subbeg) + i)' failed.
Aborted

I've reduced the original test case significantly (129024ab.pl).

00000000 24 5f 3d 0a 20 71 71 0a 2e 47 ff ff 80 00 0a 47 |$_=.
qq..G.....G|
0000000 $ _ = \n q q \n . G � � 200 \0 \n G
00000010 2d 00 80 ff ff 2d 2d 2d 54 0a 20 20 20 20 54 80 |-....---T.
T.|
0000010 - \0 200 � � - - - T \n T 200
00000020 ff 0a 2e 3b 0a 73 3b 28 5c 77 29 2e 2a 28 5c 77
|...;.s;(\w).*(\w|
0000020 � \n . ; \n s ; ( \ w ) . * ( \ w
00000030 29 2e 2a 5c 42 3b 24 5f 80 7b 20 24 2d 20 2b 2b |).*\B;$_.{ $-
++|
0000030 ) . * \ B ; $ _ 200 { $ - + +
00000040 20 3f 24 32 3a 24 31 7d 3b 67 6c 3b 0a | ?$2​:$1};gl;.|
0000040 ? $ 2 : $ 1 } ; g l ; \n
000004d

Further minimization prevented errors from valgrind, and changed the
assertion thrown​:

tony@​mars​:.../git/perl$ valgrind -q ./perl ../129024ac.pl
perl​: pp_ctl.c​:231​: Perl_pp_substcont​: Assertion
`cx->cx_u.cx_subst.sbu_strend >= s' failed.
Aborted

00000000 24 5f 3d 0a 20 71 71 0a 2e 47 ff ff 80 00 0a 47 |$_=.
qq..G.....G|
0000000 $ _ = \n q q \n . G � � 200 \0 \n G
00000010 2d 00 80 ff ff 2d 2d 2d 54 0a 20 20 20 20 54 80 |-....---T.
T.|
0000010 - \0 200 � � - - - T \n T 200
00000020 ff 0a 2e 3b 0a 73 3b 28 5c 77 29 2e 2a 28 5c 77
|...;.s;(\w).*(\w|
0000020 � \n . ; \n s ; ( \ w ) . * ( \ w
00000030 29 3b 24 5f 7b 20 24 2d 20 2b 2b 20 7d 3b 67 6c |);$_{ $- ++
};gl|
0000030 ) ; $ _ { $ - + + } ; g l
00000040 3b 0a |;.|
0000040 ; \n
0000042

which is a similar assetion to (but not the same as) the one your min556
throws.

Tony

@p5pRT
Copy link
Author

p5pRT commented Aug 30, 2016

From @khwilliamson

On 08/29/2016 11​:28 PM, Brian 'geeknik' Carpenter wrote​:

khw asked me on irc if i can check my test cases against blead. neither
test case crashes/asserts with 5.25.4-25-g109ac34*.

As I expected, 109ac34 fixed this problem, which means this bug is from
the same cause as [perl #129038], which was disclosed publicly.

I think that everybody is more naive about security issues than they
realize, but I am especially naive, and don't know if this is something
exploitable or not, so here are the details to help that determination.

To be problematic, the match must be done under 'use locale' or with the
/l pattern modifier; a POSIX class is being matched in the pattern, such
as \w, \D, or [​:punct​:]; the target string must not be encoded in UTF-8;
the character being matched against the pattern must be in the range
0xC2-0xFF; and finally, the character must match the posix class.

That leads to the pointer to the target string being incremented by
UTF8SKIP instead of the proper 1. And that means the pointer can be
after the string proper by as much as UTF8SKIP-1 bytes, if the matched
character is the final one in the string and the string is not
NUL-terminated. Subtract an additional 1 if it is NUL terminated. That
means the maximum it can extend beyond the string is 12 bytes, if the
character is \xFF, and a UV is a quad. Otherwise it is 6 bytes or less.

In this test case it is doing a s///g. I have never looked at how the
substitution/matching interact. The match succeeded, and did not
continue in this case because the pointer being beyond the string
implies that the end-of-string has been found. But apparently that
pointer is getting stored and the substitute is looking for it. I
imagine that a capturing group would be very problematic. (I haven't
seen the original test case for this ticket, as it won't download
properly for me.)

So, is this exploitable?

On Tue, Aug 23, 2016 at 11​:30 PM, Tony Cook via RT
<perl5-security-report@​perl.org <mailto​:perl5-security-report@​perl.org>>
wrote​:

On Sat Aug 20 16&#8203;:24&#8203;:41 2016\, brian\.carpenter@&#8203;gmail\.com
\<mailto&#8203;:brian\.carpenter@&#8203;gmail\.com> wrote&#8203;:
> I'm attaching 2 test cases to this bug report because the orig556 test case
> causes the heap\-buffer\-overflow\, but when minimized to min556\, it only
> triggers an assertion failure \(perl&#8203;: pp\_hot\.c&#8203;:3203&#8203;: OP \*Perl\_pp\_subst\(\)&#8203;:
> Assertion \`strend >= s' failed\)\.
>
> This was found with AFL\, ASAN and libdislocator\.so affects v5\.25\.4
> \(v5\.25\.3\-305\-g8c6b0c7\)\. Perl 5\.20\.2 complains about a bareword with orig556
> and throws a panic&#8203;:memory wrap with min556\.

Also detected by valgrind&#8203;:

tony@&#8203;mars&#8203;:\.\.\./git/perl$ valgrind \-q \./perl \.\./129024ab\.pl
\<http&#8203;://129024ab\.pl>
==17417== Invalid read of size 1
==17417==    at 0x6DCB0D&#8203;: S\_regmatch \(regexec\.c&#8203;:6058\)
==17417==    by 0x6D3946&#8203;: S\_regtry \(regexec\.c&#8203;:3619\)
==17417==    by 0x6CCB9D&#8203;: S\_find\_byclass \(regexec\.c&#8203;:2357\)
==17417==    by 0x6D2D1F&#8203;: Perl\_regexec\_flags \(regexec\.c&#8203;:3368\)
==17417==    by 0x65925F&#8203;: Perl\_pp\_substcont \(pp\_ctl\.c&#8203;:225\)
==17417==    by 0x55906B&#8203;: Perl\_runops\_debug \(dump\.c&#8203;:2234\)
==17417==    by 0x462A94&#8203;: S\_run\_body \(perl\.c&#8203;:2525\)
==17417==    by 0x4620BF&#8203;: perl\_run \(perl\.c&#8203;:2448\)
==17417==    by 0x41EFDD&#8203;: main \(perlmain\.c&#8203;:123\)
\.\.\.
perl&#8203;: regcomp\.c&#8203;:8066&#8203;: Perl\_reg\_numbered\_buff\_fetch&#8203;: Assertion
\`\(STRLEN\)rx\->sublen >= \(STRLEN\)\(\(s \- rx\->subbeg\) \+ i\)' failed\.
Aborted

I've reduced the original test case significantly \(129024ab\.pl
\<http&#8203;://129024ab\.pl>\)\.

00000000  24 5f 3d 0a 20 71 71 0a  2e 47 ff ff 80 00 0a 47  |$\_=\.
qq\.\.G\.\.\.\.\.G|
0000000   $   \_   =  \\n       q   q  \\n   \.   G   �   � 200  \\0  \\n   G
00000010  2d 00 80 ff ff 2d 2d 2d  54 0a 20 20 20 20 54 80
|\-\.\.\.\.\-\-\-T\.    T\.|
0000010   \-  \\0 200   �   �   \-   \-   \-   T  \\n                   T 200
00000020  ff 0a 2e 3b 0a 73 3b 28  5c 77 29 2e 2a 28 5c 77
|\.\.\.;\.s;\(\\w\)\.\*\(\\w|
0000020   �  \\n   \.   ;  \\n   s   ;   \(   \\   w   \)   \.   \*   \(   \\   w
00000030  29 2e 2a 5c 42 3b 24 5f  80 7b 20 24 2d 20 2b 2b
|\)\.\*\\B;$\_\.\{ $\- \+\+|
0000030   \)   \.   \*   \\   B   ;   $   \_ 200   \{       $   \-       \+   \+
00000040  20 3f 24 32 3a 24 31 7d  3b 67 6c 3b 0a           |
?$2&#8203;:$1\};gl;\.|
0000040       ?   $   2   :   $   1   \}   ;   g   l   ;  \\n
000004d

Further minimization prevented errors from valgrind\, and changed the
assertion thrown&#8203;:

tony@&#8203;mars&#8203;:\.\.\./git/perl$ valgrind \-q \./perl \.\./129024ac\.pl
\<http&#8203;://129024ac\.pl>
perl&#8203;: pp\_ctl\.c&#8203;:231&#8203;: Perl\_pp\_substcont&#8203;: Assertion
\`cx\->cx\_u\.cx\_subst\.sbu\_strend >= s' failed\.
Aborted

00000000  24 5f 3d 0a 20 71 71 0a  2e 47 ff ff 80 00 0a 47  |$\_=\.
qq\.\.G\.\.\.\.\.G|
0000000   $   \_   =  \\n       q   q  \\n   \.   G   �   � 200  \\0  \\n   G
00000010  2d 00 80 ff ff 2d 2d 2d  54 0a 20 20 20 20 54 80
|\-\.\.\.\.\-\-\-T\.    T\.|
0000010   \-  \\0 200   �   �   \-   \-   \-   T  \\n                   T 200
00000020  ff 0a 2e 3b 0a 73 3b 28  5c 77 29 2e 2a 28 5c 77
|\.\.\.;\.s;\(\\w\)\.\*\(\\w|
0000020   �  \\n   \.   ;  \\n   s   ;   \(   \\   w   \)   \.   \*   \(   \\   w
00000030  29 3b 24 5f 7b 20 24 2d  20 2b 2b 20 7d 3b 67 6c  |\);$\_\{
$\- \+\+ \};gl|
0000030   \)   ;   $   \_   \{       $   \-       \+   \+       \}   ;   g   l
00000040  3b 0a                                             |;\.|
0000040   ;  \\n
0000042

which is a similar assetion to \(but not the same as\) the one your min556
throws\.

Tony

@p5pRT
Copy link
Author

p5pRT commented Aug 31, 2016

From @khwilliamson

On 08/30/2016 10​:06 AM, Karl Williamson wrote​:

On 08/29/2016 11​:28 PM, Brian 'geeknik' Carpenter wrote​:

khw asked me on irc if i can check my test cases against blead. neither
test case crashes/asserts with 5.25.4-25-g109ac34*.

As I expected, 109ac34 fixed this problem, which means this bug is from
the same cause as [perl #129038], which was disclosed publicly.

I think that everybody is more naive about security issues than they
realize, but I am especially naive, and don't know if this is something
exploitable or not, so here are the details to help that determination.

To be problematic, the match must be done under 'use locale' or with the
/l pattern modifier; a POSIX class is being matched in the pattern, such
as \w, \D, or [​:punct​:]; the target string must not be encoded in UTF-8;
the character being matched against the pattern must be in the range
0xC2-0xFF; and finally, the character must match the posix class.

That leads to the pointer to the target string being incremented by
UTF8SKIP instead of the proper 1. And that means the pointer can be
after the string proper by as much as UTF8SKIP-1 bytes, if the matched
character is the final one in the string and the string is not
NUL-terminated. Subtract an additional 1 if it is NUL terminated. That
means the maximum it can extend beyond the string is 12 bytes, if the
character is \xFF, and a UV is a quad. Otherwise it is 6 bytes or less.

Come to think of it, the UV doesn't have to be a quad, probably.

In this test case it is doing a s///g. I have never looked at how the
substitution/matching interact. The match succeeded, and did not
continue in this case because the pointer being beyond the string
implies that the end-of-string has been found. But apparently that
pointer is getting stored and the substitute is looking for it. I
imagine that a capturing group would be very problematic. (I haven't
seen the original test case for this ticket, as it won't download
properly for me.)

So, is this exploitable?

On Tue, Aug 23, 2016 at 11​:30 PM, Tony Cook via RT
<perl5-security-report@​perl.org <mailto​:perl5-security-report@​perl.org>>
wrote​:

On Sat Aug 20 16&#8203;:24&#8203;:41 2016\, brian\.carpenter@&#8203;gmail\.com
\<mailto&#8203;:brian\.carpenter@&#8203;gmail\.com> wrote&#8203;:
> I'm attaching 2 test cases to this bug report because the

orig556 test case
> causes the heap-buffer-overflow, but when minimized to min556,
it only
> triggers an assertion failure (perl​: pp_hot.c​:3203​: OP
*Perl_pp_subst()​:
> Assertion `strend >= s' failed).
>
> This was found with AFL, ASAN and libdislocator.so affects v5.25.4
> (v5.25.3-305-g8c6b0c7). Perl 5.20.2 complains about a bareword
with orig556
> and throws a panic​:memory wrap with min556.

Also detected by valgrind&#8203;:

tony@&#8203;mars&#8203;:\.\.\./git/perl$ valgrind \-q \./perl \.\./129024ab\.pl
\<http&#8203;://129024ab\.pl>
==17417== Invalid read of size 1
==17417==    at 0x6DCB0D&#8203;: S\_regmatch \(regexec\.c&#8203;:6058\)
==17417==    by 0x6D3946&#8203;: S\_regtry \(regexec\.c&#8203;:3619\)
==17417==    by 0x6CCB9D&#8203;: S\_find\_byclass \(regexec\.c&#8203;:2357\)
==17417==    by 0x6D2D1F&#8203;: Perl\_regexec\_flags \(regexec\.c&#8203;:3368\)
==17417==    by 0x65925F&#8203;: Perl\_pp\_substcont \(pp\_ctl\.c&#8203;:225\)
==17417==    by 0x55906B&#8203;: Perl\_runops\_debug \(dump\.c&#8203;:2234\)
==17417==    by 0x462A94&#8203;: S\_run\_body \(perl\.c&#8203;:2525\)
==17417==    by 0x4620BF&#8203;: perl\_run \(perl\.c&#8203;:2448\)
==17417==    by 0x41EFDD&#8203;: main \(perlmain\.c&#8203;:123\)
\.\.\.
perl&#8203;: regcomp\.c&#8203;:8066&#8203;: Perl\_reg\_numbered\_buff\_fetch&#8203;: Assertion
\`\(STRLEN\)rx\->sublen >= \(STRLEN\)\(\(s \- rx\->subbeg\) \+ i\)' failed\.
Aborted

I've reduced the original test case significantly \(129024ab\.pl
\<http&#8203;://129024ab\.pl>\)\.

00000000  24 5f 3d 0a 20 71 71 0a  2e 47 ff ff 80 00 0a 47  |$\_=\.
qq\.\.G\.\.\.\.\.G|
0000000   $   \_   =  \\n       q   q  \\n   \.   G   �   � 200  \\0

\n G
00000010 2d 00 80 ff ff 2d 2d 2d 54 0a 20 20 20 20 54 80
|-....---T. T.|
0000010 - \0 200 � � - - - T \n
T 200
00000020 ff 0a 2e 3b 0a 73 3b 28 5c 77 29 2e 2a 28 5c 77
|...;.s;(\w).*(\w|
0000020 � \n . ; \n s ; ( \ w ) . * (
\ w
00000030 29 2e 2a 5c 42 3b 24 5f 80 7b 20 24 2d 20 2b 2b
|).*\B;$_.{ $- ++|
0000030 ) . * \ B ; $ _ 200 { $ -
+ +
00000040 20 3f 24 32 3a 24 31 7d 3b 67 6c 3b 0a |
?$2​:$1};gl;.|
0000040 ? $ 2 : $ 1 } ; g l ; \n
000004d

Further minimization prevented errors from valgrind\, and changed the
assertion thrown&#8203;:

tony@&#8203;mars&#8203;:\.\.\./git/perl$ valgrind \-q \./perl \.\./129024ac\.pl
\<http&#8203;://129024ac\.pl>
perl&#8203;: pp\_ctl\.c&#8203;:231&#8203;: Perl\_pp\_substcont&#8203;: Assertion
\`cx\->cx\_u\.cx\_subst\.sbu\_strend >= s' failed\.
Aborted

00000000  24 5f 3d 0a 20 71 71 0a  2e 47 ff ff 80 00 0a 47  |$\_=\.
qq\.\.G\.\.\.\.\.G|
0000000   $   \_   =  \\n       q   q  \\n   \.   G   �   � 200  \\0

\n G
00000010 2d 00 80 ff ff 2d 2d 2d 54 0a 20 20 20 20 54 80
|-....---T. T.|
0000010 - \0 200 � � - - - T \n
T 200
00000020 ff 0a 2e 3b 0a 73 3b 28 5c 77 29 2e 2a 28 5c 77
|...;.s;(\w).*(\w|
0000020 � \n . ; \n s ; ( \ w ) . * (
\ w
00000030 29 3b 24 5f 7b 20 24 2d 20 2b 2b 20 7d 3b 67 6c |);$_{
$- ++ };gl|
0000030 ) ; $ _ { $ - + + } ;
g l
00000040 3b 0a |;.|
0000040 ; \n
0000042

which is a similar assetion to \(but not the same as\) the one your

min556
throws.

Tony

@p5pRT
Copy link
Author

p5pRT commented Sep 7, 2016

From @tonycoz

On Tue Aug 30 09​:06​:55 2016, public@​khwilliamson.com wrote​:

So, is this exploitable?

It depends on what we consider an exploit.

a) it can crash perl, with well defined strings and regexps, which might be
usable as a denial of service.

b) even without the crash, could it be used make code that tries to
clean up user supplied data to make it "safe" to remove too much or too
little text?

Tony

@p5pRT
Copy link
Author

p5pRT commented Dec 6, 2016

From @iabyn

On Tue, Sep 06, 2016 at 06​:56​:36PM -0700, Tony Cook via RT wrote​:

On Tue Aug 30 09​:06​:55 2016, public@​khwilliamson.com wrote​:

So, is this exploitable?

It depends on what we consider an exploit.

a) it can crash perl, with well defined strings and regexps, which might be
usable as a denial of service.

Chiefly on a non-debugging perl, its likely to do a "panic​: memwrap"
rather than some random crash AFAIK.

b) even without the crash, could it be used make code that tries to
clean up user supplied data to make it "safe" to remove too much or too
little text?

The main code paths I can see cause​:

  a character class like \w can consume multiple characters

  $1, $&amp; etc can be too long or cover beyond the end of the string;

  panic​: memwrap

  pos() can be set too high

  /g can call match for a second time with start pos beyond end,
  which should immediately fail

I don't think s/// can do anything particularly nasty beyond what m// can
do, because it basically tries to append $' to the destination string, and
that is of huge length (strend -s < 0) which gives the memwrap panic. It
can or course strip too many chars s/\w/.../, or copy too many chars
s/...(\w).../$1/.

Overall my feeling is that this particular combination is unlikely to
occur in a way that's exploitable​: a sanitising regex is unlikely to be
using //l; and if the user can supply the regex (e.g. via a web form)
then its unlikley this is being applied against a non-utf8 string with
chars in the range 0xc2-0xff, and of course there are already many issues
with allowing user-supplied regexes (that's what re​::engine​::RE2 is for).
Finally, if there were an environment where both data and pattern are
supplied, then you're really got problems.

I'm not saying this *isn't* exploitable; just that that it's towards the
unlikely end of the spectrum.

Also, the fix has been public for 3+ months now.

All in all, I think we should just quietly close the ticket, after
backporting to 5.24 (and 5.22?)

--
Standards (n). Battle insignia or tribal totems.

@p5pRT
Copy link
Author

p5pRT commented Dec 26, 2016

From @xsawyerx

On Tue, Dec 6, 2016 at 5​:47 PM, Dave Mitchell <davem@​iabyn.com> wrote​:

On Tue, Sep 06, 2016 at 06​:56​:36PM -0700, Tony Cook via RT wrote​:

On Tue Aug 30 09​:06​:55 2016, public@​khwilliamson.com wrote​:

So, is this exploitable?

It depends on what we consider an exploit.

a) it can crash perl, with well defined strings and regexps, which might be
usable as a denial of service.

Chiefly on a non-debugging perl, its likely to do a "panic​: memwrap"
rather than some random crash AFAIK.

b) even without the crash, could it be used make code that tries to
clean up user supplied data to make it "safe" to remove too much or too
little text?

The main code paths I can see cause​:

a character class like \\w can consume multiple characters

$1\, $& etc can be too long or cover beyond the end of the string;

panic&#8203;: memwrap

pos\(\) can be set too high

/g can call match for a second time with start pos beyond end\,
    which should immediately fail

I don't think s/// can do anything particularly nasty beyond what m// can
do, because it basically tries to append $' to the destination string, and
that is of huge length (strend -s < 0) which gives the memwrap panic. It
can or course strip too many chars s/\w/.../, or copy too many chars
s/...(\w).../$1/.

Overall my feeling is that this particular combination is unlikely to
occur in a way that's exploitable​: a sanitising regex is unlikely to be
using //l; and if the user can supply the regex (e.g. via a web form)
then its unlikley this is being applied against a non-utf8 string with
chars in the range 0xc2-0xff, and of course there are already many issues
with allowing user-supplied regexes (that's what re​::engine​::RE2 is for).
Finally, if there were an environment where both data and pattern are
supplied, then you're really got problems.

I'm not saying this *isn't* exploitable; just that that it's towards the
unlikely end of the spectrum.

Also, the fix has been public for 3+ months now.

All in all, I think we should just quietly close the ticket, after
backporting to 5.24 (and 5.22?)

5.24 and 5.22, please.

Thank you, Dave.

@p5pRT
Copy link
Author

p5pRT commented Jan 19, 2017

From @tonycoz

On Tue, 06 Dec 2016 08​:48​:30 -0800, davem wrote​:

Also, the fix has been public for 3+ months now.

Made the ticket public.

On Mon, 26 Dec 2016 11​:09​:15 -0800, xsawyerx@​gmail.com wrote​:

5.24 and 5.22, please.

The cherry-pick to 5.22 was non-trivial, but I've backported it to tonyc/maint-5.22-129038-locale-match (a review from khw might be useful.)

I've added this to both the 5.22 and 5.24 votes files and voted for them.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jan 20, 2017

From @khwilliamson

On 01/18/2017 10​:17 PM, Tony Cook via RT wrote​:

On Tue, 06 Dec 2016 08​:48​:30 -0800, davem wrote​:

Also, the fix has been public for 3+ months now.

Made the ticket public.

On Mon, 26 Dec 2016 11​:09​:15 -0800, xsawyerx@​gmail.com wrote​:

5.24 and 5.22, please.

The cherry-pick to 5.22 was non-trivial, but I've backported it to tonyc/maint-5.22-129038-locale-match (a review from khw might be useful.)

The 5.22 patch looks good to me.

I've added this to both the 5.22 and 5.24 votes files and voted for them.

Tony

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=129024

@p5pRT
Copy link
Author

p5pRT commented Feb 2, 2017

From @iabyn

On Thu, Jan 19, 2017 at 09​:49​:37PM -0700, Karl Williamson wrote​:

On 01/18/2017 10​:17 PM, Tony Cook via RT wrote​:

On Tue, 06 Dec 2016 08​:48​:30 -0800, davem wrote​:

Also, the fix has been public for 3+ months now.

Made the ticket public.

On Mon, 26 Dec 2016 11​:09​:15 -0800, xsawyerx@​gmail.com wrote​:

5.24 and 5.22, please.

The cherry-pick to 5.22 was non-trivial, but I've backported it to tonyc/maint-5.22-129038-locale-match (a review from khw might be useful.)

The 5.22 patch looks good to me.

I've added this to both the 5.22 and 5.24 votes files and voted for them.

I've added my vote too. Along with Karl's endorsement, and with 5.22.3 and
5.24.1 out of the door, I think these backports can go in now.
Can anyone do this or do we leave it to Steve?

--
A major Starfleet emergency breaks out near the Enterprise, but
fortunately some other ships in the area are able to deal with it to
everyone's satisfaction.
  -- Things That Never Happen in "Star Trek" #13

@p5pRT
Copy link
Author

p5pRT commented Feb 9, 2017

From @steve-m-hay

On Thu, 02 Feb 2017 07​:05​:35 -0800, davem wrote​:

On Thu, Jan 19, 2017 at 09​:49​:37PM -0700, Karl Williamson wrote​:

On 01/18/2017 10​:17 PM, Tony Cook via RT wrote​:

On Tue, 06 Dec 2016 08​:48​:30 -0800, davem wrote​:

Also, the fix has been public for 3+ months now.

Made the ticket public.

On Mon, 26 Dec 2016 11​:09​:15 -0800, xsawyerx@​gmail.com wrote​:

5.24 and 5.22, please.

The cherry-pick to 5.22 was non-trivial, but I've backported it to
tonyc/maint-5.22-129038-locale-match (a review from khw might be
useful.)

The 5.22 patch looks good to me.

I've added this to both the 5.22 and 5.24 votes files and voted for
them.

I've added my vote too. Along with Karl's endorsement, and with 5.22.3
and
5.24.1 out of the door, I think these backports can go in now.
Can anyone do this or do we leave it to Steve?

Anyone can do this once it's got the required votes. The only thing to be careful about right now is that 5.22.4 and 5.24.2 are awaiting the last piece in the @​INC puzzle so the plan was to hold off from backporting general bug fixes etc just yet. However, it was agreed that other *security* fixes would still be appropriate for these releases, which probably includes this despite some questions about its likely exploitability.

@p5pRT
Copy link
Author

p5pRT commented Feb 12, 2017

From @xsawyerx

On 02/09/2017 07​:21 PM, Steve Hay via RT wrote​:

On Thu, 02 Feb 2017 07​:05​:35 -0800, davem wrote​:

On Thu, Jan 19, 2017 at 09​:49​:37PM -0700, Karl Williamson wrote​:

On 01/18/2017 10​:17 PM, Tony Cook via RT wrote​:

On Tue, 06 Dec 2016 08​:48​:30 -0800, davem wrote​:

Also, the fix has been public for 3+ months now.
Made the ticket public.

On Mon, 26 Dec 2016 11​:09​:15 -0800, xsawyerx@​gmail.com wrote​:

5.24 and 5.22, please.
The cherry-pick to 5.22 was non-trivial, but I've backported it to
tonyc/maint-5.22-129038-locale-match (a review from khw might be
useful.)
The 5.22 patch looks good to me.

I've added this to both the 5.22 and 5.24 votes files and voted for
them.
I've added my vote too. Along with Karl's endorsement, and with 5.22.3
and
5.24.1 out of the door, I think these backports can go in now.
Can anyone do this or do we leave it to Steve?
Anyone can do this once it's got the required votes. The only thing to be careful about right now is that 5.22.4 and 5.24.2 are awaiting the last piece in the @​INC puzzle so the plan was to hold off from backporting general bug fixes etc just yet. However, it was agreed that other *security* fixes would still be appropriate for these releases, which probably includes this despite some questions about its likely exploitability.

I agree.

@p5pRT
Copy link
Author

p5pRT commented Feb 21, 2017

From @steve-m-hay

On Sun, 12 Feb 2017 04​:26​:22 -0800, xsawyerx@​gmail.com wrote​:

On 02/09/2017 07​:21 PM, Steve Hay via RT wrote​:

On Thu, 02 Feb 2017 07​:05​:35 -0800, davem wrote​:

On Thu, Jan 19, 2017 at 09​:49​:37PM -0700, Karl Williamson wrote​:

On 01/18/2017 10​:17 PM, Tony Cook via RT wrote​:

On Tue, 06 Dec 2016 08​:48​:30 -0800, davem wrote​:

Also, the fix has been public for 3+ months now.
Made the ticket public.

On Mon, 26 Dec 2016 11​:09​:15 -0800, xsawyerx@​gmail.com wrote​:

5.24 and 5.22, please.
The cherry-pick to 5.22 was non-trivial, but I've backported it to
tonyc/maint-5.22-129038-locale-match (a review from khw might be
useful.)
The 5.22 patch looks good to me.

I've added this to both the 5.22 and 5.24 votes files and voted
for
them.
I've added my vote too. Along with Karl's endorsement, and with
5.22.3
and
5.24.1 out of the door, I think these backports can go in now.
Can anyone do this or do we leave it to Steve?
Anyone can do this once it's got the required votes. The only thing
to be careful about right now is that 5.22.4 and 5.24.2 are awaiting
the last piece in the @​INC puzzle so the plan was to hold off from
backporting general bug fixes etc just yet. However, it was agreed
that other *security* fixes would still be appropriate for these
releases, which probably includes this despite some questions about
its likely exploitability.

I agree.

Shall I go ahead and cherry-pick this into the maint branches then?

Do we have any other security fixes that we also want to see in 5.22.4/5.24.2?

The voting files both list ba0a415 as a security fix, and it has three votes. Any more?

@p5pRT
Copy link
Author

p5pRT commented Feb 21, 2017

From @xsawyerx

On 02/21/2017 02​:47 PM, Steve Hay via RT wrote​:

On Sun, 12 Feb 2017 04​:26​:22 -0800, xsawyerx@​gmail.com wrote​:

On 02/09/2017 07​:21 PM, Steve Hay via RT wrote​:

On Thu, 02 Feb 2017 07​:05​:35 -0800, davem wrote​:

On Thu, Jan 19, 2017 at 09​:49​:37PM -0700, Karl Williamson wrote​:

On 01/18/2017 10​:17 PM, Tony Cook via RT wrote​:

On Tue, 06 Dec 2016 08​:48​:30 -0800, davem wrote​:

Also, the fix has been public for 3+ months now.
Made the ticket public.

On Mon, 26 Dec 2016 11​:09​:15 -0800, xsawyerx@​gmail.com wrote​:

5.24 and 5.22, please.
The cherry-pick to 5.22 was non-trivial, but I've backported it to
tonyc/maint-5.22-129038-locale-match (a review from khw might be
useful.)
The 5.22 patch looks good to me.

I've added this to both the 5.22 and 5.24 votes files and voted
for
them.
I've added my vote too. Along with Karl's endorsement, and with
5.22.3
and
5.24.1 out of the door, I think these backports can go in now.
Can anyone do this or do we leave it to Steve?
Anyone can do this once it's got the required votes. The only thing
to be careful about right now is that 5.22.4 and 5.24.2 are awaiting
the last piece in the @​INC puzzle so the plan was to hold off from
backporting general bug fixes etc just yet. However, it was agreed
that other *security* fixes would still be appropriate for these
releases, which probably includes this despite some questions about
its likely exploitability.
I agree.

Shall I go ahead and cherry-pick this into the maint branches then?

Do we have any other security fixes that we also want to see in 5.22.4/5.24.2?

The voting files both list ba0a415 as a security fix, and it has three votes. Any more?

Aristotle's base.pm change.

@p5pRT
Copy link
Author

p5pRT commented Feb 21, 2017

From @steve-m-hay

On Tue, 21 Feb 2017 09​:01​:34 -0800, xsawyerx@​gmail.com wrote​:

On 02/21/2017 02​:47 PM, Steve Hay via RT wrote​:

On Sun, 12 Feb 2017 04​:26​:22 -0800, xsawyerx@​gmail.com wrote​:

On 02/09/2017 07​:21 PM, Steve Hay via RT wrote​:

On Thu, 02 Feb 2017 07​:05​:35 -0800, davem wrote​:

On Thu, Jan 19, 2017 at 09​:49​:37PM -0700, Karl Williamson wrote​:

On 01/18/2017 10​:17 PM, Tony Cook via RT wrote​:

On Tue, 06 Dec 2016 08​:48​:30 -0800, davem wrote​:

Also, the fix has been public for 3+ months now.
Made the ticket public.

On Mon, 26 Dec 2016 11​:09​:15 -0800, xsawyerx@​gmail.com wrote​:

5.24 and 5.22, please.
The cherry-pick to 5.22 was non-trivial, but I've backported it
to
tonyc/maint-5.22-129038-locale-match (a review from khw might be
useful.)
The 5.22 patch looks good to me.

I've added this to both the 5.22 and 5.24 votes files and voted
for
them.
I've added my vote too. Along with Karl's endorsement, and with
5.22.3
and
5.24.1 out of the door, I think these backports can go in now.
Can anyone do this or do we leave it to Steve?
Anyone can do this once it's got the required votes. The only thing
to be careful about right now is that 5.22.4 and 5.24.2 are
awaiting
the last piece in the @​INC puzzle so the plan was to hold off from
backporting general bug fixes etc just yet. However, it was agreed
that other *security* fixes would still be appropriate for these
releases, which probably includes this despite some questions about
its likely exploitability.
I agree.

Shall I go ahead and cherry-pick this into the maint branches then?

Do we have any other security fixes that we also want to see in
5.22.4/5.24.2?

The voting files both list ba0a415
as a security fix, and it has three votes. Any more?

Aristotle's base.pm change.

Yes, of course! :-) I meant any other security fixes that we want in these releases so that there's more in them than *just* the base.pm change.

@p5pRT
Copy link
Author

p5pRT commented Feb 23, 2017

From @steve-m-hay

On Tue, 21 Feb 2017 05​:47​:01 -0800, shay wrote​:

On Sun, 12 Feb 2017 04​:26​:22 -0800, xsawyerx@​gmail.com wrote​:

On 02/09/2017 07​:21 PM, Steve Hay via RT wrote​:

On Thu, 02 Feb 2017 07​:05​:35 -0800, davem wrote​:

On Thu, Jan 19, 2017 at 09​:49​:37PM -0700, Karl Williamson wrote​:

On 01/18/2017 10​:17 PM, Tony Cook via RT wrote​:

On Tue, 06 Dec 2016 08​:48​:30 -0800, davem wrote​:

Also, the fix has been public for 3+ months now.
Made the ticket public.

On Mon, 26 Dec 2016 11​:09​:15 -0800, xsawyerx@​gmail.com wrote​:

5.24 and 5.22, please.
The cherry-pick to 5.22 was non-trivial, but I've backported it
to
tonyc/maint-5.22-129038-locale-match (a review from khw might be
useful.)
The 5.22 patch looks good to me.

I've added this to both the 5.22 and 5.24 votes files and voted
for
them.
I've added my vote too. Along with Karl's endorsement, and with
5.22.3
and
5.24.1 out of the door, I think these backports can go in now.
Can anyone do this or do we leave it to Steve?
Anyone can do this once it's got the required votes. The only thing
to be careful about right now is that 5.22.4 and 5.24.2 are
awaiting
the last piece in the @​INC puzzle so the plan was to hold off from
backporting general bug fixes etc just yet. However, it was agreed
that other *security* fixes would still be appropriate for these
releases, which probably includes this despite some questions about
its likely exploitability.

I agree.

Shall I go ahead and cherry-pick this into the maint branches then?

Do we have any other security fixes that we also want to see in
5.22.4/5.24.2?

The voting files both list ba0a415 as
a security fix, and it has three votes. Any more?

Both this and ba0a415 are now in 5.22 and 5.24.

@p5pRT
Copy link
Author

p5pRT commented Feb 23, 2017

@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