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

[CVE-2018-6913] heap-buffer-overflow in S_pack_rec #16098

Closed
p5pRT opened this issue Aug 5, 2017 · 38 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Aug 5, 2017

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

Searchable as RT131844$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 5, 2017

From gy741.kim@gmail.com

Hi.

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

Please confirm.

Thanks.

Version​: This is perl 5, version 27, subversion 2 (v5.27.2) built for
i686-linux
OS​: Ubuntu 16.04.2 32bit
Steps to reproduce​:
1.Download the PoC files.
2.Compile the source code with ASan.
3.Execute the following command
  : ./perl $PoC

```

==2895==ERROR​: AddressSanitizer​: heap-buffer-overflow on address 0xb610081c
at pc 0x08a72387 bp 0xbfea6038 sp 0xbfea602c
WRITE of size 4 at 0xb610081c thread T0
  #0 0x8a72386 in S_pack_rec /root/karas/perl5-blead/pp_pack.c​:2703​:17
  #1 0x8a42706 in Perl_packlist /root/karas/perl5-blead/pp_pack.c​:1980​:11
  #2 0x8a73626 in Perl_pp_pack /root/karas/perl5-blead/pp_pack.c​:3135​:5
  #3 0x84dc7ac in Perl_runops_debug /root/karas/perl5-blead/dump.c​:2465​:23
  #4 0x818858a in S_fold_constants /root/karas/perl5-blead/op.c​:4557​:2
  #5 0x8186c5a in Perl_op_convert_list
/root/karas/perl5-blead/op.c​:4896​:12
  #6 0x8363e7e in Perl_yyparse /root/karas/perl5-blead/perly.y​:889​:23
  #7 0x8232350 in S_parse_body /root/karas/perl5-blead/perl.c​:2401​:9
  #8 0x82285e3 in perl_parse /root/karas/perl5-blead/perl.c​:1719​:2
  #9 0x81494a6 in main /root/karas/perl5-blead/perlmain.c​:121​:18
  #10 0xb74d5636 in __libc_start_main
/build/glibc-KM3i_a/glibc-2.23/csu/../csu/libc-start.c​:291
  #11 0x8075847 in _start (/root/karas/perl5-blead/perl+0x8075847)

0xb610081c is located 2 bytes to the right of 10-byte region
[0xb6100810,0xb610081a)
allocated by thread T0 here​:
  #0 0x8119b84 in malloc (/root/karas/perl5-blead/perl+0x8119b84)
  #1 0x84e2987 in Perl_safesysmalloc /root/karas/perl5-blead/util.c​:153​:21

SUMMARY​: AddressSanitizer​: heap-buffer-overflow
/root/karas/perl5-blead/pp_pack.c​:2703​:17 in S_pack_rec
Shadow bytes around the buggy address​:
  0x36c200b0​: fa fa fd fd fa fa fd fd fa fa fd fd fa fa 00 04
  0x36c200c0​: fa fa fd fd fa fa 00 04 fa fa 00 04 fa fa 00 04
  0x36c200d0​: fa fa 00 04 fa fa 00 04 fa fa 00 04 fa fa 00 04
  0x36c200e0​: fa fa 00 04 fa fa 00 04 fa fa 00 04 fa fa 00 04
  0x36c200f0​: fa fa fd fa fa fa fd fd fa fa 00 02 fa fa 01 fa
=>0x36c20100​: fa fa 00[02]fa fa 00 02 fa fa fd fd fa fa 00 04
  0x36c20110​: fa fa 02 fa fa fa 00 02 fa fa 07 fa fa fa 00 02
  0x36c20120​: fa fa 00 02 fa fa 00 00 fa fa 00 05 fa fa 00 01
  0x36c20130​: fa fa 00 07 fa fa 00 fa fa fa 00 02 fa fa 05 fa
  0x36c20140​: fa fa 00 02 fa fa 06 fa fa fa 00 02 fa fa 05 fa
  0x36c20150​: fa fa 00 05 fa fa 04 fa fa fa 05 fa fa fa 05 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
  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
  Left alloca redzone​: ca
  Right alloca redzone​: cb
==2895==ABORTING
```

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 5, 2017

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 8, 2017

From @tonycoz

On Sat, 05 Aug 2017 05​:55​:33 -0700, gy741.kim@​gmail.com wrote​:

Hi.

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

Please confirm.

Thanks.

Version​: This is perl 5, version 27, subversion 2 (v5.27.2) built for
i686-linux
OS​: Ubuntu 16.04.2 32bit
Steps to reproduce​:
1.Download the PoC files.
2.Compile the source code with ASan.
3.Execute the following command
: ./perl $PoC

