On 18.08.2017 at 12:02, Nikita Popov wrote: > On Tue, Aug 15, 2017 at 6:54 PM, Christoph M. Becker <cmbecke...@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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
