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

PERL-5.26.1 heap-buffer-overflow READ of size 11 #16343

Closed
p5pRT opened this issue Dec 26, 2017 · 22 comments
Closed

PERL-5.26.1 heap-buffer-overflow READ of size 11 #16343

p5pRT opened this issue Dec 26, 2017 · 22 comments

Comments

@p5pRT
Copy link

p5pRT commented Dec 26, 2017

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

Searchable as RT132655$

@p5pRT
Copy link
Author

p5pRT commented Dec 26, 2017

From sraums2498@gmail.com

=================================================================
==2940==ERROR​: AddressSanitizer​: heap-buffer-overflow on address
0x60200000d91a at pc 0x7eff7544d20b bp 0x7ffcc6b8d800 sp 0x7ffcc6b8cfa8
READ of size 11 at 0x60200000d91a thread T0
  #0 0x7eff7544d20a in __interceptor_strlen
(/usr/lib/x86_64-linux-gnu/libasan.so.2+0x7020a)
  #1 0x2df9cfb in Perl_my_atof2
/home/asan_perl/Documents/perl-5.26.1/numeric.c​:1349
  #2 0x2e04973 in Perl_my_atof
/home/asan_perl/Documents/perl-5.26.1/numeric.c​:1244
  #3 0x1e9febc in S_sv_setnv
/home/asan_perl/Documents/perl-5.26.1/sv.c​:2109
  #4 0x1e9febc in S_sv_2iuv_common
/home/asan_perl/Documents/perl-5.26.1/sv.c​:2311
  #5 0x1ea95e7 in Perl_sv_2iv_flags
/home/asan_perl/Documents/perl-5.26.1/sv.c​:2504
  #6 0x205229d in Perl_pp_multiply
/home/asan_perl/Documents/perl-5.26.1/pp.c​:1373
  #7 0x1b1bc2e in Perl_runops_standard
/home/asan_perl/Documents/perl-5.26.1/run.c​:41
  #8 0x9218a5 in S_run_body
/home/asan_perl/Documents/perl-5.26.1/perl.c​:2519
  #9 0x9218a5 in perl_run
/home/asan_perl/Documents/perl-5.26.1/perl.c​:2447
  #10 0x46b6a7 in main
/home/asan_perl/Documents/perl-5.26.1/perlmain.c​:123
  #11 0x7eff746d182f in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
  #12 0x46c888 in _start
(/home/asan_perl/Documents/perl-5.26.1/perl+0x46c888)

0x60200000d91a is located 0 bytes to the right of 10-byte region
[0x60200000d910,0x60200000d91a)
allocated by thread T0 here​:
  #0 0x7eff75475602 in malloc
(/usr/lib/x86_64-linux-gnu/libasan.so.2+0x98602)
  #1 0x167dd81 in Perl_safesysmalloc
/home/asan_perl/Documents/perl-5.26.1/util.c​:153
  #2 0x1de5477 in Perl_sv_grow
/home/asan_perl/Documents/perl-5.26.1/sv.c​:1601
  #3 0x1e07a57 in Perl_newSV
/home/asan_perl/Documents/perl-5.26.1/sv.c​:5682
  #4 0x2ebe18d in S_unpack_rec
/home/asan_perl/Documents/perl-5.26.1/pp_pack.c​:1716
  #5 0x2f6a66a in Perl_unpackstring
/home/asan_perl/Documents/perl-5.26.1/pp_pack.c​:838
  #6 0x2f6c002 in Perl_pp_unpack
/home/asan_perl/Documents/perl-5.26.1/pp_pack.c​:1848
  #7 0x1b1bc2e in Perl_runops_standard
/home/asan_perl/Documents/perl-5.26.1/run.c​:41
  #8 0x9218a5 in S_run_body
/home/asan_perl/Documents/perl-5.26.1/perl.c​:2519
  #9 0x9218a5 in perl_run
