[PATCH] Stack overflow in Storable retrieve_hook #15831
Comments
From @lightseyCreated by @lightseyThis is a bug report for perl from john@nixnuts.net, ----------------------------------------------------------------- The problem essentially is that a hook's classname length is read into Perl Info |
From @lightsey0001-Fix-stack-buffer-overflow-in-deserialization-of-hook.patchFrom 819dc7bc9ce600ef0bb505392ed949a9b6b9bc91 Mon Sep 17 00:00:00 2001
From: John Lightsey <jd@cpanel.net>
Date: Tue, 24 Jan 2017 10:30:18 -0600
Subject: [PATCH] Fix stack buffer overflow in deserialization of hooks.
The use of signed lengths resulted in a stack overflow in retrieve_hook()
when a negative length was provided in the storable data.
The retrieve_blessed() codepath had a similar problem with the placement
of the trailing null byte when negative lengths were provided.
---
dist/Storable/Storable.xs | 4 ++--
dist/Storable/t/store.t | 24 +++++++++++++++++++++++-
2 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs
index a72d84cbf5..ba5f3503da 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -4048,7 +4048,7 @@ static SV *retrieve_idx_blessed(pTHX_ stcxt_t *cxt, const char *cname)
*/
static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname)
{
- I32 len;
+ U32 len;
SV *sv;
char buf[LG_BLESS + 1]; /* Avoid malloc() if possible */
char *classname = buf;
@@ -4119,7 +4119,7 @@ static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname)
*/
static SV *retrieve_hook(pTHX_ stcxt_t *cxt, const char *cname)
{
- I32 len;
+ U32 len;
char buf[LG_BLESS + 1]; /* Avoid malloc() if possible */
char *classname = buf;
unsigned int flags;
diff --git a/dist/Storable/t/store.t b/dist/Storable/t/store.t
index 3a4b9dca8a..8c07828ed5 100644
--- a/dist/Storable/t/store.t
+++ b/dist/Storable/t/store.t
@@ -19,7 +19,7 @@ sub BEGIN {
use Storable qw(store retrieve store_fd nstore_fd fd_retrieve);
-use Test::More tests => 24;
+use Test::More tests => 25;
$a = 'toto';
$b = \$a;
@@ -101,5 +101,27 @@ isnt($@, '');
}
}
+{
+
+ my $pid = fork();
+ if ($pid) {
+ waitpid( $pid, 0 );
+ ok( $? == 0 || $? == 256, 'No stack smashing error when retieving hook' );
+ }
+ elsif ( defined $pid ) {
+ close STDOUT;
+ close STDERR;
+ my $frozen =
+ "\x70\x73\x74\x30\x04\x0a\x08\x31\x32\x33\x34\x35\x36\x37\x38\x04\x08\x08\x08\x03\x39\xff\xfa\x09\x19\x08\x17\x68\x08\x08\x08\x08\xf9\x16\x16\x13\x16\x10\x10\x10\xff\x15\x16\x16\x16\x1e\x16\x16\x16\x16\x16\x16\x16\x16\x16\x16\x13\xf0\x16\x16\x16\xfe\x16\x41\x41\x41\x41\xe8\x03\x41\x41\x41\x41\x41\x41\x41\x41\x51\x41\xa9\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xb8\xac\xac\xac\xac\xac\xac\xac\xac\x9a\xac\xac\xac\xac\xac\xac\xac\xac\xac\x93\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\x00\x64\xac\xa8\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\x2c\xac\x41\x41\x41\x41\x41\x41\x41\x41\x41\x00\x80\x41\x80\x41\x41\x41\x41\x41\x41\x51\x41\xac\xac\xac";
+ open my $fh, '<', \$frozen;
+ eval { Storable::fd_retrieve($fh); };
+ exit(0);
+ }
+ else {
+ die "Failed to fork: $!";
+ }
+
+}
+
close OUT or die "Could not close: $!";
END { 1 while unlink 'store' }
--
2.11.0
|
From @jkeenanOn Tue, 24 Jan 2017 19:22:28 GMT, john@nixnuts.net wrote:
Available for smoke-testing in this branch: smoke-me/jkeenan/130635-storable I corrected one spelling error in a test description and incremented the VERSION number. -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @jkeenanOn Wed, 25 Jan 2017 02:05:45 GMT, jkeenan wrote:
This revision failed on FreeBSD-11. See: http://perl5.test-smoke.org/report/53470 When I ran the test file individually, it hung at 'ok 24', then printed these error messages: ##### kernel: pid 18627 (perl), uid 1001, was killed: out of swap space So something is clearly amiss with this patch. Thank you very much. -- |
From @jkeenanOn Wed, 25 Jan 2017 04:04:15 GMT, jkeenan wrote:
Similar results on FreeBSD-10.3: |
From @jkeenanOn Wed, 25 Jan 2017 15:21:36 GMT, jkeenan wrote:
On (at least) FreeBSD-10.3, it appears that the test failure occurs when I configure with -DDEBUGGING: ##### # Failed test 'No stack smashing error when retrieving hook' In the same branch, at the same commit, configuring exactly the same except without -DDEBUGGING, the test file completes successfully. -- |
From @lightseyOn Wed, 2017-01-25 at 07:21 -0800, James E Keenan via RT wrote:
Makes sense.. When the bug is fixed, it ends up allocating an insanely large Maybe it's possible to limit the maximum allowed classname length to what Perl |
From @jkeenanOn Wed, 25 Jan 2017 15:42:21 GMT, jkeenan wrote:
On FreeBSD-11, I get the same difference in results between with/without -DDEBUGGING. On Linux, with -DDEBUGGING, the test file eventually completes ... but clearly hangs for some time after 'ok 21' and takes 30s to run. So the patch does not play well with -DDEBUGGING regardless of OS. Thank you very much. |
From @lightseyOn Wed, 2017-01-25 at 08:12 -0800, James E Keenan via RT wrote:
I'm attaching an updated patch that croaks before these oversized New() calls |
From @lightsey0001-Fix-stack-buffer-overflow-in-deserialization-of-hook.patchFrom 8ce5d6358ae296600b6ee2950e17f8fbbdfda586 Mon Sep 17 00:00:00 2001
From: John Lightsey <jd@cpanel.net>
Date: Tue, 24 Jan 2017 10:30:18 -0600
Subject: [PATCH] Fix stack buffer overflow in deserialization of hooks.
The use of signed lengths resulted in a stack overflow in retrieve_hook()
when a negative length was provided in the storable data.
The retrieve_blessed() codepath had a similar problem with the placement
of the trailing null byte when negative lengths were provided.
---
dist/Storable/Storable.pm | 2 +-
dist/Storable/Storable.xs | 11 +++++++++--
dist/Storable/t/store.t | 12 +++++++++++-
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm
index 397d584763..d8fd740eee 100644
--- a/dist/Storable/Storable.pm
+++ b/dist/Storable/Storable.pm
@@ -22,7 +22,7 @@ package Storable; @ISA = qw(Exporter);
use vars qw($canonical $forgive_me $VERSION);
-$VERSION = '2.61';
+$VERSION = '2.62';
BEGIN {
if (eval {
diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs
index a72d84cbf5..829c5d73ac 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -4048,7 +4048,7 @@ static SV *retrieve_idx_blessed(pTHX_ stcxt_t *cxt, const char *cname)
*/
static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname)
{
- I32 len;
+ U32 len;
SV *sv;
char buf[LG_BLESS + 1]; /* Avoid malloc() if possible */
char *classname = buf;
@@ -4069,6 +4069,9 @@ static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname)
if (len & 0x80) {
RLEN(len);
TRACEME(("** allocating %d bytes for class name", len+1));
+ if (len > I32_MAX) {
+ CROAK(("Corrupted classname length"));
+ }
New(10003, classname, len+1, char);
malloced_classname = classname;
}
@@ -4119,7 +4122,7 @@ static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname)
*/
static SV *retrieve_hook(pTHX_ stcxt_t *cxt, const char *cname)
{
- I32 len;
+ U32 len;
char buf[LG_BLESS + 1]; /* Avoid malloc() if possible */
char *classname = buf;
unsigned int flags;
@@ -4253,6 +4256,10 @@ static SV *retrieve_hook(pTHX_ stcxt_t *cxt, const char *cname)
else
GETMARK(len);
+ if (len > I32_MAX) {
+ CROAK(("Corrupted classname length"));
+ }
+
if (len > LG_BLESS) {
TRACEME(("** allocating %d bytes for class name", len+1));
New(10003, classname, len+1, char);
diff --git a/dist/Storable/t/store.t b/dist/Storable/t/store.t
index 3a4b9dca8a..d1ec55d60e 100644
--- a/dist/Storable/t/store.t
+++ b/dist/Storable/t/store.t
@@ -19,7 +19,7 @@ sub BEGIN {
use Storable qw(store retrieve store_fd nstore_fd fd_retrieve);
-use Test::More tests => 24;
+use Test::More tests => 25;
$a = 'toto';
$b = \$a;
@@ -101,5 +101,15 @@ isnt($@, '');
}
}
+{
+
+ my $frozen =
+ "\x70\x73\x74\x30\x04\x0a\x08\x31\x32\x33\x34\x35\x36\x37\x38\x04\x08\x08\x08\x03\x39\xff\xfa\x09\x19\x08\x17\x68\x08\x08\x08\x08\xf9\x16\x16\x13\x16\x10\x10\x10\xff\x15\x16\x16\x16\x1e\x16\x16\x16\x16\x16\x16\x16\x16\x16\x16\x13\xf0\x16\x16\x16\xfe\x16\x41\x41\x41\x41\xe8\x03\x41\x41\x41\x41\x41\x41\x41\x41\x51\x41\xa9\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xb8\xac\xac\xac\xac\xac\xac\xac\xac\x9a\xac\xac\xac\xac\xac\xac\xac\xac\xac\x93\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\x00\x64\xac\xa8\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\x2c\xac\x41\x41\x41\x41\x41\x41\x41\x41\x41\x00\x80\x41\x80\x41\x41\x41\x41\x41\x41\x51\x41\xac\xac\xac";
+ open my $fh, '<', \$frozen;
+ eval { Storable::fd_retrieve($fh); };
+ ok('RT 130635: No stack smashing error when retrieving hook');
+
+}
+
close OUT or die "Could not close: $!";
END { 1 while unlink 'store' }
--
2.11.0
|
From @jkeenanOn Wed, 25 Jan 2017 19:44:02 GMT, john@nixnuts.net wrote:
On Linux, with a -DDEBUGGING build, the runtime of the test came down to a reasonable length. However on both FreeBSD 10 and 11, the results under -DDEBUGGING were still unsatisfactory. Running the test manually on 10.3, I had to Ctrl-C the test after 2 minutes. Running smoke tests on 11, it appears that the test completed successfully once but then got the same swap errors previously reported, leading to the curious result of "PASS-so-far": |
From @lightseyOn Wed, 2017-01-25 at 14:31 -0800, James E Keenan via RT wrote:
I brought up a FreeBSD 10.3 AMD64 VM to test and saw a similar issue when I ran I didn't see any crashes on the FreeBSD VM when testing blead with the new ./Configure -des -DDEBUGGING -Dusedevel Is it possible that some of the failures you're seeing were caused by using the |
From @jkeenanOn Thu, 26 Jan 2017 21:02:12 GMT, john@nixnuts.net wrote:
I used the most recent version of this branch in the perl 5 repository: smoke-me/jkeenan/130635-storable ... which I believe incorporates your revised patch. As previously reported, I configure with: "-des -Dusedevel -Duseithreads -Doptimize='-O2 -pipe -fstack-protector -fno-strict-aliasing' -DDEBUGGING" ... because that gets us very close to the way that the FreeBSD port of perl is configured. Thank you very much. -- |
From @lightseyOn Thu, 2017-01-26 at 13:48 -0800, James E Keenan via RT wrote:
Excellent, thanks. The problem turned out to be that the AFL generated payload was hitting two I adjusted the test data to use more realistic sizes when it enters I also cleaned up the test output formatting a bit. An updated patch is attached. |
From @lightsey0001-Fix-stack-buffer-overflow-in-deserialization-of-hook.patchFrom f42cd5a57f1e28632f7761e29c84cb0c72acd299 Mon Sep 17 00:00:00 2001
From: John Lightsey <jd@cpanel.net>
Date: Tue, 24 Jan 2017 10:30:18 -0600
Subject: [PATCH] Fix stack buffer overflow in deserialization of hooks.
The use of signed lengths resulted in a stack overflow in retrieve_hook()
when a negative length was provided in the storable data.
The retrieve_blessed() codepath had a similar problem with the placement
of the trailing null byte when negative lengths were provided.
---
dist/Storable/Storable.pm | 2 +-
dist/Storable/Storable.xs | 11 +++++++++--
dist/Storable/t/store.t | 12 +++++++++++-
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm
index 397d584763..d8fd740eee 100644
--- a/dist/Storable/Storable.pm
+++ b/dist/Storable/Storable.pm
@@ -22,7 +22,7 @@ package Storable; @ISA = qw(Exporter);
use vars qw($canonical $forgive_me $VERSION);
-$VERSION = '2.61';
+$VERSION = '2.62';
BEGIN {
if (eval {
diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs
index a72d84cbf5..829c5d73ac 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -4048,7 +4048,7 @@ static SV *retrieve_idx_blessed(pTHX_ stcxt_t *cxt, const char *cname)
*/
static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname)
{
- I32 len;
+ U32 len;
SV *sv;
char buf[LG_BLESS + 1]; /* Avoid malloc() if possible */
char *classname = buf;
@@ -4069,6 +4069,9 @@ static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname)
if (len & 0x80) {
RLEN(len);
TRACEME(("** allocating %d bytes for class name", len+1));
+ if (len > I32_MAX) {
+ CROAK(("Corrupted classname length"));
+ }
New(10003, classname, len+1, char);
malloced_classname = classname;
}
@@ -4119,7 +4122,7 @@ static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname)
*/
static SV *retrieve_hook(pTHX_ stcxt_t *cxt, const char *cname)
{
- I32 len;
+ U32 len;
char buf[LG_BLESS + 1]; /* Avoid malloc() if possible */
char *classname = buf;
unsigned int flags;
@@ -4253,6 +4256,10 @@ static SV *retrieve_hook(pTHX_ stcxt_t *cxt, const char *cname)
else
GETMARK(len);
+ if (len > I32_MAX) {
+ CROAK(("Corrupted classname length"));
+ }
+
if (len > LG_BLESS) {
TRACEME(("** allocating %d bytes for class name", len+1));
New(10003, classname, len+1, char);
diff --git a/dist/Storable/t/store.t b/dist/Storable/t/store.t
index 3a4b9dca8a..b25dbd238f 100644
--- a/dist/Storable/t/store.t
+++ b/dist/Storable/t/store.t
@@ -19,7 +19,7 @@ sub BEGIN {
use Storable qw(store retrieve store_fd nstore_fd fd_retrieve);
-use Test::More tests => 24;
+use Test::More tests => 25;
$a = 'toto';
$b = \$a;
@@ -101,5 +101,15 @@ isnt($@, '');
}
}
+{
+
+ my $frozen =
+ "\x70\x73\x74\x30\x04\x0a\x08\x31\x32\x33\x34\x35\x36\x37\x38\x04\x08\x08\x08\x03\xff\x00\x00\x00\x19\x08\xff\x00\x00\x00\x08\x08\xf9\x16\x16\x13\x16\x10\x10\x10\xff\x15\x16\x16\x16\x1e\x16\x16\x16\x16\x16\x16\x16\x16\x16\x16\x13\xf0\x16\x16\x16\xfe\x16\x41\x41\x41\x41\xe8\x03\x41\x41\x41\x41\x41\x41\x41\x41\x51\x41\xa9\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xb8\xac\xac\xac\xac\xac\xac\xac\xac\x9a\xac\xac\xac\xac\xac\xac\xac\xac\xac\x93\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\x00\x64\xac\xa8\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\xac\x2c\xac\x41\x41\x41\x41\x41\x41\x41\x41\x41\x00\x80\x41\x80\x41\x41\x41\x41\x41\x41\x51\x41\xac\xac\xac";
+ open my $fh, '<', \$frozen;
+ eval { Storable::fd_retrieve($fh); };
+ pass('RT 130635: no stack smashing error when retrieving hook');
+
+}
+
close OUT or die "Could not close: $!";
END { 1 while unlink 'store' }
--
2.11.0
|
From @tonycozOn Sat, 28 Jan 2017 16:41:50 -0800, john@nixnuts.net wrote:
Won't the allocated buffer leak if the load_module() fails, no STORABLE_thaw is defined, or if STORABLE_thaw dies? Tony |
From @lightseyOn Sun, 2017-01-29 at 21:12 -0800, Tony Cook via RT wrote:
Yes, Storable has several codepaths where it leaks memory or crashes while |
From @jkeenanOn Sun, 29 Jan 2017 00:41:50 GMT, john@nixnuts.net wrote:
The third patch you submitted differs from the second patch submitted only in some test input data in t/store.t. You haven't changed any source code. Are you sure you're not simply fudging the test? Thank you very much. -- |
From @lightseyOn Mon, 2017-01-30 at 09:58 -0800, James E Keenan via RT wrote:
Yes, that's accurate. The original payload generated by AFL hit three different - memory allocation failure in retrieve_hash The retrieve_hash() crash, for instance, is essentially this code: RLEN(len); For some reason, FreeBSD with the flags you provided can't handle this when len My patch only fixes the stack overflow, so I changed the test payload to only |
From @tonycozOn Mon, 30 Jan 2017 09:33:44 -0800, john@nixnuts.net wrote:
There's no need to add more leaks though. You can use SAVEFREEPV() to free the allocated memory on scope exit. Tony |
From @lightseyOn Mon, 2017-01-30 at 14:17 -0800, Tony Cook via RT wrote:
It's not clear to me what memory you believe my patch is leaking that wasn't The two assertions I added are before the matching calls to New(). Which variable have I created a new memory leak on? |
From @tonycozOn Mon, 30 Jan 2017 14:53:44 -0800, john@nixnuts.net wrote:
You're right. Thanks, I've applied your patch as 3e998dd. I've fixed that specific leak in da1ec2b. Tony |
|
@tonycoz - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release today of Perl 5.26.0, this and 210 other issues have been Perl 5.26.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#130635 (status was 'resolved')
Searchable as RT130635$
The text was updated successfully, but these errors were encountered: