Re: Re: Bug 67072 resolution for 5.4/5.5

On Tue, 24 Jun 2014 11:58:11 +0400, 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.

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.

Hi Guys, I don't see any problem in the fact that you can skip actual instantiation of the object. Even if there *is* a problem with that - a lot of user-land code already uses this trick and it will be a pretty big BC break (as we already see it in the case of Symfony's test suite) and it was told a million times already. Using input data blindly is user's mistake. I think there are already cases of PHP trying to 'secure' users from themselves (hello magic quotes). And I think all we have to do is to prevent possibility of segfault, not the possibility of instantiation of the object (without calling internal constructor) itself, because skipping constructor (for whatever type classes - internal or user-land) has its use cases. Don't you think that it would be better, instead of preventing creation of the objects without calling internal constructor, to prevent calling any internal methods on those objects (apart from ctor of course) if they were instantiated the 'wrong' way. I mean we would have to add a check in zend_call_function (maybe in some other places as well) like

    if (fci.object_ptr && EX(function_state).function->type == ZEND_INTERNAL_FUNCTION && ! was_properly_instantiated(fci.object_ptr)) { ...then throw an error... }

But as I understand right now we don't have any generic means to get to know whether internal constructor was called or not, so that would be a bit problematic - we would need to add some field in the zend_object structure which could be used as a flag to indicate some things, including this one - whether internal constructor was called or not (it shouldn't be very hard to implement). The only issue here is increasing of memory usage by objects by 32 bits... What do you think?