Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:94234
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 65.55.169.146 as permitted sender)
To: Sara Golemon <pollita@php.net>, PHP internals <internals@lists.php.net>,
	Stanislav Malyshev <smalyshev@gmail.com>
Thread-Topic: [PHP-DEV] [Bug #68319] unserialize() with modified class
 definition.
Thread-Index: AQHRzXhWRyWTo/MBKkqUURTOMXm6Up/3WIqe
Date: Thu, 23 Jun 2016 18:18:12 +0000
Message-ID: <BY2PR0201MB1784E3A93854E9C2E7A08818BF2D0@BY2PR0201MB1784.namprd02.prod.outlook.com>
References: <CAESVnVpQGHe9eDCbdgkmJw3qNFX7BJ3af0xcRr7DtK9NXwykrg@mail.gmail.com>
In-Reply-To: <CAESVnVpQGHe9eDCbdgkmJw3qNFX7BJ3af0xcRr7DtK9NXwykrg@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: zend.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: multipart/alternative;
	boundary="_000_BY2PR0201MB1784E3A93854E9C2E7A08818BF2D0BY2PR0201MB1784_"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Jun 2016 18:18:12.5066
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 32210298-c08b-4829-8097-6b12c025a892
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY2PR0201MB1781
Subject: Re: [PHP-DEV] [Bug #68319] unserialize() with modified class
 definition.
From: dmitry@zend.com (Dmitry Stogov)

--_000_BY2PR0201MB1784E3A93854E9C2E7A08818BF2D0BY2PR0201MB1784_
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable

Looking into the number of unserialize() related "security" issues, I think=
 we should fix all of them once and forever, introducing a validation pass.

In case something in provided data is wrong (e.g. duplicated properties or =
array keys, unexpected types, invalid references, invalid property visibili=
ty, etc), we should just return FALSE.

I think, Stas proposed something similar some time ago.


Thanks. Dmitry.



________________________________
From: php@golemon.com <php@golemon.com> on behalf of Sara Golemon <pollita@=
php.net>
Sent: Thursday, June 23, 2016 8:53:58 PM
To: PHP internals
Subject: [PHP-DEV] [Bug #68319] unserialize() with modified class definitio=
n.

https://bugs.php.net/bug.php?id=3D68319
https://3v4l.org/irnRC

The crux is this:
* Object instance gets serialized with one definition, maybe stored in
DB/file, whatever, the serialized value lives on.
* Class definition changes slightly. In this case, a property changes
visibility.
* Serialized value is unserialized.  The prop visibilities don't match.
* PHP says, "Eh, whatevs, I'll make a dynamic prop of the same name."

Possible resolutions:
1: Raise a warning and return false (as unserialize already does for
parse errors)
2: Raise a warning and "correct" the visibility to match the current
class definition
3: Raise a warning and continue duplicating the properties

I don't think we need to be as terrible as option 3 since any code
facing this problem right now can't actually access the unserialized
value and is therefore broken in much worse ways.  I think option 2
presents its own unquantified risks and should probably be avoided.

So obviously, I vote option 1, but I'd like to get other's thoughts
and opinions before addressing this bug.

I'm going to go ahead and say ignore what HHVM does here.  In this
specific case they basically take option 2, but in the inverse case
https://3v4l.org/ecM1Q they're precisely as broken as we are.

-Sara

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php


--_000_BY2PR0201MB1784E3A93854E9C2E7A08818BF2D0BY2PR0201MB1784_--