Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

heap-buffer-overflow in Perl_do_trans #16318

Closed
p5pRT opened this issue Dec 19, 2017 · 17 comments
Closed

heap-buffer-overflow in Perl_do_trans #16318

p5pRT opened this issue Dec 19, 2017 · 17 comments

Comments

@p5pRT
Copy link

@p5pRT p5pRT commented Dec 19, 2017

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

Searchable as RT132608$

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Dec 19, 2017

From gy741.kim@gmail.com

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]

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Dec 19, 2017

From gy741.kim@gmail.com

Perl_do_trans

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 6, 2018

From @hvds

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

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 6, 2018

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

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 6, 2018

From @khwilliamson

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.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 6, 2018

From @khwilliamson

On 01/06/2018 04​:45 PM, Karl Williamson wrote​:

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

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 8, 2018

From @iabyn

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?

--
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

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 8, 2018

From @khwilliamson

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.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 9, 2018

From gy741.kim@gmail.com

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>​:

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.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 10, 2018

From @iabyn

On Tue, Jan 09, 2018 at 08​:03​:28PM +0900, GwanYeong Kim wrote​:

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.

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.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 10, 2018

From @hvds

On Wed, 10 Jan 2018 01​:36​:37 -0800, davem wrote​:

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

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 15, 2018

From @tonycoz

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.

Tony

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 17, 2018

From @iabyn

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.

--
You never really learn to swear until you learn to drive.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 19, 2018

From @iabyn

On Wed, Jan 17, 2018 at 09​:35​:57AM +0000, Dave Mitchell wrote​:

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 b1f1599
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."

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 19, 2018

@iabyn - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jun 23, 2018

From @khwilliamson

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.

@p5pRT p5pRT closed this as completed Jun 23, 2018
@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jun 23, 2018

@khwilliamson - Status changed from 'pending release' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant