Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:75966 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 66294 invoked from network); 23 Jul 2014 16:16:04 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 23 Jul 2014 16:16:04 -0000 Authentication-Results: pb1.pair.com smtp.mail=julienpauli@gmail.com; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=julienpauli@gmail.com; sender-id=pass Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.220.179 as permitted sender) X-PHP-List-Original-Sender: julienpauli@gmail.com X-Host-Fingerprint: 209.85.220.179 mail-vc0-f179.google.com Received: from [209.85.220.179] ([209.85.220.179:37600] helo=mail-vc0-f179.google.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id D4/C0-61324-0CFDFC35 for ; Wed, 23 Jul 2014 12:16:01 -0400 Received: by mail-vc0-f179.google.com with SMTP id hq11so2601704vcb.38 for ; Wed, 23 Jul 2014 09:15:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; bh=tT5Ab3vl9a9mbK3v+z7shvXRgLXN4QPA3KXBsTWmIqU=; b=Hd4EgUucuAsZwxMYrp/f3JHd8X+JUTdnuh/vQ0Udb7F6t4LeHg8AY6JdX9PNvvldfP Af+lTx5c2Pp/5MwJj6cIRKvyu3bA+X9SLPAuzMG3wVQOOaDXy+82Mk3GSyeEFue29hry c70tEJkKNaI9CJrdF/uxJ6w2NbfLfokd67ih697hvxCNRxWrXD32JaXi4H/NJAaXvYV5 ckAeuB8V4B1p+SiMCWhUL2Bquz7seFor1VcI2dnBPuUw7RB2xs4+PyvAv9wrKvX1eAu/ FZmmRL3JKLjBBmvTtikPpnO+AP3nX/wRcvt7wvx3zw9Zif0QnN1vaXvBFuuwjFNJhsCN vjng== X-Received: by 10.52.120.83 with SMTP id la19mr3130489vdb.68.1406132159254; Wed, 23 Jul 2014 09:15:59 -0700 (PDT) MIME-Version: 1.0 Sender: julienpauli@gmail.com Received: by 10.220.81.68 with HTTP; Wed, 23 Jul 2014 09:15:19 -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 18:15:19 +0200 X-Google-Sender-Auth: TQ5nwChURdt_wthX5ZKBli3vGKI Message-ID: To: Ferenc Kovacs Cc: Nikita Popov , Alexey Zakhlestin , Stas Malyshev , Marco Pivetta , Sebastian Bergmann , Remi Collet , PHP Internals Content-Type: text/plain; charset=UTF-8 Subject: Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13 From: jpauli@php.net (Julien Pauli) On Wed, Jul 23, 2014 at 11:12 AM, Julien Pauli wrote: > On Wed, Jul 23, 2014 at 11:09 AM, Ferenc Kovacs wrote: >> >> >> >> 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 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 >>> >> >> >>> >> > >>> >> > 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 mostly >>> >> > 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 initialization >>> > from the constructors into the create_object functions. >>> > I've also went ahead and created a pull request for the proposed >>> > changes: >>> > https://github.com/php/php-src/pull/733 >>> > as you can see I've taken Nikita's advice and internal classes with >>> > final >>> > 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=67072 > > Agree. > > We could start on a case by case basis, knowing we still got at least > one RC to try that. > Then anyway we're gonna fix them into next 5.6 revisions, so ... > > > Julien.P I spent some time today reviewing our internal bundled extensions searching the ones that overwrite create_object *and* have a dangerous __construct constructing internal object data. What I found *not safe*, throwing unwanted warnings hitting an undesired behavior, or even segfaulting, and thus needing patch : - Dom - Mysqli - sqlite3 (sqlite3stmt class segfaults) What I found *safe*, with checkers that check object is properly initialized, so not needing patch, but throwing exceptions or warnings when used bad constructed : - Date - fileinfo - Intl - Pdo - Reflection - SimpleXML* (not faulting, but could need a better implementation of the checks and the thrown error messages) - SPL Any extension not present in one of the two above lists is safe. The method I used is rather simple : I looked for classes having an overriden create_object and a __construct(). I passed the class to NewInstanceWithoutConstructor() (patched to allow them) , and I then called some methods on the instance. Not perfect, but that's a first pass. It still took me several hours to perform, so we can resonnably assume it is right. I'll try to patch the extensions needing care in the next few days. Julien.P