Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:75937 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 99856 invoked from network); 23 Jul 2014 09:12:51 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 23 Jul 2014 09:12:51 -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.171 as permitted sender) X-PHP-List-Original-Sender: julienpauli@gmail.com X-Host-Fingerprint: 209.85.220.171 mail-vc0-f171.google.com Received: from [209.85.220.171] ([209.85.220.171:57828] helo=mail-vc0-f171.google.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 4A/64-21666-09C7FC35 for ; Wed, 23 Jul 2014 05:12:49 -0400 Received: by mail-vc0-f171.google.com with SMTP id hq11so1590522vcb.30 for ; Wed, 23 Jul 2014 02:12:47 -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=M08Z+pKPnme08p4yXnTJqPTX0xkB/Cg/7+GE1I2sMeI=; b=0icrMTqHMK8YKR7qo8viYNrg0DxkrXXQjwdi50dlloz3sw4DxFkjoeE95n645XKNJU spho5U0YWbgebGLldjfTVEJSZVAwc16YGGB8RcXMggieRLOpjLxrF7ALT0IFrAUC8FoD 9UqpXxnWgdNUE8KIt8u0+M4PUumOHL36PcSqFQj+QDbgoKHFNpCCXcQHJ/C8ViMjDcIZ lfO7Rr0hWLW42fI7EZiE2UscA2aOiiFGLhrWu6Q58mIifMT4mZWHPphv3GICpzH0Y0lm xqEOXL/d2jqK7ap93YHvMM0LfBkQiTzsyGeJVlBc8sazYMChSH5DSptKdvBeKan/rIE7 hAdQ== X-Received: by 10.52.163.229 with SMTP id yl5mr19619500vdb.79.1406106767511; Wed, 23 Jul 2014 02:12:47 -0700 (PDT) MIME-Version: 1.0 Sender: julienpauli@gmail.com Received: by 10.220.81.68 with HTTP; Wed, 23 Jul 2014 02:12:07 -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:12:07 +0200 X-Google-Sender-Auth: uva9OOAhKvJePXI-szBLl5_FGSM 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: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