On Tue, Aug 15, 2017 at 6:54 PM, Christoph M. Becker <cmbecke...@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