Skip Menu |
Report information
Id: 132608
Status: pending release
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: gy741.kim [at] gmail.com
Cc:
AdminCc:

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



Date: Tue, 19 Dec 2017 16:38:00 +0900
To: perl5-security-report [...] perl.org
From: GwanYeong Kim <gy741.kim [...] gmail.com>
Subject: heap-buffer-overflow in Perl_do_trans
Download (untitled) / with headers
text/plain 3.4k

Hello,


I found a heap-buffer-overflow bug in perl.


Please confirm.


Thanks.


Version: This is perl 5, version 27, subversion 7 (v5.27.7)
OS: Ubuntu 17.10 64bit
Steps to reproduce:
 1.Download the PoC files.
 2.Compile the source code with ASan.
 3.Execute the following command
   : ./perl $PoC

```
=================================================================
==5177==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x63300001b9d6 at pc 0x000000a14f31 bp 0x7ffc9ef4c9f0 sp 0x7ffc9ef4c9e8
READ of size 2 at 0x63300001b9d6 thread T0
    #0 0xa14f30 in Perl_do_trans /home/karas/project/perl5-blead/doop.c
    #1 0x90adf1 in Perl_pp_trans /home/karas/project/perl5-blead/pp.c:692:10
    #2 0x7af39e in Perl_runops_debug /home/karas/project/perl5-blead/dump.c:2495:23
    #3 0x5cff58 in perl_run /home/karas/project/perl5-blead/perl.c
    #4 0x528c3d in main /home/karas/project/perl5-blead/perlmain.c:123:9
    #5 0x7fdacc3b81c0 in __libc_start_main /build/glibc-CxtIbX/glibc-2.26/csu/../csu/libc-start.c:308
    #6 0x435cc9 in _start (/home/karas/project/perl5-blead/perl+0x435cc9)

0x63300001b9d6 is located 5136 bytes to the right of 105926-byte region [0x633000000800,0x63300001a5c6)
allocated by thread T0 here:
    #0 0x4f2668 in realloc (/home/karas/project/perl5-blead/perl+0x4f2668)
    #1 0x548315 in S_pmtrans /home/karas/project/perl5-blead/op.c:6586:7
    #2 0x548315 in Perl_pmruntime /home/karas/project/perl5-blead/op.c:6763
    #3 0x6b483a in Perl_yyparse /home/karas/project/perl5-blead/perly.y:1225:23
    #4 0x5cc116 in S_parse_body /home/karas/project/perl5-blead/perl.c:2525:9
    #5 0x5c5371 in perl_parse /home/karas/project/perl5-blead/perl.c:1827:2
    #6 0x528c17 in main /home/karas/project/perl5-blead/perlmain.c:121:18
    #7 0x7fdacc3b81c0 in __libc_start_main /build/glibc-CxtIbX/glibc-2.26/csu/../csu/libc-start.c:308

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/karas/project/perl5-blead/doop.c in Perl_do_trans
Shadow bytes around the buggy address:
  0x0c667fffb6e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c667fffb6f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c667fffb700: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c667fffb710: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c667fffb720: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c667fffb730: fa fa fa fa fa fa fa fa fa fa[fa]fa fa fa fa fa
  0x0c667fffb740: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c667fffb750: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c667fffb760: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c667fffb770: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c667fffb780: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==5177==ABORTING
```
=================
[Acknowledgement]
This work was supported by ICT R&D program of MSIP/IITP. [R7518-16-1001, Innovation hub for high Performance Computing]

Download Perl_do_trans
application/octet-stream 51.8k

Message body not shown because it is not plain text.

