Everything

php

Home » Project » Newsgroups » php.internals » [RFC] Deprecate class instance deserialization in WDDX »

[RFC] Deprecate class instance deserialization in WDDX

php.internals

Christoph Becker

9 years ago

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.

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.

I'm looking forward to your feedback.

--
Christoph M. Becker

Nikita Popov

9 years ago

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

Remi Collet

9 years ago

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

Niklas Keller

9 years ago

>
> 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

Christoph Becker

9 years ago

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