Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:75936 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 97997 invoked from network); 23 Jul 2014 09:09:46 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 23 Jul 2014 09:09:46 -0000 Authentication-Results: pb1.pair.com smtp.mail=tyra3l@gmail.com; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=tyra3l@gmail.com; sender-id=pass Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.192.45 as permitted sender) X-PHP-List-Original-Sender: tyra3l@gmail.com X-Host-Fingerprint: 209.85.192.45 mail-qg0-f45.google.com Received: from [209.85.192.45] ([209.85.192.45:53964] helo=mail-qg0-f45.google.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id B2/04-21666-8DB7FC35 for ; Wed, 23 Jul 2014 05:09:45 -0400 Received: by mail-qg0-f45.google.com with SMTP id f51so1027353qge.18 for ; Wed, 23 Jul 2014 02:09:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=qUGuPYJDNi6rgWI6EmJM3W9c2RetzjYKwrdeanpmG24=; b=lT928+PtK0Z63VvDCx1nPLVYpaA6Ghsu/rapOcBu3KAunPS0kfnDdAr0J++JITpFOl st35WYHfaRCZt2d0rxPHkGEZrcZ0MMmCfWQXzKH3IpEvZUI5p3L0XXE2vUUJSZ7BPfah NYVnQUjrc8FzRIAcCuXg/b3scmrphiwI9hMkxPW3A24N6zipdl5cmXI8d7sFHRpM0A9D O0hBzCGxUPxAKu1BxW7ejar1zLlk0iWi6Mo1zih43WSnfUOejOlcHM7ZzGbWHyB872A2 6bzM8WM8s+xIw60OOGxrSPITUTNHpT/qFU4UFkQPfC06rDPp3Msei5evQa9cQh9wh8EW udtw== MIME-Version: 1.0 X-Received: by 10.224.4.73 with SMTP id 9mr67294545qaq.12.1406106583505; Wed, 23 Jul 2014 02:09:43 -0700 (PDT) Received: by 10.140.102.111 with HTTP; Wed, 23 Jul 2014 02:09:43 -0700 (PDT) In-Reply-To: References: <53A1C722.9060501@fedoraproject.org> <53A21137.6010705@sugarcrm.com> <53A2A9BD.1070603@sugarcrm.com> <53A3874E.20704@sugarcrm.com> <53A62AFF.4080302@sugarcrm.com> <53B10D59.4060206@sugarcrm.com> Date: Wed, 23 Jul 2014 11:09:43 +0200 Message-ID: To: Julien Pauli Cc: Nikita Popov , Alexey Zakhlestin , Stas Malyshev , Marco Pivetta , Sebastian Bergmann , Remi Collet , PHP Internals Content-Type: multipart/alternative; boundary=001a11c21eda6d20fa04fed8b38c Subject: Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13 From: tyra3l@gmail.com (Ferenc Kovacs) --001a11c21eda6d20fa04fed8b38c Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Wed, Jul 23, 2014 at 10:57 AM, Julien Pauli wrote: > On Wed, Jul 23, 2014 at 7:58 AM, Ferenc Kovacs wrote: > > > > > > > > On Tue, Jul 22, 2014 at 6:20 PM, Julien Pauli wrote: > >> > >> On Tue, Jul 22, 2014 at 2:04 PM, Ferenc Kovacs > wrote: > >> > > >> > > >> > > >> > On Tue, Jul 22, 2014 at 1:48 PM, Nikita Popov > >> > wrote: > >> >> > >> >> On Tue, Jul 22, 2014 at 12:27 PM, Ferenc Kovacs > >> >> 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 objec= t > >> >>> 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 clea= n > >> >>> 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 no= t > >> >> 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-cla= ss > >> >> usecase doesn't apply either, so that shouldn't be a problem. > >> >> > >> >> Nikita > >> >> > >> > > >> > Thanks for the prompt reply! > >> > I was considering mentioning the final constructors, but as we > >> > previously > >> > didn't checked that and from a quick look it seems that we are mostl= y > >> > using > >> > it as an easy/cheap way to make sure that the object is initialized > >> > properly > >> > (which could also happen when a subclass calls parent::__construct() > >> > from > >> > it's constructor) which isn't exactly the best usecase for final. > >> > But I don't really mind having that check. > >> > >> I'm +1 also with the idea. > >> > >> Just take care to have a clone_handler defined as well, as the default > >> clone handler doesn't call create_object. > >> http://lxr.php.net/xref/PHP_5_5/Zend/zend_objects.c#218 > >> > >> Julien > > > > > > thanks, I will keep that in mind when we start moving the initializatio= n > > from the constructors into the create_object functions. > > I've also went ahead and created a pull request for the proposed change= s: > > https://github.com/php/php-src/pull/733 > > as you can see I've taken Nikita's advice and internal classes with fin= al > > constructors are still not allowed to be instantiated. > > When should we start patching those ? > I guess asap ? > > Julien > Not sure, on one hand, I would be glad seeing these fixed, but on the other hand I'm a little bit worried about introducing destabilizing changes this close to the release, and these problems existed for years (triggerable either through instantiating via the unserialize O: trick or through a subclass) without any reports before https://bugs.php.net/bug.php?id=3D6707= 2 --=20 Ferenc Kov=C3=A1cs @Tyr43l - http://tyrael.hu --001a11c21eda6d20fa04fed8b38c--