[RFC] Deprecate class instance deserialization in WDDX

Le 18/08/2017 à 12:02, Nikita Popov a écrit :

On Tue, Aug 15, 2017 at 6:54 PM, Christoph M. Becker cmbecker69@gmx.de

Which is why I would suggest:

1. Deprecate the entire extension in PHP 7.2.
2. Unbundle it in PHP 7.3.

+1

Dropping part of the extension features doesn't seems a good idea.

Remi

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.

Why not immediately unbundle it in PHP 7.2?

Regards, Niklas

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