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. -- Ferenc Kovács @Tyr43l - http://tyrael.hu
