Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75057
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.220.180 as permitted sender)
MIME-Version: 1.0
Sender: julienpauli@gmail.com
In-Reply-To: <53A92F93.2060507@sugarcrm.com>
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 10:36:50 +0200
Message-ID: <CAMUwpuSHd=4tfYq+7NwLi+f7W8nSDMEgVGFrENxSa_=kMEeLCA@mail.gmail.com>
To: Stas Malyshev <smalyshev@sugarcrm.com>
Cc: Remi Collet <remi@fedoraproject.org>, PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] Re: Bug 67072 resolution for 5.4/5.5
From: jpauli@php.net (Julien Pauli)

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