php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #62852 Unserialize Invalid Date causes crash
Submitted: 2012-08-18 11:53 UTC Modified: 2013-03-15 08:15 UTC
Votes:5
Avg. Score:2.4 ± 0.8
Reproduced:0 of 1 (0.0%)
From: kasper at webmasteren dot eu Assigned: laruence
Status: Closed Package: Reproducible crash
PHP Version: Irrelevant OS: windows, linux
Private report: No CVE-ID:
 [2012-08-18 11:53 UTC] kasper at webmasteren dot eu
Description:
------------
Core PHP,every version so far, 5.3.* and 5.4.*
When unserializing this string :
O:8:"DateTime":3:{s:4:"date";s:20:"10007-06-07 
03:51:49";s:13:"timezone_type";i:3;s:8:"timezone";s:3:"UTC";}
created from: Datetime:createFromFormat("99-99-9999","j-n-Y");
then serialized, to a file. Later when read and working with, php crashes, from 
the parse_tz.c, in timelib_get_time_zone_info. the Exception is "read at offset 
0x00000010". it would appear that ts and / or tz is zero. 


Test script:
---------------
$temp =  unserialize('O:8:"DateTime":3:{s:4:"date";s:20:"10007-06-07 03:51:49";s:13:"timezone_type";i:3;s:8:"timezone";s:3:"UTC";}');
var_dump($temp);

Expected result:
----------------
error parsing invalid date or just a date with all entries 0.

Actual result:
--------------
php crash [read offset 0x00000010] ~  null pointer + offset. at the file 
"ext\date\lib\parse_tz.c"

Patches

Fix-add-exception-checking (last revision 2012-09-16 02:18 UTC) by reeze@php.net)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2012-08-19 08:08 UTC] reeze dot xia at gmail dot com
Hi, 
   I'v sent pull request to fix this:
https://github.com/php/php-src/pull/168

when unserialize it didn't check whether the date is valid.

Thanks
 [2012-08-19 10:30 UTC] laruence@php.net
Automatic comment on behalf of reeze.xia@gmail.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=46a3f257724df7b85cc8c3e6374c36ed9ee783b4
Log: Fixed bug #62852 (Unserialize invalid DateTime causes crash)
 [2012-08-19 10:31 UTC] laruence@php.net
Automatic comment on behalf of reeze.xia@gmail.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=46a3f257724df7b85cc8c3e6374c36ed9ee783b4
Log: Fixed bug #62852 (Unserialize invalid DateTime causes crash)
 [2012-08-19 10:32 UTC] laruence@php.net
Automatic comment on behalf of reeze.xia@gmail.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=46a3f257724df7b85cc8c3e6374c36ed9ee783b4
Log: Fixed bug #62852 (Unserialize invalid DateTime causes crash)
 [2012-08-19 10:36 UTC] laruence@php.net
This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.


 [2012-08-19 10:36 UTC] laruence@php.net
-Status: Open +Status: Closed -Assigned To: +Assigned To: laruence
 [2012-09-14 21:22 UTC] tstarling@php.net
-Status: Closed +Status: Re-Opened
 [2012-09-14 21:22 UTC] tstarling@php.net
The suggested patch does not fix the bug. Throwing an exception does not ensure destruction of the object. For example, you can override __wakeup() in a derived class and put a reference to the half-initialised object in a global value before calling DateTime::__wakeup(). Full test case at 

http://tstarling.com/stuff/bad-date.phps

It segfaults for me on today's git master. It looks to me like either dateobj->time should be fully initialised, or it should be set back to NULL so that DATE_CHECK_INITIALIZED() etc. will guard accesses.

In my opinion, throwing an exception from unserialize() is an unnecessary b/c break and should be reverted.
 [2012-09-15 03:30 UTC] laruence@php.net
-Status: Re-Opened +Status: Closed
 [2012-09-15 03:30 UTC] laruence@php.net
Automatic comment on behalf of laruence
Revision: http://git.php.net/?p=php-src.git;a=commit;h=e766f85405cd936a07a30a045f419199b6c02ed7
Log: Revert "Fixed bug #62852 (Unserialize invalid DateTime causes crash)"
 [2012-09-15 03:30 UTC] laruence@php.net
@tstarling  okey, I reverted. and make the test XFAIL for now, we should fix this 
in another way.
 [2012-09-15 03:32 UTC] laruence@php.net
Automatic comment on behalf of laruence
Revision: http://git.php.net/?p=php-src.git;a=commit;h=e766f85405cd936a07a30a045f419199b6c02ed7
Log: Revert "Fixed bug #62852 (Unserialize invalid DateTime causes crash)"
 [2012-09-15 03:33 UTC] laruence@php.net
Automatic comment on behalf of laruence
Revision: http://git.php.net/?p=php-src.git;a=commit;h=e766f85405cd936a07a30a045f419199b6c02ed7
Log: Revert "Fixed bug #62852 (Unserialize invalid DateTime causes crash)"
 [2012-09-15 04:26 UTC] reeze@php.net
