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