/home/asan_perl/Documents/perl-5.26.1/perl.c​:2447
  #10 0x46b6a7 in main
/home/asan_perl/Documents/perl-5.26.1/perlmain.c​:123
  #11 0x7eff746d182f in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

SUMMARY​: AddressSanitizer​: heap-buffer-overflow ??​:0 __interceptor_strlen
Shadow bytes around the buggy address​:
  0x0c047fff9ad0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9ae0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9af0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9b00​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9b10​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c047fff9b20​: fa fa 00[02]fa fa 00 02 fa fa 00 02 fa fa fd fa
  0x0c047fff9b30​: fa fa 00 02 fa fa fd fd fa fa fd fd fa fa fd fd
  0x0c047fff9b40​: fa fa 06 fa fa fa fd fa fa fa 03 fa fa fa fd fd
  0x0c047fff9b50​: fa fa fd fd fa fa fd fd fa fa fd fa fa fa fd fa
  0x0c047fff9b60​: fa fa 00 02 fa fa fd fd fa fa fd fd fa fa 02 fa
  0x0c047fff9b70​: fa fa 07 fa fa fa 00 03 fa fa 00 02 fa fa 00 06
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
  Heap right redzone​: fb
  Freed heap region​: fd
  Stack left redzone​: f1
  Stack mid redzone​: f2
  Stack right redzone​: f3
  Stack partial redzone​: f4
  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
==2940==ABORTING

--
Regards,
SRAUMS

@p5pRT
Copy link
Author

p5pRT commented Dec 26, 2017

From sraums2498@gmail.com

707

@p5pRT
Copy link
Author

p5pRT commented Jan 9, 2018

From @hvds

This looks very similar to rt132654​:
  ./miniperl -e 's/(?=)/ab/; 0 + unpack "u"'

I'll add a cross-reference, but it's a bit too early to merge them.

Hugo

@p5pRT
Copy link
Author

p5pRT commented Jan 9, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Jan 23, 2018

From @tonycoz

On Tue, 09 Jan 2018 00​:46​:06 -0800, hv wrote​:

This looks very similar to rt132654​:
./miniperl -e 's/(?=)/ab/; 0 + unpack "u"'

This can be further simplified to​:

  ./miniperl -e '0 * unpack "u", "ab"'

I'll add a cross-reference, but it's a bit too early to merge them.

It's unrelated - 132654 is about (mis-)use of the p pack format.

The problem here is the "u" decoder, which creates a new SV (upgraded to SVt_PV in the case that matters) and sets POK on it.

When the "ab" fails to decode, no changes are made to the PV, leaving it unterminated.

pp_add (or pp_multiply, depending on whether you use Hugo's or the original's test case) then tries to use that SV as a number, eventually leading to sv_2iv_flags trying to decode the (unterminated) PV as a number via Perl_my_atof() which treats the PV as a C NUL terminated string and accesses the uninitialized bytes.

Trivially fixed by ensuring the PV is NUL terminated.

