Skip Menu |
Report information
Id: 132227
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: brian.carpenter [at] gmail.com
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: (no value)
Type: (no value)
Perl Version: (no value)
Fixed In: (no value)



Date: Fri, 6 Oct 2017 02:51:52 -0500
Subject: heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
From: Brian Carpenter <brian.carpenter [...] gmail.com>
To: perl5-security-report [...] perl.org
Triggered while fuzzing Perl v5.27.4-29-gdc41635.

od -tx1 ./test514
0000000 2f 30 30 5c 4e 7b 55 2b 30 7d df df df df df df
0000020 df 30 30 30 df df 30 2f 69
0000031

==28186==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60700000ac58 at pc 0x000000846c2d bp 0x7ffe716bc7f0 sp 0x7ffe716bc7e0
WRITE of size 1 at 0x60700000ac58 thread T0
    #0 0x846c2c in S_regatom /root/perl/regcomp.c:13652
    #1 0x8587f6 in S_regpiece /root/perl/regcomp.c:11708
    #2 0x8587f6 in S_regbranch /root/perl/regcomp.c:11633
    #3 0x88830a in S_reg /root/perl/regcomp.c:11371
    #4 0x8c90dc in Perl_re_op_compile /root/perl/regcomp.c:7363
    #5 0x5297d0 in Perl_pmruntime /root/perl/op.c:5888
    #6 0x74d853 in Perl_yyparse /root/perl/perly.y:1210
    #7 0x58b9b8 in S_parse_body /root/perl/perl.c:2450
    #8 0x593622 in perl_parse /root/perl/perl.c:1753
    #9 0x42eb7d in main /root/perl/perlmain.c:121
    #10 0x7fba4cebe82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #11 0x42fe18 in _start (/root/perl/perl+0x42fe18)

0x60700000ac58 is located 0 bytes to the right of 72-byte region [0x60700000ac10,0x60700000ac58)
allocated by thread T0 here:
    #0 0x7fba4dc62602 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x98602)
    #1 0x92dfd4 in Perl_safesysmalloc /root/perl/util.c:153
    #2 0x8c6cbe in Perl_re_op_compile /root/perl/regcomp.c:7209
    #3 0x5297d0 in Perl_pmruntime /root/perl/op.c:5888
    #4 0x74d853 in Perl_yyparse /root/perl/perly.y:1210
    #5 0x58b9b8 in S_parse_body /root/perl/perl.c:2450
    #6 0x593622 in perl_parse /root/perl/perl.c:1753
    #7 0x42eb7d in main /root/perl/perlmain.c:121
    #8 0x7fba4cebe82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

SUMMARY: AddressSanitizer: heap-buffer-overflow /root/perl/regcomp.c:13652 S_regatom

When tested against Perl 5.22.1 under Valgrind, the following occurs:

==5420== Invalid write of size 1
==5420==    at 0x52F178: Perl__to_fold_latin1 (in /usr/bin/perl)
==5420==    by 0x532904: Perl__to_uni_fold_flags (in /usr/bin/perl)
==5420==    by 0x4826E7: ??? (in /usr/bin/perl)
==5420==    by 0x48479C: ??? (in /usr/bin/perl)
==5420==    by 0x4798EA: ??? (in /usr/bin/perl)
==5420==    by 0x48E942: Perl_re_op_compile (in /usr/bin/perl)
==5420==    by 0x436377: Perl_pmruntime (in /usr/bin/perl)
==5420==    by 0x46CB15: Perl_yyparse (in /usr/bin/perl)
==5420==    by 0x442FB4: perl_parse (in /usr/bin/perl)
==5420==    by 0x41CB28: main (in /usr/bin/perl)
==5420==  Address 0x5b9dd88 is 0 bytes after a block of size 72 alloc'd
==5420==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5420==    by 0x498241: Perl_safesysmalloc (in /usr/bin/perl)
==5420==    by 0x48E5B4: Perl_re_op_compile (in /usr/bin/perl)
==5420==    by 0x436377: Perl_pmruntime (in /usr/bin/perl)
==5420==    by 0x46CB15: Perl_yyparse (in /usr/bin/perl)
==5420==    by 0x442FB4: perl_parse (in /usr/bin/perl)
==5420==    by 0x41CB28: main (in /usr/bin/perl)
==5420==
==5420== Invalid write of size 1
==5420==    at 0x52F17B: Perl__to_fold_latin1 (in /usr/bin/perl)
==5420==    by 0x532904: Perl__to_uni_fold_flags (in /usr/bin/perl)
==5420==    by 0x4826E7: ??? (in /usr/bin/perl)
==5420==    by 0x48479C: ??? (in /usr/bin/perl)
==5420==    by 0x4798EA: ??? (in /usr/bin/perl)
==5420==    by 0x48E942: Perl_re_op_compile (in /usr/bin/perl)
==5420==    by 0x436377: Perl_pmruntime (in /usr/bin/perl)
==5420==    by 0x46CB15: Perl_yyparse (in /usr/bin/perl)
==5420==    by 0x442FB4: perl_parse (in /usr/bin/perl)
==5420==    by 0x41CB28: main (in /usr/bin/perl)
==5420==  Address 0x5b9dd89 is 1 bytes after a block of size 72 alloc'd
==5420==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5420==    by 0x498241: Perl_safesysmalloc (in /usr/bin/perl)
==5420==    by 0x48E5B4: Perl_re_op_compile (in /usr/bin/perl)
==5420==    by 0x436377: Perl_pmruntime (in /usr/bin/perl)
==5420==    by 0x46CB15: Perl_yyparse (in /usr/bin/perl)
==5420==    by 0x442FB4: perl_parse (in /usr/bin/perl)
==5420==    by 0x41CB28: main (in /usr/bin/perl)
==5420==
==5420== Invalid write of size 1
==5420==    at 0x482311: ??? (in /usr/bin/perl)
==5420==    by 0x48479C: ??? (in /usr/bin/perl)
==5420==    by 0x4798EA: ??? (in /usr/bin/perl)
==5420==    by 0x48E942: Perl_re_op_compile (in /usr/bin/perl)
==5420==    by 0x436377: Perl_pmruntime (in /usr/bin/perl)
==5420==    by 0x46CB15: Perl_yyparse (in /usr/bin/perl)
==5420==    by 0x442FB4: perl_parse (in /usr/bin/perl)
==5420==    by 0x41CB28: main (in /usr/bin/perl)
==5420==  Address 0x5b9dd8c is 4 bytes after a block of size 72 alloc'd
==5420==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5420==    by 0x498241: Perl_safesysmalloc (in /usr/bin/perl)
==5420==    by 0x48E5B4: Perl_re_op_compile (in /usr/bin/perl)
==5420==    by 0x436377: Perl_pmruntime (in /usr/bin/perl)
==5420==    by 0x46CB15: Perl_yyparse (in /usr/bin/perl)
==5420==    by 0x442FB4: perl_parse (in /usr/bin/perl)
==5420==    by 0x41CB28: main (in /usr/bin/perl)
==5420==
panic: reg_node overrun trying to emit 0, 5b9dd90>=5b9dd88 at test514 line 1
Download test514.gz
application/x-gzip 47b

Message body not shown because it is not plain text.

