From c34e1dd29983e5d36d367462b9b4b4b8fcd5a0f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= Date: Mon, 6 Feb 2017 15:13:41 +0100 Subject: [PATCH] Fix stack buffer overflow in deserialization of hooks. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ported from perl: commit 3e998ddfb597cfae7bdb460b22e6c50440b1de92 Author: John Lightsey Date: Tue Jan 24 10:30:18 2017 -0600 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. Signed-off-by: Petr Písař --- Storable.xs | 11 +++++++++-- t/store.t | 12 +++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/Storable.xs b/Storable.xs index bc15d1d..3cce3ed 100644 --- a/Storable.xs +++ b/Storable.xs @@ -4016,7 +4016,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; @@ -4037,6 +4037,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; } @@ -4087,7 +4090,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; @@ -4221,6 +4224,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/t/store.t b/t/store.t index be43299..1cbf021 100644 --- a/t/store.t +++ b/t/store.t @@ -19,7 +19,7 @@ sub BEGIN { use Storable qw(store retrieve store_fd nstore_fd fd_retrieve); -use Test::More tests => 21; +use Test::More tests => 22; $a = 'toto'; $b = \$a; @@ -87,5 +87,15 @@ is(&dump($r), &dump(\%a)); eval { $r = fd_retrieve(::OUT); }; 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.7.4