Re: Re: Bug 67072 resolution for 5.4/5.5

On Tue, Jun 24, 2014 at 9:39 AM, Remi Collet <remi@fedoraproject.org> wrote: > Le 23/06/2014 19:41, Stas Malyshev a écrit : > > Hi! > > > >> Minimal reproducer: > >> > >> <?php > >> class FooFile extends SplFileInfo { > >> } > >> $str = 'O:7:"FooFile":0:{}'; > >> var_dump(unserialize($str)); > >> ?> > > > > I'm afraid here we can't do much - SplFileInfo is one of the classes > > that it is unsafe to instantiate this way. It's what original 67072 was > > about and I don't think it's safe to leave it this way, since at best it > > can crash any code that uses unserialize() on external data, at worst > > you got RCE. > > I think the segfault have to be fixed in spl. > We should fix those issues, sure, but if we consider unserialize($arbitraryUserInput) a relatively common thing, then continuing to allow the Serializable Internal classes to be unserialized with the O format will allow remote DOS or worse until we "fixed" all of those classes to support instanitation without a constructor call. I don't think we can wait for that with the 5.4/5.5 releases (and there are already other sec related fixes waiting to be released). > > And if we plan to allow newInstanceArgWithoutConstructor() for internal > classes this is mandatory. > I'm not so sure about that it is mandatory, as I mentioned the problem also exists with MyClass extends SomeInternalClass {public function __construct(){}} but what makes the difference between newInstanceArgWithoutConstructor/extending the class is that it requires deliberate action, explicitly written code, vs the arbitrary unserialize call. > > See attached patch (quickly written, just for test) > > So we can allow "O:.." (perhaps only for empty data used in the > phpunit/doctrine hack, => strlen(*p)<=1) > > > Running: > echo > > > unserialize('O:13:"SplFileObject":1:{s:9:"*filename";s:15:"/home/flag/flag";}'); > echo unserialize('O:13:"SplFileObject":0:{}'); > > > Warning: Erroneous data format for unserializing 'SplFileObject' ... > Fatal error: SplFileObject::__toString(): Object not initialized ... > > Would be nice if others could also comment, because I think we are starting to argue in circles. -- Ferenc Kovács @Tyr43l - http://tyrael.hu