On Wed, Jul 23, 2014 at 10:57 AM, Julien Pauli <jpa...@php.net> wrote:
> On Wed, Jul 23, 2014 at 7:58 AM, Ferenc Kovacs <tyr...@gmail.com> wrote: > > > > > > > > On Tue, Jul 22, 2014 at 6:20 PM, Julien Pauli <jpa...@php.net> wrote: > >> > >> On Tue, Jul 22, 2014 at 2:04 PM, Ferenc Kovacs <tyr...@gmail.com> > wrote: > >> > > >> > > >> > > >> > On Tue, Jul 22, 2014 at 1:48 PM, Nikita Popov <nikita....@gmail.com> > >> > wrote: > >> >> > >> >> On Tue, Jul 22, 2014 at 12:27 PM, Ferenc Kovacs <tyr...@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 > >> >> > >> > > >> > 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 -- Ferenc Kovács @Tyr43l - http://tyrael.hu
