WDDX serialization and security

-----Original Message-----
From: Nikita Popov [mailto:nikita.ppv@gmail.com]
Sent: Sunday, August 13, 2017 6:53 PM
To: Christoph M. Becker cmbecker69@gmx.de
Cc: PHP internals internals@lists.php.net
Subject: [PHP-DEV] Re: WDDX serialization and security

On Sun, Aug 13, 2017 at 5:08 PM, Christoph M. Becker cmbecker69@gmx.de
wrote:

Same question here as with unserialize().
https://bugs.php.net/bug.php?id=75007 has recently been classified
as
not a
security bug, because WDDX should not be fed untrusted data.

To provide some context here, our WDDX implementation is generally
vulnerable to object injection (it is possible to create arbitrary
objects,
resulting in exploitable calls to __wakeup, __destruct, __toString
and similar), but it does not have the other security issues of
unserialize
(in
particular, no references).

My question is now: What's the point of having this functionality at all?
As far as I can discern, WDDX seems to be targeted as a data
interchange format (something where trust generally cannot be
assumed), but the way
we
implement it (with support for object creation), it cannot be used
as
such.

IMHO, implementing support for objects has been a most unfortunate
decision, because WDDX was indeed not designed for that
(http://xml.coverpages.org/wddx0090-dtd-19980928.txt). Considering
https://bugs.php.net/bug.php?id=75044 makes the situation worse.

As such, these functions seem pretty useless right now. You can't
use
them
for data interchange due to security issues, and it's not the
serialization
functionality you would use for local storage (for all it's issues,
serialize() is still a much better choice for that purpose.)

ACK.

I'm wondering if it might be time to remove (i.e. deprecate and move
to
PECL) the wddx extension?

Hmm, that would leave a pretty useless extension in PECL. An
alternative might be to remove support for objects and the wddx
session serialization handler. This might even be done as bug fix if
a respective ini option would be introduced. We could still move the
extension to PECL afterwards.

I'm only suggesting a move to PECL because that seems to be our standard
procedure when removing extensions.

Given that WDDX as a data interchange format seems pretty much dead, I
don't think it's worth trying to "fix" it in some way, especially a way that breaks
backwards compatibility. Even without the additional security considerations, I
would say it's long overdue to unbundle this extension.

I would lean towards doing both:

1. Move it to PECL as you suggest - regardless of the security element, as you say, it's long overdue for unbundling.
2. Disable the object support in it as Christoph and Stas suggest, so that it's not completely useless (i.e. inherently insecure) in PECL. Admittedly I haven't looked at the code but I imagine that can't be too complex..?

Given the security implications (the positive ones, that is), I would seriously consider doing that for 7.2 despite the very late point in time.

Zeev