Re: Re: Bug 67072 resolution for 5.4/5.5

On Tue, Jun 24, 2014 at 9:58 AM, Stas Malyshev <smalyshev@sugarcrm.com> wrote: > Hi! > >> I think the segfault have to be fixed in spl. > > This can be done, however can we ensure all classes in PHP and > extensions would run properly when unserialized despite explicit > prohibition from the class to serialize/unserialize it? > > Doing O: trick on such class is just not right. Note that crashes is > just the start of the problem - what if circumventing prohibited > unserialization puts the class into state that allows remote attacker to > trick it into doing something it's not supposed to do? Remember the > __dtor issue? That one worked on PHP level, this one would work on C > level, which would be much worse. +1. - Can't we just deny any unserilization with "O:" if the class has a custom serializer ? - Can't we throw an exception on any attempt to unserialize ("O:" or "C:") any class that uses zend_unserialize_deny ? I mean, we won't be able to find a solution which is - 100% garantied no BC break - 100% garantied no segfault So, I suggest we use the safe path and stop hacking, discovering a nasty bug about the hack, then hack it again, just for libraries relying on unsupported tricks about the serialize format ? > >> And if we plan to allow newInstanceArgWithoutConstructor() for internal >> classes this is mandatory. > > I'm not sure this is safe either, by the way. But at least here we don't > allow remote data to control our class' content and inject any data into > it without any controls whatsoever. So here might be a better way out of > this. But we need to be very careful with it. > >> So we can allow "O:.." (perhaps only for empty data used in the >> phpunit/doctrine hack, => strlen(*p)<=1) > > Right now we can not - it leads to remote-triggerable crash in any app > that unserializes outside data, and there are lots of these - just > search on github for unserialize($_POST or unserialize($_COOKIE. And I'm > not sure we can safely allow this in general. I'm sorry that this would > make a neat hack unavailable, but I think security of PHP apps is more > important than preserving this hack which was never documented and never > supposed to work in the first place. > > I understand that this creates a need that we do not cover of how to > mock such objects, and I welcome suggestions - including how to make > newInstanceArgWithoutConstructor safe. But currently I do not see how we > can leave the unserialize hack in for classes like SplFileObject - > unless somebody points me to a way to make it safe. Yes, we should bring sane, safe solution for such needs. But sane and safe, which is all but rushing. We'll bring solutions to 5.6 , but for 5.4 and 5.5 , we just won't be able to satisfy everybody. Julien