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 13:05, Nikita Popov wrote: > 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=&his >> t=&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 > > yeah, will refix it that way . Whereby i think it's consistent to go with zend_error as object_custom() does it already (seems logic as it's something not coming from the object itself, but parser). At least now we have one sympthom less :) Thanks a lot Anatol