Re: com php-src: Fixed bug #67072 Echoing unserialized 'SplFileObject' crash: NEWS ext/standard/tests/serialize/005.phpt
ext/standard/tests/serialize/bug67072.phpt ext/standard/var_unserializer.c ext/standard/var_unserializer.re

Hi Nikita, On Fri, April 18, 2014 11:32, Nikita Popov wrote: > On Thu, Apr 17, 2014 at 10:48 AM, Anatol Belski <ab@php.net> wrote: > > >> Commit: 5328d4289946e260232f3195ba2e0f0eb173d5ef >> Author: Anatol Belski <ab@php.net> Thu, 17 Apr 2014 10:48:14 >> +0200 >> Parents: 7a5f1663c6775bbdaf870e4c71ef8813d5d13179 >> Branches: PHP-5.4 PHP-5.5 PHP-5.6 master >> >> >> Link: >> >> http://git.php.net/?p=php-src.git;a=commitdiff;h=5328d4289946e260232f319 >> 5ba2e0f0eb173d5ef >> >> >> Log: >> Fixed bug #67072 Echoing unserialized "SplFileObject" crash >> >> >> The actual issue lays in the unserializer code which doesn't honor >> the unserialize callback. By contrast, the serialize callback is >> respected. This leads to the situation that even if a class has disabled >> the serialization explicitly, user could still construct a vulnerable >> string which would result bad things when trying to unserialize. >> >> This conserns also the classes implementing Serializable as well >> as some core classes disabling serialize/unserialize callbacks explicitly >> (PDO, SimpleXML, SplFileInfo and co). As of now, the >> flow is first to call the unserialize callback (if available), then call >> __wakeup. If the unserialize callback returns with no >> success, no object is instantiated. This makes the scheme used by >> internal classes effective, to disable unserialize just assign >> zend_class_unserialize_deny as callback. >> >> Bugs: >> https://bugs.php.net/67072 >> >> > > This was likely by design. The unserialize callback is supposed to only > be invoked for the C type, whereas wakeup is used only for the O type. > That's > why disallowing serialization was always done with unserialize_deny for C > and an erroring __wakeup for O. > > Now the O type will try to invoke the C unserialization handler as well - > which doesn't make a lot of sense, because both use entirely different > formats (O is basically an array, C is custom data format). > > In particular, the parse_iv2 value has a different meaning for O (number > of elements) and C (length of string). As such the new code introduces a > potential buffer overread vulnerability, as we can now specify a > parse_iv2 value that is larger than the input string. To see that this is > plausible, try something simple like > unserialize('O:11:"ArrayObject":100:{}'). This > will give you the following exception, which contains trailing data after > the unserialized string: > > Fatal error: Uncaught exception 'UnexpectedValueException' with message > 'Error at offset 0 of 100 bytes' in /home/nikic/dev/php-5.6/t09.php:3 > Stack trace: > #0 [internal function]: > ArrayObject->unserialize('}\x00\xF6\x9B\xC6n\xC6nQ\xBA\xC7\xE59\x10\x00... > ') > #1 /home/nikic/dev/php-5.6/t09.php(3): unserialize('O:11:"ArrayObje...') > #2 {main} > thrown in /home/nikic/dev/php-5.6/t09.php on line 3 > > Nikita > > Many internal classes hope to disable serialization this way. http://lxr.php.net/search?q=%22-%3Eunserialize%22&defs=&refs=&path=&hist=&project=PHP_5_4 I see now, in the initial snippet O:13:"SplFileObject":1:{s:9:"*filename";s:15:"/home/flag/flag";} replacing the first O with C invokes the object_custom() which cares about it. But for the case of the user composed string - we need some handling in the object_common1(). I saw the solution with __wakeup which is checked in object_common2(), but with that one would have to go through all the internal classes to check for this vulnerability. For now I have some ideas how to solve the buffer overread issue - check for the data length - check whether the class actually implements Serializable - still call the unserialize callback, especially if it is zend_class_unserialize_deny - otherwise it should throw some "wrong format" exception Whereby as far as i can see here, C is set only if a class implements Serializable http://lxr.php.net/xref/PHP_5_4/ext/standard/var.c#779 so basically if class having serializable != NULL lands in the object_common1() - it's an error per se. Maybe just calling zend_class_unserialize_deny were sufficient. Still digging on that, thanks for the good investigation. Anatol