regcomp: infinite loop in S_regbranch()

[p5pRT]

Migrated from rt.perl.org#133185 (status was 'open')

Searchable as RT133185$

[p5pRT]

From @Etsukata

# Summary

A crafted regular expression with a unicode property and (?aai) extended
pattern
can cause infinite loop in compilation which leads to DoS.

## Affected Versions

- perl-5.28.0(Commit​: 38c84d6)
- perl-5.26.2
- perl-5.24.4

## Not Affected Versions

- perl-5.26.1
- perl-5.24.3 or prior

# PoC

$ perl -e 'my $r = "\\pp(?aai)\\xdf"; qr/$r/;'

# Additional Notes

- git bisect from perl-5.26.1 to perl-5.26.2 found the following commit.
```
$ git bisect view
commit abe1e6c
Author​: Karl Williamson <khw@​cpan.org>
Date​: Fri Feb 2 15​:14​:27 2018 -0700

  (perl #132227) restart a node if we change to uni rules within the node
and encounter a sharp S

  This could lead to a buffer overflow.

  (cherry picked from commit a02c70e35d1313a5f4e245e8f863c810e991172d)
```

- sample backtrace in perl-5.26.2
```
(gdb) bt
#0 S_skip_to_be_ignored_text (pRExC_state=pRExC_state@​entry=0x7fffffffd4d0,
p=p@​entry=0x7fffffffd510,
  force_to_xmod=force_to_xmod@​entry=false) at regcomp.c​:18383
#1 0x00000000004b9d48 in S_regatom (pRExC_state=pRExC_state@​entry
=0x7fffffffd4d0,
  flagp=flagp@​entry=0x7fffffffd1d4, depth=depth@​entry=4) at
regcomp.c​:13855
#2 0x00000000004bd259 in S_regpiece (depth=3, flagp=<synthetic pointer>,
pRExC_state=0x7fffffffd4d0)
  at regcomp.c​:11673
#3 S_regbranch (pRExC_state=pRExC_state@​entry=0x7fffffffd4d0,
flagp=flagp@​entry=0x7fffffffd29c,
  first=first@​entry=1, depth=depth@​entry=2) at regcomp.c​:11598
#4 0x00000000004ae816 in S_reg (pRExC_state=pRExC_state@​entry=0x7fffffffd4d0,
paren=paren@​entry=0,
  flagp=flagp@​entry=0x7fffffffd39c, depth=depth@​entry=1) at
regcomp.c​:11336
#5 0x00000000004d107f in Perl_re_op_compile (patternp=<optimized out>,
pat_count=<optimized out>,
  expr=<optimized out>, eng=0x66b8a0 <PL_core_reg_engine>, old_re=0x0,
is_bare_re=<optimized out>,
  orig_rx_flags=0, pm_flags=1073741824) at regcomp.c​:7105
#6 0x000000000057de0f in Perl_pp_regcomp () at pp_ctl.c​:108
#7 0x00000000004d9432 in Perl_runops_debug () at dump.c​:2450
#8 0x0000000000451eca in S_run_body (oldscope=1) at perl.c​:2532
#9 perl_run (my_perl=<optimized out>) at perl.c​:2455
#10 0x000000000041edfa in main (argc=<optimized out>, argv=<optimized out>,
env=<optimized out>)
  at perlmain.c​:123
```

[p5pRT]

From @khwilliamson

On 05/08/2018 04​:12 AM, Eiichi Tsukata (via RT) wrote​:

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

# Summary

A crafted regular expression with a unicode property and (?aai) extended
pattern
can cause infinite loop in compilation which leads to DoS.

## Affected Versions

- perl-5.28.0(Commit​: 38c84d6)
- perl-5.26.2
- perl-5.24.4

## Not Affected Versions

- perl-5.26.1
- perl-5.24.3 or prior

# PoC

$ perl -e 'my $r = "\\pp(?aai)\\xdf"; qr/$r/;'

# Additional Notes

- git bisect from perl-5.26.1 to perl-5.26.2 found the following commit.
```
$ git bisect view
commit abe1e6c
Author​: Karl Williamson <khw@​cpan.org>
Date​: Fri Feb 2 15​:14​:27 2018 -0700

 \(perl \#132227\) restart a node if we change to uni rules within the node

and encounter a sharp S

 This could lead to a buffer overflow\.

 \(cherry picked from commit a02c70e35d1313a5f4e245e8f863c810e991172d\)

```

