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

On Fri, Apr 18, 2014 at 12:39 PM, Anatol Belski <ab@php.net> wrote: > 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: > >> 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. > As PHP will always generate a C serialization if ce->serialize != NULL, I think it makes sense to just throw an exception (like the one from unserialize_deny) if we reach object_common1 with ce->serialize != NULL. Btw, denying unserialization usually just fixes one particular symptom of the issue. There are other places where we do object_init_ex without calling the ctor. Those places will result in the same crash as the invalid unserialization. Nikita