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.

diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm
index 397d584..d8fd740 100644 (file)
--- 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 a72d84c..829c5d7 100644 (file)
--- 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 3a4b9dc..b25dbd2 100644 (file)
--- 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' }