Ticket #17375 (reviewing defect (bug))

Opened 13 months ago

Last modified 12 months ago

Serialzed option values broken for classes and strings on unserialize for C and S

Reported by: hakre Owned by: markjaquith
Priority: normal Milestone: Future Release
Component: General Version: 3.1
Severity: normal Keywords: has-patch 3.3-early
Cc:

Description

Wordpress has a feature build in to store arrays and objects into option values.

This is done by transparently serializing on storing and unserialize on retrieving.

The implementation is broken.

Wordpress is unable to unserialize serialized option values if they contain a serialized representation of classes implementing the  PHP serializeable interface (C instead of O).

This is because the is_serialized() does not recorgnize that type of value.

Attachments

17375.patch Download (392 bytes) - added by hakre 13 months ago.
Add C next to a and O
17375.2.patch Download (727 bytes) - added by hakre 13 months ago.
Improved patched, added S as well.
17375.3.patch Download (964 bytes) - added by hakre 13 months ago.
fix for d serialized values as well, see #9930
17375.4.patch Download (1.0 KB) - added by hakre 13 months ago.
Plus sign is available for uiv/length specifier as well.

Change History

hakre13 months ago

Add C next to a and O

  • Owner set to markjaquith
  • Status changed from new to reviewing
  • Milestone changed from Awaiting Review to 3.2

hakre13 months ago

Improved patched, added S as well.

I've reviewed the code of yesterday now.

It lacked the second missing serialized identifier: S.

It has been added, as well for the is_serialized_string because it represents a serialized string.

  • Summary changed from Serialzed option values broken for classes with Serializeable interface to Serialzed option values broken for classes and strings
  • Summary changed from Serialzed option values broken for classes and strings to Serialzed option values broken for classes and strings on unserialize for C and S

Detection on serialized float values is broken as well. There already exists a ticket for it: #9930 - I've uploaded a new patch there against current trunk. Will merge it in here, too.

Related: #17445

Last edited 13 months ago by hakre (previous) (diff)

hakre13 months ago

fix for d serialized values as well, see #9930

hakre13 months ago

Plus sign is available for uiv/length specifier as well.

#9930 is pretty messy. Should probably concentrate work on float detection there, given the existing functional tests there.

What's 'S'? Can't find a reference to it anywhere.

went on a hunt for the S modifier.

Lead me to this changeset in php:  phprev 225029 which suggests it was for PHP6(5.3?) future compatibility, which then lead to  phprev 232476, which finally gives us a  security report about the 'S' modifer.

With PHP 5.2.1 the new S: data type was added to unserialize(). It is meant as compatibility layer for exchange of serialized data with future PHP 6. The data type itself is similar to the normal s: string data type with the exception that simple escaped bytes are supported. The following string is an example.

With PHP6 never being released, AFAIK, there are no cases where the S data type should be created by PHP, looking through  PHP 5.3's branch's var.c seems to validate that.

  • Keywords early added
  • Milestone changed from 3.2 to Future Release

I'm not comfortable with changing any regex in this function this close to a release, not for something which is called as much as this is.

After a quick chat in #wordpress-dev, I'm punting this to the next release where we can do this and #9930 at the same time.

  • Keywords 3.3-early added; early removed
Note: See TracTickets for help on using tickets.