- sample backtrace in perl-5.26.2
```
(gdb) bt
#0 S_skip_to_be_ignored_text (pRExC_state=pRExC_state@​entry=0x7fffffffd4d0,
p=p@​entry=0x7fffffffd510,
force_to_xmod=force_to_xmod@​entry=false) at regcomp.c​:18383
#1 0x00000000004b9d48 in S_regatom (pRExC_state=pRExC_state@​entry
=0x7fffffffd4d0,
flagp=flagp@​entry=0x7fffffffd1d4, depth=depth@​entry=4) at
regcomp.c​:13855
#2 0x00000000004bd259 in S_regpiece (depth=3, flagp=<synthetic pointer>,
pRExC_state=0x7fffffffd4d0)
at regcomp.c​:11673
#3 S_regbranch (pRExC_state=pRExC_state@​entry=0x7fffffffd4d0,
flagp=flagp@​entry=0x7fffffffd29c,
first=first@​entry=1, depth=depth@​entry=2) at regcomp.c​:11598
#4 0x00000000004ae816 in S_reg (pRExC_state=pRExC_state@​entry=0x7fffffffd4d0,
paren=paren@​entry=0,
flagp=flagp@​entry=0x7fffffffd39c, depth=depth@​entry=1) at
regcomp.c​:11336
#5 0x00000000004d107f in Perl_re_op_compile (patternp=<optimized out>,
pat_count=<optimized out>,
expr=<optimized out>, eng=0x66b8a0 <PL_core_reg_engine>, old_re=0x0,
is_bare_re=<optimized out>,
orig_rx_flags=0, pm_flags=1073741824) at regcomp.c​:7105
#6 0x000000000057de0f in Perl_pp_regcomp () at pp_ctl.c​:108
#7 0x00000000004d9432 in Perl_runops_debug () at dump.c​:2450
#8 0x0000000000451eca in S_run_body (oldscope=1) at perl.c​:2532
#9 perl_run (my_perl=<optimized out>) at perl.c​:2455
#10 0x000000000041edfa in main (argc=<optimized out>, argv=<optimized out>,
env=<optimized out>)
at perlmain.c​:123
```

Ouch.

Do we need a CVE for a DOS attack? What maintenance versions should be
patched?

This stems from a way through the code that I never would have thought
of. To be vulnerable, the pattern must start out as /d (hence no use
5.012 or higher), and then there must be something that implicitly
forces /u (which the \pp does here), and then (?aa), and then the code
point \xDF. (German Sharp S). The /i must be in effect by the time the
DF is encountered, but it needn't come in the (?aa) which this example does.

The problem is that the conditional that is testing that we switched
away from /d rules is assuming that this happened during the
construction of the current EXACTFish node. The comments I wrote
indicate this assumption. But this example shows that the switch can
come before this node started getting constructed, and so it loops. The
attached patch explicitly saves the state at the beginning of node
construction, and only retries if it changed during that construction.
Therefore the next time through, it will see that it hasn't changed
since the previous time, and won't loop.

There is a slightly simpler patch that seems to work, but this one gets
more to the heart of the issue, so should avoid other things that I may
have overlooked.

[p5pRT]

From @khwilliamson

0003-perl-133185.patch

From f61093be9051ba745acf497cf86118069ccd2c6a Mon Sep 17 00:00:00 2001
From: Karl Williamson <khw@cpan.org>
Date: Tue, 8 May 2018 12:13:18 -0600
Subject: [PATCH 3/3] [perl #133185]

---
 regcomp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/regcomp.c b/regcomp.c
index 800ab2d444..564dcc68a3 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -13507,6 +13507,8 @@ S_regatom(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth)
              * need to figure this out until pass 2) */
             bool maybe_exactfu = PASS2;
 
+            bool uni_semantics_at_node_start = RExC_uni_semantics;
+
             /* The node_type may change below, but since the size of the node
              * doesn't change, it works */
 	    ret = reg_node(pRExC_state, node_type);
@@ -13970,8 +13972,9 @@ S_regatom(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth)
                                 ender = 's';
                                 added_len = 2;
                             }
