Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75056
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain lsces.co.uk from 217.147.176.214 cause and error)
Message-ID: <53A93611.9070603@lsces.co.uk>
Date: Tue, 24 Jun 2014 09:25:53 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0
MIME-Version: 1.0
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>
In-Reply-To: <53A866B6.4060501@sugarcrm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Re: Bug 67072 resolution for 5.4/5.5
From: lester@lsces.co.uk (Lester Caine)

On 23/06/14 18:41, Stas Malyshev wrote:
>> <?php
>> > class FooFile extends SplFileInfo {
>> > }
>> > $str = 'O:7:"FooFile":0:{}';
>> > var_dump(unserialize($str));
>> > ?>

> I'm afraid here we can't do much - SplFileInfo is one of the classes
> that it is unsafe to instantiate this way. It's what original 67072 was
> about and I don't think it's safe to leave it this way, since at best it
> can crash any code that uses unserialize() on external data, at worst
> you got RCE.

As I see it, the main problem here is trying to cheat when creating a
restored process? Rather than correctly calling the class and
instantiating it via the normal process, some third party process
creates a fake saved version of the object and tries to restart it
bypassing the proper construction process?

That many objects are not designed to be constructed in this way is the
problem, and PHP5.4 and 5.5 are not the platform to be making changes to
that process? Personally I think that bypassing the 'constructor' can be
viewed two ways. Originally PHP did not even have the need for it, as it
has been a more recent addition, so that it has unforeseen side effects
is to be expected? NOW changing things back so that it is not necessary
seems to be counter intuitive? There were good reasons for insisting on
the constructor stage, and seeding an object with a state stored by the
serialize process needs a cross check that the object is reseeded
correctly when unserialized?

That an inherent crash path needs to be plugged is a given, but all I am
seeing in the proposed patches are more bandaids on the whole process?
If the constructor can now be skipped in some instances then why have it
at all? Go back to the situation where one manually called a
'constructor' when it was needed or fix the unserialize process on
objects to include a suitable constructor? If an object has been
'created' but not 'initialized' then a process needs to be run to make
the object actually usable?

That some users of PHP have a problem with the recent changes is more
because they are using undocumented processes so while allowing them to
continue using the hacks short term may be correct - reverting the
changes - the correct path going forward is perhaps to document a
correct process and block any problem paths?

-- 
Lester Caine - G8HFL
-----------------------------
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk