Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75039
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.52 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <53A65578.6000701@sugarcrm.com>
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>
Date: Mon, 23 Jun 2014 01:10:45 +0200
Message-ID: <CAH-PCH7tw0=OacG2HFDvBZxAho0HjdOtpLR=J9aXYGxsv40ehA@mail.gmail.com>
To: Stas Malyshev <smalyshev@sugarcrm.com>
Cc: Sebastian Bergmann <sebastian@php.net>, Julien Pauli <jpauli@php.net>, 
	Remi Collet <remi@fedoraproject.org>, PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a11c035f4251cc704fc74d6d6
Subject: Re: Bug 67072 resolution for 5.4/5.5
From: tyra3l@gmail.com (Ferenc Kovacs)

--001a11c035f4251cc704fc74d6d6
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Sun, Jun 22, 2014 at 6:03 AM, Stas Malyshev <smalyshev@sugarcrm.com>
wrote:

> Hi!
>
> We're getting pretty short on time for resolving 67072 one way or
> another, since we want to release next week (we've accumulated quite a
> lot of security issues). I still think that:
>
> 1. We can not leave unserialize() segfault in the code in 5.4
> undisturbed. It's DoS opening as a minimum, and RCE potential, if
> suitable internal class can be found, for every app that accepts
> user-controlled serialized data. And there are a lot of such apps out
> there.
>

for the issue to materialize you need to feed hand-crafted input to
unserialize, anybody doing that with user controlled data already asking
for problems, but I agree that we should fix if, and even fix it in a micro
version if we won't totally cripple the the userland tools unfortunately
depending on this trick.


>
> 2. We do not want to break phpunit/doctrine, at least as much as
> possible without hurting #1.
>
>
+1


> 3. Current code does not fix the segfault problem entirely, e.g.:
> class MySplFileObject extends SplFileObject {}
> echo
>
> unserialize('O:15:"MySplFileObject":1:{s:9:"*filename";s:15:"/home/flag/f=
lag";}');
>
> still segfaults for me.
>

yeah, this is what I also noticed and reported.


>
> 4. There are other ways to cause segfaults with internal objects,
> unfortunately, but those require deliberate coding and I'm less worried
> about it since those have no external exploitation potential.
>

I would say that this also a deliberate attempt, maybe a bit more obscure.


>
> Given this, I am proposing to check the check in var_unserializer to this=
:
>
> if (ce->serialize =3D=3D NULL || ce->unserialize =3D=3D zend_user_unseria=
lize ||
> (ZEND_INTERNAL_CLASS !=3D ce->type && ce->create_object =3D=3D NULL)) {
>                 object_init_ex(*rval, ce);
> }
>
> This would allow O: to work with the following:
>
> 1. Regular user classes
> 2. User classes implementing Serializable - the result will be
> semantically broken but no segfault should be produced since it is still
> all within PHP engine, so the worst is that some PHP variables will be
> left null instead of their real values.
> 3. Internal classes that do not have their own serializer
>
> The following will still not work with O::
> 1. Internal classes with their own serializer
> 2. User classes extending classes in 1.
>
>
I prefer this over what we have in 5.4/5.5 and given how few classes does
1, actually mean, I think it would be an acceptable compromise, but let's
hear what others think.


> I've checked the Horde/phpunit test by Remi, and it works fine, but I'd
> like more feedback on this. Please note we need some solution one way or
> another in 3 days, so if we need more discussion on this I'd advise
> maybe set up some session on IRC to hash it out.
>

I think  #php.pecl on efnet would be the best place, as some of us already
lurks there.


>
> P.S. For 5.6, I'd just remove the usage of O: hack for any class that
> produces C: in serialize. If serializer does not produce it, it should
> not accept it.
>

agree, but as I mentioned I would like to provide some alternative for
creating those classes (eg. removing the restriction on internal classes
for ReflectionClass::newInstanceWithoutConstructor)

ps: I've seen that you created a pull request with the patch, if somebody
don't wanna copypaste the patch from the mail, here it is:
https://github.com/php/php-src/pull/701

--=20
Ferenc Kov=C3=A1cs
@Tyr43l - http://tyrael.hu

--001a11c035f4251cc704fc74d6d6--