-                            else if (RExC_uni_semantics) {
-
+                            else if (   uni_semantics_at_node_start
+                                     != RExC_uni_semantics)
+                            {
                                 /* Here, we are supossed to be using Unicode
                                  * rules, but this folding node is not.  This
                                  * happens during pass 1 when the node started
-- 
2.11.0

[p5pRT]

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

[p5pRT]

From @khwilliamson

On 05/08/2018 12​:16 PM, Karl Williamson wrote​:

On 05/08/2018 04​:12 AM, Eiichi Tsukata (via RT) wrote​:

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

# Summary

A crafted regular expression with a unicode property and (?aai) extended
pattern
can cause infinite loop in compilation which leads to DoS.

## Affected Versions

- perl-5.28.0(Commit​: 38c84d6)
- perl-5.26.2
- perl-5.24.4

## Not Affected Versions

- perl-5.26.1
- perl-5.24.3 or prior

# PoC

$ perl -e 'my $r = "\\pp(?aai)\\xdf"; qr/$r/;'

# Additional Notes

- git bisect from perl-5.26.1 to perl-5.26.2 found the following commit.
```
$ git bisect view
commit abe1e6c
Author​: Karl Williamson <khw@​cpan.org>
Date​:   Fri Feb 2 15​:14​:27 2018 -0700

     (perl #132227) restart a node if we change to uni rules within
the node
and encounter a sharp S

     This could lead to a buffer overflow.

     (cherry picked from commit a02c70e35d1313a5f4e245e8f863c810e991172d)
```

- sample backtrace in perl-5.26.2
```
(gdb) bt
#0  S_skip_to_be_ignored_text
(pRExC_state=pRExC_state@​entry=0x7fffffffd4d0,
p=p@​entry=0x7fffffffd510,
     force_to_xmod=force_to_xmod@​entry=false) at regcomp.c​:18383
#1  0x00000000004b9d48 in S_regatom (pRExC_state=pRExC_state@​entry
=0x7fffffffd4d0,
     flagp=flagp@​entry=0x7fffffffd1d4, depth=depth@​entry=4) at
regcomp.c​:13855
#2  0x00000000004bd259 in S_regpiece (depth=3, flagp=<synthetic pointer>,
pRExC_state=0x7fffffffd4d0)
     at regcomp.c​:11673
#3  S_regbranch (pRExC_state=pRExC_state@​entry=0x7fffffffd4d0,
flagp=flagp@​entry=0x7fffffffd29c,
     first=first@​entry=1, depth=depth@​entry=2) at regcomp.c​:11598
#4  0x00000000004ae816 in S_reg
(pRExC_state=pRExC_state@​entry=0x7fffffffd4d0,
paren=paren@​entry=0,
     flagp=flagp@​entry=0x7fffffffd39c, depth=depth@​entry=1) at
regcomp.c​:11336
#5  0x00000000004d107f in Perl_re_op_compile (patternp=<optimized out>,
pat_count=<optimized out>,
     expr=<optimized out>, eng=0x66b8a0 <PL_core_reg_engine>, old_re=0x0,
is_bare_re=<optimized out>,
     orig_rx_flags=0, pm_flags=1073741824) at regcomp.c​:7105
#6  0x000000000057de0f in Perl_pp_regcomp () at pp_ctl.c​:108
#7  0x00000000004d9432 in Perl_runops_debug () at dump.c​:2450
#8  0x0000000000451eca in S_run_body (oldscope=1) at perl.c​:2532
#9  perl_run (my_perl=<optimized out>) at perl.c​:2455
#10 0x000000000041edfa in main (argc=<optimized out>, argv=<optimized
out>,
env=<optimized out>)
     at perlmain.c​:123
```

Ouch.

Do we need a CVE for a DOS attack?  What maintenance versions should be
patched?

Dave Mitchell emailed me privately that this is not a security issue in
his view, because it involves a crafted regular expression, and there
are plenty of other, simpler, patterns that an attacker could use
instead. So if you're an application that executes arbitrary user-input
regular expressions, you're already vulnerable, without this new one.

So it doesn't need a CVE, and doesn't need an immediate maintenance
release, but should be on the list for the next maintenance release(s).

This stems from a way through the code that I never would have thought
of.  To be vulnerable, the pattern must start out as /d (hence no use
5.012 or higher), and then there must be something that implicitly
forces /u (which the \pp does here), and then (?aa), and then the code
point \xDF. (German Sharp S).  The /i must be in effect by the time the
DF is encountered, but it needn't come in the (?aa) which this example
does.

The problem is that the conditional that is testing that we switched
away from /d rules is assuming that this happened during the
construction of the current EXACTFish node.  The comments I wrote
indicate this assumption.  But this example shows that the switch can
come before this node started getting constructed, and so it loops.  The
attached patch explicitly saves the state at the beginning of node
construction, and only retries if it changed during that construction.
Therefore the next time through, it will see that it hasn't changed
since the previous time, and won't loop.

There is a slightly simpler patch that seems to work, but this one gets
more to the heart of the issue, so should avoid other things that I may
have overlooked.

Attached is a patch to the patch, that just moves a statement. I don't
think it actually is needed, but there is a possibility of a loop via a
goto a label, and so the defensive thing to guard against future
breakage is to squash the two patches together.

As far as a test, I can add one using a watchdog timer to interrupt the
loop. I'm a little leery of doing this this close to the final ship
date, just because it's a little complicated, and I worry about
deliberately adding something known to loop should the timer not work
out. But I would defer to expert opinion on this, accompanied by a code
review.

[p5pRT]

From @khwilliamson

0004-follow-up-to-133185.patch

From 5a2c491a4775fa7c58d87a76caf6b5b038662665 Mon Sep 17 00:00:00 2001
From: Karl Williamson <khw@cpan.org>
Date: Wed, 9 May 2018 18:25:47 -0600
Subject: [PATCH 4/4] follow up to 133185

---
 regcomp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/regcomp.c b/regcomp.c
index 564dcc68a3..e1a048ff97 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -13507,7 +13507,9 @@ S_regatom(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth)
              * need to figure this out until pass 2) */
             bool maybe_exactfu = PASS2;
 
-            bool uni_semantics_at_node_start = RExC_uni_semantics;
+            /* To see if RExC_uni_semantics changes during parsing of the node.
+             * */
+            bool uni_semantics_at_node_start;
 
             /* The node_type may change below, but since the size of the node
              * doesn't change, it works */
@@ -13535,6 +13537,8 @@ S_regatom(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth)
                    || UTF8_IS_INVARIANT(UCHARAT(RExC_parse))
                    || UTF8_IS_START(UCHARAT(RExC_parse)));
 
+            uni_semantics_at_node_start = RExC_uni_semantics;
+
             /* Here, we have a literal character.  Find the maximal string of
              * them in the input that we can fit into a single EXACTish node.
              * We quit at the first non-literal or when the node gets full, or
-- 
2.11.0

[p5pRT]

From @Etsukata

So if you're an application that executes arbitrary user-input
regular expressions, you're already vulnerable, without this new one.

Yes, actually I have the same idea.
The reason because I reported this bug closely is the regcomp related bugs
are listed as CVEs.

- https://www.cvedetails.com/cve/CVE-2017-12837/
- https://www.cvedetails.com/cve/CVE-2017-12883/
- https://www.cvedetails.com/cve/CVE-2013-7422/

But here a question, when should I report these kind of bugs publicly(or
closely)?
Should this report be publicly available now?

I understand it's very difficult to determine whether a bug is worth
reporting as a security issue.
I'd like to hear about the approximate rule of perl community.

2018-05-10 9​:28 GMT+09​:00 karl williamson via RT <
perl5-security-report-followup@​perl.org>​:

On 05/08/2018 12​:16 PM, Karl Williamson wrote​:

On 05/08/2018 04​:12 AM, Eiichi Tsukata (via RT) wrote​:

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

# Summary

A crafted regular expression with a unicode property and (?aai) extended
pattern
can cause infinite loop in compilation which leads to DoS.

## Affected Versions

- perl-5.28.0(Commit​: 38c84d6)
- perl-5.26.2
- perl-5.24.4

## Not Affected Versions

- perl-5.26.1
- perl-5.24.3 or prior

# PoC

$ perl -e 'my $r = "\\pp(?aai)\\xdf"; qr/$r/;'

# Additional Notes

- git bisect from perl-5.26.1 to perl-5.26.2 found the following commit.
```
$ git bisect view
commit abe1e6c
Author​: Karl Williamson <khw@​cpan.org>
Date​: Fri Feb 2 15​:14​:27 2018 -0700

 \(perl \#132227\) restart a node if we change to uni rules within

the node
and encounter a sharp S

 This could lead to a buffer overflow\.

 \(cherry picked from commit a02c70e35d1313a5f4e245e8f863c8

10e991172d)

```

- sample backtrace in perl-5.26.2
```
(gdb) bt
#0 S_skip_to_be_ignored_text
(pRExC_state=pRExC_state@​entry=0x7fffffffd4d0,
p=p@​entry=0x7fffffffd510,
force_to_xmod=force_to_xmod@​entry=false) at regcomp.c​:18383
#1 0x00000000004b9d48 in S_regatom (pRExC_state=pRExC_state@​entry
=0x7fffffffd4d0,
flagp=flagp@​entry=0x7fffffffd1d4, depth=depth@​entry=4) at
regcomp.c​:13855
#2 0x00000000004bd259 in S_regpiece (depth=3, flagp=<synthetic
pointer>,
pRExC_state=0x7fffffffd4d0)
at regcomp.c​:11673
#3 S_regbranch (pRExC_state=pRExC_state@​entry=0x7fffffffd4d0,
flagp=flagp@​entry=0x7fffffffd29c,
first=first@​entry=1, depth=depth@​entry=2) at regcomp.c​:11598
#4 0x00000000004ae816 in S_reg
(pRExC_state=pRExC_state@​entry=0x7fffffffd4d0,
paren=paren@​entry=0,
flagp=flagp@​entry=0x7fffffffd39c, depth=depth@​entry=1) at
regcomp.c​:11336
#5 0x00000000004d107f in Perl_re_op_compile (patternp=<optimized out>,
pat_count=<optimized out>,
expr=<optimized out>, eng=0x66b8a0 <PL_core_reg_engine>,
old_re=0x0,
is_bare_re=<optimized out>,
orig_rx_flags=0, pm_flags=1073741824) at regcomp.c​:7105
#6 0x000000000057de0f in Perl_pp_regcomp () at pp_ctl.c​:108
#7 0x00000000004d9432 in Perl_runops_debug () at dump.c​:2450
#8 0x0000000000451eca in S_run_body (oldscope=1) at perl.c​:2532
#9 perl_run (my_perl=<optimized out>) at perl.c​:2455
#10 0x000000000041edfa in main (argc=<optimized out>, argv=<optimized
out>,
env=<optimized out>)
at perlmain.c​:123
```

Ouch.

Do we need a CVE for a DOS attack? What maintenance versions should be
patched?

Dave Mitchell emailed me privately that this is not a security issue in
his view, because it involves a crafted regular expression, and there
are plenty of other, simpler, patterns that an attacker could use
instead. So if you're an application that executes arbitrary user-input
regular expressions, you're already vulnerable, without this new one.

So it doesn't need a CVE, and doesn't need an immediate maintenance
release, but should be on the list for the next maintenance release(s).

This stems from a way through the code that I never would have thought
of. To be vulnerable, the pattern must start out as /d (hence no use
5.012 or higher), and then there must be something that implicitly
forces /u (which the \pp does here), and then (?aa), and then the code
point \xDF. (German Sharp S). The /i must be in effect by the time the
DF is encountered, but it needn't come in the (?aa) which this example
does.

The problem is that the conditional that is testing that we switched
away from /d rules is assuming that this happened during the
construction of the current EXACTFish node. The comments I wrote
indicate this assumption. But this example shows that the switch can
come before this node started getting constructed, and so it loops. The
attached patch explicitly saves the state at the beginning of node
construction, and only retries if it changed during that construction.
Therefore the next time through, it will see that it hasn't changed
since the previous time, and won't loop.

There is a slightly simpler patch that seems to work, but this one gets
more to the heart of the issue, so should avoid other things that I may
have overlooked.

Attached is a patch to the patch, that just moves a statement. I don't
think it actually is needed, but there is a possibility of a loop via a
goto a label, and so the defensive thing to guard against future
breakage is to squash the two patches together.

As far as a test, I can add one using a watchdog timer to interrupt the
loop. I'm a little leery of doing this this close to the final ship
date, just because it's a little complicated, and I worry about
deliberately adding something known to loop should the timer not work
out. But I would defer to expert opinion on this, accompanied by a code
review.

[p5pRT]

From @iabyn

On Wed, May 09, 2018 at 06​:27​:30PM -0600, Karl Williamson wrote​:

Dave Mitchell emailed me privately

Whoops, I meant to Cc the list

As far as a test, I can add one using a watchdog timer to interrupt the
loop. I'm a little leery of doing this this close to the final ship date,
just because it's a little complicated, and I worry about deliberately
adding something known to loop should the timer not work out. But I would
defer to expert opinion on this, accompanied by a code review.

That's what t/re/speed.t is for. A test which should be almost
instantaneous but would take forever if buggy, is ideal for that test
script. Make sure that the pattern is compiled at run time though.

--
A power surge on the Bridge is rapidly and correctly diagnosed as a faulty
capacitor by the highly-trained and competent engineering staff.
  -- Things That Never Happen in "Star Trek" #9

[p5pRT]

From @iabyn

On Thu, May 10, 2018 at 10​:53​:59AM +0900, Eiichi Tsukata wrote​:

So if you're an application that executes arbitrary user-input
regular expressions, you're already vulnerable, without this new one.

Yes, actually I have the same idea.
The reason because I reported this bug closely is the regcomp related bugs
are listed as CVEs.

- https://www.cvedetails.com/cve/CVE-2017-12837/
- https://www.cvedetails.com/cve/CVE-2017-12883/
- https://www.cvedetails.com/cve/CVE-2013-7422/

Those bugs aren't simply 'burns lots of CPU' but include buffer overruns
and the like, so are potentially more serious.

But here a question, when should I report these kind of bugs publicly(or
closely)?

I've you're not sure, sending to the security address is best - we can
always make the ticket public later.

Should this report be publicly available now?

We'll transfer it to the public queue sometime soon (assuming we don't
forget!)

I understand it's very difficult to determine whether a bug is worth
reporting as a security issue.
I'd like to hear about the approximate rule of perl community.

The rough rule of thumb is that if the perl interpreter can be compromised
with cracker-crafted data, then it's a security issue (e.g. malformed utf8
data from a malicious web server). If it takes cracker-crafted code, then
the cracker could just feed in 'system "rm -rf /"' as code anyway, so we
don't generally regard that as a security issue.

Regex patterns are a bit of a grey area​: allowing a cracker to specify a
match pattern (e.g. via a web search form) is a bad idea, but people do it
anyway - so we tend to take regexp bugs more seriously. But something
which is just a pure DoS (e.g. burning up CPU) doesn't usually worry us.

--
Nothing ventured, nothing lost.

[p5pRT]

From @Etsukata

But something
which is just a pure DoS (e.g. burning up CPU) doesn't usually worry us.

A little confusing, so let me clarify the perl5 community's criteria.

Case​: (user input) regular expression, unicode character, JSON, XML, etc
... handling

- Out-of-Bound read or write => a security issue
- Infinite loop => *NOT* a security issue
- other => it depends

Right?

2018-05-10 18​:16 GMT+09​:00 Dave Mitchell via RT <
perl5-security-report-followup@​perl.org>​:

On Thu, May 10, 2018 at 10​:53​:59AM +0900, Eiichi Tsukata wrote​:

So if you're an application that executes arbitrary user-input
regular expressions, you're already vulnerable, without this new one.

Yes, actually I have the same idea.
The reason because I reported this bug closely is the regcomp related
bugs
are listed as CVEs.

- https://www.cvedetails.com/cve/CVE-2017-12837/
- https://www.cvedetails.com/cve/CVE-2017-12883/
- https://www.cvedetails.com/cve/CVE-2013-7422/

Those bugs aren't simply 'burns lots of CPU' but include buffer overruns
and the like, so are potentially more serious.

But here a question, when should I report these kind of bugs publicly(or
closely)?

I've you're not sure, sending to the security address is best - we can
always make the ticket public later.

Should this report be publicly available now?

We'll transfer it to the public queue sometime soon (assuming we don't
forget!)

I understand it's very difficult to determine whether a bug is worth
reporting as a security issue.
I'd like to hear about the approximate rule of perl community.

The rough rule of thumb is that if the perl interpreter can be compromised
with cracker-crafted data, then it's a security issue (e.g. malformed utf8
data from a malicious web server). If it takes cracker-crafted code, then
the cracker could just feed in 'system "rm -rf /"' as code anyway, so we
don't generally regard that as a security issue.

Regex patterns are a bit of a grey area​: allowing a cracker to specify a
match pattern (e.g. via a web search form) is a bad idea, but people do it
anyway - so we tend to take regexp bugs more seriously. But something
which is just a pure DoS (e.g. burning up CPU) doesn't usually worry us.

--
Nothing ventured, nothing lost.

[p5pRT]

From @Etsukata

Yes to those three criteria *if* we are talking about a regex pattern.

If we are talking about something else, like data being read from a file
or socket, then our criteria would be stricter.

I understand.
Thanks a lot!

2018-05-10 21​:40 GMT+09​:00 Dave Mitchell <davem@​iabyn.com>​:

On Thu, May 10, 2018 at 09​:29​:29PM +0900, Eiichi Tsukata wrote​:

But something
which is just a pure DoS (e.g. burning up CPU) doesn't usually worry
us.

A little confusing, so let me clarify the perl5 community's criteria.

Case​: (user input) regular expression, unicode character, JSON, XML, etc
... handling

- Out-of-Bound read or write => a security issue
- Infinite loop => *NOT* a security issue
- other => it depends

Yes to those three criteria *if* we are talking about a regex pattern.

If we are talking about something else, like data being read from a file
or socket, then our criteria would be stricter.

--
This email is confidential, and now that you have read it you are legally
obliged to shoot yourself. Or shoot a lawyer, if you prefer. If you have
received this email in error, place it in its original wrapping and return
for a full refund. By opening this email, you accept that Elvis lives.

[p5pRT]

From @khwilliamson

On 05/10/2018 03​:08 AM, Dave Mitchell wrote​:

On Wed, May 09, 2018 at 06​:27​:30PM -0600, Karl Williamson wrote​:

Dave Mitchell emailed me privately

Whoops, I meant to Cc the list

As far as a test, I can add one using a watchdog timer to interrupt the
loop. I'm a little leery of doing this this close to the final ship date,
just because it's a little complicated, and I worry about deliberately
adding something known to loop should the timer not work out. But I would
defer to expert opinion on this, accompanied by a code review.

That's what t/re/speed.t is for. A test which should be almost
instantaneous but would take forever if buggy, is ideal for that test
script. Make sure that the pattern is compiled at run time though.

Thanks, attached is a patch that adds a test.

[p5pRT]

From @khwilliamson

0005-Test-for-133185.patch

From 4022f74bed7920597f2be29b94a496d30c961e67 Mon Sep 17 00:00:00 2001
From: Karl Williamson <khw@cpan.org>
Date: Thu, 10 May 2018 10:52:40 -0600
Subject: [PATCH 5/5] Test for #133185

---
 t/re/speed.t | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/re/speed.t b/t/re/speed.t
index 4a4830f602..9a57de167f 100644
--- a/t/re/speed.t
+++ b/t/re/speed.t
@@ -24,7 +24,7 @@ BEGIN {
 skip_all('no re module') unless defined &DynaLoader::boot_DynaLoader;
 skip_all_without_unicode_tables();
 
-plan tests => 58;  #** update watchdog timeouts proportionally when adding tests
+plan tests => 59;  #** update watchdog timeouts proportionally when adding tests
 
 use strict;
 use warnings;
@@ -156,6 +156,9 @@ PROG
         ok( $elapsed <= 1, "should not COW on long string with substr and m//g");
     }
 
+    # [perl #133185] Infinite loop
+    like("!\xdf", eval 'qr/\pp(?aai)\xdf/',
+         'Compiling qr/\pp(?aai)\xdf/ doesn\'t loop');
 
 } # End of sub run_tests
 
-- 
2.11.0

[p5pRT]

From @khwilliamson

On 05/10/2018 10​:58 AM, Karl Williamson wrote​:

On 05/10/2018 03​:08 AM, Dave Mitchell wrote​:

And, the attached patch squashes together the related ones and adds a
commit message, so it should be good to be applied.

On Wed, May 09, 2018 at 06​:27​:30PM -0600, Karl Williamson wrote​:

Dave Mitchell emailed me privately

Whoops, I meant to Cc the list

As far as a test, I can add one using a watchdog timer to interrupt the
loop.  I'm a little leery of doing this this close to the final ship
date,
just because it's a little complicated, and I worry about deliberately
adding something known to loop should the timer not work out.  But I
would
defer to expert opinion on this, accompanied by a code review.

That's what t/re/speed.t is for. A test which should be almost
instantaneous but would take forever if buggy, is ideal for that test
script. Make sure that the pattern is compiled at run time though.

Thanks, attached is a patch that adds a test.

[p5pRT]

From @khwilliamson

0003-PATCH-perl-133185-Infinite-loop-in-qr.patch

From 7f8cd3af1b53891ac7440bc8d37456b6acff6fb1 Mon Sep 17 00:00:00 2001
From: Karl Williamson <khw@cpan.org>
Date: Tue, 8 May 2018 12:13:18 -0600
Subject: [PATCH 3/3] PATCH: [perl #133185] Infinite loop in qr//

This loop was inadvertently introduced as part of patches to fix
(perl #132227 CVE-2018-6797] heap-buffer-overflow".  The commit in 5.27
responsible was f8fb8615ddc5a80e3bbd4386a8914497f921b62d.

To be vulnerable, the pattern must start out as /d (hence no use 5.012
or higher), and then there must be something that implicitly forces /u
(which the \pp does in the test case added by this patch), and then
(?aa), and then the code point \xDF.  (German Sharp S).  The /i must be
in effect by the time the DF is encountered, but it needn't come in the
(?aa) which the test does.

The problem is that the conditional that is testing that we switched
away from /d rules is assuming that this happened during the
construction of the current EXACTFish node.  The comments I wrote
indicate this assumption.  But this example shows that the switch can
come before this node started getting constructed, and so it loops.

The patch explicitly saves the state at the beginning of this node's
construction, and only retries if it changed during that construction.
Therefore the next time through, it will see that it hasn't changed
since the previous time, and won't loop.
---
 regcomp.c    | 11 +++++++++--
 t/re/speed.t |  5 ++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/regcomp.c b/regcomp.c
index 800ab2d444..e1a048ff97 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -13507,6 +13507,10 @@ S_regatom(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth)
              * need to figure this out until pass 2) */
             bool maybe_exactfu = PASS2;
 
+            /* To see if RExC_uni_semantics changes during parsing of the node.
+             * */
+            bool uni_semantics_at_node_start;
+
             /* The node_type may change below, but since the size of the node
              * doesn't change, it works */
 	    ret = reg_node(pRExC_state, node_type);
@@ -13533,6 +13537,8 @@ S_regatom(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth)
                    || UTF8_IS_INVARIANT(UCHARAT(RExC_parse))
                    || UTF8_IS_START(UCHARAT(RExC_parse)));
 