RT-Send-CC: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 107b
I'll look at this when I get around to reworking this portion of the code later in 5.27 -- Karl Williamson
RT-Send-CC: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 356b
On Fri, 06 Oct 2017 16:46:50 -0700, khw wrote: Show quoted text
> I'll look at this when I get around to reworking this portion of the > code later in 5.27
Simplifies to the attached, which doesn't assert under valgrind etc, but does cause a panic: $ ./perl ../132227b.pl panic: reg_node overrun trying to emit 0, 5584354b4bf8>=5584354b4bf8 at ../132227b.pl line 1. Tony
Subject: 132227b.pl
Download 132227b.pl
text/x-perl 17b
/00\N{U+0}\xDF/i
RT-Send-CC: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 1007b
On Tue, 30 Jan 2018 20:53:29 -0800, tonyc wrote: Show quoted text
> On Fri, 06 Oct 2017 16:46:50 -0700, khw wrote:
> > I'll look at this when I get around to reworking this portion of the > > code later in 5.27
> > Simplifies to the attached, which doesn't assert under valgrind etc, > but does cause a panic: > > $ ./perl ../132227b.pl > panic: reg_node overrun trying to emit 0, 5584354b4bf8>=5584354b4bf8 > at ../132227b.pl line 1.
What's happening is the first pass is emitting the double-s as a single byte, but the second pass emits "ss", and there isn't enough room for it. If you have a larger number of ß this can push the following bytes emitted well beyond the end of the allocated block. At minimum this has the potential to cause a segmentation fault, or to corrupt the arena, leading to a crash further on. Depending on the heap implementation it may be possible to perform a nastier exploit - an attacker has almost complete control over the bytes written. I do believe this is a security issue. Tony
CC: perl5-security-report [...] perl.org
Date: Thu, 1 Feb 2018 10:27:28 -0700
Subject: Re: [perl #132227] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
To: perl5-security-report-followup [...] perl.org
From: Karl Williamson <public [...] khwilliamson.com>
Download (untitled) / with headers
text/plain 1.2k
On 01/31/2018 08:44 PM, Tony Cook via RT wrote: Show quoted text
> On Tue, 30 Jan 2018 20:53:29 -0800, tonyc wrote:
>> On Fri, 06 Oct 2017 16:46:50 -0700, khw wrote:
>>> I'll look at this when I get around to reworking this portion of the >>> code later in 5.27
>> >> Simplifies to the attached, which doesn't assert under valgrind etc, >> but does cause a panic: >> >> $ ./perl ../132227b.pl >> panic: reg_node overrun trying to emit 0, 5584354b4bf8>=5584354b4bf8 >> at ../132227b.pl line 1.
> > What's happening is the first pass is emitting the double-s as a single byte, but the second pass emits "ss", and there isn't enough room for it. > > If you have a larger number of ß this can push the following bytes emitted well beyond the end of the allocated block. > > At minimum this has the potential to cause a segmentation fault, or to corrupt the arena, leading to a crash further on. > > Depending on the heap implementation it may be possible to perform a nastier exploit - an attacker has almost complete control over the bytes written. > > I do believe this is a security issue. > > Tony >
Since you've done so much analysis, do you have a patch, or know the line of code in error? I've fixed several similar ones involving β in the past, but missed this one.
CC: perl5-security-report-followup [...] perl.org, perl5-security-report [...] perl.org
Date: Fri, 2 Feb 2018 13:21:23 +0100
Subject: Re: [perl #132227] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
To: Karl Williamson <public [...] khwilliamson.com>
From: demerphq <demerphq [...] gmail.com>
Download (untitled) / with headers
text/plain 1.7k
FWIW, this looks like a reparse bug. The pattern starts out latin-1. We should trigger "reparse as utf8" when we see the \N{U+0}, which then should do the correct sizing. So I would look into what is happening with \N{U+0} and why it isnt triggering a reparse of the pattern. Yves On 1 February 2018 at 18:27, Karl Williamson <public@khwilliamson.com> wrote: Show quoted text
> On 01/31/2018 08:44 PM, Tony Cook via RT wrote:
>> >> On Tue, 30 Jan 2018 20:53:29 -0800, tonyc wrote:
>>> >>> On Fri, 06 Oct 2017 16:46:50 -0700, khw wrote:
>>>> >>>> I'll look at this when I get around to reworking this portion of the >>>> code later in 5.27
>>> >>> >>> Simplifies to the attached, which doesn't assert under valgrind etc, >>> but does cause a panic: >>> >>> $ ./perl ../132227b.pl >>> panic: reg_node overrun trying to emit 0, 5584354b4bf8>=5584354b4bf8 >>> at ../132227b.pl line 1.
>> >> >> What's happening is the first pass is emitting the double-s as a single >> byte, but the second pass emits "ss", and there isn't enough room for it. >> >> If you have a larger number of ß this can push the following bytes emitted >> well beyond the end of the allocated block. >> >> At minimum this has the potential to cause a segmentation fault, or to >> corrupt the arena, leading to a crash further on. >> >> Depending on the heap implementation it may be possible to perform a >> nastier exploit - an attacker has almost complete control over the bytes >> written. >> >> I do believe this is a security issue. >> >> Tony >>
> > Since you've done so much analysis, do you have a patch, or know the line of > code in error? I've fixed several similar ones involving β in the past, but > missed this one.
-- perl -Mre=debug -e "/just|another|perl|hacker/"
CC: perl5-security-report-followup [...] perl.org, perl5-security-report [...] perl.org
Date: Fri, 2 Feb 2018 17:40:12 +0100
Subject: Re: [perl #132227] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
To: Karl Williamson <public [...] khwilliamson.com>
From: demerphq <demerphq [...] gmail.com>
Download (untitled) / with headers
text/plain 3.9k
On 2 February 2018 at 13:21, demerphq <demerphq@gmail.com> wrote: Show quoted text
> FWIW, this looks like a reparse bug. The pattern starts out latin-1. > We should trigger "reparse as utf8" when we see the \N{U+0}, which > then should do the correct sizing. > > So I would look into what is happening with \N{U+0} and why it isnt > triggering a reparse of the pattern.
Yep, that is it, or more or less, although we ARE trying to trigger reparse, but that logic has been "optimized" such that it does not do the reparse if it has not seen \xDF first, and later when it DOES see the \xDF it also does not realize it needs to upgrade the pattern. The following patch fixes it, although its not the right patch. $ git diff diff --git a/regcomp.c b/regcomp.c index 6dbfed5..2668df5 100644 --- a/regcomp.c +++ b/regcomp.c @@ -352,7 +352,7 @@ struct RExC_state_t { assert(PASS1); \ set_regex_charset(&RExC_flags, REGEX_UNICODE_CHARSET); \ RExC_uni_semantics = 1; \ - if (RExC_seen_unfolded_sharp_s) { \ + if (!UTF || RExC_seen_unfolded_sharp_s) { \ *flagp |= RESTART_PASS1; \ return restart_retval; \ } You have evolved some of this handling beyond my current understanding of the subtleties, so it will take me some time to work a better patch. With my patch: $ ./perl -Ilib -Mre=Debug,ALL -le'/00\N{U+2}\xDF/i' Assembling pattern from 1 elements Compiling REx "00\N{U+2}\xDF" Starting first pass (sizing) <00\N{U+2}\xDF> | 1| reg | | brnc | | piec | | atom Need to redo pass 1 Starting first pass (sizing) <00\N{U+2}\xDF> | 1| reg | | brnc | | piec | | atom Required size 4 nodes Starting second pass (creation) <00\N{U+2}\xDF> | 1| reg | | brnc | | piec | | atom <> | 5| lsbr~ tying lastbr EXACTFU <00\2ss> (1) to ender END (4) offset 3 | | tail~ EXACTFU <00\2ss> (1) -> END Starting post parse optimization first:> 1: EXACTFU <00\2ss> (4) first at 1 RExC_seen: study_chunk stopparen=-1 recursed_count=1 depth=0 recursed_depth=0 scan=1118124 last=1118134 Peep: Pos:0/0 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' 0:0/0 *Fixed: '' @ 0/0 Float: '' @ 0/0 Show quoted text
Peep> 1: EXACTFU <00\2ss> (4) [ SCF_DO_SUBSTR SCF_WHILEM_VISITED_POS ]
Show quoted text
join> 1: EXACTFU <00\2ss> (4)
commit: Pos:0/0 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 *Fixed: '' @ 0/0 Float: '' @ 0/0 pre-fin: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: '' @ 0/0 *Float: '' @ 0/0 post-fin: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: '' @ 0/0 *Float: '' @ 0/0 commit: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: '' @ 0/0 *Float: '' @ 0/0 minlen: 4 r->minlen:0 maxlen:5 study_chunk_recursed_count: 1 RExC_seen: Final program: 1: EXACTFU_SS <00\2ss> (4) 4: END (0) stclass EXACTFU_SS <00\2ss> minlen 4 r->extflags: FOLD UNICODE r->intflags: [none-set] Freeing REx: "00\N{U+2}\xDF" ---------->8--------------->8------------- Without: $ ./perl -Ilib -Mre=Debug,ALL -le'/00\N{U+2}\xDF/i' Assembling pattern from 1 elements Compiling REx "00\N{U+2}\xDF" Starting first pass (sizing) <00\N{U+2}\xDF> | 1| reg | | brnc | | piec | | atom Required size 3 nodes Starting second pass (creation) <00\N{U+2}\xDF> | 1| reg | | brnc | | piec | | atom panic: reg_node overrun trying to emit 0, 2133840>=2133840 at -e line 1. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
From: demerphq <demerphq [...] gmail.com>
CC: perl5-security-report-followup [...] perl.org, perl5-security-report [...] perl.org
To: Karl Williamson <public [...] khwilliamson.com>
Subject: Re: [perl #132227] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
Date: Fri, 2 Feb 2018 22:26:58 +0100
Download (untitled) / with headers
text/plain 4.2k
On 2 February 2018 at 17:40, demerphq <demerphq@gmail.com> wrote: Show quoted text
> On 2 February 2018 at 13:21, demerphq <demerphq@gmail.com> wrote:
>> FWIW, this looks like a reparse bug. The pattern starts out latin-1. >> We should trigger "reparse as utf8" when we see the \N{U+0}, which >> then should do the correct sizing. >> >> So I would look into what is happening with \N{U+0} and why it isnt >> triggering a reparse of the pattern.
> > Yep, that is it, or more or less, although we ARE trying to trigger > reparse, but that logic has been "optimized" such that it does not do > the reparse if it has not seen \xDF first, and later when it DOES see > the \xDF it also does not realize it needs to upgrade the pattern. The > following patch fixes it, although its not the right patch. > > $ git diff > diff --git a/regcomp.c b/regcomp.c > index 6dbfed5..2668df5 100644 > --- a/regcomp.c > +++ b/regcomp.c > @@ -352,7 +352,7 @@ struct RExC_state_t { > assert(PASS1); \ > set_regex_charset(&RExC_flags, REGEX_UNICODE_CHARSET); \ > RExC_uni_semantics = 1; \ > - if (RExC_seen_unfolded_sharp_s) { \ > + if (!UTF || RExC_seen_unfolded_sharp_s) { \ > *flagp |= RESTART_PASS1; \ > return restart_retval; \ > } > > You have evolved some of this handling beyond my current understanding > of the subtleties, so it will take me some time to work a better > patch. > > With my patch: > > $ ./perl -Ilib -Mre=Debug,ALL -le'/00\N{U+2}\xDF/i' > Assembling pattern from 1 elements > Compiling REx "00\N{U+2}\xDF" > Starting first pass (sizing) > <00\N{U+2}\xDF> | 1| reg > | | brnc > | | piec > | | atom > Need to redo pass 1 > Starting first pass (sizing) > <00\N{U+2}\xDF> | 1| reg > | | brnc > | | piec > | | atom > Required size 4 nodes > Starting second pass (creation) > <00\N{U+2}\xDF> | 1| reg > | | brnc > | | piec > | | atom > <> | 5| lsbr~ tying lastbr EXACTFU <00\2ss> (1) to > ender END (4) offset 3 > | | tail~ EXACTFU <00\2ss> (1) -> END > Starting post parse optimization > first:> 1: EXACTFU <00\2ss> (4) > first at 1 > RExC_seen: > study_chunk stopparen=-1 recursed_count=1 depth=0 recursed_depth=0 > scan=1118124 last=1118134 > Peep: Pos:0/0 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' 0:0/0 *Fixed: '' @ > 0/0 Float: '' @ 0/0
> Peep> 1: EXACTFU <00\2ss> (4) [ SCF_DO_SUBSTR SCF_WHILEM_VISITED_POS ]
> join> 1: EXACTFU <00\2ss> (4)
> commit: Pos:0/0 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 *Fixed: > '' @ 0/0 Float: '' @ 0/0 > pre-fin: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: > '' @ 0/0 *Float: '' @ 0/0 > post-fin: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: > '' @ 0/0 *Float: '' @ 0/0 > commit: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: '' > @ 0/0 *Float: '' @ 0/0 > minlen: 4 r->minlen:0 maxlen:5 > study_chunk_recursed_count: 1 > RExC_seen: > Final program: > 1: EXACTFU_SS <00\2ss> (4) > 4: END (0) > stclass EXACTFU_SS <00\2ss> minlen 4 > r->extflags: FOLD UNICODE > r->intflags: [none-set] > Freeing REx: "00\N{U+2}\xDF" > > ---------->8--------------->8------------- > > Without: > > $ ./perl -Ilib -Mre=Debug,ALL -le'/00\N{U+2}\xDF/i' > Assembling pattern from 1 elements > Compiling REx "00\N{U+2}\xDF" > Starting first pass (sizing) > <00\N{U+2}\xDF> | 1| reg > | | brnc > | | piec > | | atom > Required size 3 nodes > Starting second pass (creation) > <00\N{U+2}\xDF> | 1| reg > | | brnc > | | piec > | | atom > panic: reg_node overrun trying to emit 0, 2133840>=2133840 at -e line 1.
I think the attached patch is better. Basically we only need to reparse if we see a \xDF and we are in unicode semantics. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
Download rt_132227.patch
text/plain 751b

Message body is not shown because sender requested not to inline it.

To: demerphq <demerphq [...] gmail.com>
Subject: Re: [perl #132227] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
Date: Fri, 2 Feb 2018 15:39:50 -0700
From: Karl Williamson <public [...] khwilliamson.com>
CC: perl5-security-report-followup [...] perl.org, perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 5.5k
On 02/02/2018 02:26 PM, demerphq wrote: Show quoted text
> On 2 February 2018 at 17:40, demerphq <demerphq@gmail.com> wrote:
>> On 2 February 2018 at 13:21, demerphq <demerphq@gmail.com> wrote:
>>> FWIW, this looks like a reparse bug. The pattern starts out latin-1. >>> We should trigger "reparse as utf8" when we see the \N{U+0}, which >>> then should do the correct sizing. >>> >>> So I would look into what is happening with \N{U+0} and why it isnt >>> triggering a reparse of the pattern.
>> >> Yep, that is it, or more or less, although we ARE trying to trigger >> reparse, but that logic has been "optimized" such that it does not do >> the reparse if it has not seen \xDF first, and later when it DOES see >> the \xDF it also does not realize it needs to upgrade the pattern. The >> following patch fixes it, although its not the right patch. >> >> $ git diff >> diff --git a/regcomp.c b/regcomp.c >> index 6dbfed5..2668df5 100644 >> --- a/regcomp.c >> +++ b/regcomp.c >> @@ -352,7 +352,7 @@ struct RExC_state_t { >> assert(PASS1); \ >> set_regex_charset(&RExC_flags, REGEX_UNICODE_CHARSET); \ >> RExC_uni_semantics = 1; \ >> - if (RExC_seen_unfolded_sharp_s) { \ >> + if (!UTF || RExC_seen_unfolded_sharp_s) { \ >> *flagp |= RESTART_PASS1; \ >> return restart_retval; \ >> } >> >> You have evolved some of this handling beyond my current understanding >> of the subtleties, so it will take me some time to work a better >> patch. >> >> With my patch: >> >> $ ./perl -Ilib -Mre=Debug,ALL -le'/00\N{U+2}\xDF/i' >> Assembling pattern from 1 elements >> Compiling REx "00\N{U+2}\xDF" >> Starting first pass (sizing) >> <00\N{U+2}\xDF> | 1| reg >> | | brnc >> | | piec >> | | atom >> Need to redo pass 1 >> Starting first pass (sizing) >> <00\N{U+2}\xDF> | 1| reg >> | | brnc >> | | piec >> | | atom >> Required size 4 nodes >> Starting second pass (creation) >> <00\N{U+2}\xDF> | 1| reg >> | | brnc >> | | piec >> | | atom >> <> | 5| lsbr~ tying lastbr EXACTFU <00\2ss> (1) to >> ender END (4) offset 3 >> | | tail~ EXACTFU <00\2ss> (1) -> END >> Starting post parse optimization >> first:> 1: EXACTFU <00\2ss> (4) >> first at 1 >> RExC_seen: >> study_chunk stopparen=-1 recursed_count=1 depth=0 recursed_depth=0 >> scan=1118124 last=1118134 >> Peep: Pos:0/0 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' 0:0/0 *Fixed: '' @ >> 0/0 Float: '' @ 0/0
>> Peep> 1: EXACTFU <00\2ss> (4) [ SCF_DO_SUBSTR SCF_WHILEM_VISITED_POS ]
>> join> 1: EXACTFU <00\2ss> (4)
>> commit: Pos:0/0 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 *Fixed: >> '' @ 0/0 Float: '' @ 0/0 >> pre-fin: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: >> '' @ 0/0 *Float: '' @ 0/0 >> post-fin: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: >> '' @ 0/0 *Float: '' @ 0/0 >> commit: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: '' >> @ 0/0 *Float: '' @ 0/0 >> minlen: 4 r->minlen:0 maxlen:5 >> study_chunk_recursed_count: 1 >> RExC_seen: >> Final program: >> 1: EXACTFU_SS <00\2ss> (4) >> 4: END (0) >> stclass EXACTFU_SS <00\2ss> minlen 4 >> r->extflags: FOLD UNICODE >> r->intflags: [none-set] >> Freeing REx: "00\N{U+2}\xDF" >> >> ---------->8--------------->8------------- >> >> Without: >> >> $ ./perl -Ilib -Mre=Debug,ALL -le'/00\N{U+2}\xDF/i' >> Assembling pattern from 1 elements >> Compiling REx "00\N{U+2}\xDF" >> Starting first pass (sizing) >> <00\N{U+2}\xDF> | 1| reg >> | | brnc >> | | piec >> | | atom >> Required size 3 nodes >> Starting second pass (creation) >> <00\N{U+2}\xDF> | 1| reg >> | | brnc >> | | piec >> | | atom >> panic: reg_node overrun trying to emit 0, 2133840>=2133840 at -e line 1.
> > I think the attached patch is better. > > Basically we only need to reparse if we see a \xDF and we are in > unicode semantics. > > Yves > > > >
Yves' patch unnecessarily upgrades to UTF-8. And on irc, he indicated he was conflating in his mind upgrading and reparsing. Attached is my patch. The problem occurs when we are under /d semantics. An EXACTF node is generated, which means it may have different meanings if the eventual matched target string is in UTF-8. Then the \N{} is seen, which changes the rules to unicode, as \N is a unicode construct. Then the \xDF comes along and sees that it is an EXACTF node and so doesn't do anything special besides setting a flag. What really should happen in this situation is addressed in my patch. It restarts parsing, having set it to be using unicode rules. Then instead of an EXACTF node, it creates an EXACTFU. In these nodes, the \xDF gets folded in pass1 and you don't have this problem Yves wanted to know why it works now if the \xDF comes before the \N{}. The reason is that flag. The \N{} comes along after the flag is set, and is smart enough to look at the flag and restart the parsing with unicode rules, so that the EXACTFU is generated before the \xDF is seen.
Download 0010-132227.patch
text/plain 1.3k

Message body is not shown because sender requested not to inline it.

From: demerphq <demerphq [...] gmail.com>
CC: perl5-security-report-followup [...] perl.org, perl5-security-report [...] perl.org
To: Karl Williamson <public [...] khwilliamson.com>
Subject: Re: [perl #132227] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
Date: Sat, 3 Feb 2018 14:32:36 +0100
Download (untitled) / with headers
text/plain 6.9k
On 2 February 2018 at 23:39, Karl Williamson <public@khwilliamson.com> wrote: Show quoted text
> On 02/02/2018 02:26 PM, demerphq wrote:
>> >> On 2 February 2018 at 17:40, demerphq <demerphq@gmail.com> wrote:
>>> >>> On 2 February 2018 at 13:21, demerphq <demerphq@gmail.com> wrote:
>>>> >>>> FWIW, this looks like a reparse bug. The pattern starts out latin-1. >>>> We should trigger "reparse as utf8" when we see the \N{U+0}, which >>>> then should do the correct sizing. >>>> >>>> So I would look into what is happening with \N{U+0} and why it isnt >>>> triggering a reparse of the pattern.
>>> >>> >>> Yep, that is it, or more or less, although we ARE trying to trigger >>> reparse, but that logic has been "optimized" such that it does not do >>> the reparse if it has not seen \xDF first, and later when it DOES see >>> the \xDF it also does not realize it needs to upgrade the pattern. The >>> following patch fixes it, although its not the right patch. >>> >>> $ git diff >>> diff --git a/regcomp.c b/regcomp.c >>> index 6dbfed5..2668df5 100644 >>> --- a/regcomp.c >>> +++ b/regcomp.c >>> @@ -352,7 +352,7 @@ struct RExC_state_t { >>> assert(PASS1); >>> \ >>> set_regex_charset(&RExC_flags, REGEX_UNICODE_CHARSET); >>> \ >>> RExC_uni_semantics = 1; >>> \ >>> - if (RExC_seen_unfolded_sharp_s) { >>> \ >>> + if (!UTF || RExC_seen_unfolded_sharp_s) { >>> \ >>> *flagp |= RESTART_PASS1; >>> \ >>> return restart_retval; >>> \ >>> } >>> >>> You have evolved some of this handling beyond my current understanding >>> of the subtleties, so it will take me some time to work a better >>> patch. >>> >>> With my patch: >>> >>> $ ./perl -Ilib -Mre=Debug,ALL -le'/00\N{U+2}\xDF/i' >>> Assembling pattern from 1 elements >>> Compiling REx "00\N{U+2}\xDF" >>> Starting first pass (sizing) >>> <00\N{U+2}\xDF> | 1| reg >>> | | brnc >>> | | piec >>> | | atom >>> Need to redo pass 1 >>> Starting first pass (sizing) >>> <00\N{U+2}\xDF> | 1| reg >>> | | brnc >>> | | piec >>> | | atom >>> Required size 4 nodes >>> Starting second pass (creation) >>> <00\N{U+2}\xDF> | 1| reg >>> | | brnc >>> | | piec >>> | | atom >>> <> | 5| lsbr~ tying lastbr EXACTFU <00\2ss> (1) to >>> ender END (4) offset 3 >>> | | tail~ EXACTFU <00\2ss> (1) -> END >>> Starting post parse optimization >>> first:> 1: EXACTFU <00\2ss> (4) >>> first at 1 >>> RExC_seen: >>> study_chunk stopparen=-1 recursed_count=1 depth=0 recursed_depth=0 >>> scan=1118124 last=1118134 >>> Peep: Pos:0/0 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' 0:0/0 *Fixed: '' @ >>> 0/0 Float: '' @ 0/0
>>> Peep> 1: EXACTFU <00\2ss> (4) [ SCF_DO_SUBSTR SCF_WHILEM_VISITED_POS ]
>>> join> 1: EXACTFU <00\2ss> (4)
>>> commit: Pos:0/0 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 *Fixed: >>> '' @ 0/0 Float: '' @ 0/0 >>> pre-fin: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: >>> '' @ 0/0 *Float: '' @ 0/0 >>> post-fin: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: >>> '' @ 0/0 *Float: '' @ 0/0 >>> commit: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: '' >>> @ 0/0 *Float: '' @ 0/0 >>> minlen: 4 r->minlen:0 maxlen:5 >>> study_chunk_recursed_count: 1 >>> RExC_seen: >>> Final program: >>> 1: EXACTFU_SS <00\2ss> (4) >>> 4: END (0) >>> stclass EXACTFU_SS <00\2ss> minlen 4 >>> r->extflags: FOLD UNICODE >>> r->intflags: [none-set] >>> Freeing REx: "00\N{U+2}\xDF" >>> >>> ---------->8--------------->8------------- >>> >>> Without: >>> >>> $ ./perl -Ilib -Mre=Debug,ALL -le'/00\N{U+2}\xDF/i' >>> Assembling pattern from 1 elements >>> Compiling REx "00\N{U+2}\xDF" >>> Starting first pass (sizing) >>> <00\N{U+2}\xDF> | 1| reg >>> | | brnc >>> | | piec >>> | | atom >>> Required size 3 nodes >>> Starting second pass (creation) >>> <00\N{U+2}\xDF> | 1| reg >>> | | brnc >>> | | piec >>> | | atom >>> panic: reg_node overrun trying to emit 0, 2133840>=2133840 at -e line 1.
>> >> >> I think the attached patch is better. >> >> Basically we only need to reparse if we see a \xDF and we are in >> unicode semantics. >> >> Yves >> >> >> >>
> > Yves' patch unnecessarily upgrades to UTF-8. And on irc, he indicated he > was conflating in his mind upgrading and reparsing.
Yes, im a bit behind the times. Show quoted text
> Attached is my patch.
Yep, makes sense to me. Should we wrap this logic in a well named macro like we do with the other related macros? Show quoted text
> The problem occurs when we are under /d semantics. An EXACTF node is > generated, which means it may have different meanings if the eventual > matched target string is in UTF-8. Then the \N{} is seen, which changes the > rules to unicode, as \N is a unicode construct. Then the \xDF comes along > and sees that it is an EXACTF node and so doesn't do anything special > besides setting a flag. > > What really should happen in this situation is addressed in my patch. It > restarts parsing, having set it to be using unicode rules. Then instead of > an EXACTF node, it creates an EXACTFU. In these nodes, the \xDF gets folded > in pass1 and you don't have this problem > > Yves wanted to know why it works now if the \xDF comes before the \N{}. The > reason is that flag. The \N{} comes along after the flag is set, and is > smart enough to look at the flag and restart the parsing with unicode rules, > so that the EXACTFU is generated before the \xDF is seen.
No, that wasn't my point :-), I was conflating reparse and upgrade in some of my communications and thinking, since upgrade implies reparse, and when I added the reparse logic it always upgraded. So when you were saying "it shouldnt upgrade", and I was looking at the DEBUG 'ALL' output, I was seeing reparse messages in the non-failing case and assumed that meant upgrade. Once the fact that you can reparse without upgrading penetrated my skull things became clear. I think we should add diagnostic messages that shows what is going on. All I was concerned with was that /00\N{U+0}\xDF/ and /00\xDF\N{U+0}/ should reparse (and also upgrade) the same number of times, be it 0 or 1, for each possibility. FWIW, I would like to push a different patch to blead, and we can use your patch for the maint-releases. I wrapped all the macros that are used for RESTART_PASS1 (including your patch) and gave them names, etc. I think we can silently fix this in blead in such a patch without worrying about it revealing anything about the CVE aspect of this. But we need to get back patches done, and then do a mega-cve release along with the other issue we have pending. ;-) cheers, Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
Download wrap_restart.patch
text/plain 11.7k

Message body is not shown because sender requested not to inline it.

CC: perl5-security-report-followup [...] perl.org, perl5-security-report [...] perl.org
To: demerphq <demerphq [...] gmail.com>
Date: Sat, 3 Feb 2018 17:11:20 -0700
Subject: Re: [perl #132227] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
From: Karl Williamson <public [...] khwilliamson.com>
Download (untitled) / with headers
text/plain 10.7k
On 02/03/2018 06:32 AM, demerphq wrote: Show quoted text
> On 2 February 2018 at 23:39, Karl Williamson <public@khwilliamson.com> wrote:
>> On 02/02/2018 02:26 PM, demerphq wrote:
>>> >>> On 2 February 2018 at 17:40, demerphq <demerphq@gmail.com> wrote:
>>>> >>>> On 2 February 2018 at 13:21, demerphq <demerphq@gmail.com> wrote:
>>>>> >>>>> FWIW, this looks like a reparse bug. The pattern starts out latin-1. >>>>> We should trigger "reparse as utf8" when we see the \N{U+0}, which >>>>> then should do the correct sizing. >>>>> >>>>> So I would look into what is happening with \N{U+0} and why it isnt >>>>> triggering a reparse of the pattern.
>>>> >>>> >>>> Yep, that is it, or more or less, although we ARE trying to trigger >>>> reparse, but that logic has been "optimized" such that it does not do >>>> the reparse if it has not seen \xDF first, and later when it DOES see >>>> the \xDF it also does not realize it needs to upgrade the pattern. The >>>> following patch fixes it, although its not the right patch. >>>> >>>> $ git diff >>>> diff --git a/regcomp.c b/regcomp.c >>>> index 6dbfed5..2668df5 100644 >>>> --- a/regcomp.c >>>> +++ b/regcomp.c >>>> @@ -352,7 +352,7 @@ struct RExC_state_t { >>>> assert(PASS1); >>>> \ >>>> set_regex_charset(&RExC_flags, REGEX_UNICODE_CHARSET); >>>> \ >>>> RExC_uni_semantics = 1; >>>> \ >>>> - if (RExC_seen_unfolded_sharp_s) { >>>> \ >>>> + if (!UTF || RExC_seen_unfolded_sharp_s) { >>>> \ >>>> *flagp |= RESTART_PASS1; >>>> \ >>>> return restart_retval; >>>> \ >>>> } >>>> >>>> You have evolved some of this handling beyond my current understanding >>>> of the subtleties, so it will take me some time to work a better >>>> patch. >>>> >>>> With my patch: >>>> >>>> $ ./perl -Ilib -Mre=Debug,ALL -le'/00\N{U+2}\xDF/i' >>>> Assembling pattern from 1 elements >>>> Compiling REx "00\N{U+2}\xDF" >>>> Starting first pass (sizing) >>>> <00\N{U+2}\xDF> | 1| reg >>>> | | brnc >>>> | | piec >>>> | | atom >>>> Need to redo pass 1 >>>> Starting first pass (sizing) >>>> <00\N{U+2}\xDF> | 1| reg >>>> | | brnc >>>> | | piec >>>> | | atom >>>> Required size 4 nodes >>>> Starting second pass (creation) >>>> <00\N{U+2}\xDF> | 1| reg >>>> | | brnc >>>> | | piec >>>> | | atom >>>> <> | 5| lsbr~ tying lastbr EXACTFU <00\2ss> (1) to >>>> ender END (4) offset 3 >>>> | | tail~ EXACTFU <00\2ss> (1) -> END >>>> Starting post parse optimization >>>> first:> 1: EXACTFU <00\2ss> (4) >>>> first at 1 >>>> RExC_seen: >>>> study_chunk stopparen=-1 recursed_count=1 depth=0 recursed_depth=0 >>>> scan=1118124 last=1118134 >>>> Peep: Pos:0/0 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' 0:0/0 *Fixed: '' @ >>>> 0/0 Float: '' @ 0/0
>>>> Peep> 1: EXACTFU <00\2ss> (4) [ SCF_DO_SUBSTR SCF_WHILEM_VISITED_POS ]
>>>> join> 1: EXACTFU <00\2ss> (4)
>>>> commit: Pos:0/0 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 *Fixed: >>>> '' @ 0/0 Float: '' @ 0/0 >>>> pre-fin: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: >>>> '' @ 0/0 *Float: '' @ 0/0 >>>> post-fin: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: >>>> '' @ 0/0 *Float: '' @ 0/0 >>>> commit: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: '' >>>> @ 0/0 *Float: '' @ 0/0 >>>> minlen: 4 r->minlen:0 maxlen:5 >>>> study_chunk_recursed_count: 1 >>>> RExC_seen: >>>> Final program: >>>> 1: EXACTFU_SS <00\2ss> (4) >>>> 4: END (0) >>>> stclass EXACTFU_SS <00\2ss> minlen 4 >>>> r->extflags: FOLD UNICODE >>>> r->intflags: [none-set] >>>> Freeing REx: "00\N{U+2}\xDF" >>>> >>>> ---------->8--------------->8------------- >>>> >>>> Without: >>>> >>>> $ ./perl -Ilib -Mre=Debug,ALL -le'/00\N{U+2}\xDF/i' >>>> Assembling pattern from 1 elements >>>> Compiling REx "00\N{U+2}\xDF" >>>> Starting first pass (sizing) >>>> <00\N{U+2}\xDF> | 1| reg >>>> | | brnc >>>> | | piec >>>> | | atom >>>> Required size 3 nodes >>>> Starting second pass (creation) >>>> <00\N{U+2}\xDF> | 1| reg >>>> | | brnc >>>> | | piec >>>> | | atom >>>> panic: reg_node overrun trying to emit 0, 2133840>=2133840 at -e line 1.
>>> >>> >>> I think the attached patch is better. >>> >>> Basically we only need to reparse if we see a \xDF and we are in >>> unicode semantics. >>> >>> Yves >>> >>> >>> >>>
>> >> Yves' patch unnecessarily upgrades to UTF-8. And on irc, he indicated he >> was conflating in his mind upgrading and reparsing.
> > Yes, im a bit behind the times. >
>> Attached is my patch.
> > Yep, makes sense to me. Should we wrap this logic in a well named > macro like we do with the other related macros? >
>> The problem occurs when we are under /d semantics. An EXACTF node is >> generated, which means it may have different meanings if the eventual >> matched target string is in UTF-8. Then the \N{} is seen, which changes the >> rules to unicode, as \N is a unicode construct. Then the \xDF comes along >> and sees that it is an EXACTF node and so doesn't do anything special >> besides setting a flag. >> >> What really should happen in this situation is addressed in my patch. It >> restarts parsing, having set it to be using unicode rules. Then instead of >> an EXACTF node, it creates an EXACTFU. In these nodes, the \xDF gets folded >> in pass1 and you don't have this problem >> >> Yves wanted to know why it works now if the \xDF comes before the \N{}. The >> reason is that flag. The \N{} comes along after the flag is set, and is >> smart enough to look at the flag and restart the parsing with unicode rules, >> so that the EXACTFU is generated before the \xDF is seen.
> > No, that wasn't my point :-), > > I was conflating reparse and upgrade in some of my communications and > thinking, since upgrade implies reparse, and when I added the reparse > logic it always upgraded. > > So when you were saying "it shouldnt upgrade", and I was looking at > the DEBUG 'ALL' output, I was seeing reparse messages in the > non-failing case and assumed that meant upgrade. > > Once the fact that you can reparse without upgrading penetrated my > skull things became clear. I think we should add diagnostic messages > that shows what is going on. > > All I was concerned with was that /00\N{U+0}\xDF/ and /00\xDF\N{U+0}/ > should reparse (and also upgrade) the same number of times, be it 0 or > 1, for each possibility. > > FWIW, I would like to push a different patch to blead, and we can use > your patch for the maint-releases. > > I wrapped all the macros that are used for RESTART_PASS1 (including > your patch) and gave them names, etc. I think we can silently fix this > in blead in such a patch without worrying about it revealing anything > about the CVE aspect of this. But we need to get back patches done, > and then do a mega-cve release along with the other issue we have > pending. ;-) > > cheers, > Yves > > >
7 months ago I made a proposal, fully explained here, and in followup posts: http://nntp.perl.org/group/perl.perl5.porters/245342 But briefly, under /i matching there may very well be characters included in the pattern that only match themselves even under /i. Spaces and punctuation are two prominent members of this class. One of the things the optimizer does is to look for sequences of bytes that have to exist in the target string in order for it to match. It can then search the target using boyer moore to quickly find the possible places in the string to start the match, or to rule out a match immediately. But if the whole pattern is /i, there is no such sequence of bytes that the optimizer can grab onto. What I proposed is for the regex parser to split the EXACTish nodes into two types, one that consists solely of characters that can match a different character than themselves under /i, and nodes that consist solely of characters that match only themselves even under /i. That would mean the optimizer would very likely be able to find a fixed sequence in just about any /i pattern longer than a few bytes. I chatted with Yves about this yesterday, and he liked it; which he also did last July. I now have a branch that implements this. The reason it is relevant to this security ticket, is that implementing this causes this bug to go away with no special effort required. Thus if we were to put it in blead, no one would notice that it is fixing a bug. I therefore propose that we do this, and leave my basic patch to include in the CVE that must happen for other reasons, like Yves suggested. In the proposal of July, I suggested that a new compile-time only node be used to mark those nodes which are under /i but contain no characters that match other than themselves under /i. Having talked with Yves, and recently looked at the pattern matching code, I'm not so certain about this. The reason to do it would be to join these nodes back in, after the optimizer is done with them, so that the results are packed into fewer nodes. The reason for doing that is that there is overhead in the engine whenever we switch to the next node. But the reason for not doing it is that it is slower to match /i nodes than ones that an memEQ can be used on. I suppose we could join short nodes, and leave longer ones separate. And finally, I have realized some more things related to this ticket. The bottom line cause is that we start parsing and we assume /d rules, we find the \N{} which tells us we need to use Unicode rules, and then we find the \xDF which expands to ss under Unicode rules but remains a single byte otherwise. But the code doesn't change the rules in mid parse, so the DF is parsed still assuming that /d is in effect so it doesn't expand, until pass2 when the space has already been calculated. The reason my new code works is that it will change the rules in midparse, so we know immediately that the DF needs two bytes. If the order in the ticket's pattern were reversed. and the DF came first, it currently sets a flag, which the \N{} code sees and restarts the parse. That would continue under my new scheme. Except, that we don't actually need to restart the parse here. We need only restart parsing the current EXACTish node. That is far less work. In general, we do need to restart the parse, because the \N{} could be a long ways away in the pattern, in a different node, and then we have to got back and restart the whole parse. But a variable locale to the loop that parses this node could be used to avoid restarting the whole parse.
From: demerphq <demerphq [...] gmail.com>
CC: perl5-security-report-followup [...] perl.org, perl5-security-report [...] perl.org
Date: Sun, 4 Feb 2018 10:10:32 +0100
Subject: Re: [perl #132227] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
To: Karl Williamson <public [...] khwilliamson.com>
Download (untitled) / with headers
text/plain 1.9k
On 4 February 2018 at 01:11, Karl Williamson <public@khwilliamson.com> wrote: Show quoted text
>> On 02/03/2018 06:32 AM, demerphq wrote:
[snip] Show quoted text
>> I wrapped all the macros that are used for RESTART_PASS1 (including >> your patch) and gave them names, etc. I think we can silently fix this >> in blead in such a patch without worrying about it revealing anything >> about the CVE aspect of this. But we need to get back patches done, >> and then do a mega-cve release along with the other issue we have >> pending. ;-)
>
[snip] Show quoted text
> > 7 months ago I made a proposal, fully explained here, and in followup posts: > http://nntp.perl.org/group/perl.perl5.porters/245342
[snip] Show quoted text
> I now have a branch that implements this. The reason it is relevant to this > security ticket, is that implementing this causes this bug to go away with > no special effort required. Thus if we were to put it in blead, no one > would notice that it is fixing a bug.
I don't follow that part entirely. I would like to see the branch. But I am fine with the plan. It sounds like a good thing regardless. I think we should do the following at this point for /blead/: I will push the patch i posted in my previous reply, with the cleanup of the restart logic that includes Karls original patch. It removes a ton of replicated code, and makes things easier to understand, and fixes the original bug. Because the fix is inside of a much larger set of "trivial" changes the impact is going to be non-obvious, it isnt going to set of some wave of exploits. Then afterwards Karl can apply his changes to EXACT joining and typing at different phases on top of them. For backports we will do the minimal patch proposed by Karl, and release along with the Trie issue, (132063) which is MUCH more serious IMO. Then we fix both with one back-release. Personally I think the Trie issue justifies posting fixes back to 5.22 cheers, Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
RT-Send-CC: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 8.1k
On Fri, 02 Feb 2018 14:40:35 -0800, public@khwilliamson.com wrote: Show quoted text
> On 02/02/2018 02:26 PM, demerphq wrote:
> > On 2 February 2018 at 17:40, demerphq <demerphq@gmail.com> wrote:
> >> On 2 February 2018 at 13:21, demerphq <demerphq@gmail.com> wrote:
> >>> FWIW, this looks like a reparse bug. The pattern starts out latin- > >>> 1. > >>> We should trigger "reparse as utf8" when we see the \N{U+0}, which > >>> then should do the correct sizing. > >>> > >>> So I would look into what is happening with \N{U+0} and why it isnt > >>> triggering a reparse of the pattern.
> >> > >> Yep, that is it, or more or less, although we ARE trying to trigger > >> reparse, but that logic has been "optimized" such that it does not > >> do > >> the reparse if it has not seen \xDF first, and later when it DOES > >> see > >> the \xDF it also does not realize it needs to upgrade the pattern. > >> The > >> following patch fixes it, although its not the right patch. > >> > >> $ git diff > >> diff --git a/regcomp.c b/regcomp.c > >> index 6dbfed5..2668df5 100644 > >> --- a/regcomp.c > >> +++ b/regcomp.c > >> @@ -352,7 +352,7 @@ struct RExC_state_t { > >> assert(PASS1); > >> \ > >> set_regex_charset(&RExC_flags, > >> REGEX_UNICODE_CHARSET); \ > >> RExC_uni_semantics = 1; > >> \ > >> - if (RExC_seen_unfolded_sharp_s) { > >> \ > >> + if (!UTF || RExC_seen_unfolded_sharp_s) { > >> \ > >> *flagp |= RESTART_PASS1; > >> \ > >> return restart_retval; > >> \ > >> } > >> > >> You have evolved some of this handling beyond my current > >> understanding > >> of the subtleties, so it will take me some time to work a better > >> patch. > >> > >> With my patch: > >> > >> $ ./perl -Ilib -Mre=Debug,ALL -le'/00\N{U+2}\xDF/i' > >> Assembling pattern from 1 elements > >> Compiling REx "00\N{U+2}\xDF" > >> Starting first pass (sizing) > >> <00\N{U+2}\xDF> | 1| reg > >> | | brnc > >> | | piec > >> | | atom > >> Need to redo pass 1 > >> Starting first pass (sizing) > >> <00\N{U+2}\xDF> | 1| reg > >> | | brnc > >> | | piec > >> | | atom > >> Required size 4 nodes > >> Starting second pass (creation) > >> <00\N{U+2}\xDF> | 1| reg > >> | | brnc > >> | | piec > >> | | atom > >> <> | 5| lsbr~ tying lastbr EXACTFU <00\2ss> (1) to > >> ender END (4) offset 3 > >> | | tail~ EXACTFU <00\2ss> (1) -> END > >> Starting post parse optimization > >> first:> 1: EXACTFU <00\2ss> (4) > >> first at 1 > >> RExC_seen: > >> study_chunk stopparen=-1 recursed_count=1 depth=0 recursed_depth=0 > >> scan=1118124 last=1118134 > >> Peep: Pos:0/0 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' 0:0/0 *Fixed: '' > >> @ > >> 0/0 Float: '' @ 0/0
> >> Peep> 1: EXACTFU <00\2ss> (4) [ SCF_DO_SUBSTR SCF_WHILEM_VISITED_POS > >> Peep> ]
> >> join> 1: EXACTFU <00\2ss> (4)
> >> commit: Pos:0/0 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 *Fixed: > >> '' @ 0/0 Float: '' @ 0/0 > >> pre-fin: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: > >> '' @ 0/0 *Float: '' @ 0/0 > >> post-fin: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 > >> Fixed: > >> '' @ 0/0 *Float: '' @ 0/0 > >> commit: Pos:4/1 Flags: 0x0 Whilem_c: 0 Lcp: 0 Last:'' -1:0/0 Fixed: > >> '' > >> @ 0/0 *Float: '' @ 0/0 > >> minlen: 4 r->minlen:0 maxlen:5 > >> study_chunk_recursed_count: 1 > >> RExC_seen: > >> Final program: > >> 1: EXACTFU_SS <00\2ss> (4) > >> 4: END (0) > >> stclass EXACTFU_SS <00\2ss> minlen 4 > >> r->extflags: FOLD UNICODE > >> r->intflags: [none-set] > >> Freeing REx: "00\N{U+2}\xDF" > >> > >> ---------->8--------------->8------------- > >> > >> Without: > >> > >> $ ./perl -Ilib -Mre=Debug,ALL -le'/00\N{U+2}\xDF/i' > >> Assembling pattern from 1 elements > >> Compiling REx "00\N{U+2}\xDF" > >> Starting first pass (sizing) > >> <00\N{U+2}\xDF> | 1| reg > >> | | brnc > >> | | piec > >> | | atom > >> Required size 3 nodes > >> Starting second pass (creation) > >> <00\N{U+2}\xDF> | 1| reg > >> | | brnc > >> | | piec > >> | | atom > >> panic: reg_node overrun trying to emit 0, 2133840>=2133840 at -e > >> line 1.
> > > > I think the attached patch is better. > > > > Basically we only need to reparse if we see a \xDF and we are in > > unicode semantics. > > > > Yves > > > > > > > >
> > Yves' patch unnecessarily upgrades to UTF-8. And on irc, he indicated > he was conflating in his mind upgrading and reparsing. > > Attached is my patch. > > The problem occurs when we are under /d semantics. An EXACTF node is > generated, which means it may have different meanings if the eventual > matched target string is in UTF-8. Then the \N{} is seen, which > changes > the rules to unicode, as \N is a unicode construct. Then the \xDF > comes > along and sees that it is an EXACTF node and so doesn't do anything > special besides setting a flag. > > What really should happen in this situation is addressed in my patch. > It restarts parsing, having set it to be using unicode rules. Then > instead of an EXACTF node, it creates an EXACTFU. In these nodes, the > \xDF gets folded in pass1 and you don't have this problem > > Yves wanted to know why it works now if the \xDF comes before the > \N{}. > The reason is that flag. The \N{} comes along after the flag is set, > and is smart enough to look at the flag and restart the parsing with > unicode rules, so that the EXACTFU is generated before the \xDF is > seen.
The patch makes sense to me, and prevents the failures in both my simple test case and the original. I was confused about the meaning of seen_unfolded_sharp_s, since we were seeing the \xDF in a /i regexp, thinking "hey this is folded", but I assume it means something like "we've emitted a \xDF, but because we weren't in unicode rules, we didn't fold it". The original test case causes overflows back to 5.18, but the patch applies back to through the perls we support, 5.24 and 5.26. The CVE form requests the versions an issue applies to, while the specific code that's failing here only exists in 5.24 and later, I suspect the issue was inherited from those earlier versions when the code was re-worked. So I expect I'll put versions 5.18 through 5.26 on the CVE request. The initial request won't include enough detail to reproduce the issue, here's what I expect to update the form with once the issue is public: Vulnerability type: Buffer Overflow Vendor of the product: Perl5 Porters Product: perl Version: 5.18 through 5.26 Has vendor confirmed or acknowledged the vulnerability? Yes Attack vector: Context dependent Impact: Denial of service (it can crash perl) - the other choices for Impact are: Code Execution - this may be possible, but we don't have a confirmed exploit Information Disclosure - no direct disclosure Escalation of Privilege - nope Other - not that I can think of Affected Components: regcomp.c, S_regatom() Attack vector An attacker supplies a regular expression containing one or more \xDF characters after an escape putting the regexp into unicode matching mode, such as a \N{} escape. Each \xDF character adds one byte of overflow, and any other text in the regular expression is written in order, providing the attacker control over the bytes written to the overflowed region. Suggested description of the vulnerability for use in the CVE A crafted regular expression can cause a heap buffer overflow, with control over the bytes written Discoverer(s)/Credits Brian Carpenter <brian.carpenter@gmail.com> Reference(s) https://rt.perl.org/Public/Bug/Display.html?id=132227 Additional Information (blank) I'll only fill in Vulnerability Type through version and the description when initially requesting it (tomorrow) and fill the rest in when it goes public. To see the form, visit: https://cveform.mitre.org/ and select "Request a CVE ID". Tony
RT-Send-CC: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 330b
On Sun, 04 Feb 2018 16:17:13 -0800, tonyc wrote: Show quoted text
> Suggested description of the vulnerability for use in the CVE > > A crafted regular expression can cause a heap buffer overflow, with > control over the bytes written
A crafted regular expression can cause a heap buffer write overflow, with control over the bytes written Tony
Subject: Re: [perl #132227] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
From: Karl Williamson <public [...] khwilliamson.com>
Date: Sun, 4 Feb 2018 20:08:44 -0700
To: demerphq <demerphq [...] gmail.com>
CC: perl5-security-report-followup [...] perl.org, perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 1.9k
On 02/04/2018 02:10 AM, demerphq wrote: Show quoted text
>> I now have a branch that implements this. The reason it is relevant to this >> security ticket, is that implementing this causes this bug to go away with >> no special effort required. Thus if we were to put it in blead, no one >> would notice that it is fixing a bug.
> I don't follow that part entirely. I would like to see the branch. But > I am fine with the plan. It sounds like a good thing regardless. > > I think we should do the following at this point for/blead/: > > I will push the patch i posted in my previous reply, with the cleanup > of the restart logic that includes Karls original patch. It removes a > ton of replicated code, and makes things easier to understand, and > fixes the original bug. Because the fix is inside of a much larger > set of "trivial" changes the impact is going to be non-obvious, it > isnt going to set of some wave of exploits.
I think it would be better to to do my (attached) patch first, but I'm not wedded to the idea. Basically, the problem goes away, in passing, because a new algorithm is used that completely side steps the issue. Thus, there is nothing to hide that we hope people won't notice. It's simply a different way of doing things that happens to avoid the current pitfalls. The algorithm is to always start out with an EXACT node. As long as only non-folding characters are found, they get put into that node. At the first one found, the old node is closed up, and a new one started consisting of only folding characters, until a non-folding one is found.... The reason blead fails is the node type is computed before the \N{} is processed. It sees that there has been no sharp s so far so continues on with the node (now with the wrong type in Pass1), so that the sharp s is later processed (only in pass 1) under the wrong type. The new algorithm changes the node type as we go along, so that the sharp s is processed with the correct type even in pass 1.

Message body is not shown because sender requested not to inline it.

CC: perl5-security-report-followup [...] perl.org, perl5-security-report [...] perl.org
From: Karl Williamson <public [...] khwilliamson.com>
Date: Sun, 4 Feb 2018 20:38:57 -0700
Subject: Re: [perl #132227] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
To: demerphq <demerphq [...] gmail.com>
That patch doesn't apply to blead; the very slightly revised one here does

Message body is not shown because sender requested not to inline it.

To: demerphq <demerphq [...] gmail.com>
Date: Mon, 5 Feb 2018 10:41:10 -0700
Subject: Re: [perl #132227] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
CC: perl5-security-report-followup [...] perl.org, perl5-security-report [...] perl.org
From: Karl Williamson <public [...] khwilliamson.com>
Download (untitled) / with headers
text/plain 1.3k
On 02/04/2018 08:38 PM, Karl Williamson wrote: Show quoted text
> That patch doesn't apply to blead; the very slightly revised one here does
And, in thinking about it overnight, I realized that my patch does not actually fix the problem. It just means that it's harder to trigger. You have to have an ASCII folding character before the \N{}, which also has to an ASCII folding character, then the sharp s. It also did not trigger with these unless I added more sharp s's. Here's a case where it triggers: qr/0b\N{U+41}\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF/i' Compiling REx "0b\N{U+41}\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\x"... panic: reg_node overrun trying to emit 34, 560fca5bd924>=560fca5bd8c4 at -e line 1. panic: bad free, ->next->prev=7373737373737373, header=560fca5bdb60, ->prev->next=560fca5bdb60 at (null) line 1 during global destruction.
CC: perl5-security-report-followup [...] perl.org, perl5-security-report [...] perl.org
To: demerphq <demerphq [...] gmail.com>
Subject: Re: [perl #132227] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
Date: Mon, 5 Feb 2018 13:38:52 -0700
From: Karl Williamson <public [...] khwilliamson.com>
Download (untitled) / with headers
text/plain 2.5k
On 02/05/2018 10:41 AM, Karl Williamson wrote: Show quoted text
> On 02/04/2018 08:38 PM, Karl Williamson wrote:
>> That patch doesn't apply to blead; the very slightly revised one here >> does
> > And, in thinking about it overnight, I realized that my patch does not > actually fix the problem.  It just means that it's harder to trigger. > You have to have an ASCII folding character before the \N{}, which also > has to an ASCII folding character, then the sharp s.  It also did not > trigger with these unless I added more sharp s's.  Here's a case where > it triggers: > > qr/0b\N{U+41}\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF/i' > > Compiling REx > "0b\N{U+41}\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\x"... > panic: reg_node overrun trying to emit 34, 560fca5bd924>=560fca5bd8c4 at > -e line 1.
Show quoted text
> panic: bad free, ->next->prev=7373737373737373, header=560fca5bdb60, > ->prev->next=560fca5bdb60 at (null) line 1 during global destruction. >
The small patch I originally sent turns out to be overkill. We don't need to restart all the parsing; we just need to restart filling the node. A patch that does just that is attached. And that patch is easily incorporated into my other one, which the 2nd attachment does. So here's what I think should happen We need to decide if we're going to sneak this in or wait for the CVE date. If we sneak it in, do we do it with Yves or my patch? Note that his patch assumed that we would restart the whole parse, and doesn't make sense if it's just restarting the current node. For efficiency, we would want to change at some point to just restarting the node if we went with his patch. I think we should just do it once and not make any changes until the CVE is out, which we could do with my patch. Also my patch is doing a bunch of stuff in the area of the code this affects, including several times using the same paradigm it does. But this is all moot if we decide to wait for the CVE. We can apply the portions of Yves patch and mine that don't specifically address this bug, and wait for the CVE for the rest.
Download 0002-132227.patch
text/plain 1.3k

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.

RT-Send-CC: perl5-security-report [...] perl.org
On Mon, 05 Feb 2018 12:39:26 -0800, public@khwilliamson.com wrote: Show quoted text
> On 02/05/2018 10:41 AM, Karl Williamson wrote:
> > On 02/04/2018 08:38 PM, Karl Williamson wrote:
> >> That patch doesn't apply to blead; the very slightly revised one > >> here > >> does
> > > > And, in thinking about it overnight, I realized that my patch does > > not > > actually fix the problem.  It just means that it's harder to trigger. > > You have to have an ASCII folding character before the \N{}, which > > also > > has to an ASCII folding character, then the sharp s.  It also did not > > trigger with these unless I added more sharp s's.  Here's a case > > where > > it triggers: > > > > qr/0b\N{U+41}\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF/i' > > > > Compiling REx > > "0b\N{U+41}\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\x"... > > panic: reg_node overrun trying to emit 34, 560fca5bd924>=560fca5bd8c4 > > at > > -e line 1.
>
> > panic: bad free, ->next->prev=7373737373737373, header=560fca5bdb60, > > ->prev->next=560fca5bdb60 at (null) line 1 during global destruction. > >
> > The small patch I originally sent turns out to be overkill. We don't > need to restart all the parsing; we just need to restart filling the > node. A patch that does just that is attached.
Thanks. Show quoted text
> And that patch is easily incorporated into my other one, which the 2nd > attachment does. > > So here's what I think should happen > > We need to decide if we're going to sneak this in or wait for the CVE > date. > > If we sneak it in, do we do it with Yves or my patch? Note that his > patch assumed that we would restart the whole parse, and doesn't make > sense if it's just restarting the current node. For efficiency, we > would want to change at some point to just restarting the node if we > went with his patch. I think we should just do it once and not make > any > changes until the CVE is out, which we could do with my patch. Also > my > patch is doing a bunch of stuff in the area of the code this affects, > including several times using the same paradigm it does. But this is > all moot if we decide to wait for the CVE. We can apply the portions > of > Yves patch and mine that don't specifically address this bug, and wait > for the CVE for the rest.
I'd prefer to wait until the issue is public before updating blead, but it's up to you. Could you please provide a version of your most recent patch for maint-5.26/maint-5.24 with a better note than "132227"? This will need to go downstream for vendors to update their perl packages. I've requested a CVE ID for this issue. Tony
From: Sawyer X <xsawyerx [...] gmail.com>
Date: Tue, 6 Feb 2018 14:34:27 +0200
Subject: Re: [perl #132227] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
To: perl5-security-report-followup [...] perl.org
CC: Perl 5 Security Report <perl5-security-report [...] perl.org>
Download (untitled) / with headers
text/plain 3.4k
I will contact the vendors. I just need a short description, a fix, and a test case. I'll take the description from the CVE request, but which patch do I take? Also, do we have different patches for different versions? I would need one for each version. On 6 February 2018 at 07:44, Tony Cook via RT <perl5-security-report-followup@perl.org> wrote: Show quoted text
> On Mon, 05 Feb 2018 12:39:26 -0800, public@khwilliamson.com wrote:
>> On 02/05/2018 10:41 AM, Karl Williamson wrote:
>> > On 02/04/2018 08:38 PM, Karl Williamson wrote:
>> >> That patch doesn't apply to blead; the very slightly revised one >> >> here >> >> does
>> > >> > And, in thinking about it overnight, I realized that my patch does >> > not >> > actually fix the problem. It just means that it's harder to trigger. >> > You have to have an ASCII folding character before the \N{}, which >> > also >> > has to an ASCII folding character, then the sharp s. It also did not >> > trigger with these unless I added more sharp s's. Here's a case >> > where >> > it triggers: >> > >> > qr/0b\N{U+41}\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF/i' >> > >> > Compiling REx >> > "0b\N{U+41}\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\x"... >> > panic: reg_node overrun trying to emit 34, 560fca5bd924>=560fca5bd8c4 >> > at >> > -e line 1.
>>
>> > panic: bad free, ->next->prev=7373737373737373, header=560fca5bdb60, >> > ->prev->next=560fca5bdb60 at (null) line 1 during global destruction. >> >
>> >> The small patch I originally sent turns out to be overkill. We don't >> need to restart all the parsing; we just need to restart filling the >> node. A patch that does just that is attached.
> > Thanks. >
>> And that patch is easily incorporated into my other one, which the 2nd >> attachment does. >> >> So here's what I think should happen >> >> We need to decide if we're going to sneak this in or wait for the CVE >> date. >> >> If we sneak it in, do we do it with Yves or my patch? Note that his >> patch assumed that we would restart the whole parse, and doesn't make >> sense if it's just restarting the current node. For efficiency, we >> would want to change at some point to just restarting the node if we >> went with his patch. I think we should just do it once and not make >> any >> changes until the CVE is out, which we could do with my patch. Also >> my >> patch is doing a bunch of stuff in the area of the code this affects, >> including several times using the same paradigm it does. But this is >> all moot if we decide to wait for the CVE. We can apply the portions >> of >> Yves patch and mine that don't specifically address this bug, and wait >> for the CVE for the rest.
> > I'd prefer to wait until the issue is public before updating blead, but it's up to you. > > Could you please provide a version of your most recent patch for maint-5.26/maint-5.24 with a better note than "132227"? > > This will need to go downstream for vendors to update their perl packages. > > I've requested a CVE ID for this issue. > > Tony
RT-Send-CC: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 128b
On Mon, 05 Feb 2018 21:44:23 -0800, tonyc wrote: Show quoted text
> I've requested a CVE ID for this issue.
This issue is CVE-2018-6797. Tony
RT-Send-CC: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 323b
On Tue, 06 Feb 2018 21:01:35 -0800, tonyc wrote: Show quoted text
> On Mon, 05 Feb 2018 21:44:23 -0800, tonyc wrote:
> > I've requested a CVE ID for this issue.
> > This issue is CVE-2018-6797.
Patches for 5.24 and 5.26, merged and de-conflicted (and with a more useful subject than "132227") Karl fixed this in blead in 141513f22. Tony
Subject: 5.24-0005-perl-132227-restart-a-node-if-we-change-to-uni-rules.patch
From e02d7478ebfc399a9d10ba0df60eee217aa7ab8f Mon Sep 17 00:00:00 2001 From: Karl Williamson <khw@cpan.org> Date: Fri, 2 Feb 2018 15:14:27 -0700 Subject: (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. --- regcomp.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/regcomp.c b/regcomp.c index c6c7cb4925..d79bd191c9 100644 --- a/regcomp.c +++ b/regcomp.c @@ -13323,6 +13323,18 @@ S_regatom(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth) * /u. This includes the multi-char fold SHARP S to * 'ss' */ if (UNLIKELY(ender == LATIN_SMALL_LETTER_SHARP_S)) { + + /* If the node started out having uni rules, we + * wouldn't have gotten here. So this means + * something in the middle has changed it, but + * didn't think it needed to reparse. But this + * sharp s now does indicate the need for + * reparsing. */ + if (RExC_uni_semantics) { + p = oldp; + goto loopdone; + } + RExC_seen_unfolded_sharp_s = 1; maybe_exactfu = FALSE; } -- 2.11.0
Subject: 5.26-0006-perl-132227-restart-a-node-if-we-change-to-uni-rules.patch
From 6041b59a21326bd9c2d58034943f07233d5ac1ec Mon Sep 17 00:00:00 2001 From: Karl Williamson <khw@cpan.org> Date: Fri, 2 Feb 2018 15:14:27 -0700 Subject: (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. --- regcomp.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/regcomp.c b/regcomp.c index b2de7f05e1..943029b154 100644 --- a/regcomp.c +++ b/regcomp.c @@ -13538,6 +13538,18 @@ S_regatom(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth) * /u. This includes the multi-char fold SHARP S to * 'ss' */ if (UNLIKELY(ender == LATIN_SMALL_LETTER_SHARP_S)) { + + /* If the node started out having uni rules, we + * wouldn't have gotten here. So this means + * something in the middle has changed it, but + * didn't think it needed to reparse. But this + * sharp s now does indicate the need for + * reparsing. */ + if (RExC_uni_semantics) { + p = oldp; + goto loopdone; + } + RExC_seen_unfolded_sharp_s = 1; maybe_exactfu = FALSE; } -- 2.11.0
From: Karl Williamson <public [...] khwilliamson.com>
CC: perl5-security-report [...] perl.org
Subject: Re: [perl #132227] [CVE-2018-6797] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
Date: Mon, 19 Feb 2018 21:07:17 -0700
To: perl5-security-report-followup [...] perl.org
Download (untitled) / with headers
text/plain 1.5k
On 02/19/2018 08:31 PM, Tony Cook via RT wrote: Show quoted text
> On Tue, 06 Feb 2018 21:01:35 -0800, tonyc wrote:
>> On Mon, 05 Feb 2018 21:44:23 -0800, tonyc wrote:
>>> I've requested a CVE ID for this issue.
>> >> This issue is CVE-2018-6797.
> > Patches for 5.24 and 5.26, merged and de-conflicted (and with a more useful subject than "132227") > > Karl fixed this in blead in 141513f22. > > Tony >
Actually, Tony asked me not to fix it in blead, and so I didn't. 141513f22 has the side effect of causing the test case that was submitted in this ticket to not trigger this bug. But the bug remains; it just needs a somewhat different set of circumstances to get triggered. I wrote earlier in the thread that the following pattern still triggers it, and I just verified that it still fails in blead $ blead -le 'qr/0b\N{U+41}\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF/i' panic: reg_node overrun trying to emit 34, 55ba06753904>=55ba067538a4 at -e line 1. panic: free from wrong pool, 7373737373737373!=55ba0670dc20 at (null) line 1 during global destruction.
RT-Send-CC: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 1.7k
On Mon, 19 Feb 2018 20:07:30 -0800, public@khwilliamson.com wrote: Show quoted text
> On 02/19/2018 08:31 PM, Tony Cook via RT wrote:
> > On Tue, 06 Feb 2018 21:01:35 -0800, tonyc wrote:
> >> On Mon, 05 Feb 2018 21:44:23 -0800, tonyc wrote:
> >>> I've requested a CVE ID for this issue.
> >> > >> This issue is CVE-2018-6797.
> > > > Patches for 5.24 and 5.26, merged and de-conflicted (and with a more > > useful subject than "132227") > > > > Karl fixed this in blead in 141513f22. > > > > Tony > >
> > Actually, Tony asked me not to fix it in blead, and so I didn't. > 141513f22 has the side effect of causing the test case that was > submitted in this ticket to not trigger this bug. But the bug > remains; > it just needs a somewhat different set of circumstances to get > triggered. > > I wrote earlier in the thread that the following pattern still > triggers > it, and I just verified that it still fails in blead > > $ blead -le > 'qr/0b\N{U+41}\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF/i' > > panic: reg_node overrun trying to emit 34, 55ba06753904>=55ba067538a4 > at > -e line 1. > panic: free from wrong pool, 7373737373737373!=55ba0670dc20 at (null) > line 1 during global destruction.
Ok, thanks. Do you have a patch that applies to blead to fix the problem? Tony
CC: perl5-security-report [...] perl.org
To: perl5-security-report-followup [...] perl.org
From: Karl Williamson <public [...] khwilliamson.com>
Date: Thu, 22 Feb 2018 22:48:54 -0700
Subject: Re: [perl #132227] [CVE-2018-6797] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
Download (untitled) / with headers
text/plain 1.6k
On 02/19/2018 09:07 PM, Karl Williamson wrote: Show quoted text
> On 02/19/2018 08:31 PM, Tony Cook via RT wrote:
>> On Tue, 06 Feb 2018 21:01:35 -0800, tonyc wrote:
>>> On Mon, 05 Feb 2018 21:44:23 -0800, tonyc wrote:
>>>> I've requested a CVE ID for this issue.
>>> >>> This issue is CVE-2018-6797.
>> >> Patches for 5.24 and 5.26, merged and de-conflicted (and with a more >> useful subject than "132227") >> >> Karl fixed this in blead in 141513f22. >> >> Tony >>
> > Actually, Tony asked me not to fix it in blead, and so I didn't. > 141513f22 has the side effect of causing the test case that was > submitted in this ticket to not trigger this bug.  But the bug remains; > it just needs a somewhat different set of circumstances to get triggered. > > I wrote earlier in the thread that the following pattern still triggers > it, and I just verified that it still fails in blead > > $ blead -le > 'qr/0b\N{U+41}\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF\xDF/i' > > panic: reg_node overrun trying to emit 34, 55ba06753904>=55ba067538a4 at > -e line 1. > panic: free from wrong pool, 7373737373737373!=55ba0670dc20 at (null) > line 1 during global destruction. >
Blead patch attached.

Message body is not shown because sender requested not to inline it.

CC: perl5-security-report-followup [...] perl.org, Perl 5 Security Report <perl5-security-report [...] perl.org>
Subject: Re: [perl #132227] [CVE-2018-6797] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
Date: Sat, 24 Feb 2018 18:55:36 +0200
To: Karl Williamson <public [...] khwilliamson.com>
From: Sawyer X <xsawyerx [...] gmail.com>
The public disclosure date is set for March 15th.
To: perl5-security-report-followup [...] perl.org
Subject: Re: [perl #132227] [CVE-2018-6797] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
From: "Brian C." <brian.carpenter [...] gmail.com>
Date: Thu, 15 Mar 2018 23:38:58 -0500
Is it okay to disclose this bug now?
Date: Fri, 16 Mar 2018 23:27:02 +1100
From: Tony Cook <tony [...] develop-help.com>
Subject: Re: [perl #132227] [CVE-2018-6797] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
To: "Brian C." <brian.carpenter [...] gmail.com>
CC: perl5-security-report-followup [...] perl.org
Download (untitled) / with headers
text/plain 205b
On Thu, Mar 15, 2018 at 11:38:58PM -0500, Brian C. wrote: Show quoted text
> Is it okay to disclose this bug now?
Sorry, no, downstream has delayed disclosure. The actual disclosure date is under discussion AFAIK. Tony
To: perl5-security-report-followup [...] perl.org
Subject: Re: [perl #132227] [CVE-2018-6797] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
Date: Fri, 16 Mar 2018 18:26:11 +0000
From: "Brian C." <brian.carpenter [...] gmail.com>
We are long past the industry standard of 90 days now.
To: "Brian C." <brian.carpenter [...] gmail.com>
CC: perl5-security-report-followup [...] perl.org
Subject: Re: [perl #132227] [CVE-2018-6797] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
From: Sawyer X <xsawyerx [...] gmail.com>
Date: Fri, 16 Mar 2018 18:28:03 +0000
Download (untitled) / with headers
text/plain 309b
Industry standard is also to respect downstream vendors concerns and we have always done this, sometimes to far longer than 90 days. Please be patient. We will update shortly.

On Fri, Mar 16, 2018, 20:26 Brian C. <brian.carpenter@gmail.com> wrote:
Show quoted text
We are long past the industry standard of 90 days now.
From: Sawyer X <xsawyerx [...] gmail.com>
Date: Tue, 20 Mar 2018 00:26:49 +0200
Subject: Re: [perl #132227] [CVE-2018-6797] heap-buffer-overflow (WRITE of size 1) in S_regatom (regcomp.c)
CC: perl5-security-report-followup [...] perl.org
To: "Brian C." <brian.carpenter [...] gmail.com>
Download (untitled) / with headers
text/plain 449b
The disclosure date has been postponed officially to April 14th. On 16 March 2018 at 20:28, Sawyer X <xsawyerx@gmail.com> wrote: Show quoted text
> Industry standard is also to respect downstream vendors concerns and we have > always done this, sometimes to far longer than 90 days. Please be patient. > We will update shortly. > > On Fri, Mar 16, 2018, 20:26 Brian C. <brian.carpenter@gmail.com> wrote:
>> >> We are long past the industry standard of 90 days now.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 152b
On Mon, 19 Mar 2018 15:27:19 -0700, xsawyerx@gmail.com wrote: Show quoted text
> The disclosure date has been postponed officially to April 14th.
Moved to public queue.
RT-Send-CC: perl5-porters [...] perl.org
Blead commit that fixed this is 2407a17ad5d780a1625dddfb668056ab05459194 -- Karl Williamson
Download (untitled) / with headers
text/plain 317b
Thank you for filing this report. You have helped make Perl better. With the release yesterday of Perl 5.28.0, this and 185 other issues have been resolved. Perl 5.28.0 may be downloaded via: https://metacpan.org/release/XSAWYERX/perl-5.28.0 If you find that the problem persists, feel free to reopen this ticket.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org