Yeah, the segfault is bad.

but the test script is wired, why do you want to refer to it before wakeup?

When construct the DateTime if invalid date it throw exception, so
when unserialize from an invalid date throw exception is reasonable.
 [2012-09-15 06:57 UTC] laruence@php.net
closed by commit email, reopen
 [2012-09-15 06:57 UTC] laruence@php.net
-Status: Closed +Status: Re-Opened
 [2012-09-16 02:18 UTC] reeze@php.net
The following patch has been added/updated:

Patch Name: Fix-add-exception-checking
Revision:   1347761929
URL:        https://bugs.php.net/patch-display.php?bug=62852&patch=Fix-add-exception-checking&revision=1347761929
 [2012-09-16 02:23 UTC] reeze@php.net
@tstarling the partially initialize problem could be fixed by adding
and exception checking. (the attache patch is a fix for this)

As the exception throwing, I think it's not bc break, since before
the fix, it will just crash, fix the crash is not bc break, and
the use could define __walkup method, it may throw exception too,
so I think throw exception won't make unserialize inconsistant.

Just my 2 cents;
 [2012-09-16 02:31 UTC] reeze@php.net
@laruence, What do you think about this, if you have any better solutions
will be much appreciated :)
 [2012-09-16 03:53 UTC] laruence@php.net
@reeze 
first:  it's not about why he want to do this, like:"why do you want to 
unserialize a abnormal data?"

and your new fix, will allow a incomplete date object in $foo, right? 

I don't this this fix is acceptable, the fix should warning about the wrong data, 
and result a NULL or FALSE.
 [2013-03-04 12:41 UTC] ab@php.net
Here's corresponding BT on windows:

 php5.dll!fetch_timezone_offset(timelib_tzinfo * tz, __int64 ts, __int64 * transition_time) Line 341C
 php5.dll!timelib_get_time_zone_info(__int64 ts, timelib_tzinfo * tz) Line 415C
 php5.dll!date_format(char * format, int format_len, timelib_time * t, int localtime) Line 1046C
 php5.dll!date_object_get_properties(_zval_struct * object) Line 2117C
 php5.dll!php_var_dump(_zval_struct * * struc, int level) Line 129C
 php5.dll!zif_var_dump(int ht, _zval_struct * return_value, _zval_struct * * return_value_ptr, _zval_struct * this_ptr, int return_value_used) Line 181C
 php5.dll!zend_do_fcall_common_helper_SPEC(_zend_execute_data * execute_data) Line 320C
 php5.dll!ZEND_DO_FCALL_SPEC_CONST_HANDLER(_zend_execute_data * execute_data) Line 1629C
 php5.dll!execute(_zend_op_array * op_array) Line 107C
 php5.dll!zend_execute_scripts(int type, _zval_struct * * retval, int file_count, ...) Line 1259C
 php5.dll!php_execute_script(_zend_file_handle * primary_file) Line 2316C
 php.exe!main(int argc, char * * argv) Line 1190C
 php.exe!__tmainCRTStartup() Line 586C
 kernel32.dll!@BaseThreadInitThunk@12()Unknown
 ntdll.dll!___RtlUserThreadStart@8()Unknown
 ntdll.dll!__RtlUserThreadStart@8()Unknown
 [2013-03-14 07:50 UTC] ab@php.net
Looks like there is no other plausible alternative to affect the return value of unserialize from the __wakeup perspective other than throwing an exception. Looking at what @laruence has done in bug #64354 I think we can throw an exception in __wakeup and __set_state and integrate DATE_CHECK_INITIALIZED wherever it's missing. This way it won't delete the invalid date object from the scope, but that object will respond only with false on each method. Here's the slightly modified snippet from @tstarling


<?php

$s2 = 'O:3:"Foo":3:{s:4:"date";s:20:"10007-06-07 03:51:49";s:13:"timezone_type";i:3;s:8:"timezone";s:3:"UTC";}';

global $foo;

class Foo extends DateTime {
    function __construct() {
        global $foo;
        $foo = $this;
        parent::__construct();
    }
    function __wakeup() {
        global $foo;
        $foo = $this;
        parent::__wakeup();
    }
}

try {
        new Foo("10007-06-07 03:51:49");
} catch ( Exception $e ) {}
var_dump( $foo );

try {
    unserialize( $s2 );
} catch ( Exception $e ) {}
var_dump( $foo );

Either in both cases after normal construct or after unserialize user will end up with an invalid $foo object. So there is no BC breach as __construct() already throws an exception, making __wakeup() do the same and checking dateobj->time != NULL in every method after that should be a sufficient solution.
 [2013-03-15 08:15 UTC] ab@php.net
This is also related to bug #53437, where the current implementation suggests 
E_ERROR. The fix for ticket should have exactly the same handling the other one 
has.
 [2013-03-15 20:31 UTC] ab@php.net
