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

[CVE-2017-12883]Buffer over-read in S_grok_bslash_N #16025

Closed
p5pRT opened this issue Jun 18, 2017 · 30 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Jun 18, 2017

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

Searchable as RT131598$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 18, 2017

From @jwilk

Consider the following program​:

m'\N{U+.}';
__END__

When you run it under valgrind, you get​:

  Invalid read of size 2
  at 0x402E020​: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
  by 0x815243E​: Perl_sv_vcatpvfn_flags (sv.c​:13067)
  by 0x814D860​: Perl_sv_vsetpvfn (sv.c​:10953)
  by 0x810985B​: Perl_vmess (util.c​:1485)
  by 0x8109E4C​: Perl_vcroak (util.c​:1714)
  by 0x8109EE5​: Perl_croak (util.c​:1761)
  by 0x80E8FFF​: S_grok_bslash_N (regcomp.c​:12168)
  by 0x80EB5B8​: S_regatom (regcomp.c​:12880)
  by 0x80E6A9D​: S_regpiece (regcomp.c​:11669)
  by 0x80E6872​: S_regbranch (regcomp.c​:11594)
  by 0x80E58D4​: S_reg (regcomp.c​:11332)
  by 0x80D7CCD​: Perl_re_op_compile (regcomp.c​:7101)
  Address 0x4288d1e is 6 bytes after a block of size 120 free'd
  at 0x402A3A8​: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
  by 0x810837B​: Perl_safesysfree (util.c​:388)
  by 0x816F501​: Perl_leave_scope (scope.c​:929)
  by 0x816DA76​: Perl_pop_scope (scope.c​:124)
  by 0x809BF63​: S_sublex_done (toke.c​:2569)
  by 0x80A1E24​: Perl_yylex (toke.c​:4918)
  by 0x80BE6A1​: Perl_yyparse (perly.c​:340)
  by 0x8086637​: S_parse_body (perl.c​:2377)
  by 0x808578B​: perl_parse (perl.c​:1692)
  by 0x8061751​: main (perlmain.c​:121)

The above was tested on v5.26.0 built for i686-linux.

The exact symptoms seem to vary with Perl version. For example, when I run it
on Debian perl v5.24.1 on i386 (without valgrind this time), I get the
following warning on stderr​:

  Invalid hexadecimal number in \N{U+...} in regex; marked by <-- HERE in m/ <-- HERE <binary garbage>

