Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:100254
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmx.de designates 212.227.15.19 as permitted sender)
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: PHP internals <internals@lists.php.net>
References: <72449705-4871-d317-857e-388f53800bbd@gmx.de>
 <CAF+90c_bxYCEMWSL7QO2BSTq6gOVwkmTnCj1OEt3xo-7QHTuAg@mail.gmail.com>
Message-ID: <3ce9a1ae-a82e-00a4-8905-3b5f453cc1a7@gmx.de>
Date: Fri, 18 Aug 2017 14:38:29 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <CAF+90c_bxYCEMWSL7QO2BSTq6gOVwkmTnCj1OEt3xo-7QHTuAg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: de-DE
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [RFC] Deprecate class instance deserialization in WDDX
From: cmbecker69@gmx.de ("Christoph M. Becker")

On 18.08.2017 at 12:02, Nikita Popov wrote:

> On Tue, Aug 15, 2017 at 6:54 PM, Christoph M. Becker <cmbecker69@gmx.de>
> wrote:
> 
>> 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>

Thanks for the constructive criticism! :)

> 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.

Me neither, but I wouldn't bet on any extension having no security issues.

> 3. Irrelevance: It's 2017, nobody uses WDDX. (With the usual qualifications
> on "nobody".)

I'm not sure about this.  After all, there have been 11 bug reports
during the last year.  While most of these may have been the result of
research, at least one (#73793) appears to have been detected during
practical use of the extension.  There still *might* be a lot of code
using the WDDX extension.

> 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.

I do not necessarily agree that a deprecation breaks BC. semver.org
(which we do not necessarily follow, though) states:

| Deprecating existing functionality is a normal part of software
| development and is often required to make forward progress. When you
| deprecate part of your public API, you should do two things: (1)
| update your documentation to let users know about the change, (2)
| issue a new minor release with the deprecation in place. Before you
| completely remove the functionality in a new major release there
| should be at least one minor release that contains the deprecation so
| that users can smoothly transition to the new API.

> 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.

Well, this appears to achieve more general consent, so I'm going to
withdraw this RFC, and will come forth with a new one, *if* it is still
time to deprecate the WDDX extension for PHP 7.2.  After all, 7.2.0 RC1
is scheduled for August, 31th.

> It has been documented previously: There is an existing warning in the
> "Notes" section. Now the warning is repeated twice on the same page ^^

Thanks, I'm going to fix that right away.

-- 
Christoph M. Becker