Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:100251
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.214.53 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <72449705-4871-d317-857e-388f53800bbd@gmx.de>
References: <72449705-4871-d317-857e-388f53800bbd@gmx.de>
Date: Fri, 18 Aug 2017 12:02:12 +0200
Message-ID: <CAF+90c_bxYCEMWSL7QO2BSTq6gOVwkmTnCj1OEt3xo-7QHTuAg@mail.gmail.com>
To: "Christoph M. Becker" <cmbecker69@gmx.de>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="001a114057601b639405570439eb"
Subject: Re: [PHP-DEV] [RFC] Deprecate class instance deserialization in WDDX
From: nikita.ppv@gmail.com (Nikita Popov)

--001a114057601b639405570439eb
Content-Type: text/plain; charset="UTF-8"

On Tue, Aug 15, 2017 at 6:54 PM, Christoph M. Becker <cmbecker69@gmx.de>
wrote:

> Hi internals!
>
> Due to the recent discussion regarding WDDX serialization and security
> (<http://marc.info/?l=php-internals&m=150245739612076&w=2>), I've
> written an RFC that proposes to deprecate class instance deserialization
> in WDDX:
>
> <https://wiki.php.net/rfc/wddx-deprecate-class-instance-deserialization>
>
> I hereby put this RFC under discussion.
>
> Note that I have fully intentional left out issues like moving the WDDX
> extension to PECL, actually removing the class instance deserialization
> and the `wddx` session serialization handler, to eschew lengthy
> discussions, because I would like to see the deprecation already
> happening in *PHP 7.2*, since this is a rather sensitive issue.
>

As I've already said in the previous thread, I don't think this is the
right way to go about this issue. Instead we should push harder to remove
this extension entirely.

Let me recapitulate what the issues with this extension are:

1. Security (object injection): __wakeup() can be triggered by untrusted
input, usually exploitable with enough effort.
2. Security (other): While WDDX doesn't have any of the fundamental issues
of unserialize(), the extension has a very bad track record where security
is concerned. For two recent relevant bugs see #74145 (segfault on 5.6) and
#73173 (memleak). These are by no means isolated occurrences, the wddx
extension has seen quite a few security patches in the past. Maybe
everything is fixed now? I wouldn't bet on it.
3. Irrelevance: It's 2017, nobody uses WDDX. (With the usual qualifications
on "nobody".)

On top of that the API is quite ridiculous, with wddx_add_vars() and
wddx_serialize_vars() taking variable names (!!!) to serialize. This API
must be from a time when register_globals not only still existed but was
probably the preferred way of doing things.

What this RFC solves is the first point, in a backwards-compatibility
breaking way. Even with this resolved, I would still be wary of using this
on untrusted input due to the second point. The third point just means that
we shouldn't waste time on elaborate solutions.

Which is why I would suggest:
 1. Deprecate the entire extension in PHP 7.2.
 2. Unbundle it in PHP 7.3.
 3. (Optional -- someone who needs it can do it) Provide a PHP polyfill
implementation for wddx_serialize_value and wddx_deserialize.

I'm not going to vote against just deprecating the object deserialization,
I just think that it's moving forward unnecessarily slowly. I think
everybody will benefit from removing this particular blight sooner rather
than later.

Of course, just deprecating this "feature" does not directly prevent the
> associated security issues, but it may help to make developers aware of
> those, especially because these issues have only been recently be
> documented (<http://svn.php.net/viewvc?view=revision&revision=342852>).
> Furthermore, the deprecation is in my opinion a necessary prerequisite
> for eventual removal of this "feature".  I don't think that we can
> suddenly remove functionality that has been available since PHP 4.0.0.
>

It has been documented previously: There is an existing warning in the
"Notes" section. Now the warning is repeated twice on the same page ^^

Nikita

--001a114057601b639405570439eb--