On Wed, Jul 23, 2014 at 11:12 AM, Julien Pauli <jpa...@php.net> wrote: > On Wed, Jul 23, 2014 at 11:09 AM, Ferenc Kovacs <tyr...@gmail.com> wrote: >> >> >> >> 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 > > 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