Automatic comment on behalf of ab
Revision: http://git.php.net/?p=php-src.git;a=commit;h=f8b91d9acff10ede7bd3f2bc631794a3abef8ff7
Log: Fixed bug #62852 Unserialize Invalid Date crash
 [2013-03-15 20:31 UTC] ab@php.net
-Status: Re-Opened +Status: Closed
 [2013-04-19 20:28 UTC] webmaster at thedigitalorchard dot ca
I'm getting an error since this bug was "fixed". In one of my databases, a 
DateTime object was inadvertently serialized as a child object. Now, with this bug 
fix, I'm getting the following error.

The serialized object is represented by this short string:

O:8:"DateTime":0:{}

Running that through unserialize presents this error:
"Invalid serialization data for DateTime object"

I'm unable to catch this error and handle it gracefully (ie. ignoring this object 
unserialization entirely).
 [2013-04-19 20:42 UTC] webmaster at thedigitalorchard dot ca
My [ugly] workaround for this problem is to manually replace instances of 
serialized DateTime objects with a fake, non-existent class name, which avoids 
this crash.

$str = 'O:8:"DateTime":0:{}';
$str = str_replace('O:8:"DateTime"', 'O:12:"PHP_DateTime"', $str);

Of course, if the serialized data needed to be recovered, an alternate approach 
would be needed. In my own case, I want to be discarding this object. I'm hoping 
this issue that ran into is an unforeseen issue with this latest bug fix, and a 
proper fix can be made in a future update. I don't like adding in workarounds. :-)
 [2013-11-05 02:59 UTC] mkwan at corp dot oodle dot com
According to the documentation, if "the passed string is not unserializeable, FALSE is returned and E_NOTICE is issued."
http://php.net/manual/en/function.unserialize.php

Why is it that if the string happens to looks like a DateTime, instead an unrecoverable E_ERROR is issued?
 [2013-11-17 09:31 UTC] laruence@php.net
Automatic comment on behalf of ab
Revision: http://git.php.net/?p=php-src.git;a=commit;h=f8b91d9acff10ede7bd3f2bc631794a3abef8ff7
Log: Fixed bug #62852 Unserialize Invalid Date crash
 [2014-01-23 12:12 UTC] mail at ericreiche dot net
I'm also still getting this with empty DateTime object in session data
O:8:"DateTime":0:{}.
 [2014-01-23 12:13 UTC] mail at ericreiche dot net
Forgot to mention, version is 5.4.21-1~dotdeb.1
 [2014-03-07 23:48 UTC] Matthew dot J dot Mucklo at qvc dot com
We've been seeing the same thing in production.

It's been tough to trace down exactly how to reproduce it, so we ended up patching php_memcached.c so that these sorts of things don't get saved into memcache (we also had to do a similar filter for session saving as the serialization happens in a different place):

Diff is below:


--- php_memcached.c	2014-02-05 16:00:37.000000000 -0800
+++ memcached_master_02_05_2014/php_memcached.c	2014-02-06 11:01:27.000000000 -0800
@@ -3193,6 +3193,44 @@
 				php_error_docref(NULL TSRMLS_CC, E_WARNING, "could not serialize value");
 				return 0;
 			}
+
+            const char * const cmp = "\"DateTime\":0:{}";
+            const size_t cmplen = 15;
+            if (buf->len >= cmplen)
+            {
+                size_t len = buf->len - cmplen;
+                char *c = buf->c;
+                size_t i = 0;
+                while(i <= len)
+                {
+                    if (*c == *cmp) {
+                        char *cmp1 = cmp + 1;
+
+                        // Run the comparison
+                        while (*cmp1) {
+                            c++;
+                            i++;
+                            if (*c != *cmp1)
+                            {
+                                break;
+                            }
+                            cmp1++;
+                        }
+
+                        // match
+                        if (!*cmp1)
+                        {
+                            php_error_docref(NULL TSRMLS_CC, E_WARNING, "could not serialize DateTime value");
+                            return 0;
+                        }
+                    }
+                    else
+                    {
+                        c++;
+                        i++;
+                    }
+                }
+            }
 			MEMC_VAL_SET_TYPE(*flags, MEMC_VAL_IS_SERIALIZED);
 		}
 			break;
 [2014-03-27 12:33 UTC] justinasu at gmail dot com
The problem still persists, using PHP v5.4.26.
Invalid date produces Fatal error:
Fatal error: Invalid serialization data for DateTime object
 [2014-03-31 02:17 UTC] michael at currinda dot com
I believe this issue is still happening in 5.5.5.

After looking at the code, I suspect that a new DateTime(null); breaks, but the almost equivalent 'new DateTime("now");' passes.
 
PHP Copyright © 2001-2014 The PHP Group
All rights reserved.
Last updated: Thu Apr 17 12:01:59 2014 UTC