Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:94235
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.192.181 as permitted sender)
To: Dmitry Stogov <dmitry@zend.com>, Sara Golemon <pollita@php.net>,
 PHP internals <internals@lists.php.net>
References: <CAESVnVpQGHe9eDCbdgkmJw3qNFX7BJ3af0xcRr7DtK9NXwykrg@mail.gmail.com>
 <BY2PR0201MB1784E3A93854E9C2E7A08818BF2D0@BY2PR0201MB1784.namprd02.prod.outlook.com>
Message-ID: <099f7db1-0166-b641-c04f-a22581fd3879@gmail.com>
Date: Thu, 23 Jun 2016 11:25:12 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:45.0)
 Gecko/20100101 Thunderbird/45.1.1
MIME-Version: 1.0
In-Reply-To: <BY2PR0201MB1784E3A93854E9C2E7A08818BF2D0@BY2PR0201MB1784.namprd02.prod.outlook.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [Bug #68319] unserialize() with modified class
 definition.
From: smalyshev@gmail.com (Stanislav Malyshev)

Hi!

> 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
> visibility, etc), we should just return FALSE.
> 
> I think, Stas proposed something similar some time ago.

I don't remember proposing exactly that, though the idea looks worthy to
me :) I did something different though - allowing limiting unserialize
to accept only certain set of classes, which alleviates some security
issues, but not all of them. This probably would catch more cases,
though also not all of them.

As for validation pass, the only issue I foresee is handling custom
serializers. We'd need to either add validation handler too (probably a
good idea) or somehow handle that. Needs some investigation there.

As for this specific bug, it looks like an oversight. I'm not sure why
we encode visibility in the property at all... Probably for efficiency,
but I think we need to be smarter there.

-- 
Stas Malyshev
smalyshev@gmail.com