Inline Patch
diff --git a/pp_pack.c b/pp_pack.c
index 8937d6d715..50e9b30221 100644
--- a/pp_pack.c
+++ b/pp_pack.c
@@ -1710,7 +1710,10 @@ S_unpack_rec(pTHX_ tempsym_t* symptr, const char *s, const char *strbeg, const c
            if (!checksum) {
                 const STRLEN l = (STRLEN) (strend - s) * 3 / 4;
                sv = sv_2mortal(newSV(l));
-               if (l) SvPOK_on(sv);
+               if (l) {
+                    SvPOK_on(sv);
+                    *SvEND(sv) = '\0';
+                }
            }
 
             /* Note that all legal uuencoded strings are ASCII printables, so

(alternatively we could use SvPVCLEAR(), but that does work that isn't required here.)

I think it's highly likely there's code accepting untrusted uuencoded data and passing it through C<unpack "u">.

There's a (low) chance of an attacker who can control the uudecoded data causing perl to SEGV if the strlen happens to run off the end of the mapped memory block.

This should only cause perl to crash, there's nothing that will write to the memory block.

This issue can only occur if the first byte in the string is not a uuencoding character - so no decoded data is emitted, so the attacker has zero control over what ends up the SV that's causing the problem.

In the past we've treated similar issues as *not* being security issues.

I don't think this is a security issue.

Tony

@p5pRT
Copy link
Author

p5pRT commented Apr 9, 2018

From @iabyn

On Mon, Jan 22, 2018 at 09​:14​:09PM -0800, Tony Cook via RT wrote​:

On Tue, 09 Jan 2018 00​:46​:06 -0800, hv wrote​:

This looks very similar to rt132654​:
./miniperl -e 's/(?=)/ab/; 0 + unpack "u"'

This can be further simplified to​:

./miniperl -e '0 * unpack "u", "ab"'

I'll add a cross-reference, but it's a bit too early to merge them.

It's unrelated - 132654 is about (mis-)use of the p pack format.

The problem here is the "u" decoder, which creates a new SV (upgraded to SVt_PV in the case that matters) and sets POK on it.

When the "ab" fails to decode, no changes are made to the PV, leaving it unterminated.

pp_add (or pp_multiply, depending on whether you use Hugo's or the original's test case) then tries to use that SV as a number, eventually leading to sv_2iv_flags trying to decode the (unterminated) PV as a number via Perl_my_atof() which treats the PV as a C NUL terminated string and accesses the uninitialized bytes.

Trivially fixed by ensuring the PV is NUL terminated.

diff --git a/pp_pack.c b/pp_pack.c
index 8937d6d715..50e9b30221 100644
--- a/pp_pack.c
+++ b/pp_pack.c
@​@​ -1710,7 +1710,10 @​@​ S_unpack_rec(pTHX_ tempsym_t* symptr, const char *s, const char *strbeg, const c
if (!checksum) {
const STRLEN l = (STRLEN) (strend - s) * 3 / 4;
sv = sv_2mortal(newSV(l));
- if (l) SvPOK_on(sv);
+ if (l) {
+ SvPOK_on(sv);
+ *SvEND(sv) = '\0';
+ }
}

         /\* Note that all legal uuencoded strings are ASCII printables\, so

(alternatively we could use SvPVCLEAR(), but that does work that isn't required here.)

I think it's highly likely there's code accepting untrusted uuencoded data and passing it through C<unpack "u">.

There's a (low) chance of an attacker who can control the uudecoded data causing perl to SEGV if the strlen happens to run off the end of the mapped memory block.

This should only cause perl to crash, there's nothing that will write to the memory block.

This issue can only occur if the first byte in the string is not a uuencoding character - so no decoded data is emitted, so the attacker has zero control over what ends up the SV that's causing the problem.

In the past we've treated similar issues as *not* being security issues.

I don't think this is a security issue.

Agreed. I think you should go ahead apply the patch.

--
The Enterprise is involved in a bizarre time-warp experience which is in
some way unconnected with the Late 20th Century.
  -- Things That Never Happen in "Star Trek" #14

@p5pRT
Copy link
Author

p5pRT commented Jun 25, 2018

From @geeknik

This overflow was triggered while fuzzing Perl v5.28.0-RC2-2-g197e798,
compiled with ASan and Clang 7.0.0 (trunk 335091).

echo "dW5wYWNrInV1LwAiLCQw" | base64 -d | tee test099.pl ; ./perl test099.pl

==22268==ERROR​: AddressSanitizer​: heap-buffer-overflow on address
0x602000000f5a at pc 0x00000044a6a9 bp 0x7ffeec648830 sp 0x7ffeec647fd8
READ of size 11 at 0x602000000f5a thread T0
  #0 0x44a6a8 in __interceptor_strlen
/b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc​:356​:5
  #1 0xac75e4 in Perl_my_atof2 /root/perl/numeric.c​:1373​:28
  #2 0xac6947 in Perl_my_atof /root/perl/numeric.c​:1269​:13
  #3 0x8c694e in S_sv_setnv /root/perl/sv.c​:2115​:9
  #4 0x856158 in S_sv_2iuv_common /root/perl/sv.c​:2317​:13
  #5 0x8539c1 in Perl_sv_2iv_flags /root/perl/sv.c​:2510​:6
  #6 0xae8a76 in S_unpack_rec /root/perl/pp_pack.c​:1828​:23
  #7 0xadfa5a in Perl_unpackstring /root/perl/pp_pack.c​:851​:12
  #8 0xaef641 in Perl_pp_unpack /root/perl/pp_pack.c​:1861​:11
  #9 0x77aaad in Perl_runops_debug /root/perl/dump.c​:2536​:23
  #10 0x5a16d7 in S_run_body /root/perl/perl.c
  #11 0x5a0dcd in perl_run /root/perl/perl.c​:2617​:2
  #12 0x50dea0 in main /root/perl/perlmain.c​:122​:9
  #13 0x7f3bc2fe582f in __libc_start_main
/build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c​:291
  #14 0x4371a8 in _start (/root/perl/perl+0x4371a8)

0x602000000f5a is located 0 bytes to the right of 10-byte region
[0x602000000f50,0x602000000f5a)
allocated by thread T0 here​:
  #0 0x4df0a3 in malloc
/b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cc​:146​:3
  #1 0x77f0ed in Perl_safesysmalloc /root/perl/util.c​:153​:21
  #2 0x84b9b7 in Perl_sv_grow /root/perl/sv.c​:1603​:17
  #3 0x882555 in Perl_newSV /root/perl/sv.c​:5601​:2
  #4 0xae6d85 in S_unpack_rec /root/perl/pp_pack.c​:1729​:8
  #5 0xadfa5a in Perl_unpackstring /root/perl/pp_pack.c​:851​:12
  #6 0xaef641 in Perl_pp_unpack /root/perl/pp_pack.c​:1861​:11
  #7 0x77aaad in Perl_runops_debug /root/perl/dump.c​:2536​:23
  #8 0x5a16d7 in S_run_body /root/perl/perl.c
  #9 0x5a0dcd in perl_run /root/perl/perl.c​:2617​:2
  #10 0x50dea0 in main /root/perl/perlmain.c​:122​:9
  #11 0x7f3bc2fe582f in __libc_start_main
/build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c​:291

SUMMARY​: AddressSanitizer​: heap-buffer-overflow
/b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc​:356​:5
in __interceptor_strlen

@p5pRT
Copy link
Author

p5pRT commented Jun 25, 2018

From @iabyn

On Mon, Jun 25, 2018 at 12​:11​:33PM -0700, Brian Carpenter wrote​:

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

This overflow was triggered while fuzzing Perl v5.28.0-RC2-2-g197e798,
compiled with ASan and Clang 7.0.0 (trunk 335091).

echo "dW5wYWNrInV1LwAiLCQw" | base64 -d | tee test099.pl ; ./perl test099.pl

Or more prozaically,

$ valgrind ./perl -e'unpack "uu/\0", "abc";'

The direct fault is that the 'u' action is creating an SV with a
zero-length string *without* a terminating \0, and the '/' action
(which I don't fully understand and the description in perlfunc makes my
eyes bleed) causes that string to be popped off the stack and treated as a
number, which eventually causes the string to passed to atof(), which
because it isn't null terminated, reads garbage off the end.

I'm not yet sure how serious it is.

--
"Foul and greedy Dwarf - you have eaten the last candle."
  -- "Hordes of the Things", BBC Radio.

@p5pRT
Copy link
Author

p5pRT commented Jun 25, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Jun 26, 2018

From @khwilliamson

On 06/25/2018 05​:07 PM, Dave Mitchell wrote​:

On Mon, Jun 25, 2018 at 12​:11​:33PM -0700, Brian Carpenter wrote​:

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

This overflow was triggered while fuzzing Perl v5.28.0-RC2-2-g197e798,
compiled with ASan and Clang 7.0.0 (trunk 335091).

echo "dW5wYWNrInV1LwAiLCQw" | base64 -d | tee test099.pl ; ./perl test099.pl

Or more prozaically,

$ valgrind ./perl -e'unpack "uu/\0", "abc";'

The direct fault is that the 'u' action is creating an SV with a
zero-length string *without* a terminating \0, and the '/' action
(which I don't fully understand and the description in perlfunc makes my
eyes bleed) causes that string to be popped off the stack and treated as a
number, which eventually causes the string to passed to atof(), which
because it isn't null terminated, reads garbage off the end.

I'm not yet sure how serious it is.

See also
https://rt.perl.org/Public/Bug/Display.html?id=131642

@p5pRT
Copy link
Author

p5pRT commented Jun 27, 2018

From @iabyn

On Tue, Jun 26, 2018 at 12​:07​:36AM +0100, Dave Mitchell wrote​:

On Mon, Jun 25, 2018 at 12​:11​:33PM -0700, Brian Carpenter wrote​:

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

This overflow was triggered while fuzzing Perl v5.28.0-RC2-2-g197e798,
compiled with ASan and Clang 7.0.0 (trunk 335091).

echo "dW5wYWNrInV1LwAiLCQw" | base64 -d | tee test099.pl ; ./perl test099.pl

Or more prozaically,

$ valgrind ./perl -e'unpack "uu/\0", "abc";'

The direct fault is that the 'u' action is creating an SV with a
zero-length string *without* a terminating \0, and the '/' action
(which I don't fully understand and the description in perlfunc makes my
eyes bleed) causes that string to be popped off the stack and treated as a
number, which eventually causes the string to passed to atof(), which
because it isn't null terminated, reads garbage off the end.

I'm not yet sure how serious it is.

In general, unpack('u', 'nonlegal-uuencode-chars') will return
an SV which is POK, length 0, but which *doesn't* have a terminating \0
at position 0 in the string buffer. If that SV is passed to something that
uses strlen() or equivalent rather than SvCUR(), it may overrun the
buffer; for example​:

  0.0 + unpack("u", "abc");

trips up in atof().

It's very plausible that an attacker can provide such malformed uuencoded
text; it's unlikely that an attacker could find perl code which uses the
return value of unpack in a way that relies on the \0 in a dangerous way.

Typically you would expect the result of unpack('u') to be fed to
a concat, a print, or a match etc. All these are safe against
non-\0-terminated strings.

My example above of treating the uudecoded result as a number is, I think,
unlikely. The original failing example for this ticket, unpack "uu/\0", ..
uses a nonsensical unpack template which wouldn't appear in nature.

So I think I should just fix it (the fix is trivial) and not treat it as a
security issue.

--
This is a great day for France!
  -- Nixon at Charles De Gaulle's funeral

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2018

From @tonycoz

On Wed, 27 Jun 2018 01​:37​:07 -0700, davem wrote​:

In general, unpack('u', 'nonlegal-uuencode-chars') will return
an SV which is POK, length 0, but which *doesn't* have a terminating
\0
at position 0 in the string buffer. If that SV is passed to something
that
uses strlen() or equivalent rather than SvCUR(), it may overrun the
buffer; for example​:

0.0 + unpack("u", "abc");

trips up in atof().

I believe this is the same issue as #132655.

Tony

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2018

From @tonycoz

On Mon, 09 Apr 2018 08​:53​:03 -0700, davem wrote​:

On Mon, Jan 22, 2018 at 09​:14​:09PM -0800, Tony Cook via RT wrote​:

On Tue, 09 Jan 2018 00​:46​:06 -0800, hv wrote​:

This looks very similar to rt132654​:
./miniperl -e 's/(?=)/ab/; 0 + unpack "u"'

This can be further simplified to​:

./miniperl -e '0 * unpack "u", "ab"'

I'll add a cross-reference, but it's a bit too early to merge them.

It's unrelated - 132654 is about (mis-)use of the p pack format.

The problem here is the "u" decoder, which creates a new SV (upgraded
to SVt_PV in the case that matters) and sets POK on it.

When the "ab" fails to decode, no changes are made to the PV, leaving
it unterminated.

pp_add (or pp_multiply, depending on whether you use Hugo's or the
original's test case) then tries to use that SV as a number,
eventually leading to sv_2iv_flags trying to decode the
(unterminated) PV as a number via Perl_my_atof() which treats the PV
as a C NUL terminated string and accesses the uninitialized bytes.

Trivially fixed by ensuring the PV is NUL terminated.

diff --git a/pp_pack.c b/pp_pack.c
index 8937d6d715..50e9b30221 100644
--- a/pp_pack.c
+++ b/pp_pack.c
@​@​ -1710,7 +1710,10 @​@​ S_unpack_rec(pTHX_ tempsym_t* symptr, const
char *s, const char *strbeg, const c
if (!checksum) {
const STRLEN l = (STRLEN) (strend - s) * 3 / 4;
sv = sv_2mortal(newSV(l));
- if (l) SvPOK_on(sv);
+ if (l) {
+ SvPOK_on(sv);
+ *SvEND(sv) = '\0';
+ }
}

/* Note that all legal uuencoded strings are ASCII printables, so

(alternatively we could use SvPVCLEAR(), but that does work that
isn't required here.)

I think it's highly likely there's code accepting untrusted uuencoded
data and passing it through C<unpack "u">.

There's a (low) chance of an attacker who can control the uudecoded
data causing perl to SEGV if the strlen happens to run off the end of
the mapped memory block.

This should only cause perl to crash, there's nothing that will write
to the memory block.

This issue can only occur if the first byte in the string is not a
uuencoding character - so no decoded data is emitted, so the attacker
has zero control over what ends up the SV that's causing the problem.

In the past we've treated similar issues as *not* being security
issues.

I don't think this is a security issue.

Agreed. I think you should go ahead apply the patch.

Here's the patch, I'll apply it in a couple of days, unless someone suggests we make this a security issue.

Tony

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2018

From @tonycoz

0001-perl-132655-nul-terminate-result-of-unpack-u-of-inva.patch
From 49bb9eff715af452bcbeb2f94716891d185a0f54 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Mon, 20 Aug 2018 16:31:45 +1000
Subject: (perl #132655) nul terminate result of unpack "u" of invalid data

In the given test case, Perl_atof2() would run off the end of the PV,
producing an error from ASAN.
---
 pp_pack.c   | 5 ++++-
 t/op/pack.t | 9 ++++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/pp_pack.c b/pp_pack.c
index 5e9cc64301..f8be9d48ae 100644
--- a/pp_pack.c
+++ b/pp_pack.c
@@ -1727,7 +1727,10 @@ S_unpack_rec(pTHX_ tempsym_t* symptr, const char *s, const char *strbeg, const c
 	    if (!checksum) {
                 const STRLEN l = (STRLEN) (strend - s) * 3 / 4;
 		sv = sv_2mortal(newSV(l));
-		if (l) SvPOK_on(sv);
+		if (l) {
+                    SvPOK_on(sv);
+                    *SvEND(sv) = '\0';
+                }
 	    }
 
             /* Note that all legal uuencoded strings are ASCII printables, so
diff --git a/t/op/pack.t b/t/op/pack.t
index cf0e286509..bb9f865091 100644
--- a/t/op/pack.t
+++ b/t/op/pack.t
@@ -12,7 +12,7 @@ my $no_endianness = $] > 5.009 ? '' :
 my $no_signedness = $] > 5.009 ? '' :
   "Signed/unsigned pack modifiers not available on this perl";
 
-plan tests => 14717;
+plan tests => 14718;
 
 use strict;
 use warnings qw(FATAL all);
@@ -2081,3 +2081,10 @@ SKIP:
     fresh_perl_like('pack "c10f1073741824"', qr/Out of memory during pack/, { stderr => 1 },
 		    "integer overflow calculating allocation (multiply)");
 }
+
+{
+    # [perl #132655] heap-buffer-overflow READ of size 11
+    # only expect failure under ASAN (and maybe valgrind)
+    fresh_perl_is('0.0 + unpack("u", "ab")', "", { stderr => 1 },
+                  "ensure unpack u of invalid data nul terminates result");
+}
-- 
2.11.0

@p5pRT
Copy link
Author

p5pRT commented Aug 21, 2018

From hanno@hboeck.de

Hi,

An out of bounds heap read can be triggered in perl with a call to
unpack​:

perl -e 'unpack("u/X", "r0");'

It will read 11 bytes beyond a defined buffer.

This can be detected by compiling perl with address sanitizer. Works
with latest perl5 git code. I'm attaching a stack trace from address
sanitizer below.


==26194==ERROR​: AddressSanitizer​: heap-buffer-overflow on address 0x60200000165a at pc 0x00000044b1fb bp 0x7ffedc131b90 sp 0x7ffedc131338
READ of size 11 at 0x60200000165a thread T0
  #0 0x44b1fa in __interceptor_strlen (/r/perl/perl+0x44b1fa)
  #1 0xb2e253 in Perl_my_atof3 /f/perl/perl/numeric.c​:1412​:30
  #2 0xb2dc39 in Perl_my_atof /f/perl/perl/numeric.c​:1271​:13
  #3 0x892311 in S_sv_setnv /f/perl/perl/sv.c​:2112​:9
  #4 0x892311 in S_sv_2iuv_common /f/perl/perl/sv.c​:2314
  #5 0x88fced in Perl_sv_2iv_flags /f/perl/perl/sv.c​:2507​:6
  #6 0xb55726 in S_unpack_rec /f/perl/perl/pp_pack.c​:1828​:23
  #7 0xb42c24 in Perl_unpackstring /f/perl/perl/pp_pack.c​:851​:12
  #8 0xb5b012 in Perl_pp_unpack /f/perl/perl/pp_pack.c​:1861​:11
  #9 0x8498e7 in Perl_runops_standard /f/perl/perl/run.c​:41​:26
  #10 0x5b9804 in S_run_body /f/perl/perl/perl.c
  #11 0x5b8633 in perl_run /f/perl/perl/perl.c​:2611​:2
  #12 0x5076fe in main /f/perl/perl/perlmain.c​:122​:9
  #13 0x7f9376bcaae6 in __libc_start_main (/lib64/libc.so.6+0x21ae6)
  #14 0x436c89 in _start (/r/perl/perl+0x436c89)

0x60200000165a is located 0 bytes to the right of 10-byte region [0x602000001650,0x60200000165a)
allocated by thread T0 here​:
  #0 0x4d9723 in malloc (/r/perl/perl+0x4d9723)
  #1 0x7b3f14 in Perl_safesysmalloc /f/perl/perl/util.c​:153​:21
  #2 0x8891f3 in Perl_sv_grow /f/perl/perl/sv.c​:1600​:17
  #3 0x8b2b1a in Perl_newSV /f/perl/perl/sv.c​:5601​:2
  #4 0xb472cd in S_unpack_rec /f/perl/perl/pp_pack.c​:1729​:8
  #5 0xb42c24 in Perl_unpackstring /f/perl/perl/pp_pack.c​:851​:12
  #6 0xb5b012 in Perl_pp_unpack /f/perl/perl/pp_pack.c​:1861​:11
  #7 0x8498e7 in Perl_runops_standard /f/perl/perl/run.c​:41​:26
  #8 0x5b9804 in S_run_body /f/perl/perl/perl.c
  #9 0x5b8633 in perl_run /f/perl/perl/perl.c​:2611​:2
  #10 0x5076fe in main /f/perl/perl/perlmain.c​:122​:9
  #11 0x7f9376bcaae6 in __libc_start_main (/lib64/libc.so.6+0x21ae6)
  #12 0x436c89 in _start (/r/perl/perl+0x436c89)

SUMMARY​: AddressSanitizer​: heap-buffer-overflow (/r/perl/perl+0x44b1fa) in __interceptor_strlen
Shadow bytes around the buggy address​:
  0x0c047fff8270​: fa fa 06 fa fa fa 00 fa fa fa 00 02 fa fa 00 fa
  0x0c047fff8280​: fa fa 00 05 fa fa 00 00 fa fa 00 02 fa fa 00 00
  0x0c047fff8290​: fa fa 05 fa fa fa 00 01 fa fa 00 fa fa fa 00 06
  0x0c047fff82a0​: fa fa 00 01 fa fa 00 02 fa fa 02 fa fa fa fd fd
  0x0c047fff82b0​: fa fa fd fd fa fa 00 02 fa fa fd fa fa fa 00 02
=>0x0c047fff82c0​: fa fa fd fa fa fa 00 02 fa fa 00[02]fa fa fa fa
  0x0c047fff82d0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff82e0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff82f0​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8300​: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8310​: 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
==26194==ABORTING

--
Hanno Böck
https://hboeck.de/

mail/jabber​: hanno@​hboeck.de
GPG​: FE73757FA60E4E21B937579FA5880072BBB51E42

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2018

From @tonycoz

On Sun, 19 Aug 2018 23​:32​:55 -0700, tonyc wrote​:

On Mon, 09 Apr 2018 08​:53​:03 -0700, davem wrote​:

On Mon, Jan 22, 2018 at 09​:14​:09PM -0800, Tony Cook via RT wrote​:

The problem here is the "u" decoder, which creates a new SV
(upgraded
to SVt_PV in the case that matters) and sets POK on it.

When the "ab" fails to decode, no changes are made to the PV,
leaving
it unterminated.
This should only cause perl to crash, there's nothing that will
write
to the memory block.

This issue can only occur if the first byte in the string is not a
uuencoding character - so no decoded data is emitted, so the
attacker
has zero control over what ends up the SV that's causing the
problem.

In the past we've treated similar issues as *not* being security
issues.

I don't think this is a security issue.

Agreed. I think you should go ahead apply the patch.

Here's the patch, I'll apply it in a couple of days, unless someone
suggests we make this a security issue.

More than a couple of days.

Applied as 12cad9b. This ticket is now public.

Tony

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2018

From @tonycoz

On Tue, 21 Aug 2018 04​:19​:15 -0700, hanno@​hboeck.de wrote​:

Hi,

An out of bounds heap read can be triggered in perl with a call to
unpack​:

perl -e 'unpack("u/X", "r0");'

It will read 11 bytes beyond a defined buffer.

This can be detected by compiling perl with address sanitizer. Works
with latest perl5 git code. I'm attaching a stack trace from address
sanitizer below.

This is a duplicate of 132655, and the fix, 12cad9b, also fixes this.

Tony

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2018

From @tonycoz

On Sun, 19 Aug 2018 22​:18​:33 -0700, tonyc wrote​:

I believe this is the same issue as #132655.

The fix for 132655 also fixes this, merging into 132655.

Tony

@p5pRT
Copy link
Author

p5pRT commented May 22, 2019

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release today of Perl 5.30.0, this and 160 other issues have been
resolved.

Perl 5.30.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.30.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT
Copy link
Author

p5pRT commented May 22, 2019

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

@p5pRT p5pRT closed this as completed May 22, 2019
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