Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75836
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.219.43 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAH-PCH6Tb-QbTaB3_qthpiGo4fcs0afViDhACmcur=vZ+=HnFg@mail.gmail.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>
	<53A62AFF.4080302@sugarcrm.com>
	<CADyq6s+N_mNc-n1g=S5LwWBZ9fkZ+hqRc3Ycu8M7EBd9Hc1ABw@mail.gmail.com>
	<53B10D59.4060206@sugarcrm.com>
	<F5324819-A85F-47BA-8FE5-27FBE87E1B89@gmail.com>
	<CAH-PCH6Tb-QbTaB3_qthpiGo4fcs0afViDhACmcur=vZ+=HnFg@mail.gmail.com>
Date: Tue, 22 Jul 2014 13:48:58 +0200
Message-ID: <CAF+90c9r18LYsfFOhTj9faqBy2w+MxKAk9WmVi3PZ0uzABwQdQ@mail.gmail.com>
To: Ferenc Kovacs <tyra3l@gmail.com>
Cc: Alexey Zakhlestin <indeyets@gmail.com>, Stas Malyshev <smalyshev@sugarcrm.com>, 
	Marco Pivetta <ocramius@gmail.com>, Sebastian Bergmann <sebastian@php.net>, Julien Pauli <jpauli@php.net>, 
	Remi Collet <remi@fedoraproject.org>, PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a11c2c25016659204fec6cf9c
Subject: Re: [PHP-DEV] Problems with the fix for the BC break introduced in
 5.4.29 and 5.5.13
From: nikita.ppv@gmail.com (Nikita Popov)

--001a11c2c25016659204fec6cf9c
Content-Type: text/plain; charset=UTF-8

On Tue, Jul 22, 2014 at 12:27 PM, Ferenc Kovacs <tyra3l@gmail.com> wrote:

> sorry for the late reply.
> you are right and after looking into the implementation I think classes
> having custom object storage (eg. create_object) shouldn't assume that
> their __construct will be called, but either do the initialization in the
> create_object hook or validate in the other methods that the object was
> properly initialized.
> given that this lack of initialization problem is already present(you can
> extend such a class and have a __construct() in the subclass which doesn't
> call the parent constructor), and we want to keep the unserialize trick
> fixed (as that exposes this problems to the remote attacker when some code
> accepts arbitrary serialized data from the client) I see no reason to keep
> the limitation in the ReflectionClass::newInstanceWithoutConstructor() and
> allowing the instantiation of internal classes will provide a clean upgrade
> path to doctrine and co.
> ofc. we have to fix the internal classes misusing the constructor for
> proper initialization one by one, but that can happen after the initial
> 5.6.0 release (ofc it would be wonderful if people could lend me a hand for
> fixing as much as we can before the release), but we have to fix those
> anyways.
>

This sounds reasonable to me. newInstanceWithoutConstructor does not have
the same remote exploitation concerns as serialize, so allowing crashes
here that can also be caused by other means seems okay to me (especially if
we're planning to fix them lateron). Only additional restriction I'd add is
to disallow calling it on an internal + final class. For those the
__construct argument does not apply. For them the
entity-extending-internal-class usecase doesn't apply either, so that
shouldn't be a problem.

Nikita

--001a11c2c25016659204fec6cf9c--