```

==2895==ERROR​: AddressSanitizer​: heap-buffer-overflow on address 0xb610081c
at pc 0x08a72387 bp 0xbfea6038 sp 0xbfea602c
WRITE of size 4 at 0xb610081c thread T0
#0 0x8a72386 in S_pack_rec /root/karas/perl5-blead/pp_pack.c​:2703​:17
#1 0x8a42706 in Perl_packlist /root/karas/perl5-blead/pp_pack.c​:1980​:11

As written this only reproduces on 32-bit systems.

This is caused by a pointer wrap when calculating (cur)+glen in​:

  if ((cur) + glen >= (start) + SvLEN(cat)) {

since in this case glen is ~3GB, and the new pointer ends up less than (start), let alone (start)+SvLEN(cat).

Going through the code I found three other issues, and added tests and
fixes for those too, per the attached patch.

Note that the first case results in a panic with the fix - since the test case attempts to allocate a large amount of memory, which fails, resulting in a call to croak_no_mem(), which triggers a my_exit(), which the constant folding code doesn't know how to handle.

The issues here could result in an exploit in code that accepts either large blocks of data from untrusted sources and/or duplicates such blocks.

The supplied case is a compile-time failure while constant folding, but the same issue could be triggered during runtime with attacker supplied data.

Such code is already subject to denial-of-service attacks (the code can be made to allocate large amounts of memory, as it does on 64-bit systems), but this expands such an attack to a malloced buffer overflow, which is
more serious.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 8, 2017

From @tonycoz

0001-perl-131844-fix-various-space-calculation-issues-in-.patch
From 5a66253379c8be70130f8d4e5961e8d26c3a9efc Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@debian9-x32.tony.develop-help.com>
Date: Tue, 8 Aug 2017 09:32:58 +1000
Subject: (perl #131844) fix various space calculation issues in pp_pack.c

- for the originally reported case, if the start/cur pointer is in the
  top 75% of the address space the add (cur) + glen addition would
  overflow, resulting in the condition failing incorrectly.

- the addition of the existing space used to the space needed could
  overflow, resulting in too small an allocation and a buffer overflow.

- the scaling for UTF8 could overflow.

- the multiply to calculate the space needed for many items could
  overflow.

For the first case, do a space calculation without making new pointers.

For the other cases, detect the overflow and croak if there's an
overflow.
---
 pp_pack.c   | 25 +++++++++++++++++++++----
 t/op/pack.t | 24 +++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/pp_pack.c b/pp_pack.c
index 86d138bb05..53f74c82f5 100644
--- a/pp_pack.c
+++ b/pp_pack.c
@@ -361,11 +361,28 @@ STMT_START {							\
     }								\
 } STMT_END
 
+#define SAFE_UTF8_EXPAND(var)	\
+STMT_START {				\
+    if ((var) > Size_t_MAX / UTF8_EXPAND) \
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()"); \
+    (var) = (var) * UTF8_EXPAND; \
+} STMT_END
+
+#define GROWING2(utf8, cat, start, cur, item_size, item_count)	\
+STMT_START {							\
+    if (Size_t_MAX / (item_size) < (item_count))		\
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()");	\
+    GROWING((utf8), (cat), (start), (cur), (item_size) * (item_count)); \
+} STMT_END
+
 #define GROWING(utf8, cat, start, cur, in_len)	\
 STMT_START {					\
     STRLEN glen = (in_len);			\
-    if (utf8) glen *= UTF8_EXPAND;		\
-    if ((cur) + glen >= (start) + SvLEN(cat)) {	\
+    STRLEN catcur = (STRLEN)((cur) - (start));	\
+    if (utf8) SAFE_UTF8_EXPAND(glen);		\
+    if (Size_t_MAX - glen < catcur)		\
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()"); \
+    if (catcur + glen >= SvLEN(cat)) {	\
 	(start) = sv_exp_grow(cat, glen);	\
 	(cur) = (start) + SvCUR(cat);		\
     }						\
@@ -375,7 +392,7 @@ STMT_START {					\
 STMT_START {					\
     const STRLEN glen = (in_len);		\
     STRLEN gl = glen;				\
-    if (utf8) gl *= UTF8_EXPAND;		\
+    if (utf8) SAFE_UTF8_EXPAND(gl);		\
     if ((cur) + gl >= (start) + SvLEN(cat)) {	\
         *cur = '\0';				\
         SvCUR_set((cat), (cur) - (start));	\
@@ -2135,7 +2152,7 @@ S_pack_rec(pTHX_ SV *cat, tempsym_t* symptr, SV **beglist, SV **endlist )
 	    if (props && !(props & PACK_SIZE_UNPREDICTABLE)) {
 		/* We can process this letter. */
 		STRLEN size = props & PACK_SIZE_MASK;
-		GROWING(utf8, cat, start, cur, (STRLEN) len * size);
+		GROWING2(utf8, cat, start, cur, size, (STRLEN)len);
 	    }
         }
 
diff --git a/t/op/pack.t b/t/op/pack.t
index 919e4c55c6..6e025b4741 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 => 14713;
+plan tests => 14717;
 
 use strict;
 use warnings qw(FATAL all);
@@ -2059,3 +2059,25 @@ print pack("ucW", "0000", 0, 140737488355327) eq "\$,#`P,```\n\0\x{7fffffffffff}
  ? "ok\n" : "not ok\n";
 EOS
 }
+
+SKIP:
+{
+  # [perl #131844] pointer addition overflow
+    $Config{ptrsize} == 4
+      or skip "[perl $131844] need 32-bit build for this test", 1;
+    # prevent ASAN just crashing on the allocation failure
+    local $ENV{ASAN_OPTIONS} = $ENV{ASAN_OPTIONS};
+    $ENV{ASAN_OPTIONS} .= ",allocator_may_return_null=1";
+    fresh_perl_like('pack "f999999999"', qr/Out of memory!/, { stderr => 1 },
+		    "pointer addition overflow");
+
+    # integer (STRLEN) overflow from addition of glen to current length
+    fresh_perl_like('pack "c10f1073741823"', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (addition)");
+
+    fresh_perl_like('pack "W10f536870913", 256', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (utf8)");
+
+    fresh_perl_like('pack "c10f1073741824"', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (multiply)");
+}
-- 
2.11.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 8, 2017

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 29, 2017

From @iabyn

On Sat, Aug 05, 2017 at 05​:55​:33AM -0700, GwanYeong Kim wrote​:

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

Tony, any reason you haven't applied your proposed pp_pack.c fix from back
in August?

--
Art is anything that has a label (especially if the label is "untitled 1")

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 25, 2018

From @tonycoz

On Wed, 29 Nov 2017 01​:08​:14 -0800, davem wrote​:

On Sat, Aug 05, 2017 at 05​:55​:33AM -0700, GwanYeong Kim wrote​:

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

Tony, any reason you haven't applied your proposed pp_pack.c fix from back
in August?

Do we treat it as a security issue?

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 31, 2018

From @tonycoz

On Wed, 24 Jan 2018 20​:49​:53 -0800, tonyc wrote​:

On Wed, 29 Nov 2017 01​:08​:14 -0800, davem wrote​:

On Sat, Aug 05, 2017 at 05​:55​:33AM -0700, GwanYeong Kim wrote​:

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

Tony, any reason you haven't applied your proposed pp_pack.c fix from back
in August?

Do we treat it as a security issue?

After reviewing it, I think this is a security issue.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 8, 2018

From @tonycoz

On Tue, 30 Jan 2018 16​:28​:44 -0800, tonyc wrote​:

On Wed, 24 Jan 2018 20​:49​:53 -0800, tonyc wrote​:

On Wed, 29 Nov 2017 01​:08​:14 -0800, davem wrote​:

On Sat, Aug 05, 2017 at 05​:55​:33AM -0700, GwanYeong Kim wrote​:

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

Tony, any reason you haven't applied your proposed pp_pack.c fix
from back
in August?

Do we treat it as a security issue?

After reviewing it, I think this is a security issue.

I plan to request a CVE ID for this one too, since it's a buffer overflow and an attacker may have control over the data written.

Vulnerability type​:
  Buffer Overflow

Vendor of the product​:
  Perl5 Porters

Product​:
  perl

Version​:
  5.8 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​:
  pp_pack.c, S_pack_rec()

Attack vector
 
A program that accepts a large pack count may misallocate a buffer due to an integer overflow. If an attacker can also supply pack data this may result in a heap buffer write overflow with data controlled by the attacker.

Suggested description of the vulnerability for use in the CVE

  pack() may cause a heap buffer write overflow with a large item count

Discoverer(s)/Credits

  GwanYeong Kim <gy741.kim@​gmail.com>

Reference(s)

  https://rt.perl.org/Public/Bug/Display.html?id=131844

Additional Information

(blank)

I don't plan to supply the suggested description until issue is public.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 12, 2018

From @tonycoz

On Wed, 07 Feb 2018 18​:15​:16 -0800, tonyc wrote​:

I plan to request a CVE ID for this one too, since it's a buffer
overflow and an attacker may have control over the data written.

I've requested a CVE id for this issue.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 12, 2018

From @tonycoz

On Sun, 11 Feb 2018 19​:31​:18 -0800, tonyc wrote​:

On Wed, 07 Feb 2018 18​:15​:16 -0800, tonyc wrote​:

I plan to request a CVE ID for this one too, since it's a buffer
overflow and an attacker may have control over the data written.

I've requested a CVE id for this issue.

This is CVE-2018-6913.

There are no other tickets I'm planning on requesting a CVE id for, if you think any need one please speak up.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 13, 2018

From @tonycoz

On Mon, 07 Aug 2017 18​:34​:44 -0700, tonyc wrote​:

Going through the code I found three other issues, and added tests and
fixes for those too, per the attached patch.

There were two problems (the email address and the skip count), fixed in the attached.

The non-5.24 patch applies to both blead and maint-5.26, the other to 5.24.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 13, 2018

From @tonycoz

0001-perl-131844-5.24-fix-various-space-calculation-issue.patch
From 8a34d2c1718e7cd9ca9d88b71d5932b4e931276d Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 8 Aug 2017 09:32:58 +1000
Subject: (perl #131844) 5.24: fix various space calculation issues in
 pp_pack.c

- for the originally reported case, if the start/cur pointer is in the
  top 75% of the address space the add (cur) + glen addition would
  overflow, resulting in the condition failing incorrectly.

- the addition of the existing space used to the space needed could
  overflow, resulting in too small an allocation and a buffer overflow.

- the scaling for UTF8 could overflow.

- the multiply to calculate the space needed for many items could
  overflow.

For the first case, do a space calculation without making new pointers.

For the other cases, detect the overflow and croak if there's an
overflow.
---
 pp_pack.c   | 25 +++++++++++++++++++++----
 t/op/pack.t | 24 +++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/pp_pack.c b/pp_pack.c
index f6964c3f30..c35525aae9 100644
--- a/pp_pack.c
+++ b/pp_pack.c
@@ -358,11 +358,28 @@ STMT_START {							\
     }								\
 } STMT_END
 
+#define SAFE_UTF8_EXPAND(var)	\
+STMT_START {				\
+    if ((var) > Size_t_MAX / UTF8_EXPAND) \
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()"); \
+    (var) = (var) * UTF8_EXPAND; \
+} STMT_END
+
+#define GROWING2(utf8, cat, start, cur, item_size, item_count)	\
+STMT_START {							\
+    if (Size_t_MAX / (item_size) < (item_count))		\
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()");	\
+    GROWING((utf8), (cat), (start), (cur), (item_size) * (item_count)); \
+} STMT_END
+
 #define GROWING(utf8, cat, start, cur, in_len)	\
 STMT_START {					\
     STRLEN glen = (in_len);			\
-    if (utf8) glen *= UTF8_EXPAND;		\
-    if ((cur) + glen >= (start) + SvLEN(cat)) {	\
+    STRLEN catcur = (STRLEN)((cur) - (start));	\
+    if (utf8) SAFE_UTF8_EXPAND(glen);		\
+    if (Size_t_MAX - glen < catcur)		\
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()"); \
+    if (catcur + glen >= SvLEN(cat)) {	\
 	(start) = sv_exp_grow(cat, glen);	\
 	(cur) = (start) + SvCUR(cat);		\
     }						\
@@ -372,7 +389,7 @@ STMT_START {					\
 STMT_START {					\
     const STRLEN glen = (in_len);		\
     STRLEN gl = glen;				\
-    if (utf8) gl *= UTF8_EXPAND;		\
+    if (utf8) SAFE_UTF8_EXPAND(gl);		\
     if ((cur) + gl >= (start) + SvLEN(cat)) {	\
         *cur = '\0';				\
         SvCUR_set((cat), (cur) - (start));	\
@@ -2126,7 +2143,7 @@ S_pack_rec(pTHX_ SV *cat, tempsym_t* symptr, SV **beglist, SV **endlist )
 	    if (props && !(props & PACK_SIZE_UNPREDICTABLE)) {
 		/* We can process this letter. */
 		STRLEN size = props & PACK_SIZE_MASK;
-		GROWING(utf8, cat, start, cur, (STRLEN) len * size);
+		GROWING2(utf8, cat, start, cur, size, (STRLEN)len);
 	    }
         }
 
diff --git a/t/op/pack.t b/t/op/pack.t
index a2da63689b..0b640fb6eb 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 => 14712;
+plan tests => 14716;
 
 use strict;
 use warnings qw(FATAL all);
@@ -2044,3 +2044,25 @@ ok(1, "argument underflow did not crash");
     is(pack("H40", $up_nul), $twenty_nuls,
        "check pack H zero fills (utf8 source)");
 }
+
+SKIP:
+{
+  # [perl #131844] pointer addition overflow
+    $Config{ptrsize} == 4
+      or skip "[perl #131844] need 32-bit build for this test", 4;
+    # prevent ASAN just crashing on the allocation failure
+    local $ENV{ASAN_OPTIONS} = $ENV{ASAN_OPTIONS};
+    $ENV{ASAN_OPTIONS} .= ",allocator_may_return_null=1";
+    fresh_perl_like('pack "f999999999"', qr/Out of memory!/, { stderr => 1 },
+		    "pointer addition overflow");
+
+    # integer (STRLEN) overflow from addition of glen to current length
+    fresh_perl_like('pack "c10f1073741823"', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (addition)");
+
+    fresh_perl_like('pack "W10f536870913", 256', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (utf8)");
+
+    fresh_perl_like('pack "c10f1073741824"', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (multiply)");
+}
-- 
2.11.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 13, 2018

From @tonycoz

0001-perl-131844-fix-various-space-calculation-issues-in-.patch
From 14451983c134d2390238d8e77b15e90873c5b596 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 8 Aug 2017 09:32:58 +1000
Subject: [PATCH] (perl #131844) fix various space calculation issues in
 pp_pack.c

- for the originally reported case, if the start/cur pointer is in the
  top 75% of the address space the add (cur) + glen addition would
  overflow, resulting in the condition failing incorrectly.

- the addition of the existing space used to the space needed could
  overflow, resulting in too small an allocation and a buffer overflow.

- the scaling for UTF8 could overflow.

- the multiply to calculate the space needed for many items could
  overflow.

For the first case, do a space calculation without making new pointers.

For the other cases, detect the overflow and croak if there's an
overflow.
---
 pp_pack.c   | 25 +++++++++++++++++++++----
 t/op/pack.t | 24 +++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/pp_pack.c b/pp_pack.c
index 8937d6d715..12a850284d 100644
--- a/pp_pack.c
+++ b/pp_pack.c
@@ -357,11 +357,28 @@ STMT_START {							\
     }								\
 } STMT_END
 
+#define SAFE_UTF8_EXPAND(var)	\
+STMT_START {				\
+    if ((var) > Size_t_MAX / UTF8_EXPAND) \
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()"); \
+    (var) = (var) * UTF8_EXPAND; \
+} STMT_END
+
+#define GROWING2(utf8, cat, start, cur, item_size, item_count)	\
+STMT_START {							\
+    if (Size_t_MAX / (item_size) < (item_count))		\
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()");	\
+    GROWING((utf8), (cat), (start), (cur), (item_size) * (item_count)); \
+} STMT_END
+
 #define GROWING(utf8, cat, start, cur, in_len)	\
 STMT_START {					\
     STRLEN glen = (in_len);			\
-    if (utf8) glen *= UTF8_EXPAND;		\
-    if ((cur) + glen >= (start) + SvLEN(cat)) {	\
+    STRLEN catcur = (STRLEN)((cur) - (start));	\
+    if (utf8) SAFE_UTF8_EXPAND(glen);		\
+    if (Size_t_MAX - glen < catcur)		\
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()"); \
+    if (catcur + glen >= SvLEN(cat)) {	\
 	(start) = sv_exp_grow(cat, glen);	\
 	(cur) = (start) + SvCUR(cat);		\
     }						\
@@ -371,7 +388,7 @@ STMT_START {					\
 STMT_START {					\
     const STRLEN glen = (in_len);		\
     STRLEN gl = glen;				\
-    if (utf8) gl *= UTF8_EXPAND;		\
+    if (utf8) SAFE_UTF8_EXPAND(gl);		\
     if ((cur) + gl >= (start) + SvLEN(cat)) {	\
         *cur = '\0';				\
         SvCUR_set((cat), (cur) - (start));	\
@@ -2131,7 +2148,7 @@ S_pack_rec(pTHX_ SV *cat, tempsym_t* symptr, SV **beglist, SV **endlist )
 	    if (props && !(props & PACK_SIZE_UNPREDICTABLE)) {
 		/* We can process this letter. */
 		STRLEN size = props & PACK_SIZE_MASK;
-		GROWING(utf8, cat, start, cur, (STRLEN) len * size);
+		GROWING2(utf8, cat, start, cur, size, (STRLEN)len);
 	    }
         }
 
diff --git a/t/op/pack.t b/t/op/pack.t
index 664aaaf1b0..439e74ee17 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 => 14713;
+plan tests => 14717;
 
 use strict;
 use warnings qw(FATAL all);
@@ -2059,3 +2059,25 @@ print pack("ucW", "0000", 0, 140737488355327) eq "\$,#`P,```\n\0\x{7fffffffffff}
  ? "ok\n" : "not ok\n";
 EOS
 }
+
+SKIP:
+{
+  # [perl #131844] pointer addition overflow
+    $Config{ptrsize} == 4
+      or skip "[perl #131844] need 32-bit build for this test", 4;
+    # prevent ASAN just crashing on the allocation failure
+    local $ENV{ASAN_OPTIONS} = $ENV{ASAN_OPTIONS};
+    $ENV{ASAN_OPTIONS} .= ",allocator_may_return_null=1";
+    fresh_perl_like('pack "f999999999"', qr/Out of memory!/, { stderr => 1 },
+		    "pointer addition overflow");
+
+    # integer (STRLEN) overflow from addition of glen to current length
+    fresh_perl_like('pack "c10f1073741823"', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (addition)");
+
+    fresh_perl_like('pack "W10f536870913", 256', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (utf8)");
+
+    fresh_perl_like('pack "c10f1073741824"', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (multiply)");
+}
-- 
2.11.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 24, 2018

From @xsawyerx

The public disclosure date is set for March 15th.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 25, 2018

From @khwilliamson

On 02/24/2018 09​:56 AM, Sawyer X wrote​:

The public disclosure date is set for March 15th.

I believe this indicates that we are committing to put out maintenance
releases on that date. And that would imply that everyone needs to be
encouraged to nominate and vote on patches that should go into these
releases.

Correct?

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 25, 2018

From @xsawyerx

On 25 February 2018 at 19​:49, Karl Williamson <public@​khwilliamson.com>
wrote​:

On 02/24/2018 09​:56 AM, Sawyer X wrote​:

The public disclosure date is set for March 15th.

I believe this indicates that we are committing to put out maintenance
releases on that date. And that would imply that everyone needs to be
encouraged to nominate and vote on patches that should go into these
releases.

Correct?

I believe so. Steve?

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 25, 2018

From @Tux

On Sun, 25 Feb 2018 20​:16​:25 +0200, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 25 February 2018 at 19​:49, Karl Williamson <public@​khwilliamson.com>
wrote​:

On 02/24/2018 09​:56 AM, Sawyer X wrote​:

The public disclosure date is set for March 15th.

I believe this indicates that we are committing to put out maintenance
releases on that date. And that would imply that everyone needs to be
encouraged to nominate and vote on patches that should go into these
releases.

Correct?

I believe so. Steve?

Are CVE's not exempt from voting?

--
H.Merijn Brand http​://tux.nl Perl Monger http​://amsterdam.pm.org/
using perl5.00307 .. 5.27 porting perl5 on HP-UX, AIX, and openSUSE
http​://mirrors.develooper.com/hpux/ http​://www.test-smoke.org/
http​://qa.perl.org http​://www.goldmark.org/jeff/stupid-disclaimers/

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 25, 2018

From @xsawyerx

CVEs should be back-ported. I don't know of any objection to this in the
past.

On 25 February 2018 at 20​:24, H.Merijn Brand <h.m.brand@​xs4all.nl> wrote​:

On Sun, 25 Feb 2018 20​:16​:25 +0200, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 25 February 2018 at 19​:49, Karl Williamson <public@​khwilliamson.com>
wrote​:

On 02/24/2018 09​:56 AM, Sawyer X wrote​:

The public disclosure date is set for March 15th.

I believe this indicates that we are committing to put out maintenance
releases on that date. And that would imply that everyone needs to be
encouraged to nominate and vote on patches that should go into these
releases.

Correct?

I believe so. Steve?

Are CVE's not exempt from voting?

--
H.Merijn Brand http​://tux.nl Perl Monger http​://amsterdam.pm.org/
using perl5.00307 .. 5.27 porting perl5 on HP-UX, AIX, and openSUSE
http​://mirrors.develooper.com/hpux/ http​://www.test-smoke.org/
http​://qa.perl.org http​://www.goldmark.org/jeff/stupid-disclaimers/

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 26, 2018

From @steve-m-hay

On 25 February 2018 at 18​:16, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 25 February 2018 at 19​:49, Karl Williamson <public@​khwilliamson.com>
wrote​:

On 02/24/2018 09​:56 AM, Sawyer X wrote​:

The public disclosure date is set for March 15th.

I believe this indicates that we are committing to put out maintenance
releases on that date. And that would imply that everyone needs to be
encouraged to nominate and vote on patches that should go into these
releases.

Correct?

I believe so. Steve?

Yes. I will start committing things that are already voted on soon
(except for the CVE patches themselves? they might be best left until
nearer the date) and go through everything since 5.26.0 to look for
good candidates to back-port. I meant to have started on this already
but life (and death) has intervened. I will certainly get the ball
rolling in the next couple of days.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 1, 2018

From @steve-m-hay

On 26 February 2018 at 16​:01, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

On 25 February 2018 at 18​:16, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 25 February 2018 at 19​:49, Karl Williamson <public@​khwilliamson.com>
wrote​:

On 02/24/2018 09​:56 AM, Sawyer X wrote​:

The public disclosure date is set for March 15th.

I believe this indicates that we are committing to put out maintenance
releases on that date. And that would imply that everyone needs to be
encouraged to nominate and vote on patches that should go into these
releases.

Correct?

I believe so. Steve?

Yes. I will start committing things that are already voted on soon
(except for the CVE patches themselves? they might be best left until
nearer the date) and go through everything since 5.26.0 to look for
good candidates to back-port. I meant to have started on this already
but life (and death) has intervened. I will certainly get the ball
rolling in the next couple of days.

This is a very tight timescale for a release which we also hope will
have a good bunch of other fixes in it as well.

I normally wait a fortnight after an RC before making a release final,
so on that basis I should be making RC1s today.

We could shorten the waiting period to maybe a week, but any less than
that doesn't sound sensible to me. That would give me a week to round
up some fixes and get RC1s out, which is still too soon to get many
fixes in.

The other approach might be to release RC1s on 15th March, but I don't
know if that's really acceptable? It certainly gets tricky if the
final releases are delayed by very much after that, which is
unpredictable.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 1, 2018

From @xsawyerx

I was contacted by a vendor requesting postponement. I will update once I
have more details.

On Mar 1, 2018 15​:46, "Steve Hay" <steve.m.hay@​googlemail.com> wrote​:

On 26 February 2018 at 16​:01, Steve Hay <steve.m.hay@​googlemail.com>
wrote​:

On 25 February 2018 at 18​:16, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 25 February 2018 at 19​:49, Karl Williamson <public@​khwilliamson.com>
wrote​:

On 02/24/2018 09​:56 AM, Sawyer X wrote​:

The public disclosure date is set for March 15th.

I believe this indicates that we are committing to put out maintenance
releases on that date. And that would imply that everyone needs to be
encouraged to nominate and vote on patches that should go into these
releases.

Correct?

I believe so. Steve?

Yes. I will start committing things that are already voted on soon
(except for the CVE patches themselves? they might be best left until
nearer the date) and go through everything since 5.26.0 to look for
good candidates to back-port. I meant to have started on this already
but life (and death) has intervened. I will certainly get the ball
rolling in the next couple of days.

This is a very tight timescale for a release which we also hope will
have a good bunch of other fixes in it as well.

I normally wait a fortnight after an RC before making a release final,
so on that basis I should be making RC1s today.

We could shorten the waiting period to maybe a week, but any less than
that doesn't sound sensible to me. That would give me a week to round
up some fixes and get RC1s out, which is still too soon to get many
fixes in.

The other approach might be to release RC1s on 15th March, but I don't
know if that's really acceptable? It certainly gets tricky if the
final releases are delayed by very much after that, which is
unpredictable.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 1, 2018

From @khwilliamson

On 03/01/2018 06​:49 AM, Sawyer X wrote​:

I was contacted by a vendor requesting postponement. I will update once
I have more details.

We should go through the security queue to see what else could be made
ready soon, even if that means postponing the date.

No one has responded to my query about what makes some things security
bugs, and somethings not. I am at a loss to know what to do about
#132055, even though I have a patch, as it is the same bug as the public
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132553
hence the same patch.

On Mar 1, 2018 15​:46, "Steve Hay" <steve.m.hay@​googlemail.com
<mailto​:steve.m.hay@​googlemail.com>> wrote​:

On 26 February 2018 at 16&#8203;:01\, Steve Hay \<steve\.m\.hay@&#8203;googlemail\.com
\<mailto&#8203;:steve\.m\.hay@&#8203;googlemail\.com>> wrote&#8203;:
 > On 25 February 2018 at 18&#8203;:16\, Sawyer X \<xsawyerx@&#8203;gmail\.com
\<mailto&#8203;:xsawyerx@&#8203;gmail\.com>> wrote&#8203;:
 >>
 >>
 >> On 25 February 2018 at 19&#8203;:49\, Karl Williamson
\<public@&#8203;khwilliamson\.com \<mailto&#8203;:public@&#8203;khwilliamson\.com>>
 >> wrote&#8203;:
 >>>
 >>> On 02/24/2018 09&#8203;:56 AM\, Sawyer X wrote&#8203;:
 >>>>
 >>>> The public disclosure date is set for March 15th\.
 >>>>
 >>>
 >>> I believe this indicates that we are committing to put out
maintenance
 >>> releases on that date\.  And that would imply that everyone
needs to be
 >>> encouraged to nominate and vote on patches that should go into
these
 >>> releases\.
 >>>
 >>> Correct?
 >>
 >>
 >> I believe so\. Steve?
 >
 > Yes\. I will start committing things that are already voted on soon
 > \(except for the CVE patches themselves? they might be best left until
 > nearer the date\) and go through everything since 5\.26\.0 to look for
 > good candidates to back\-port\. I meant to have started on this already
 > but life \(and death\) has intervened\. I will certainly get the ball
 > rolling in the next couple of days\.

This is a very tight timescale for a release which we also hope will
have a good bunch of other fixes in it as well\.

I normally wait a fortnight after an RC before making a release final\,
so on that basis I should be making RC1s today\.

We could shorten the waiting period to maybe a week\, but any less than
that doesn't sound sensible to me\. That would give me a week to round
up some fixes and get RC1s out\, which is still too soon to get many
fixes in\.

The other approach might be to release RC1s on 15th March\, but I don't
know if that's really acceptable? It certainly gets tricky if the
final releases are delayed by very much after that\, which is
unpredictable\.
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 1, 2018

From @tonycoz

On Thu, Mar 01, 2018 at 11​:13​:36AM -0700, Karl Williamson wrote​:

On 03/01/2018 06​:49 AM, Sawyer X wrote​:

I was contacted by a vendor requesting postponement. I will update once
I have more details.

We should go through the security queue to see what else could be made ready
soon, even if that means postponing the date.

No one has responded to my query about what makes some things security bugs,
and somethings not. I am at a loss to know what to do about #132055, even
though I have a patch, as it is the same bug as the public
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132553
hence the same patch.

We don't really define what we consider security bugs all that well.

If your web server crashed (with no other security implications) every
time an attacker sent a particular request, that could be considered a
denial of service attack, but we typically haven't considered simple
crash bugs as security issues in perl.

It would be nice to document what is and isn't a security issue, both
to speed up processing of non-security issues reported to the security
list and to (hopefully) reduce the number of non-security issues
reported as security issues.

Note that treating denial of service attacks as security issues
doesn't mean all crash bugs would be security issues, eg. stack not
refcounted bugs wouldn't be a security issue, since the code that
misbehaves generally misbehaves in some way whatever its input.

The other side of the problem is whether to treat potential security
issues reported to the public list as security issues, or inversely,
issues that have been reported as security issues that we would
otherwise have just fixed.

From my own point of view, dealing with security issues is a massive
pain, so if an issue hasn't been reported as a security issue, I
generally won't treat it as such. I occasionally feel some
professional guilt about this, but we all have more then enough to do.

However if an issue has been reported as such, I think we owe it to
the reporter, ourselves and the userbase to take it seriously.

The three issues in the current batch I think are ambiguously security
issues, two allow writing potentially user supplied data beyond the
end of a buffer while the other allows reading such data.

As to the rest of the tickets in the security list, they're hard to
decide on otherwise they'd be in the current batch or moved to the
public queue.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 4, 2018

From @tonycoz

On Thu, 01 Mar 2018 15​:01​:01 -0800, tonyc wrote​:

The three issues in the current batch I think are ambiguously security

Urr, *un*-ambiguously.

issues, two allow writing potentially user supplied data beyond the
end of a buffer while the other allows reading such data.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 5, 2018

From @tonycoz

On Thu, 01 Mar 2018 05​:46​:45 -0800, shay wrote​:

This is a very tight timescale for a release which we also hope will
have a good bunch of other fixes in it as well.

I normally wait a fortnight after an RC before making a release final,
so on that basis I should be making RC1s today.

We could shorten the waiting period to maybe a week, but any less than
that doesn't sound sensible to me. That would give me a week to round
up some fixes and get RC1s out, which is still too soon to get many
fixes in.

The other approach might be to release RC1s on 15th March, but I don't
know if that's really acceptable? It certainly gets tricky if the
final releases are delayed by very much after that, which is
unpredictable.

An alternative, which might be safer, might be to release 5.26.2 as 5.26.1 plus only the security related fixes, and similarly for 5.24.4.

This would mean that we're (or mostly you) aren't rushing to integrate all of the voted on changes.

It would mean that 5.26.2 and 5.24.4 wouldn't be on their corresponding maint-* branches, but I think it would be less risky and stressful than setting a fixed target date for 5.26.1 + various changes + security fixes.

It does mean the maint releaser is going through an extra release process for each branch, but it should be relatively simpler than a release with a more extensive set of changes.

If we do separate security releases, we might treat them as outside the maint releaser's (volunteer anyway) responsibility, so that doing a security release might be done by someone else (probably me in this case.)

Of course, the changes in the security releases would still need to be merged into the maint-* branches, but it would reduce the time stress for normal maint releases.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 5, 2018

From @tonycoz

On Tue, 13 Feb 2018 15​:47​:49 -0800, tonyc wrote​:

On Mon, 07 Aug 2017 18​:34​:44 -0700, tonyc wrote​:

Going through the code I found three other issues, and added tests
and
fixes for those too, per the attached patch.

There were two problems (the email address and the skip count), fixed
in the attached.

The non-5.24 patch applies to both blead and maint-5.26, the other to
5.24.

Niko Tyni pointed out the first test fails on -DDEBUGGING builts on 32-bit systems, since realloc() (in this case) panics on allocations over 2GB (on 32-bit systems) rather than failing, changing the error message.

To prevent this the macros in pp_pack.c in the attached patches limit the allocation size to half the address space, rather than all of the address space.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 5, 2018

From @tonycoz

5.24-0001-perl-131844-fix-various-space-calculation-issues-in-.patch
From c3d9db7eb4dc1747fff423ebaf0c1bcd62c2e8a9 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 8 Aug 2017 09:32:58 +1000
Subject: (perl #131844) fix various space calculation issues in pp_pack.c

- for the originally reported case, if the start/cur pointer is in the
  top 75% of the address space the add (cur) + glen addition would
  overflow, resulting in the condition failing incorrectly.

- the addition of the existing space used to the space needed could
  overflow, resulting in too small an allocation and a buffer overflow.

- the scaling for UTF8 could overflow.

- the multiply to calculate the space needed for many items could
  overflow.

For the first case, do a space calculation without making new pointers.

For the other cases, detect the overflow and croak if there's an
overflow.

Originally this used Size_t_MAX as the maximum size of a memory
allocation, but for -DDEBUGGING builds realloc() throws a panic for
allocations over half the address space in size, changing the error
reported for the allocation.

For non-DEBUGGING builds the Size_t_MAX limit has the small chance
of finding a system that has 3GB of contiguous space available, and
allocating that space, which could be a denial of servce in some cases.

Unfortunately changing the limit to half the address space means that
the exact case with the original issue can no longer occur, so the
test is no longer testing against the address + length issue that
caused the original problem, since the allocation is failing earlier.

One option would be to change the test so the size request by pack is
just under 2GB, but this has a higher (but still low) probability that
the system has the address space available, and will actually try to
allocate the memory, so let's not do that.
---
 pp_pack.c   | 25 +++++++++++++++++++++----
 t/op/pack.t | 24 +++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/pp_pack.c b/pp_pack.c
index f6964c3f30..c0de5ab82b 100644
--- a/pp_pack.c
+++ b/pp_pack.c
@@ -358,11 +358,28 @@ STMT_START {							\
     }								\
 } STMT_END
 
+#define SAFE_UTF8_EXPAND(var)	\
+STMT_START {				\
+    if ((var) > SSize_t_MAX / UTF8_EXPAND) \
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()"); \
+    (var) = (var) * UTF8_EXPAND; \
+} STMT_END
+
+#define GROWING2(utf8, cat, start, cur, item_size, item_count)	\
+STMT_START {							\
+    if (SSize_t_MAX / (item_size) < (item_count))		\
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()");	\
+    GROWING((utf8), (cat), (start), (cur), (item_size) * (item_count)); \
+} STMT_END
+
 #define GROWING(utf8, cat, start, cur, in_len)	\
 STMT_START {					\
     STRLEN glen = (in_len);			\
-    if (utf8) glen *= UTF8_EXPAND;		\
-    if ((cur) + glen >= (start) + SvLEN(cat)) {	\
+    STRLEN catcur = (STRLEN)((cur) - (start));	\
+    if (utf8) SAFE_UTF8_EXPAND(glen);		\
+    if (SSize_t_MAX - glen < catcur)		\
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()"); \
+    if (catcur + glen >= SvLEN(cat)) {	\
 	(start) = sv_exp_grow(cat, glen);	\
 	(cur) = (start) + SvCUR(cat);		\
     }						\
@@ -372,7 +389,7 @@ STMT_START {					\
 STMT_START {					\
     const STRLEN glen = (in_len);		\
     STRLEN gl = glen;				\
-    if (utf8) gl *= UTF8_EXPAND;		\
+    if (utf8) SAFE_UTF8_EXPAND(gl);		\
     if ((cur) + gl >= (start) + SvLEN(cat)) {	\
         *cur = '\0';				\
         SvCUR_set((cat), (cur) - (start));	\
@@ -2126,7 +2143,7 @@ S_pack_rec(pTHX_ SV *cat, tempsym_t* symptr, SV **beglist, SV **endlist )
 	    if (props && !(props & PACK_SIZE_UNPREDICTABLE)) {
 		/* We can process this letter. */
 		STRLEN size = props & PACK_SIZE_MASK;
-		GROWING(utf8, cat, start, cur, (STRLEN) len * size);
+		GROWING2(utf8, cat, start, cur, size, (STRLEN)len);
 	    }
         }
 
diff --git a/t/op/pack.t b/t/op/pack.t
index a2da63689b..a480c3ad7f 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 => 14712;
+plan tests => 14716;
 
 use strict;
 use warnings qw(FATAL all);
@@ -2044,3 +2044,25 @@ ok(1, "argument underflow did not crash");
     is(pack("H40", $up_nul), $twenty_nuls,
        "check pack H zero fills (utf8 source)");
 }
+
+SKIP:
+{
+  # [perl #131844] pointer addition overflow
+    $Config{ptrsize} == 4
+      or skip "[perl #131844] need 32-bit build for this test", 4;
+    # prevent ASAN just crashing on the allocation failure
+    local $ENV{ASAN_OPTIONS} = $ENV{ASAN_OPTIONS};
+    $ENV{ASAN_OPTIONS} .= ",allocator_may_return_null=1";
+    fresh_perl_like('pack "f999999999"', qr/Out of memory during pack/, { stderr => 1 },
+		    "pointer addition overflow");
+
+    # integer (STRLEN) overflow from addition of glen to current length
+    fresh_perl_like('pack "c10f1073741823"', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (addition)");
+
+    fresh_perl_like('pack "W10f536870913", 256', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (utf8)");
+
+    fresh_perl_like('pack "c10f1073741824"', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (multiply)");
+}
-- 
2.11.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 5, 2018

From @tonycoz

blead-0001-perl-131844-fix-various-space-calculation-issues-in-.patch
From 4fe5c257e1cc53bec43f8dffea6f93c7e2597b37 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 8 Aug 2017 09:32:58 +1000
Subject: (perl #131844) fix various space calculation issues in pp_pack.c

- for the originally reported case, if the start/cur pointer is in the
  top 75% of the address space the add (cur) + glen addition would
  overflow, resulting in the condition failing incorrectly.

- the addition of the existing space used to the space needed could
  overflow, resulting in too small an allocation and a buffer overflow.

- the scaling for UTF8 could overflow.

- the multiply to calculate the space needed for many items could
  overflow.

For the first case, do a space calculation without making new pointers.

For the other cases, detect the overflow and croak if there's an
overflow.

Originally this used Size_t_MAX as the maximum size of a memory
allocation, but for -DDEBUGGING builds realloc() throws a panic for
allocations over half the address space in size, changing the error
reported for the allocation.

For non-DEBUGGING builds the Size_t_MAX limit has the small chance
of finding a system that has 3GB of contiguous space available, and
allocating that space, which could be a denial of servce in some cases.

Unfortunately changing the limit to half the address space means that
the exact case with the original issue can no longer occur, so the
test is no longer testing against the address + length issue that
caused the original problem, since the allocation is failing earlier.

One option would be to change the test so the size request by pack is
just under 2GB, but this has a higher (but still low) probability that
the system has the address space available, and will actually try to
allocate the memory, so let's not do that.
---
 pp_pack.c   | 25 +++++++++++++++++++++----
 t/op/pack.t | 24 +++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/pp_pack.c b/pp_pack.c
index 8937d6d715..5e9cc64301 100644
--- a/pp_pack.c
+++ b/pp_pack.c
@@ -357,11 +357,28 @@ STMT_START {							\
     }								\
 } STMT_END
 
+#define SAFE_UTF8_EXPAND(var)	\
+STMT_START {				\
+    if ((var) > SSize_t_MAX / UTF8_EXPAND) \
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()"); \
+    (var) = (var) * UTF8_EXPAND; \
+} STMT_END
+
+#define GROWING2(utf8, cat, start, cur, item_size, item_count)	\
+STMT_START {							\
+    if (SSize_t_MAX / (item_size) < (item_count))		\
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()");	\
+    GROWING((utf8), (cat), (start), (cur), (item_size) * (item_count)); \
+} STMT_END
+
 #define GROWING(utf8, cat, start, cur, in_len)	\
 STMT_START {					\
     STRLEN glen = (in_len);			\
-    if (utf8) glen *= UTF8_EXPAND;		\
-    if ((cur) + glen >= (start) + SvLEN(cat)) {	\
+    STRLEN catcur = (STRLEN)((cur) - (start));	\
+    if (utf8) SAFE_UTF8_EXPAND(glen);		\
+    if (SSize_t_MAX - glen < catcur)		\
+        Perl_croak(aTHX_ "%s", "Out of memory during pack()"); \
+    if (catcur + glen >= SvLEN(cat)) {	\
 	(start) = sv_exp_grow(cat, glen);	\
 	(cur) = (start) + SvCUR(cat);		\
     }						\
@@ -371,7 +388,7 @@ STMT_START {					\
 STMT_START {					\
     const STRLEN glen = (in_len);		\
     STRLEN gl = glen;				\
-    if (utf8) gl *= UTF8_EXPAND;		\
+    if (utf8) SAFE_UTF8_EXPAND(gl);		\
     if ((cur) + gl >= (start) + SvLEN(cat)) {	\
         *cur = '\0';				\
         SvCUR_set((cat), (cur) - (start));	\
@@ -2131,7 +2148,7 @@ S_pack_rec(pTHX_ SV *cat, tempsym_t* symptr, SV **beglist, SV **endlist )
 	    if (props && !(props & PACK_SIZE_UNPREDICTABLE)) {
 		/* We can process this letter. */
 		STRLEN size = props & PACK_SIZE_MASK;
-		GROWING(utf8, cat, start, cur, (STRLEN) len * size);
+		GROWING2(utf8, cat, start, cur, size, (STRLEN)len);
 	    }
         }
 
diff --git a/t/op/pack.t b/t/op/pack.t
index 664aaaf1b0..cf0e286509 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 => 14713;
+plan tests => 14717;
 
 use strict;
 use warnings qw(FATAL all);
@@ -2059,3 +2059,25 @@ print pack("ucW", "0000", 0, 140737488355327) eq "\$,#`P,```\n\0\x{7fffffffffff}
  ? "ok\n" : "not ok\n";
 EOS
 }
+
+SKIP:
+{
+  # [perl #131844] pointer addition overflow
+    $Config{ptrsize} == 4
+      or skip "[perl #131844] need 32-bit build for this test", 4;
+    # prevent ASAN just crashing on the allocation failure
+    local $ENV{ASAN_OPTIONS} = $ENV{ASAN_OPTIONS};
+    $ENV{ASAN_OPTIONS} .= ",allocator_may_return_null=1";
+    fresh_perl_like('pack "f999999999"', qr/Out of memory during pack/, { stderr => 1 },
+		    "pointer addition overflow");
+
+    # integer (STRLEN) overflow from addition of glen to current length
+    fresh_perl_like('pack "c10f1073741823"', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (addition)");
+
+    fresh_perl_like('pack "W10f536870913", 256', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (utf8)");
+
+    fresh_perl_like('pack "c10f1073741824"', qr/Out of memory during pack/, { stderr => 1 },
+		    "integer overflow calculating allocation (multiply)");
+}
-- 
2.11.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 5, 2018

From @steve-m-hay

On 5 March 2018 at 05​:25, Tony Cook via RT
<perl5-security-report-followup@​perl.org> wrote​:

On Thu, 01 Mar 2018 05​:46​:45 -0800, shay wrote​:

This is a very tight timescale for a release which we also hope will
have a good bunch of other fixes in it as well.

I normally wait a fortnight after an RC before making a release final,
so on that basis I should be making RC1s today.

We could shorten the waiting period to maybe a week, but any less than
that doesn't sound sensible to me. That would give me a week to round
up some fixes and get RC1s out, which is still too soon to get many
fixes in.

The other approach might be to release RC1s on 15th March, but I don't
know if that's really acceptable? It certainly gets tricky if the
final releases are delayed by very much after that, which is
unpredictable.

An alternative, which might be safer, might be to release 5.26.2 as 5.26.1 plus only the security related fixes, and similarly for 5.24.4.

This would mean that we're (or mostly you) aren't rushing to integrate all of the voted on changes.

It would mean that 5.26.2 and 5.24.4 wouldn't be on their corresponding maint-* branches, but I think it would be less risky and stressful than setting a fixed target date for 5.26.1 + various changes + security fixes.

It does mean the maint releaser is going through an extra release process for each branch, but it should be relatively simpler than a release with a more extensive set of changes.

If we do separate security releases, we might treat them as outside the maint releaser's (volunteer anyway) responsibility, so that doing a security release might be done by someone else (probably me in this case.)

Of course, the changes in the security releases would still need to be merged into the maint-* branches, but it would reduce the time stress for normal maint releases.

I asked perl5-security on 6 Feb whether to do security-only releases
or get a good bunch of other fixes in too, and the consensus was the
latter. I've since committed those fixes that already had votes, and
have started looking through the long list of commits since 5.26.0
(not since 5.26.1, because 5.26.1 was itself security-only) for other
candidates, though I have been too slow in doing this :-( (I'm only
talking about 5.26 here. For 5.24 I was only ever intending the
security fixes (plus anything already voted on) anyway.)

But then a proposed release date of 15 Mar came up, making that look
impossible, although the last I heard (1 Mar, on this ticket) Sawyer
was considering a postponement at some vendor's request.

However, unless it's quite a long postponement then a full 5.26.2 is
probably still too ambitious.

So I'd be happy with largely security-only 5.26.2/5.24.4 releases, but
it's probably simpler if they're done from the maint-5.26/24 branches
as usual - they're already updated with all the mundane stuff and are
in a good state to go, needing only the security fixes themselves plus
perldelta updates.

The main problem is that we'll have no time left after this to get a
full 5.26.3 out before 5.28.0 - we can't have both releases being made
in parallel because it's too much to ask people to be testing
thoroughly all at once.

So then we'll have gone through the entire 5.26 cycle with no full
bug-fix release -- which is exactly what happened with 5.24 :-/

It seems like every time I plan on doing a maint release it gets
derailed by a sudden urgent need for a security release. I don't know
what the answer is here. A second person producing these security-only
maint releases is fine with me, but it doesn't get us any extra time.
We still need to get all the releases made one after another
in-between successive .0 releases. In this case it would just mean
that my planned 5.26.2 gets postponed while the security-only 5.26.2
is made, but we're still left with no time for 5.26.3 afterwards.
(5.28.0 is slated for 20th April! If it's really going to make that
date then presumably RC1 will be about a month from now.)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 5, 2018

From @xsawyerx

On 5 March 2018 at 10​:51, Steve Hay via perl5-security-report
<perl5-security-report@​perl.org> wrote​:

On 5 March 2018 at 05​:25, Tony Cook via RT
<perl5-security-report-followup@​perl.org> wrote​:

On Thu, 01 Mar 2018 05​:46​:45 -0800, shay wrote​:

This is a very tight timescale for a release which we also hope will
have a good bunch of other fixes in it as well.

I normally wait a fortnight after an RC before making a release final,
so on that basis I should be making RC1s today.

We could shorten the waiting period to maybe a week, but any less than
that doesn't sound sensible to me. That would give me a week to round
up some fixes and get RC1s out, which is still too soon to get many
fixes in.

The other approach might be to release RC1s on 15th March, but I don't
know if that's really acceptable? It certainly gets tricky if the
final releases are delayed by very much after that, which is
unpredictable.

An alternative, which might be safer, might be to release 5.26.2 as 5.26.1 plus only the security related fixes, and similarly for 5.24.4.

This would mean that we're (or mostly you) aren't rushing to integrate all of the voted on changes.

It would mean that 5.26.2 and 5.24.4 wouldn't be on their corresponding maint-* branches, but I think it would be less risky and stressful than setting a fixed target date for 5.26.1 + various changes + security fixes.

It does mean the maint releaser is going through an extra release process for each branch, but it should be relatively simpler than a release with a more extensive set of changes.

If we do separate security releases, we might treat them as outside the maint releaser's (volunteer anyway) responsibility, so that doing a security release might be done by someone else (probably me in this case.)

Of course, the changes in the security releases would still need to be merged into the maint-* branches, but it would reduce the time stress for normal maint releases.

[...]

But then a proposed release date of 15 Mar came up, making that look
impossible, although the last I heard (1 Mar, on this ticket) Sawyer
was considering a postponement at some vendor's request.

I'm still waiting to hear from the vendor on their proposed date. I
pinged them again this morning.

I will postpone it anyway. I'm just not sure for how long. See next comment.

[...]

It seems like every time I plan on doing a maint release it gets
derailed by a sudden urgent need for a security release. I don't know
what the answer is here.

Security tickets will come up. I think the answer is for me to not
just throw a date but instead confer with you to understand what would
be the right time. Vendors are all too happy leaving it for later, and
if we do it to match release efforts we are making it sustainable.

Please accept my apology, and with this next set of patches, once I
know what is the date the vendor wishes, I will loop you in for us to
decide on a date together. Sounds good?

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 7, 2018

From @steve-m-hay

On 5 March 2018 at 13​:02, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 5 March 2018 at 10​:51, Steve Hay via perl5-security-report
<perl5-security-report@​perl.org> wrote​:

On 5 March 2018 at 05​:25, Tony Cook via RT
<perl5-security-report-followup@​perl.org> wrote​:

On Thu, 01 Mar 2018 05​:46​:45 -0800, shay wrote​:

This is a very tight timescale for a release which we also hope will
have a good bunch of other fixes in it as well.

I normally wait a fortnight after an RC before making a release final,
so on that basis I should be making RC1s today.

We could shorten the waiting period to maybe a week, but any less than
that doesn't sound sensible to me. That would give me a week to round
up some fixes and get RC1s out, which is still too soon to get many
fixes in.

The other approach might be to release RC1s on 15th March, but I don't
know if that's really acceptable? It certainly gets tricky if the
final releases are delayed by very much after that, which is
unpredictable.

An alternative, which might be safer, might be to release 5.26.2 as 5.26.1 plus only the security related fixes, and similarly for 5.24.4.

This would mean that we're (or mostly you) aren't rushing to integrate all of the voted on changes.

It would mean that 5.26.2 and 5.24.4 wouldn't be on their corresponding maint-* branches, but I think it would be less risky and stressful than setting a fixed target date for 5.26.1 + various changes + security fixes.

It does mean the maint releaser is going through an extra release process for each branch, but it should be relatively simpler than a release with a more extensive set of changes.

If we do separate security releases, we might treat them as outside the maint releaser's (volunteer anyway) responsibility, so that doing a security release might be done by someone else (probably me in this case.)

Of course, the changes in the security releases would still need to be merged into the maint-* branches, but it would reduce the time stress for normal maint releases.

[...]

But then a proposed release date of 15 Mar came up, making that look
impossible, although the last I heard (1 Mar, on this ticket) Sawyer
was considering a postponement at some vendor's request.

I'm still waiting to hear from the vendor on their proposed date. I
pinged them again this morning.

I will postpone it anyway. I'm just not sure for how long. See next comment.

[...]

It seems like every time I plan on doing a maint release it gets
derailed by a sudden urgent need for a security release. I don't know
what the answer is here.

Security tickets will come up. I think the answer is for me to not
just throw a date but instead confer with you to understand what would
be the right time. Vendors are all too happy leaving it for later, and
if we do it to match release efforts we are making it sustainable.

Please accept my apology, and with this next set of patches, once I
know what is the date the vendor wishes, I will loop you in for us to
decide on a date together. Sounds good?

Sorry for not replying sooner. Yes, that sounds good to me.

I've now pushed an update to the 5.26 voting file containing my list
of candidates for cherry-picking. (I'm not proposing anything other
than security fixes for 5.24.)

Hopefully the list is sufficiently short that we can get these done,
together with the security fixes, in time for whatever the new
proposed date is -- and still leaving time for 5.28.0 to arrive on
April 20th (or at least fairly soon after).

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 19, 2018

From @xsawyerx

The disclosure date has been postponed officially to April 14th.

On 7 March 2018 at 20​:10, Steve Hay <steve.m.hay@​googlemail.com> wrote​:

On 5 March 2018 at 13​:02, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 5 March 2018 at 10​:51, Steve Hay via perl5-security-report
<perl5-security-report@​perl.org> wrote​:

On 5 March 2018 at 05​:25, Tony Cook via RT
<perl5-security-report-followup@​perl.org> wrote​:

On Thu, 01 Mar 2018 05​:46​:45 -0800, shay wrote​:

This is a very tight timescale for a release which we also hope will
have a good bunch of other fixes in it as well.

I normally wait a fortnight after an RC before making a release final,
so on that basis I should be making RC1s today.

We could shorten the waiting period to maybe a week, but any less than
that doesn't sound sensible to me. That would give me a week to round
up some fixes and get RC1s out, which is still too soon to get many
fixes in.

The other approach might be to release RC1s on 15th March, but I don't
know if that's really acceptable? It certainly gets tricky if the
final releases are delayed by very much after that, which is
unpredictable.

An alternative, which might be safer, might be to release 5.26.2 as 5.26.1 plus only the security related fixes, and similarly for 5.24.4.

This would mean that we're (or mostly you) aren't rushing to integrate all of the voted on changes.

It would mean that 5.26.2 and 5.24.4 wouldn't be on their corresponding maint-* branches, but I think it would be less risky and stressful than setting a fixed target date for 5.26.1 + various changes + security fixes.

It does mean the maint releaser is going through an extra release process for each branch, but it should be relatively simpler than a release with a more extensive set of changes.

If we do separate security releases, we might treat them as outside the maint releaser's (volunteer anyway) responsibility, so that doing a security release might be done by someone else (probably me in this case.)

Of course, the changes in the security releases would still need to be merged into the maint-* branches, but it would reduce the time stress for normal maint releases.

[...]

But then a proposed release date of 15 Mar came up, making that look
impossible, although the last I heard (1 Mar, on this ticket) Sawyer
was considering a postponement at some vendor's request.

I'm still waiting to hear from the vendor on their proposed date. I
pinged them again this morning.

I will postpone it anyway. I'm just not sure for how long. See next comment.

[...]

It seems like every time I plan on doing a maint release it gets
derailed by a sudden urgent need for a security release. I don't know
what the answer is here.

Security tickets will come up. I think the answer is for me to not
just throw a date but instead confer with you to understand what would
be the right time. Vendors are all too happy leaving it for later, and
if we do it to match release efforts we are making it sustainable.

Please accept my apology, and with this next set of patches, once I
know what is the date the vendor wishes, I will loop you in for us to
decide on a date together. Sounds good?

Sorry for not replying sooner. Yes, that sounds good to me.

I've now pushed an update to the 5.26 voting file containing my list
of candidates for cherry-picking. (I'm not proposing anything other
than security fixes for 5.24.)

Hopefully the list is sufficiently short that we can get these done,
together with the security fixes, in time for whatever the new
proposed date is -- and still leaving time for 5.28.0 to arrive on
April 20th (or at least fairly soon after).

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 14, 2018

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 14, 2018

From @xsawyerx

On Mon, 19 Mar 2018 15​:26​:25 -0700, xsawyerx@​gmail.com wrote​:

The disclosure date has been postponed officially to April 14th.

Moved to public queue.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 16, 2018

From @tonycoz

On Sat, 14 Apr 2018 07​:35​:39 -0700, xsawyerx@​cpan.org wrote​:

On Mon, 19 Mar 2018 15​:26​:25 -0700, xsawyerx@​gmail.com wrote​:

The disclosure date has been postponed officially to April 14th.

Moved to public queue.

Fix for blead pushed as f17fed5.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator 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

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 23, 2018

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

@p5pRT p5pRT closed this Jun 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.