where binary garbage looks like a large chunk of the program memory, including
values of environment variables. :(

--
Jakub Wilk

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 19, 2017

From @hvds

On Sun, 18 Jun 2017 08​:53​:27 -0700, jwilk@​jwilk.net wrote​:

Consider the following program​:

m'\N{U+.}';
__END__

When you run it under valgrind, you get​:

Invalid read of size 2
[...]

I tried a few builds of perl - both blead and 5.26.0 - to reproduce this, without success. Please could you supply the output of 'perl -V' for the build with which you found this?

For future reports, it would be helpful if you could use 'perlbug', which would include that sort of information automatically.

Cheers,

Hugo

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 19, 2017

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 20, 2017

From @khwilliamson

On 06/18/2017 09​:53 AM, Jakub Wilk (via RT) wrote​:

# New Ticket Created by Jakub Wilk
# Please include the string​: [perl #131598]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=131598 >

Consider the following program​:

m'\N{U+.}';
__END__

When you run it under valgrind, you get​:

 Invalid read of size 2
   at 0x402E020&#8203;: memcpy \(in /usr/lib/valgrind/vgpreload\_memcheck\-x86\-linux\.so\)
   by 0x815243E&#8203;: Perl\_sv\_vcatpvfn\_flags \(sv\.c&#8203;:13067\)
   by 0x814D860&#8203;: Perl\_sv\_vsetpvfn \(sv\.c&#8203;:10953\)
   by 0x810985B&#8203;: Perl\_vmess \(util\.c&#8203;:1485\)
   by 0x8109E4C&#8203;: Perl\_vcroak \(util\.c&#8203;:1714\)
   by 0x8109EE5&#8203;: Perl\_croak \(util\.c&#8203;:1761\)
   by 0x80E8FFF&#8203;: S\_grok\_bslash\_N \(regcomp\.c&#8203;:12168\)
   by 0x80EB5B8&#8203;: S\_regatom \(regcomp\.c&#8203;:12880\)
   by 0x80E6A9D&#8203;: S\_regpiece \(regcomp\.c&#8203;:11669\)
   by 0x80E6872&#8203;: S\_regbranch \(regcomp\.c&#8203;:11594\)
   by 0x80E58D4&#8203;: S\_reg \(regcomp\.c&#8203;:11332\)
   by 0x80D7CCD&#8203;: Perl\_re\_op\_compile \(regcomp\.c&#8203;:7101\)
 Address 0x4288d1e is 6 bytes after a block of size 120 free'd
   at 0x402A3A8&#8203;: free \(in /usr/lib/valgrind/vgpreload\_memcheck\-x86\-linux\.so\)
   by 0x810837B&#8203;: Perl\_safesysfree \(util\.c&#8203;:388\)
   by 0x816F501&#8203;: Perl\_leave\_scope \(scope\.c&#8203;:929\)
   by 0x816DA76&#8203;: Perl\_pop\_scope \(scope\.c&#8203;:124\)
   by 0x809BF63&#8203;: S\_sublex\_done \(toke\.c&#8203;:2569\)
   by 0x80A1E24&#8203;: Perl\_yylex \(toke\.c&#8203;:4918\)
   by 0x80BE6A1&#8203;: Perl\_yyparse \(perly\.c&#8203;:340\)
   by 0x8086637&#8203;: S\_parse\_body \(perl\.c&#8203;:2377\)
   by 0x808578B&#8203;: perl\_parse \(perl\.c&#8203;:1692\)
   by 0x8061751&#8203;: main \(perlmain\.c&#8203;:121\)

The above was tested on v5.26.0 built for i686-linux.

The exact symptoms seem to vary with Perl version. For example, when I run it
on Debian perl v5.24.1 on i386 (without valgrind this time), I get the
following warning on stderr​:

 Invalid hexadecimal number in \\N\{U\+\.\.\.\} in regex; marked by \<\-\- HERE in m/ \<\-\- HERE \<binary garbage>

where binary garbage looks like a large chunk of the program memory, including
values of environment variables. :(

I single stepped through this in blead. On a DEBUGGING build, instead
an assertion fails​:

==15879== Process terminating with default action of signal 6 (SIGABRT)
==15879== at 0x5B5A77F​: raise (raise.c​:58)
==15879== by 0x5B5C379​: abort (abort.c​:89)
==15879== by 0x5B52B46​: __assert_fail_base (assert.c​:92)
==15879== by 0x5B52BF1​: __assert_fail (assert.c​:101)
==15879== by 0x37E8AE​: Perl_sv_vcatpvfn_flags (sv.c​:12485)
==15879== by 0x37A431​: Perl_sv_vsetpvfn (sv.c​:10961)
==15879== by 0x2B3BEC​: Perl_vmess (util.c​:1487)
==15879== by 0x2B50C9​: Perl_vcroak (util.c​:1716)
==15879== by 0x2B54CA​: Perl_croak (util.c​:1763)
==15879== by 0x26D817​: S_grok_bslash_N (regcomp.c​:12168)
==15879== by 0x271CA3​: S_regatom (regcomp.c​:12932)
==15879== by 0x2694D5​: S_regpiece (regcomp.c​:11668)

Without DEBUGGING, and with -O2 optimization, I get

Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==16556== Command​: ./perl -le m'\\N{U+.}';
==16556==
==16556== Invalid read of size 2
==16556== at 0x4C34618​: memmove (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16556== by 0x1EB097​: memcpy (string3.h​:53)
==16556== by 0x1EB097​: Perl_sv_vcatpvfn_flags (sv.c​:13338)
==16556== by 0x1EE430​: Perl_sv_vsetpvfn (sv.c​:10961)
==16556== by 0x1BB42D​: Perl_vmess (util.c​:1487)
==16556== by 0x1BABB2​: Perl_vcroak (util.c​:1716)
==16556== by 0x1BB693​: Perl_croak (util.c​:1763)
==16556== by 0x19B638​: S_grok_bslash_N (regcomp.c​:12168)
==16556== by 0x1A20B5​: S_regatom (regcomp.c​:12932)
==16556== by 0x1A4C8C​: S_regpiece (regcomp.c​:11668)
==16556== by 0x1A4C8C​: S_regbranch (regcomp.c​:11593)
==16556== by 0x198431​: S_reg (regcomp.c​:11331)
==16556== by 0x1AED3B​: Perl_re_op_compile (regcomp.c​:7100)
==16556== by 0x14DE8C​: Perl_pmruntime (op.c​:5882)
==16556== Address 0x5bb16a6 is 14 bytes after a block of size 120 free'd
==16556== at 0x4C2ED5B​: free (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16556== by 0x2194BB​: Perl_leave_scope (scope.c​:929)
==16556== by 0x185986​: S_sublex_done (toke.c​:2562)
==16556== by 0x174DC9​: Perl_yylex (toke.c​:4912)
==16556== by 0x1865FB​: Perl_yyparse (perly.c​:340)
==16556== by 0x15A815​: S_parse_body (perl.c​:2401)
==16556== by 0x15A815​: perl_parse (perl.c​:1719)
==16556== by 0x1331F2​: main (perlmain.c​:121)
==16556== Block was alloc'd at
==16556== at 0x4C2DB2F​: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16556== by 0x1BA191​: Perl_safesysmalloc (util.c​:153)
==16556== by 0x1703F8​: S_sublex_push (toke.c​:2475)
==16556== by 0x1703F8​: Perl_yylex (toke.c​:4908)
==16556== by 0x1865FB​: Perl_yyparse (perly.c​:340)
==16556== by 0x15A815​: S_parse_body (perl.c​:2401)
==16556== by 0x15A815​: perl_parse (perl.c​:1719)
==16556== by 0x1331F2​: main (perlmain.c​:121)
==16556==
==16556== Invalid read of size 2
==16556== at 0x4C34627​: memmove (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16556== by 0x1EB097​: memcpy (string3.h​:53)
==16556== by 0x1EB097​: Perl_sv_vcatpvfn_flags (sv.c​:13338)
==16556== by 0x1EE430​: Perl_sv_vsetpvfn (sv.c​:10961)
==16556== by 0x1BB42D​: Perl_vmess (util.c​:1487)
==16556== by 0x1BABB2​: Perl_vcroak (util.c​:1716)
==16556== by 0x1BB693​: Perl_croak (util.c​:1763)
==16556== by 0x19B638​: S_grok_bslash_N (regcomp.c​:12168)
==16556== by 0x1A20B5​: S_regatom (regcomp.c​:12932)
==16556== by 0x1A4C8C​: S_regpiece (regcomp.c​:11668)
==16556== by 0x1A4C8C​: S_regbranch (regcomp.c​:11593)
==16556== by 0x198431​: S_reg (regcomp.c​:11331)
==16556== by 0x1AED3B​: Perl_re_op_compile (regcomp.c​:7100)
==16556== by 0x14DE8C​: Perl_pmruntime (op.c​:5882)
==16556== Address 0x5bb16aa is 18 bytes after a block of size 120 free'd
==16556== at 0x4C2ED5B​: free (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16556== by 0x2194BB​: Perl_leave_scope (scope.c​:929)
==16556== by 0x185986​: S_sublex_done (toke.c​:2562)
==16556== by 0x174DC9​: Perl_yylex (toke.c​:4912)
==16556== by 0x1865FB​: Perl_yyparse (perly.c​:340)
==16556== by 0x15A815​: S_parse_body (perl.c​:2401)
==16556== by 0x15A815​: perl_parse (perl.c​:1719)
==16556== by 0x1331F2​: main (perlmain.c​:121)
==16556== Block was alloc'd at
==16556== at 0x4C2DB2F​: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16556== by 0x1BA191​: Perl_safesysmalloc (util.c​:153)
==16556== by 0x1703F8​: S_sublex_push (toke.c​:2475)
==16556== by 0x1703F8​: Perl_yylex (toke.c​:4908)
==16556== by 0x1865FB​: Perl_yyparse (perly.c​:340)
==16556== by 0x15A815​: S_parse_body (perl.c​:2401)
==16556== by 0x15A815​: perl_parse (perl.c​:1719)
==16556== by 0x1331F2​: main (perlmain.c​:121)
==16556==
Invalid hexadecimal number in \N{U+...} in regex; marked by <-- HERE in
m/ <-- HERE �PPp0��pP\N{U+.}/ at -e line 1.

It looks to me that S_grok_bslash_N is passing correct stuff to the
error routines. I single stepped through it, but I would have to do
much more detailed study of an area I don't really understand to figure
out what's going wrong.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 20, 2017

From @iabyn

On Mon, Jun 19, 2017 at 07​:23​:10PM -0600, Karl Williamson wrote​:

It looks to me that S_grok_bslash_N is passing correct stuff to the error
routines.

Or maybe it isn't :-)

vFAIL() is using the "%"UTF8f mechanism to pass a (possibly utf8) string
buffer for printing the diagnostic error.

It uses this​:

#define REPORT_LOCATION_ARGS(xC) \
  UTF8fARG(UTF, \
  (xI(xC) > eC) /* Don't run off end */ \
  ? eC - sC /* Length before the <--HERE */ \
  : xI_offset(xC), \
  sC), /* The input pattern printed up to the <--HERE */ \
  UTF8fARG(UTF, \
  (xI(xC) > eC) ? 0 : eC - xI(xC), /* Length after <--HERE */ \
  (xI(xC) > eC) ? eC : xI(xC)) /* pattern after <--HERE */

Looking at the second UTF8fARG,

eC expands to​: pRExC_state->precomp_end

xI(xC) expands to​: pRExC_state->precomp
  + ((IV) (pRExC_state->precomp_adj
  + (pRExC_state->parse - pRExC_state->adjusted_start)))

At this point I see the following values​:

(gdb) p pRExC_state->precomp
$3 = 0xb30288 "\\N{U+.}"
(gdb) p pRExC_state->precomp_end
$4 = 0xb3028f ""
(gdb) p pRExC_state->precomp_adj
$7 = 0x0
(gdb) p pRExC_state->parse
$8 = 0xb3028e "}"
(gdb) p pRExC_state->adjusted_start
$9 = 0xb2ef48 "?​:\\x{})"

It appears that pRExC_state->adjusted_start is "wild", causing potentially
large chunks of garbage to be output.

Can you handle it from here? (regcomp.c is, to me, a riddle wrapped in
a mystery wrapped in an enigma).

--
Red sky at night - gerroff my land!
Red sky at morning - gerroff my land!
  -- old farmers' sayings #14

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 20, 2017

From @khwilliamson

On 06/20/2017 01​:58 AM, Dave Mitchell wrote​:

On Mon, Jun 19, 2017 at 07​:23​:10PM -0600, Karl Williamson wrote​:

It looks to me that S_grok_bslash_N is passing correct stuff to the error
routines.

Or maybe it isn't :-)

vFAIL() is using the "%"UTF8f mechanism to pass a (possibly utf8) string
buffer for printing the diagnostic error.

It uses this​:

#define REPORT_LOCATION_ARGS(xC) \
UTF8fARG(UTF, \
(xI(xC) > eC) /* Don't run off end */ \
? eC - sC /* Length before the <--HERE */ \
: xI_offset(xC), \
sC), /* The input pattern printed up to the <--HERE */ \
UTF8fARG(UTF, \
(xI(xC) > eC) ? 0 : eC - xI(xC), /* Length after <--HERE */ \
(xI(xC) > eC) ? eC : xI(xC)) /* pattern after <--HERE */

Looking at the second UTF8fARG,

eC expands to​: pRExC_state->precomp_end

xI(xC) expands to​: pRExC_state->precomp
+ ((IV) (pRExC_state->precomp_adj
+ (pRExC_state->parse - pRExC_state->adjusted_start)))

At this point I see the following values​:

(gdb) p pRExC_state->precomp
$3 = 0xb30288 "\\N{U+.}"
(gdb) p pRExC_state->precomp_end
$4 = 0xb3028f ""
(gdb) p pRExC_state->precomp_adj
$7 = 0x0
(gdb) p pRExC_state->parse
$8 = 0xb3028e "}"
(gdb) p pRExC_state->adjusted_start
$9 = 0xb2ef48 "?​:\\x{})"

It appears that pRExC_state->adjusted_start is "wild", causing potentially
large chunks of garbage to be output.

Can you handle it from here?

Yes, and thank you for the diagnosis. I have a trivial fix. This is a
case of macros hiding stuff not quite completely, allowing me to forget
about that stuff, but not hiding it enough that it can safely be
completely forgotten about.

I will do an audit looking for similar cases in regcomp.c.

The question now is how do we handle this. Since the fix is so trivial,
it can be backported to whatever releases we deem doing it to. I don't
understand what the consequences of this are for an attack.

(regcomp.c is, to me, a riddle wrapped in

a mystery wrapped in an enigma).

That's how I feel about a bunch of the other core code. (And about the
regex optimizer)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 20, 2017

From @khwilliamson

On 06/20/2017 10​:46 AM, Karl Williamson wrote​:

On 06/20/2017 01​:58 AM, Dave Mitchell wrote​:

On Mon, Jun 19, 2017 at 07​:23​:10PM -0600, Karl Williamson wrote​:

It looks to me that S_grok_bslash_N is passing correct stuff to the
error
routines.

Or maybe it isn't :-)

vFAIL() is using the "%"UTF8f mechanism to pass a (possibly utf8) string
buffer for printing the diagnostic error.

It uses this​:

#define
REPORT_LOCATION_ARGS(xC) \

UTF8fARG(UTF, \
(xI(xC) > eC) /* Don't run off end
*/ \
? eC - sC /* Length before the <--HERE
*/ \
:
xI_offset(xC), \
sC), /* The input pattern printed up to the
<--HERE */ \

UTF8fARG(UTF, \
(xI(xC) > eC) ? 0 : eC - xI(xC), /* Length after <--HERE
*/ \
(xI(xC) > eC) ? eC : xI(xC)) /* pattern after
<--HERE */

Looking at the second UTF8fARG,

eC expands to​: pRExC_state->precomp_end

xI(xC) expands to​: pRExC_state->precomp
+ ((IV) (pRExC_state->precomp_adj
+ (pRExC_state->parse -
pRExC_state->adjusted_start)))

At this point I see the following values​:

(gdb) p pRExC_state->precomp
$3 = 0xb30288 "\\N{U+.}"
(gdb) p pRExC_state->precomp_end
$4 = 0xb3028f ""
(gdb) p pRExC_state->precomp_adj
$7 = 0x0
(gdb) p pRExC_state->parse
$8 = 0xb3028e "}"
(gdb) p pRExC_state->adjusted_start
$9 = 0xb2ef48 "?​:\\x{})"

It appears that pRExC_state->adjusted_start is "wild", causing
potentially
large chunks of garbage to be output.

Can you handle it from here?

Yes, and thank you for the diagnosis. I have a trivial fix. This is a
case of macros hiding stuff not quite completely, allowing me to forget
about that stuff, but not hiding it enough that it can safely be
completely forgotten about.

I will do an audit looking for similar cases in regcomp.c.

My audit indicates this is the only problematic case in regcomp.c

The question now is how do we handle this. Since the fix is so trivial,
it can be backported to whatever releases we deem doing it to. I don't
understand what the consequences of this are for an attack.

(regcomp.c is, to me, a riddle wrapped in

a mystery wrapped in an enigma).

That's how I feel about a bunch of the other core code. (And about the
regex optimizer)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 21, 2017

From @iabyn

On Tue, Jun 20, 2017 at 10​:46​:34AM -0600, Karl Williamson wrote​:

The question now is how do we handle this. Since the fix is so trivial, it
can be backported to whatever releases we deem doing it to. I don't
understand what the consequences of this are for an attack.

It seems that for certain types of syntax error in a pattern,
the error message will either contain the contents of random, possibly
large, chunk of memory(*), or will SEGV.

So if an attacker can control the pattern, they make be able to DoS, fill
up log files, or potentially expose sensitive data containing in the
process's address space.

I think the last one is least likely - for example if a web page accepted
a regex search expression, the error message would go the log file rather
than back to the browser.

Personally I think we we just fix it in blead, backport in the normal
course of affairs, and CVE etc.

(*) you probably have a better idea than me about how much memory could be
exposed based on the specifics of the "wildness" of that pointer.

--
Never work with children, animals, or actors.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 21, 2017

From @jwilk

* Dave Mitchell, 2017-06-21, 05​:03​:

So if an attacker can control the pattern, they make be able to DoS, fill up
log files, or potentially expose sensitive data containing in the process's
address space.

I think the last one is least likely - for example if a web page accepted a
regex search expression, the error message would go the log file rather than
back to the browser.

It's a bad user experience if the search engine tells you that your query is
wrong, but it doesn't tell you why.

So for the implementer, it makes sense to wrap regex compilation in eval{} and
send $@​ back to the user when compilation fails.

--
Jakub Wilk

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 22, 2017

From @khwilliamson

On 06/21/2017 06​:02 AM, Dave Mitchell wrote​:

On Tue, Jun 20, 2017 at 10​:46​:34AM -0600, Karl Williamson wrote​:

The question now is how do we handle this. Since the fix is so trivial, it
can be backported to whatever releases we deem doing it to. I don't
understand what the consequences of this are for an attack.

It seems that for certain types of syntax error in a pattern,
the error message will either contain the contents of random, possibly
large, chunk of memory(*), or will SEGV.

The particular error happens only if someone has a single quotish
pattern that contains precisely this sequence

\N{U+.}

In raising a fatal error, there is a wild pointer. I believe that
should point to a copy of a portion of the pattern. The problem is the
length is wild, and depends on where libc allocated this space which
came from a newSV() call.

So if an attacker can control the pattern, they make be able to DoS, fill
up log files, or potentially expose sensitive data containing in the
process's address space.

I think the last one is least likely - for example if a web page accepted
a regex search expression, the error message would go the log file rather
than back to the browser.

Personally I think we we just fix it in blead, backport in the normal
course of affairs, and CVE etc.

I don't understand. I thought one wanted to synchronize things so the
patch isn't disclosed until all releases are ready, at the same time as
the CVE.

(*) you probably have a better idea than me about how much memory could be
exposed based on the specifics of the "wildness" of that pointer.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 22, 2017

From @iabyn

On Wed, Jun 21, 2017 at 10​:15​:19PM -0600, Karl Williamson wrote​:

On 06/21/2017 06​:02 AM, Dave Mitchell wrote​:
In raising a fatal error, there is a wild pointer. I believe that should
point to a copy of a portion of the pattern. The problem is the length is
wild, and depends on where libc allocated this space which came from a
newSV() call.

So it sounds like it has the potential to dump an random large
chunk of memory.

Personally I think we we just fix it in blead, backport in the normal
course of affairs, and CVE etc.

I don't understand. I thought one wanted to synchronize things so the patch
isn't disclosed until all releases are ready, at the same time as the CVE.

I meant to say "and no CVE etc", although the OP's comment about web
server code accepting a pattern string, compiling it under eval {} then
returning $@​ to the browser perhaps pushes this into CVE territory.

--
Please note that ash-trays are provided for the use of smokers,
whereas the floor is provided for the use of all patrons.
  -- Bill Royston

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 11, 2017

From @khwilliamson

On 06/22/2017 03​:31 AM, Dave Mitchell wrote​:

On Wed, Jun 21, 2017 at 10​:15​:19PM -0600, Karl Williamson wrote​:

On 06/21/2017 06​:02 AM, Dave Mitchell wrote​:
In raising a fatal error, there is a wild pointer. I believe that should
point to a copy of a portion of the pattern. The problem is the length is
wild, and depends on where libc allocated this space which came from a
newSV() call.

So it sounds like it has the potential to dump an random large
chunk of memory.

Personally I think we we just fix it in blead, backport in the normal
course of affairs, and CVE etc.

I don't understand. I thought one wanted to synchronize things so the patch
isn't disclosed until all releases are ready, at the same time as the CVE.

I meant to say "and no CVE etc", although the OP's comment about web
server code accepting a pattern string, compiling it under eval {} then
returning $@​ to the browser perhaps pushes this into CVE territory.

FWIW, at dmq's suggestion at the core hackathon, I have code in the
pipeline for 5.27 that makes this moot. But there is a trivial fix
available for 5.26.1 and previous maintenance releases.

I pinged sawyer about your idea of combining this and 131582. I could
do some more investigation about the actual consequences

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 11, 2017

From @tonycoz

On Wed, 21 Jun 2017 21​:15​:51 -0700, public@​khwilliamson.com wrote​:

On 06/21/2017 06​:02 AM, Dave Mitchell wrote​:

On Tue, Jun 20, 2017 at 10​:46​:34AM -0600, Karl Williamson wrote​:

The question now is how do we handle this. Since the fix is so
trivial, it
can be backported to whatever releases we deem doing it to. I don't
understand what the consequences of this are for an attack.

It seems that for certain types of syntax error in a pattern,
the error message will either contain the contents of random,
possibly
large, chunk of memory(*), or will SEGV.

The particular error happens only if someone has a single quotish
pattern that contains precisely this sequence

\N{U+.}

In raising a fatal error, there is a wild pointer. I believe that
should point to a copy of a portion of the pattern. The problem is
the
length is wild, and depends on where libc allocated this space which
came from a newSV() call.

If this only happens for single-quotish patterns, then this isn't a security issue, since single quotish patterns don't do interpolation.

I wasn't able to reproduce it (but I only tested on x64).

Can it happen in other cases, like​:

  $x = "\\N{U+.}";
  $foo =~ $x

?

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 11, 2017

From @jwilk

* Tony Cook via RT <perl5-security-report@​perl.org>, 2017-08-11, 02​:30​:

If this only happens for single-quotish patterns, then this isn't a security
issue, since single quotish patterns don't do interpolation.

I wasn't able to reproduce it (but I only tested on x64).

Can it happen in other cases, like​:

$x = "\\N{U+.}";
$foo =~ $x

?

Yes; I get the same symptoms for this code as for the original one.

--
Jakub Wilk

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 12, 2017

From @tonycoz

On Fri, 11 Aug 2017 05​:45​:54 -0700, jwilk@​jwilk.net wrote​:

* Tony Cook via RT <perl5-security-report@​perl.org>, 2017-08-11,
02​:30​:

If this only happens for single-quotish patterns, then this isn't a
security
issue, since single quotish patterns don't do interpolation.

I wasn't able to reproduce it (but I only tested on x64).

Can it happen in other cases, like​:

$x = "\\N{U+.}";
$foo =~ $x

?

Yes; I get the same symptoms for this code as for the original one.

Thanks, so it's a security issue.

I think it should be treated as a separate CVE from #131582, since they're different bugs and different consequences.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 12, 2017

From @khwilliamson

On Fri, 11 Aug 2017 18​:00​:15 -0700, tonyc wrote​:

On Fri, 11 Aug 2017 05​:45​:54 -0700, jwilk@​jwilk.net wrote​:

* Tony Cook via RT <perl5-security-report@​perl.org>, 2017-08-11,
02​:30​:

If this only happens for single-quotish patterns, then this isn't a
security
issue, since single quotish patterns don't do interpolation.

I wasn't able to reproduce it (but I only tested on x64).

Can it happen in other cases, like​:

$x = "\\N{U+.}";
$foo =~ $x

So my claim that it only happens with a single-quoted pattern was wrong

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 16, 2017

From @tonycoz

On Fri, 11 Aug 2017 18​:00​:15 -0700, tonyc wrote​:

On Fri, 11 Aug 2017 05​:45​:54 -0700, jwilk@​jwilk.net wrote​:

* Tony Cook via RT <perl5-security-report@​perl.org>, 2017-08-11,
02​:30​:

If this only happens for single-quotish patterns, then this isn't a
security
issue, since single quotish patterns don't do interpolation.

I wasn't able to reproduce it (but I only tested on x64).

Can it happen in other cases, like​:

$x = "\\N{U+.}";
$foo =~ $x

?

Yes; I get the same symptoms for this code as for the original one.

Thanks, so it's a security issue.

I think it should be treated as a separate CVE from #131582, since
they're different bugs and different consequences.

This is CVE-2017-12883.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 16, 2017

From @tonycoz

On Mon, 10 Jul 2017 20​:37​:20 -0700, public@​khwilliamson.com wrote​:

FWIW, at dmq's suggestion at the core hackathon, I have code in the
pipeline for 5.27 that makes this moot. But there is a trivial fix
available for 5.26.1 and previous maintenance releases.

Could you post that fix here as a patch please?

Thanks,
Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 24, 2017

From @khwilliamson

On Wed, 16 Aug 2017 16​:55​:52 -0700, tonyc wrote​:

On Mon, 10 Jul 2017 20​:37​:20 -0700, public@​khwilliamson.com wrote​:

FWIW, at dmq's suggestion at the core hackathon, I have code in the
pipeline for 5.27 that makes this moot. But there is a trivial fix
available for 5.26.1 and previous maintenance releases.

Could you post that fix here as a patch please?

Thanks,
Tony

Will do, somehow this request didn't get emailed to me. It should go in 26.1, but that discloses it
--
Karl Williamson

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 25, 2017

From @tonycoz

On Thu, 24 Aug 2017 13​:08​:10 -0700, khw wrote​:

On Wed, 16 Aug 2017 16​:55​:52 -0700, tonyc wrote​:

On Mon, 10 Jul 2017 20​:37​:20 -0700, public@​khwilliamson.com wrote​:

FWIW, at dmq's suggestion at the core hackathon, I have code in the
pipeline for 5.27 that makes this moot. But there is a trivial fix
available for 5.26.1 and previous maintenance releases.

Could you post that fix here as a patch please?

Thanks,
Tony

Will do, somehow this request didn't get emailed to me.

Yeah, it's possible not all RT responses are going through to the list.

It should go
in 26.1, but that discloses it

I'm not asking that it go into blead, ideally the patch would get disclosed publicly and committed to blead, maint all at the same time.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 25, 2017

From @khwilliamson

On Thu, 24 Aug 2017 21​:45​:31 -0700, tonyc wrote​:

On Thu, 24 Aug 2017 13​:08​:10 -0700, khw wrote​:

On Wed, 16 Aug 2017 16​:55​:52 -0700, tonyc wrote​:

On Mon, 10 Jul 2017 20​:37​:20 -0700, public@​khwilliamson.com wrote​:

FWIW, at dmq's suggestion at the core hackathon, I have code in
the
pipeline for 5.27 that makes this moot. But there is a trivial
fix
available for 5.26.1 and previous maintenance releases.

Could you post that fix here as a patch please?

Thanks,
Tony

Will do, somehow this request didn't get emailed to me.

Yeah, it's possible not all RT responses are going through to the
list.

Is there any way to fix this?

It should go
in 26.1, but that discloses it

I'm not asking that it go into blead, ideally the patch would get
disclosed publicly and committed to blead, maint all at the same time.

Tony

Attached. What I neglected to say is that I think this should go into 5.24.3, so that would mean that release and 5.26.1 and blead all have to be updated in sync.
--
Karl Williamson

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 25, 2017

From @khwilliamson

0002-PATCH-perl-131598.patch
From d4857ae370c5a111b5f0ae61c3ae05e0b99e2237 Mon Sep 17 00:00:00 2001
From: Karl Williamson <khw@cpan.org>
Date: Fri, 25 Aug 2017 11:33:58 -0600
Subject: [PATCH 2/2] PATCH: [perl #131598]

The cause of this is that the vFAIL macro uses RExC_parse, and that
variable has just been changed in preparation for code after the vFAIL.
The solution is to not change RExC_parse until after the vFAIL.

This is a case where the macro hides stuff that can bite you.
---
 regcomp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/regcomp.c b/regcomp.c
index 6a07bf2c70..4eff83e50e 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -12199,14 +12199,16 @@ S_grok_bslash_N(pTHX_ RExC_state_t *pRExC_state,
 	}
         sv_catpv(substitute_parse, ")");
 
-        RExC_parse = RExC_start = RExC_adjusted_start = SvPV(substitute_parse,
-                                                             len);
+        len = SvCUR(substitute_parse);
 
 	/* Don't allow empty number */
 	if (len < (STRLEN) 8) {
             RExC_parse = endbrace;
 	    vFAIL("Invalid hexadecimal number in \\N{U+...}");
 	}
+
+        RExC_parse = RExC_start = RExC_adjusted_start
+                                              = SvPV_nolen(substitute_parse);
 	RExC_end = RExC_parse + len;
 
         /* The values are Unicode, and therefore not subject to recoding, but
-- 
2.11.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 10, 2017

From @steve-m-hay

Now in blead as commit 2be4ede. Will shortly also be in 5.24.3-RC1 and 5.26.1-RC1...

1 similar comment
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 10, 2017

From @steve-m-hay

Now in blead as commit 2be4ede. Will shortly also be in 5.24.3-RC1 and 5.26.1-RC1...

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 11, 2017

From @xsawyerx

This was set to be disclosed on September 22nd. Disclosure list was informed.

On 10 September 2017 at 16​:16, Steve Hay via RT <rt-comment@​perl.org> wrote​:

Now in blead as commit 2be4ede. Will shortly also be in 5.24.3-RC1 and 5.26.1-RC1...

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 11, 2017

From @tonycoz

On Wed, 16 Aug 2017 16​:53​:18 -0700, tonyc wrote​:

This is CVE-2017-12883.

The details I entered when requesting the CVE ID​:

[Suggested description]
Buffer over-read when compiling a crafted regular expression
------------------------------------------
[Vulnerability Type]
Buffer Overflow

------------------------------------------
[Vendor of Product]
Perl5 Porters
------------------------------------------
[Affected Product Code Base]
perl - 5.26.0
------------------------------------------
[Affected Component]

------------------------------------------
[Attack Type]
Local

------------------------------------------
[Impact ]

[-] CVE_Request.Impact_Code_execution
[-] CVE_Request.Impact_Denial_of_Service
[-] CVE_Request.Impact_Escalation_of_Privileges
[+] CVE_Request.Impact_Information_Disclosure

Proposed update for the CVE entry once the issue is public (the field names are from the CVE allocation form)​:

Affected components​:

regular expression compiler, S_grok_bslash_N() in regcomp.c

Attack vector​:

An attacker can provide a crafted regular expression with an
invalid \N{U+...} escape, the error message produced will access
memory beyond the end of a memory block, possibly including
sensitive information stored after the regular expression string,
or crashing perl.

Discoverer​:

Jakub Wilk <jwilk@​jwilk.net>

Affected Product Code Base​:

perl - 5.26.0, fixed in 5.26.1
perl - 5.24.x, fixed in 5.24.3
perl - 5.22.x and earlier

References​:

https://rt.perl.org/Public/Bug/Display.html?id=131598

Additional information​:

(none)

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 24, 2017

From @tonycoz

On Mon, 11 Sep 2017 15​:48​:41 -0700, tonyc wrote​:

Affected Product Code Base​:

perl - 5.26.0, fixed in 5.26.1
perl - 5.24.x, fixed in 5.24.3
perl - 5.22.x and earlier

perl - 5.26.0, fixed in 5.26.1
perl - 5.24.x, fixed in 5.24.3
perl - 5.20 through 5.22.x

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 25, 2017

@xsawyerx - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this Sep 25, 2017
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 25, 2017

From @xsawyerx

Now open.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 25, 2017

From @tonycoz

On Mon, 25 Sep 2017 03​:10​:11 -0700, xsawyerx@​cpan.org wrote​:

Now open.

Update to CVE requested.

Tony

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.