Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75058
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.50 as permitted sender)
Content-Type: text/plain; charset=utf-8; format=flowed; delsp=yes
To: internals@lists.php.net
References: <CAH-PCH6xEXspisLOHtiTbViA4vDTk7q6LM3t_Bh9Dzr1cj_Nag@mail.gmail.com> <53A1C722.9060501@fedoraproject.org> <53A21137.6010705@sugarcrm.com> <CAMUwpuT_S2EE6cdJM95RzY4vmORGx3vJ9+gau8G3EZCMsNSydA@mail.gmail.com> <53A2A9BD.1070603@sugarcrm.com> <CAH-PCH5-vtCVvGvceMqt6rDXq4zo0Vb34cyDK3KdBUNfg_oLyg@mail.gmail.com> <CADyq6sKQbV+Avzfs3OAdGM9Hc85i18t0LSATD6Rk=HjqKOMgBg@mail.gmail.com> <53A3874E.20704@sugarcrm.com> <CAH-PCH6nSk0ir_TSjhCd0BVhiY5UsOsLKzqKhqfaU1G0ghi4Tg@mail.gmail.com> <53A65578.6000701@sugarcrm.com> <CAH-PCH7tw0=OacG2HFDvBZxAho0HjdOtpLR=J9aXYGxsv40ehA@mail.gmail.com> <53A8626B.701@fedoraproject.org> <53A866B6.4060501@sugarcrm.com> <53A92B24.40706@fedoraproject.org> <53A92F93.2060507@sugarcrm.com>
Date: Tue, 24 Jun 2014 13:59:38 +0400
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Message-ID: <op.xhyd07yec9evq2@nikita-optiplex-9020>
In-Reply-To: <53A92F93.2060507@sugarcrm.com>
User-Agent: Opera Mail/12.16 (Linux)
Subject: Re: [PHP-DEV] Re: Bug 67072 resolution for 5.4/5.5
From: inefedor@gmail.com ("Nikita Nefedov")

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?