RT-Send-CC: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 1.2k
Using a simplified test case and building without asan for debuggability, we can see that tbl[0x100] ends up as a negative signed short, which we then copy into STRLEN rlen: % gdb -q -ex "break doop.c:219" -ex run --args ./miniperl -e \ '$ch = " " x 33600; $s = "tr//$ch/cs"; eval $s for "\x{12b}\x{d8ea}" x 65' Starting program: [...] Breakpoint 1, S_do_trans_complex (sv=0x86e190) at doop.c:219 219 ch = (rlen == 0) ? (I32)comp : (gdb) p tbl[0x100] $1 = -32192 (gdb) p rlen $2 = 18446744073709519424 The (comp - 0x100 < rlen) check at that statement is then used to see if it can read into tbl[], and decides it can do so even for a high codepoint such as 0xd8ea. This particular problem can be avoided with a cast, but it feels like rather more than that would need to change to cover the full range of possibilities: --- a/doop.c +++ b/doop.c @@ -198,7 +198,7 @@ S_do_trans_complex(pTHX_ SV * const sv) d = s; dstart = d; if (complement && !del) - rlen = tbl[0x100]; + rlen = (unsigned short) tbl[0x100]; if (PL_op->op_private & OPpTRANS_SQUASH) { UV pch = 0xfeedface; At first glance, I don't think there are significant security concerns here. Hugo
CC: perl5-security-report [...] perl.org
To: perl5-security-report-followup [...] perl.org
Subject: Re: [perl #132608] heap-buffer-overflow in Perl_do_trans
Date: Sat, 6 Jan 2018 16:45:28 -0700
From: Karl Williamson <public [...] khwilliamson.com>
Download (untitled) / with headers
text/plain 1.6k
On 01/06/2018 08:44 AM, Hugo van der Sanden via RT wrote: Show quoted text
> Using a simplified test case and building without asan for debuggability, we can see that tbl[0x100] ends up as a negative signed short, which we then copy into STRLEN rlen: > > % gdb -q -ex "break doop.c:219" -ex run --args ./miniperl -e \ > '$ch = " " x 33600; $s = "tr//$ch/cs"; eval $s for "\x{12b}\x{d8ea}" x 65' > Starting program: [...] > Breakpoint 1, S_do_trans_complex (sv=0x86e190) at doop.c:219 > 219 ch = (rlen == 0) ? (I32)comp : > (gdb) p tbl[0x100] > $1 = -32192 > (gdb) p rlen > $2 = 18446744073709519424 > > The (comp - 0x100 < rlen) check at that statement is then used to see if it can read into tbl[], and decides it can do so even for a high codepoint such as 0xd8ea. > > This particular problem can be avoided with a cast, but it feels like rather more than that would need to change to cover the full range of possibilities: > --- a/doop.c > +++ b/doop.c > @@ -198,7 +198,7 @@ S_do_trans_complex(pTHX_ SV * const sv) > d = s; > dstart = d; > if (complement && !del) > - rlen = tbl[0x100]; > + rlen = (unsigned short) tbl[0x100]; > > if (PL_op->op_private & OPpTRANS_SQUASH) { > UV pch = 0xfeedface; > > At first glance, I don't think there are significant security concerns here. > > Hugo >
FWIW, I have work in progress to rewrite this code. The motivation is to avoid the use of swashes. If it's really not a security issue, then we need not investigate further, I think. If it's possible to make a TODO test, then that would be useful.
From: Karl Williamson <public [...] khwilliamson.com>
To: perl5-security-report-followup [...] perl.org
Subject: Re: [perl #132608] heap-buffer-overflow in Perl_do_trans
Date: Sat, 6 Jan 2018 16:48:50 -0700
CC: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 1.8k
On 01/06/2018 04:45 PM, Karl Williamson wrote: Show quoted text
> On 01/06/2018 08:44 AM, Hugo van der Sanden via RT wrote:
>> Using a simplified test case and building without asan for >> debuggability, we can see that tbl[0x100] ends up as a negative signed >> short, which we then copy into STRLEN rlen: >> >> % gdb -q -ex "break doop.c:219" -ex run --args ./miniperl -e \ >>      '$ch = " " x 33600; $s = "tr//$ch/cs"; eval $s for >> "\x{12b}\x{d8ea}" x 65' >> Starting program: [...] >> Breakpoint 1, S_do_trans_complex (sv=0x86e190) at doop.c:219 >> 219                                 ch = (rlen == 0) ? (I32)comp : >> (gdb) p tbl[0x100] >> $1 = -32192 >> (gdb) p rlen >> $2 = 18446744073709519424 >> >> The (comp - 0x100 < rlen) check at that statement is then used to see >> if it can read into tbl[], and decides it can do so even for a high >> codepoint such as 0xd8ea. >> >> This particular problem can be avoided with a cast, but it feels like >> rather more than that would need to change to cover the full range of >> possibilities: >> --- a/doop.c >> +++ b/doop.c >> @@ -198,7 +198,7 @@ S_do_trans_complex(pTHX_ SV * const sv) >>              d = s; >>          dstart = d; >>          if (complement && !del) >> -           rlen = tbl[0x100]; >> +           rlen = (unsigned short) tbl[0x100]; >>          if (PL_op->op_private & OPpTRANS_SQUASH) { >>              UV pch = 0xfeedface; >> >> At first glance, I don't think there are significant security concerns >> here. >> >> Hugo >>
> > FWIW, I have work in progress to rewrite this code.  The motivation is > to avoid the use of swashes.  If it's really not a security issue, then > we need not investigate further, I think.  If it's possible to make a > TODO test, then that would be useful. >
Though this work is unlikely to get into 5.28
CC: perl5-security-report-followup [...] perl.org, perl5-security-report [...] perl.org
To: Karl Williamson <public [...] khwilliamson.com>
From: Dave Mitchell <davem [...] iabyn.com>
Date: Mon, 8 Jan 2018 09:47:09 +0000
Subject: Re: [perl #132608] heap-buffer-overflow in Perl_do_trans
Download (untitled) / with headers
text/plain 970b
On Sat, Jan 06, 2018 at 04:45:28PM -0700, Karl Williamson wrote: Show quoted text
> FWIW, I have work in progress to rewrite this code. The motivation is to > avoid the use of swashes. If it's really not a security issue, then we need > not investigate further, I think. If it's possible to make a TODO test, > then that would be useful.
I've been independently working on this ticket. The bug is in the non-swash part of the code, and I know how to fix it. I've also uncovered some further issues and written a bunch of tests. it turns out that tr///c is almost entirely untested in core. My branch also adds a bunch of commentary to S_pmtrans(), and adds comments at the top of each S_do_trans_foo() function. It this work likely to interfere with yours? -- More than any other time in history, mankind faces a crossroads. One path leads to despair and utter hopelessness. The other, to total extinction. Let us pray we have the wisdom to choose correctly. -- Woody Allen
From: Karl Williamson <public [...] khwilliamson.com>
To: Dave Mitchell <davem [...] iabyn.com>
Date: Mon, 8 Jan 2018 13:45:46 -0700
Subject: Re: [perl #132608] heap-buffer-overflow in Perl_do_trans
CC: perl5-security-report-followup [...] perl.org, perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 1.1k
On 01/08/2018 02:47 AM, Dave Mitchell wrote: Show quoted text
> On Sat, Jan 06, 2018 at 04:45:28PM -0700, Karl Williamson wrote:
>> FWIW, I have work in progress to rewrite this code. The motivation is to >> avoid the use of swashes. If it's really not a security issue, then we need >> not investigate further, I think. If it's possible to make a TODO test, >> then that would be useful.
> > I've been independently working on this ticket. > > The bug is in the non-swash part of the code, and I know how to fix it. > I've also uncovered some further issues and written a bunch of tests. > it turns out that tr///c is almost entirely untested in core. > > My branch also adds a bunch of commentary to S_pmtrans(), and adds comments > at the top of each S_do_trans_foo() function. > > It this work likely to interfere with yours? >
Probably, but go ahead. I mothballed my work about a year ago when it became clear it would not get done in time for 5.26. It involves building up inversion lists as the parsing gets done. But since then we discovered that you can't use tr''' and that should be fixed, so a bunch of stuff would have to be rethought.
Subject: Re: [perl #132608] heap-buffer-overflow in Perl_do_trans
Date: Tue, 9 Jan 2018 20:03:28 +0900
From: GwanYeong Kim <gy741.kim [...] gmail.com>
To: perl5-security-report-followup [...] perl.org
Download (untitled) / with headers
text/plain 1.3k
Hello, 

Happy New Year. :

Do you see this as a security bug?

I can not find this issue in the public queue.

Thanks.

2018-01-09 5:46 GMT+09:00 karl williamson via RT <perl5-security-report-followup@perl.org>:
Show quoted text
On 01/08/2018 02:47 AM, Dave Mitchell wrote:
> On Sat, Jan 06, 2018 at 04:45:28PM -0700, Karl Williamson wrote:
>> FWIW, I have work in progress to rewrite this code.  The motivation is to
>> avoid the use of swashes.  If it's really not a security issue, then we need
>> not investigate further, I think.  If it's possible to make a TODO test,
>> then that would be useful.
>
> I've been independently working on this ticket.
>
> The bug is in the non-swash part of the code, and I know how to fix it.
> I've also uncovered some further issues and written a bunch of tests.
> it turns out that tr///c is almost entirely untested in core.
>
> My branch also adds a bunch of commentary to S_pmtrans(), and adds comments
> at the top of each S_do_trans_foo() function.
>
> It this work likely to interfere with yours?
>

Probably, but go ahead.

I mothballed my work about a year ago when it became clear it would not
get done in time for 5.26.  It involves building up inversion lists as
the parsing gets done.  But since then we discovered that you can't use
tr''' and that should be fixed, so a bunch of stuff would have to be
rethought.


CC: perl5-security-report-followup [...] perl.org
To: GwanYeong Kim <gy741.kim [...] gmail.com>
Date: Wed, 10 Jan 2018 09:36:20 +0000
From: Dave Mitchell <davem [...] iabyn.com>
Subject: Re: [perl #132608] heap-buffer-overflow in Perl_do_trans
Download (untitled) / with headers
text/plain 1.1k
On Tue, Jan 09, 2018 at 08:03:28PM +0900, GwanYeong Kim wrote: Show quoted text
> Do you see this as a security bug?
I think we have mostly decided that it isn't. It only occurs on tr/.../.../c, where: 1) the search and replacement charlists are non-utf8; 2) the replacement charlist is longer than 0x7fff chars 3) the string being modified contains a codepoint > U+7FFF Under those circumstances, those codepoints will be replaced with chars found modulo 0x7fff (i.e. the wrong chars) from the replacement, or from 0..0x7fff bytes in memory before where the translation table is stored. But since tr/// doesn't accept interpolated values (i.e. tr/.../$s/ refers to the two chars '$' and 's'), this will only be an issue if the source code contains such a whacky tr/// already, which is exceedingly unlikely. Show quoted text
> I can not find this issue in the public queue.
It will be moved to the public queue once fixed. -- Music lesson: a symbiotic relationship whereby a pupil's embellishments concerning the amount of practice performed since the last lesson are rewarded with embellishments from the teacher concerning the pupil's progress over the corresponding period.
RT-Send-CC: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 560b
On Wed, 10 Jan 2018 01:36:37 -0800, davem wrote: Show quoted text
> But since tr/// doesn't accept interpolated values (i.e. tr/.../$s/ > refers to the two chars '$' and 's'), this will only be an issue if the > source code contains such a whacky tr/// already, which is exceedingly > unlikely.
I've seen people work around the non-interpolation by instead constructing an eval - I've probably done this myself - so it doesn't have to be a literal whacky tr///, it could be a buggy attempt to construct one. My inclination is that your conclusion is still sound though. Hugo
RT-Send-CC: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 240b
On Wed, 10 Jan 2018 01:36:37 -0800, davem wrote: Show quoted text
> > I can not find this issue in the public queue.
> > It will be moved to the public queue once fixed.
Why wait? We've typically moved non-security bugs from this queue immediately. Tony
From: Dave Mitchell <davem [...] iabyn.com>
Date: Wed, 17 Jan 2018 09:35:57 +0000
Subject: Re: [perl #132608] heap-buffer-overflow in Perl_do_trans
To: Tony Cook via RT <perl5-security-report-followup [...] perl.org>
CC: perl5-security-report [...] perl.org
Download (untitled) / with headers
text/plain 625b
On Sun, Jan 14, 2018 at 08:13:43PM -0800, Tony Cook via RT wrote: Show quoted text
> On Wed, 10 Jan 2018 01:36:37 -0800, davem wrote:
> > > I can not find this issue in the public queue.
> > > > It will be moved to the public queue once fixed.
> > Why wait? > > We've typically moved non-security bugs from this queue immediately.
Because actually fixing the bug, rather than just a provisional diagnosis, is likely to give me a better understanding of the issue, and thus a better assessment of any security issues. But having said that, I'm happy for it to be moved now. -- You never really learn to swear until you learn to drive.
To: perl5-porters [...] perl.org
Subject: Re: [perl #132608] heap-buffer-overflow in Perl_do_trans
Date: Fri, 19 Jan 2018 14:15:07 +0000
From: Dave Mitchell <davem [...] iabyn.com>
Download (untitled) / with headers
text/plain 2.3k
On Wed, Jan 17, 2018 at 09:35:57AM +0000, Dave Mitchell wrote: Show quoted text
> On Sun, Jan 14, 2018 at 08:13:43PM -0800, Tony Cook via RT wrote:
> > On Wed, 10 Jan 2018 01:36:37 -0800, davem wrote:
> > > > I can not find this issue in the public queue.
> > > > > > It will be moved to the public queue once fixed.
> > > > Why wait? > > > > We've typically moved non-security bugs from this queue immediately.
> > Because actually fixing the bug, rather than just a provisional diagnosis, > is likely to give me a better understanding of the issue, and thus a > better assessment of any security issues. But having said that, I'm happy > for it to be moved now.
I've now moved this ticket to the public queue. The bug is fixed by v5.27.7-195-g6d63cc8, which is part of this merge commit which I've just pushed: commit b1f1599cf3d4ae0cb1f1f3c9d379d89dca1873a0 Merge: 840d136 d503fd6 Author: David Mitchell <davem@iabyn.com> AuthorDate: Fri Jan 19 14:08:28 2018 +0000 Commit: David Mitchell <davem@iabyn.com> CommitDate: Fri Jan 19 14:08:28 2018 +0000 [MERGE] various tr/// fixups, esp for /c and /d This branch does the following: Fixes an issue with tr/non_utf8/long_non_utf8/c, where length(long_non_utf8) > 0x7fff. Fixes an issue with tr/non_utf8/non_utf8/cd: basically, the implicit \x{100}-\x{7fffffff} added to the searchlist by /c wasn't being added. Adds a lot of code comments to the various tr/// functions. Adds tr///c tests - basically /c was almost completely untested. Changes the layout of the op_pv transliteration table: it used to be roughly 256 x short - basic table 1 x short - length of extended table (n) n x short - extended table where the 2 and 3rd items were only present under /c. Its now 1 x Size_t - length of table (256+n) (256+n) x short - table - both basic and extended where n == 0 apart from under /c. The new table format also allowed the tr/non_utf8/non_utf8/ code branches to be considerably simplified. op_dump() now dumps the contents of the (non-utf8 variant) transliteration table. Removes I32's from the tr/non_utf8/non_utf8/ code paths, making it fully 64-bit clean. Improves the pod for tr///. -- "Emacs isn't a bad OS once you get used to it. It just lacks a decent editor."


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