+            uni_semantics_at_node_start = RExC_uni_semantics;
+
             /* Here, we have a literal character.  Find the maximal string of
              * them in the input that we can fit into a single EXACTish node.
              * We quit at the first non-literal or when the node gets full, or
@@ -13970,8 +13976,9 @@ S_regatom(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth)
                                 ender = 's';
                                 added_len = 2;
                             }
-                            else if (RExC_uni_semantics) {
-
+                            else if (   uni_semantics_at_node_start
+                                     != RExC_uni_semantics)
+                            {
                                 /* Here, we are supossed to be using Unicode
                                  * rules, but this folding node is not.  This
                                  * happens during pass 1 when the node started
diff --git a/t/re/speed.t b/t/re/speed.t
index 4a4830f602..9a57de167f 100644
--- a/t/re/speed.t
+++ b/t/re/speed.t
@@ -24,7 +24,7 @@ BEGIN {
 skip_all('no re module') unless defined &DynaLoader::boot_DynaLoader;
 skip_all_without_unicode_tables();
 
-plan tests => 58;  #** update watchdog timeouts proportionally when adding tests
+plan tests => 59;  #** update watchdog timeouts proportionally when adding tests
 
 use strict;
 use warnings;
@@ -156,6 +156,9 @@ PROG
         ok( $elapsed <= 1, "should not COW on long string with substr and m//g");
     }
 
+    # [perl #133185] Infinite loop
+    like("!\xdf", eval 'qr/\pp(?aai)\xdf/',
+         'Compiling qr/\pp(?aai)\xdf/ doesn\'t loop');
 
 } # End of sub run_tests
 
-- 
2.11.0