Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] WDDX serialization and security

Posted by Nikita Popov 
Nikita Popov
[PHP-DEV] WDDX serialization and security
August 11, 2017 03:20PM
Hi internals,

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.

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

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

Regards,
Nikita
Remi Collet
Re: [PHP-DEV] WDDX serialization and security
August 11, 2017 03:40PM
Le 11/08/2017 à 15:15, Nikita Popov a écrit :

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

+1
Sebastian Bergmann
Re: [PHP-DEV] WDDX serialization and security
August 11, 2017 05:10PM
Am 11.08.2017 um 15:15 schrieb Nikita Popov:
> I'm wondering if it might be time to remove (i.e. deprecate and move to
> PECL) the wddx extension?

I have never seen it used in the wild. So +1 from me for deprecation in
7.2 and removal in 8.0.

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Kalle Sommer Nielsen
Re: [PHP-DEV] WDDX serialization and security
August 11, 2017 05:10PM
On 11 Aug 2017 15.38, "Remi Collet" <[email protected]> wrote:

Le 11/08/2017 à 15:15, Nikita Popov a écrit :

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

+1



+2
Christoph M. Becker
[PHP-DEV] Re: WDDX serialization and security
August 13, 2017 05:10PM
On 11.08.2017 at 15:15, Nikita Popov 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.

Anyhow, I've added a respective warning to the docs
(http://svn.php.net/viewvc?view=revision&revision=342852).

--
Christoph M. Becker

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Stanislav Malyshev
Re: [PHP-DEV] Re: WDDX serialization and security
August 13, 2017 05:50PM
Hi!

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

Agreed, and it was also implemented as a horrible and undocumented hack
:( I wonder if we could drop support for objects from it? Maybe it'd be
better than dropping the whole thing? I don't know, we need to ask
somebody who actually uses it, and I have no idea who those are...

--
Stas Malyshev
smalyshev@gmail.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Nikita Popov
[PHP-DEV] Re: WDDX serialization and security
August 13, 2017 06:00PM
On Sun, Aug 13, 2017 at 5:08 PM, Christoph M. Becker <[email protected]>
wrote:

> On 11.08.2017 at 15:15, Nikita Popov 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.

Nikita
Zeev Suraski
RE: [PHP-DEV] Re: WDDX serialization and security
August 14, 2017 01:10PM
> -----Original Message-----
> From: Nikita Popov [mailto:[email protected]]
> Sent: Sunday, August 13, 2017 6:53 PM
> To: Christoph M. Becker <[email protected]>
> Cc: PHP internals <[email protected]>
> Subject: [PHP-DEV] Re: WDDX serialization and security
>
> On Sun, Aug 13, 2017 at 5:08 PM, Christoph M. Becker <[email protected]>
> wrote:
>
> > On 11.08.2017 at 15:15, Nikita Popov 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
Christoph M. Becker
Re: [PHP-DEV] Re: WDDX serialization and security
August 15, 2017 02:00AM
On 14.08.2017 at 13:04, Zeev Suraski wrote:

> On Sunday, August 13, 2017 6:53 PM, Nikita Popov wrote:
>
>> On Sun, Aug 13, 2017 at 5:08 PM, Christoph M. Becker
>> <[email protected]> wrote:
>>
>>> On 11.08.2017 at 15:15, Nikita Popov wrote:
>>>
>>>> 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..?

FWIW, I've did this in my wddx branch, see
https://github.com/cmb69/php-src/tree/wddx.

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

It might be sensible to only *deprecate* object unserialization for 7.2,
and to (re)move the WDDX extension in 7.3. After all, we already tell
users that `wddx_deserialize()` should not be used on untrusted input:

* http://docs.php.net/manual/en/intro.wddx.php
* http://docs.php.net/manual/en/function.wddx-deserialize.php

Either way, we most certainly need an RFC to proceed…

--
Christoph M. Becker

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sorry, only registered users may post in this forum.

Click here to login