Re: Re: com php-src: Improved the fix for bug #67072, thanks Nikita: ext/standard/tests/serialize/005.phpt ext/standard/tests/serialize/bug67072.phpt
ext/standard/var_unserializer.c ext/standard/var_unserializer.re
Hi Stas,
On Tue, May 6, 2014 10:26, Stas Malyshev wrote:
> Hi!
>
>
>> The initial bug is that an unserialization string misuses "C:" vs "O:"
>> format which would call an incorrect method "unserialize" vs.
>> "__wakeup".
>> And that leads to a crash with that particular ticket, due to an
>> incompletely initialized object. Formally this fix just brings the
>> functionality inline with the documentation,
>> http://de1.php.net/serializable . It says
>> "Classes that implement this
>> interface no longer support __sleep() and __wakeup().". So that dirty
>> tricks should actually have been impossible, according to the doc. So
>> per fact, it breaks something which was already broken but users was
>> relying on it.
>
> I'm not sure I understand this point. The manual says __sleep/__wakeup
> won't be called, but that doesn't seem to me a problem here, the problem
> here seems to be that unserialize throws a warning, not that __wakeup is
> not called.
>
Please take the case from #67072
- SplFileObject implements Serializable interface and serialization is
disabled by setting the callbacks to NULL or to the special Zend callback
- imagine the serialization were enabled on it, so the string would look like
'C:13:"SplFileObject":.......'
- when unserializing, the ->unserialize() method implementation should be
called (while it's disabled)
- the users wants to do the trick and constructs this
'O:13:"SplFileObject":1:{s:9:"*filename";s:15:"/home/flag/flag";}'
- the var_unserializer code thinks it's an object with possible __wakeup,
so no ->unserialize method is called, and no __wakeup as it's not
implemented. If it would call ->unserialize(), it would lead to a similar
error triggered from zend_class_unserialize_deny
Result - there's a C struct with unititialized members which would lead to
crashes when accessed. That will affect any internal class implementing
Serializable. The warning is just an indication that a class implementing
Serializable was passed with O: format and not with C:. That class per
definition shouldn't even have __wakeup as it implements Serializable,
while it's not disallowed.
>> The patches fixes it globally preventing incomplete objects due to the
>> incorrect callback invocation. A fix for that particular ticket could be
>>
>
> This also breaks code that worked before, and does it in stable
> versions. I do not think it is an acceptable solution for stable versions.
> Can't we make the fix in a way that does not have BC breakage
> for stable versions?
>
>> shortly defer the issue because 5.6 is coming out in a few weeks, so
>> the bad user code will have to be fixed in the userspace soon. As
>> conclusion -
>
> Only if that code is going to be run on 5.6.0 - which it won't be for
> some time because nobody runs .0 versions in production next day they are
> released. But even with 5.6, we are trying to minimize amount of code
> we're breaking in minor releases, and here we're breaking phpunit, which
> is one of the most used PHP tools. So I think we need to look for a better
> solution. CCing also Sebastian, maybe he could explain more about this
> hack and how it can be made to work more smoothly maybe. --
Yeah, Sebastian knows that at best. Generally, if no userspace fix is
possible, one could still revert and put __wakeup() to SplFileObject
class, while as mentioned it's discards the doc and brings possible
vulnerability back. At least 5.6.0 should have that fix IMHO, or master.
Cheers
Anatol
Thread (4 messages)