Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75055
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.53 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <53A92B24.40706@fedoraproject.org>
References: <CAH-PCH6xEXspisLOHtiTbViA4vDTk7q6LM3t_Bh9Dzr1cj_Nag@mail.gmail.com>
	<53A1C722.9060501@fedoraproject.org>
	<53A21137.6010705@sugarcrm.com>
	<CAMUwpuT_S2EE6cdJM95RzY4vmORGx3vJ9+gau8G3EZCMsNSydA@mail.gmail.com>
	<53A2A9BD.1070603@sugarcrm.com>
	<CAH-PCH5-vtCVvGvceMqt6rDXq4zo0Vb34cyDK3KdBUNfg_oLyg@mail.gmail.com>
	<CADyq6sKQbV+Avzfs3OAdGM9Hc85i18t0LSATD6Rk=HjqKOMgBg@mail.gmail.com>
	<53A3874E.20704@sugarcrm.com>
	<CAH-PCH6nSk0ir_TSjhCd0BVhiY5UsOsLKzqKhqfaU1G0ghi4Tg@mail.gmail.com>
	<53A65578.6000701@sugarcrm.com>
	<CAH-PCH7tw0=OacG2HFDvBZxAho0HjdOtpLR=J9aXYGxsv40ehA@mail.gmail.com>
	<53A8626B.701@fedoraproject.org>
	<53A866B6.4060501@sugarcrm.com>
	<53A92B24.40706@fedoraproject.org>
Date: Tue, 24 Jun 2014 10:07:41 +0200
Message-ID: <CAH-PCH4xxE=CDTWZtMiLZkXFOghn8DFmuo05wQ03LRBYHUncFQ@mail.gmail.com>
To: Remi Collet <remi@fedoraproject.org>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=089e01538d502c3d1a04fc9074c0
Subject: Re: [PHP-DEV] Re: Bug 67072 resolution for 5.4/5.5
From: tyra3l@gmail.com (Ferenc Kovacs)

--089e01538d502c3d1a04fc9074c0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Tue, Jun 24, 2014 at 9:39 AM, Remi Collet <remi@fedoraproject.org> wrote=
:

> Le 23/06/2014 19:41, Stas Malyshev a =C3=A9crit :
> > Hi!
> >
> >> Minimal reproducer:
> >>
> >> <?php
> >> class FooFile extends SplFileInfo {
> >> }
> >> $str =3D '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 i=
t
> > 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, =3D> strlen(*p)<=3D1)
>
>
> Running:
> echo
>
> unserialize('O:13:"SplFileObject":1:{s:9:"*filename";s:15:"/home/flag/fla=
g";}');
> 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.

--=20
Ferenc Kov=C3=A1cs
@Tyr43l - http://tyrael.hu

--089e01538d502c3d1a04fc9074c0--